Dex 1435 new notifications to fe #463
Conversation
…hemas for the new notifications
| @@ -142,6 +142,7 @@ export default function AppHeader() { | |||
| refreshNotifications={refreshNotifications} | |||
| shouldOpen={shouldOpenNotificationPane} | |||
| setShouldOpen={setShouldOpenNotificationPane} | |||
| currentUserGuid={meData?.guid} | |||
There was a problem hiding this comment.
I fluctuated a little bit about whether I wanted this to be a prop here or just derive it with an api call in the child component.
There was a problem hiding this comment.
I ran into the same question on #464. I passed it as a prop too. It looks like React Query recommends just deriving it though.
TanStack/query#463
There was a problem hiding this comment.
I kept it as a prop for this ticket...
There was a problem hiding this comment.
That seems like a good idea to me. It is something we can look into and try to implement going forward.
| @@ -0,0 +1,30 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
All of these new dialogs are essentially duplicates with different names. I still don't love this pattern, because it's much DRYer to just have a baseNotificationDialog component do this job, but Ben long ago recommended to do it this way in case the notifications needed to diverge farther from one another in the future.
There was a problem hiding this comment.
It looks to me like notifications have already diverged pretty far from each other, but instead of putting those differences in these files, we put them in the NotificationDetailsDialog.
There was a problem hiding this comment.
Yeah... NotificationDetailsDialog might be a great example of the abstraction monster situation described in https://www.deconstructconf.com/2019/dan-abramov-the-wet-codebase. Hoping to get the chance to make this better/more human-friendly in the future (but not for current ticket).
Pull request checklist:
developbranch, notmainWhich JIRA ticket(s) and/or GitHub issues does this PR address?
DEX-1435
Thanks for keeping it clean! Feel free to merge your own pull request after it has been approved - just use the "squash & merge" option.
QA Notes
collaboration_manager_edit_approved:
Let's say there's a collaboration between use1 and user2 such that they are already view collaborators, and user1 has already asked user2 to raise their collaboration to edit level. If at this point, a user manager updates that collab to edit, both users should see:
collaboration_manager_edit_revoke
Let's say there's a collaboration between use1 and user2 such that they are already edit collaborators. If at this point, a user manager updates that collab to view only, both users should see:
collaboration_edit_denied
Let's say there's a collaboration between use1 and user2 such that they are already view collaborators. If at this point, a user1 requests edit access of user2, and user2 denies the request, user1 will see:
collaboration_manager_edit_denied
I can't currently think of a way to create this notification. User managers can only edit the current state of a collaboration. If user1 and user2 are in an approved view state, and user1 has requested edit access of user2, the user manager can change the state to edit, in which case, there will be an approved notification. The user manager cannot change the state to view, because that's already the existing state:
It might also be a good idea to spot check for regressions in older notification types.