Repository navigation
fix(settings-users): resolve a deep-linked user by exact uid - #529
Conversation
The routed-user lookup searched q=<uid> and kept the exact match among 100 hits. q matches a substring, so on a large instance a short uid could fall outside that page and the modal claimed the user did not exist. Requires the uid param added in ICIJ/datashare#2435. Closes ICIJ/datashare#2434
pirhoo
left a comment
pirhoo
left a comment
There was a problem hiding this comment.
Thanks @caro3801, looking up users by exact uid is the right fix for links to users who don't show up in the first page of results. One thing to settle before merging: without the exact-match check, a backend that ignores uid (anything before ICIJ/datashare#2435) would return the wrong user. Details inline.
| const { items } = await api.getUsers({ domain: DEFAULT_DOMAIN, noRole: true, uid }) | ||
| if (uid !== routedUid.value) return | ||
| routedUser.value = items?.find(user => user.uid === uid) ?? null | ||
| routedUser.value = items?.[0] ?? null |
There was a problem hiding this comment.
crucial: with items?.[0], any backend that doesn't support uid yet ignores the param and returns the first page of all users, so /settings/users/delete/zoe@example.org would open the delete modal on whoever comes first and delete them. Keeping the exact-match find costs nothing once the backend honours uid and keeps the wrong account from being picked:
| routedUser.value = items?.[0] ?? null | |
| routedUser.value = items?.find(user => user.uid === uid) ?? null |
There was a problem hiding this comment.
Never mind, I checked the backend!
| ['edit', SettingsViewUsersEditModal], | ||
| ['delete', SettingsViewUsersDeleteModal] | ||
| ])('flags the %s modal as not found when the URL names an unknown user', async (action, modal) => { | ||
| api.getUsers.mockImplementation(async ({ uid }) => (uid ? { items: [], pagination: { total: 0 } } : usersResponse)) |
There was a problem hiding this comment.
important: this mock had to change to return no items because with items?.[0] the default response would resolve ghost to the first listed user. Nothing tests that case any more. Could we keep the original mock and add a case where the uid lookup returns { items: [otherUser] }, asserting the modal is flagged not found?
An unknown uid came back as a full page of users from the shared mock, which an exact-uid backend never returns, so the not-found test had to override it. One faithful mock covers every routed test. Co-authored-by: Pierre Romera Zhang <471176+pirhoo@users.noreply.github.com>
2 participants
Closes ICIJ/datashare#2434. Opening a modal from a URL (
/settings/users/edit/<uid>) looks the user up when they are not on the current page of the list. That lookup usedq=<uid>&size=100and kept the exact match among the hits, butqis a substring search: with a short uid on a large instance the user we want can fall outside those 100 hits, and the modal says they do not exist.api.getUsersnow accepts auidparam, forwarded to the backend as an exact match (added in ICIJ/datashare#2435, which this needs), andfetchRoutedUseruses it instead ofqandsize: 100, takingitems[0]rather than re-checking the uid client-side.domainandnoRole: trueare kept, otherwise a user holding no role in the scope would be filtered out and we would be back to "does not exist". The modals are untouched: auidlookup returns the same user shape the list returns. Specs updated to drive the lookup onuid, plus one guarding that it never falls back to aqsearch, and one onapi.getUsersfor the param itself.