Fix/raps edit permissions - #338
Conversation
Bundle ReportBundle size has no change ✅ |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #338 +/- ##
==========================================
+ Coverage 44.70% 44.90% +0.19%
==========================================
Files 1063 941 -122
Lines 52127 49119 -3008
Branches 6133 6579 +446
==========================================
- Hits 23304 22055 -1249
+ Misses 27848 26125 -1723
+ Partials 975 939 -36
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
🔵 Needs a closer look
It modifies the shared qtable.js table utility consumed by many RAPS views and adds a framework-level JSON converter whose UI behavior across those views cannot be runtime-verified here, warranting human final review.
Pull request overview
This PR fixes two RAPS permission/role-editing bugs. First, three edit forms (add member to role, add member to permission, add permission to member) returned HTTP 400 when submitted without both dates, because QuasarTable.submit sent empty strings for blank dates while the API expected DateTime?/DateOnly?. Second, the member-roles table hid view-assigned roles after a PUT due to a race between restoring a swapped urlBase and the reload triggered by submit. The fix routes read-only query params through the previously-unused query field on quasarTable, keeping urlBase clean so getUpdateURL() builds correct PUT/DELETE URLs, and adds an opt-in JsonConverter plus frontend || null guards for date fields.
Changes:
- Front end: send
nullinstead of""for blank dates in the three affectedcreateBodyfunctions, and moveincludeViewMembersinto the stablequeryfield instead of mutatingurlBase. - Backend: add
EmptyStringAsNullConverter<T>and apply it to the date properties ofRoleMemberCreateUpdateandMemberPermissionCreateUpdateto defensively bind empty strings as null. - Tests: add
EmptyDateBindingTestscovering empty/real/null/omitted date bodies and a serialization round-trip.
File summaries
| File | Description |
|---|---|
| web/wwwroot/js/qtable.js | Merges the query object into GET params in load(), decoupling read-only params from urlBase. |
| web/Classes/Utilities/EmptyStringAsNullConverter.cs | New generic converter that binds empty/whitespace strings to null for nullable value types. |
| web/Areas/RAPS/Models/RoleMemberCreateUpdate.cs | Applies the converter to StartDate/EndDate (DateOnly?). |
| web/Areas/RAPS/Models/MemberPermissionCreateUpdate.cs | Applies the converter to StartDate/EndDate (DateTime?). |
| web/Areas/RAPS/Views/Roles/Members.cshtml | Adds ` |
| web/Areas/RAPS/Views/Members/Roles.cshtml | Seeds query: { includeViewMembers: true }, removes the urlBase swap, keeps urlBase clean. |
| web/Areas/RAPS/Views/Permissions/Members.cshtml | Adds ` |
| web/Areas/RAPS/Views/Members/Permissions.cshtml | Adds ` |
| test/RAPS/EmptyDateBindingTests.cs | New tests validating empty-string-as-null binding and serialization round-trip. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| { | ||
| public override T? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) | ||
| { | ||
| if (reader.TokenType == JsonTokenType.Null) |
There was a problem hiding this comment.
Low priority, non-blocking.
Both null branches in this class are unreachable. For a converter over Nullable<T>, STJ handles null itself unless HandleNull is overridden to true, so Read is never handed a Null token and Write is never handed a null value.
Checked on .NET 10 with hit counters on both branches:
- deserializing
{"startDate":null,"endDate":""}callsReadonce, for the empty string - serializing a model with
StartDateset andEndDatenull callsWriteonce, forStartDate
Lines 16-19 and the else on 34-37 can go.
There was a problem hiding this comment.
Removed both unreachable branches
| var model = DeserializeMemberPermission( | ||
| @"{""memberId"":""12345678"",""permissionId"":5,""access"":1,""startDate"":""2026-01-15T00:00:00"",""endDate"":""2026-06-30T00:00:00""}"); | ||
|
|
||
| Assert.Equal(new DateTime(2026, 1, 15, 0, 0, 0, DateTimeKind.Local), model.StartDate); |
There was a problem hiding this comment.
Low priority.
DateTimeKind.Local isn't asserting what it looks like it's asserting. STJ parses "2026-01-15T00:00:00" (no offset, no Z) as Unspecified, and Assert.Equal on DateTime compares ticks and ignores Kind. The same line with DateTimeKind.Utc would also pass.
Drop DateTimeKind.Local from the DateTime constructor, or assert model.StartDate.Value.Kind separately if you want it pinned.
There was a problem hiding this comment.
Since SonarAnalyzer S6562 expects an explicit Kind on this type of constructor, switched from DateTimeKind.Local to DateTimeKind.Unspecified.
This PR addresses 2 small RAPS bugs, one in each commit.
400 on editing users' permissions and roles
submitsending empty strings for blank dates, when the API expectedDateTime?nullinstead of an empty string.JsonConverterto cover any future paths to these endpoints without changing the general repo behavior.Prevent role to member table from hiding view roles after PUT
submitto update the table that was always lost.queryfield inqtable.jsto allow a stable URL path with varying parameters. view roles now persist through a PUT.