Attach username to upload dropdown#1350
Open
david-roper wants to merge 26 commits intoDouglasNeuroInformatics:mainfrom
Open
Attach username to upload dropdown#1350david-roper wants to merge 26 commits intoDouglasNeuroInformatics:mainfrom
david-roper wants to merge 26 commits intoDouglasNeuroInformatics:mainfrom
Conversation
…add it in standalone styled box
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds optional ChangesUsername Threading in Instrument Records Upload
Sequence DiagramsequenceDiagram
participant User as User
participant UI as Upload Route (web)
participant Hook as useUsersQuery
participant API as Web API
participant Backend as InstrumentRecordsService
participant Users as UsersService
participant Sessions as SessionsService
User->>UI: open upload, select username & CSV
UI->>Hook: fetch users (groupId)
Hook->>API: GET /v1/users?groupId=...
API-->>Hook: users list
Hook-->>UI: users loaded
User->>UI: submit upload
UI->>API: POST /v1/instrument-records/upload (payload includes username)
API->>Backend: upload(payload with username)
Backend->>Users: findByUsername(username) // validate if provided
Users-->>Backend: user found / not found
Backend->>Sessions: create({ type: "RETROSPECTIVE", username })
Sessions-->>Backend: session created
Backend-->>API: upload complete
API-->>UI: response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure uploaded instrument-record CSV data is associated with a username for audit/traceability, addressing issue #1342 by extending the upload payload and propagating a username through the upload flow into created sessions.
Changes:
- Extend the upload schema/payload to optionally include a
username. - Update the web upload flow to send a username alongside uploaded records and add UI to choose the username.
- Update the API upload service to pass the provided username into session creation.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/schemas/src/instrument-records/instrument-records.ts | Adds optional username to the upload payload schema. |
| apps/web/src/utils/upload.ts | Adds currentUsername input and sends username in upload payload. |
| apps/web/src/routes/_app/upload/$instrumentId.tsx | Adds a username selection UI and wires it into upload submission. |
| apps/web/src/hooks/useUsersQuery.ts | Adds optional groupId query param support for fetching users. |
| apps/api/src/instrument-records/instrument-records.service.ts | Passes username into session creation during upload. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }) | ||
| ) | ||
| ), | ||
| username: z.string().optional() |
Comment on lines
+28
to
+32
| const groupUsers = useUsersQuery({ | ||
| params: { | ||
| groupId: currentGroup?.id | ||
| } | ||
| }); |
Comment on lines
+375
to
+376
| type: 'RETROSPECTIVE', | ||
| username: username ?? undefined |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/web/src/utils/upload.ts (1)
1026-1031: ⚡ Quick winSend
undefinedinstead of'N/A'— the backend does an unnecessaryfindFirston the literal string.
username: currentUsername ?? 'N/A'always sends a non-empty string. The backend'ssessionsService.createthen hits the DB with{ where: { username: 'N/A' } }for every record in the batch, finds nothing, and setsuser = null. Since$UploadInstrumentRecordsDatadeclaresusernameas.optional(), sendingundefined(or omitting the field) achieves the same null-user outcome with no DB round-trip.♻️ Proposed fix
const reformatForSending: UploadInstrumentRecordsData = { groupId: currentGroup?.id, instrumentId: instrument.id, records: recordsList, - username: currentUsername ?? 'N/A' + username: currentUsername ?? undefined };And in
$instrumentId.tsxthe 'N/A' Select.Item value can remain for display purposes — just map it back toundefinedbefore passing toreformatInstrumentData:- currentUsername: selectedUsername ?? currentUser?.username, + currentUsername: (selectedUsername === 'N/A' ? undefined : selectedUsername) ?? currentUser?.username,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/utils/upload.ts` around lines 1026 - 1031, reformatForSending is currently sending the literal 'N/A' for missing users which causes unnecessary DB lookups; change the assignment in the UploadInstrumentRecordsData object (reformatForSending.username) to use undefined when currentUsername is not set (e.g., currentUsername ?? undefined) so the field is omitted/optional, and in the UI where the Select.Item displays 'N/A' (e.g., in $instrumentId.tsx) map the display value 'N/A' back to undefined before calling reformatInstrumentData or constructing reformatForSending so the backend receives no username instead of the string 'N/A'.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/web/src/hooks/useUsersQuery.ts`:
- Around line 11-18: usersQueryOptions builds a query whose cache key is
constant (queryKey: [USERS_QUERY_KEY]) so different params (e.g., groupId)
collide; update usersQueryOptions to include the params in the queryKey (for
example use queryKey: [USERS_QUERY_KEY, params ?? null] or a stable
serialization of params) so React Query separates caches per params, and ensure
the same symbol names (usersQueryOptions, params, USERS_QUERY_KEY, queryKey) are
used when modifying the function.
In `@apps/web/src/routes/_app/upload/`$instrumentId.tsx:
- Around line 171-187: The Select can display currentUser?.username even when
that username isn't in groupUsers.data, causing mismatch between shown value and
selectable items; update initialization and/or options: ensure selectedUsername
is pre-seeded to currentUser?.username when the component mounts (or when
currentUser becomes available) and/or add a Select.Item for currentUser if
currentUser.username is not present in groupUsers.data; adjust logic around
selectedUsername, currentUser and groupUsers.data and the Select rendering
(references: selectedUsername, setSelectedUsername, currentUser,
groupUsers.data, Select.Item) so the displayed value always matches an available
option.
---
Nitpick comments:
In `@apps/web/src/utils/upload.ts`:
- Around line 1026-1031: reformatForSending is currently sending the literal
'N/A' for missing users which causes unnecessary DB lookups; change the
assignment in the UploadInstrumentRecordsData object
(reformatForSending.username) to use undefined when currentUsername is not set
(e.g., currentUsername ?? undefined) so the field is omitted/optional, and in
the UI where the Select.Item displays 'N/A' (e.g., in $instrumentId.tsx) map the
display value 'N/A' back to undefined before calling reformatInstrumentData or
constructing reformatForSending so the backend receives no username instead of
the string 'N/A'.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a1afdfd-ab16-4f51-b58e-c5e8aeedcf4b
📒 Files selected for processing (5)
apps/api/src/instrument-records/instrument-records.service.tsapps/web/src/hooks/useUsersQuery.tsapps/web/src/routes/_app/upload/$instrumentId.tsxapps/web/src/utils/upload.tspackages/schemas/src/instrument-records/instrument-records.ts
Comment on lines
+171
to
+187
| <Select value={selectedUsername ?? currentUser?.username} onValueChange={setSelectedUsername}> | ||
| <Select.Trigger> | ||
| <Select.Value placeholder={currentUser?.username} /> | ||
| </Select.Trigger> | ||
| <Select.Content> | ||
| <Select.Group> | ||
| {groupUsers.data.map((user) => ( | ||
| <Select.Item key={user.username} value={user.username}> | ||
| {user.username} | ||
| </Select.Item> | ||
| ))} | ||
| <Select.Item key={'N/A'} value={'N/A'}> | ||
| {'N/A'} | ||
| </Select.Item> | ||
| </Select.Group> | ||
| </Select.Content> | ||
| </Select> |
Contributor
There was a problem hiding this comment.
If currentUser is not a member of currentGroup, the Select shows a value (currentUser?.username) that has no matching Select.Item.
When selectedUsername is undefined (initial state) and currentUser is not in groupUsers.data, most Radix-based Select implementations will show the placeholder instead of the value — so the displayed username and the submitted username may appear to diverge. Consider pre-seeding selectedUsername from currentUser?.username at initialization, or adding currentUser explicitly to the options list if they could be absent.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/web/src/routes/_app/upload/`$instrumentId.tsx around lines 171 - 187,
The Select can display currentUser?.username even when that username isn't in
groupUsers.data, causing mismatch between shown value and selectable items;
update initialization and/or options: ensure selectedUsername is pre-seeded to
currentUser?.username when the component mounts (or when currentUser becomes
available) and/or add a Select.Item for currentUser if currentUser.username is
not present in groupUsers.data; adjust logic around selectedUsername,
currentUser and groupUsers.data and the Select rendering (references:
selectedUsername, setSelectedUsername, currentUser, groupUsers.data,
Select.Item) so the displayed value always matches an available option.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
… currentUser username
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts (1)
81-140: ⚡ Quick winTests cover the happy/sad paths; consider adding a cross-group case.
The three scenarios (valid, unknown, omitted) look right and assert the correct interactions. If you adopt the group-membership check from the service review, please add a test that supplies a
usernamebelonging to a different group thangroupIdand asserts rejection plus no session creation. Also worth tightening: in the unknown-username test, additionally assertsubjectsService.createManywas not called, to lock in the "validate before any side effects" ordering.♻️ Suggested extra assertion
await expect(instrumentRecordsService.upload({ ...baseUploadData, username: 'spoofed' })).rejects.toBeInstanceOf( NotFoundException ); + expect(subjectsService.createMany).not.toHaveBeenCalled(); expect(sessionsService.create).not.toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts` around lines 81 - 140, Add a test that verifies upload rejects when the provided username exists but belongs to a different group than the upload's groupId: mock usersService.findByUsername to return a user whose groupId !== baseUploadData.groupId, call instrumentRecordsService.upload({ ...baseUploadData, username: 'otherGroupUser' }), expect it to reject (e.g., toBeInstanceOf or matching error) and assert sessionsService.create was not called; also tighten the existing "unknown username" test to assert subjectsService.createMany was not called to ensure validation happens before side effects (refer to instrumentRecordsService.upload, usersService.findByUsername, sessionsService.create, and subjectsService.createMany).apps/api/src/instrument-records/instrument-records.service.ts (1)
334-352: 💤 Low valueConsider validating username before the SERIES check, or document the order.
Minor: the SERIES rejection happens before username validation, so an invalid-instrument upload won't surface a stale "user not found" error — that's fine. But note that
instrumentsService.findByIdruns even whenusernameis bogus; if you'd prefer fail-fast on bad usernames (cheaper DB call, clearer error precedence), move theif (username)block above the instrument fetch. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/api/src/instrument-records/instrument-records.service.ts` around lines 334 - 352, The upload method currently fetches the instrument and checks instrumentsService.findById(...)/instrument.kind === 'SERIES' before validating the username; to prefer fail-fast username validation, move the username existence check (the if (username) { await this.usersService.findByUsername(username, options); } call) to run before calling instrumentsService.findById and before the SERIES rejection, so a missing or invalid user returns first; alternatively, if you want to preserve current behavior, add a comment in upload documenting that instrument kind is validated prior to username lookup to make the order explicit.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/api/src/instrument-records/instrument-records.service.ts`:
- Around line 349-351: The code calls this.usersService.findByUsername(username,
options) but never verifies the returned user's group membership against the
upload target groupId, allowing a GROUP_MANAGER who spans multiple groups to
attach users from the wrong group; after resolving the user (the result of
this.usersService.findByUsername call) check that the user's groups array
includes the upload's target groupId (the groupId variable used in the upload
flow) and if not, throw a permission/validation error (e.g., ForbiddenException
or a clear ValidationError) so only users belonging to the target group can be
attached; update the upload/attach path in InstrumentRecordsService (the method
around where username is checked) to perform this membership validation using
the resolvedUser.groups before proceeding.
---
Nitpick comments:
In
`@apps/api/src/instrument-records/__tests__/instrument-records.service.spec.ts`:
- Around line 81-140: Add a test that verifies upload rejects when the provided
username exists but belongs to a different group than the upload's groupId: mock
usersService.findByUsername to return a user whose groupId !==
baseUploadData.groupId, call instrumentRecordsService.upload({
...baseUploadData, username: 'otherGroupUser' }), expect it to reject (e.g.,
toBeInstanceOf or matching error) and assert sessionsService.create was not
called; also tighten the existing "unknown username" test to assert
subjectsService.createMany was not called to ensure validation happens before
side effects (refer to instrumentRecordsService.upload,
usersService.findByUsername, sessionsService.create, and
subjectsService.createMany).
In `@apps/api/src/instrument-records/instrument-records.service.ts`:
- Around line 334-352: The upload method currently fetches the instrument and
checks instrumentsService.findById(...)/instrument.kind === 'SERIES' before
validating the username; to prefer fail-fast username validation, move the
username existence check (the if (username) { await
this.usersService.findByUsername(username, options); } call) to run before
calling instrumentsService.findById and before the SERIES rejection, so a
missing or invalid user returns first; alternatively, if you want to preserve
current behavior, add a comment in upload documenting that instrument kind is
validated prior to username lookup to make the order explicit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1cf19243-ddb2-4abe-90d4-4c1ff2b935b4
📒 Files selected for processing (5)
apps/api/src/instrument-records/__tests__/instrument-records.service.spec.tsapps/api/src/instrument-records/instrument-records.module.tsapps/api/src/instrument-records/instrument-records.service.tsapps/web/src/routes/_app/upload/$instrumentId.tsxapps/web/src/utils/upload.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/routes/_app/upload/$instrumentId.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ly show users of current group
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/routes/_app/dashboard.tsx (1)
31-35: ⚡ Quick winGuard the users query when
currentGroupis not ready.Right now the query can run with
groupIdundefined. Add anenabledguard so this path never makes an unscoped request.Proposed change
const userInfoQuery = useUsersQuery({ + enabled: Boolean(currentGroup?.id), params: { groupId: currentGroup?.id } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/routes/_app/dashboard.tsx` around lines 31 - 35, The users query (userInfoQuery using useUsersQuery) can run with an undefined groupId and make an unscoped request; update the call to useUsersQuery to include an enabled guard (e.g., enabled: !!currentGroup?.id or Boolean(currentGroup?.id)) so the query only runs when currentGroup.id is available, keeping params.groupId as-is and ensuring the request is not sent when currentGroup is not ready.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@apps/web/src/routes/_app/dashboard.tsx`:
- Around line 31-35: The users query (userInfoQuery using useUsersQuery) can run
with an undefined groupId and make an unscoped request; update the call to
useUsersQuery to include an enabled guard (e.g., enabled: !!currentGroup?.id or
Boolean(currentGroup?.id)) so the query only runs when currentGroup.id is
available, keeping params.groupId as-is and ensuring the request is not sent
when currentGroup is not ready.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9d3e9a8f-4e7c-4193-a34c-460cf6c109f1
📒 Files selected for processing (1)
apps/web/src/routes/_app/dashboard.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The other option to close issue #1342
This pr removes the optional checkbox and make it always attach the user to the uploaded data whenever possible.
If we select this option the pr #1344 will be removed
The new implementation now includes a dropdown of all the usernames in the group, which allows the user to select which username to attach to the upload. The dropdown defaults to the current user's username and also provides an 'N/A' option if needed.
small change to update user backend that fixes issue #1354
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests