-
Notifications
You must be signed in to change notification settings - Fork 42
Add Contributor Request/Response Workflow and Events UI #3528
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3528 +/- ##
===========================================
+ Coverage 44.57% 71.32% +26.75%
===========================================
Files 620 51 -569
Lines 31685 1960 -29725
Branches 1485 0 -1485
===========================================
- Hits 14123 1398 -12725
+ Misses 17423 562 -16861
+ Partials 139 0 -139 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nellh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd be good to try to break things like this up into smaller PRs in the future, this could probably be several component PRs and an API change one. Most of it is looking pretty good and working. Made a few recommendations. It looks intentional that the notification status buttons are failing, that is fine to save for another PR.
packages/openneuro-app/src/scripts/dataset/mutations/request-contributor-status.tsx
Outdated
Show resolved
Hide resolved
| # Notes associated with the event | ||
| note: String | ||
| # top-level datasetId field | ||
| datasetId: ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to duplicate the datasetId from the parent in the model. Why does it need a second copy of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i was thinking that we might have various places outside of dataset that we are querying for events. The new notifications resolver queries for DatasetEvent objects by userID. Outside of a dataset- datasetId is the only way for the frontend to know which dataset an event belongs to. Does that make sense to do here?
| await updateNotificationStatus({ | ||
| variables: { | ||
| datasetEventId: id, | ||
| status: backendStatus, | ||
| }, | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran into 400 error, Cannot query field "updateNotificationStatus" on type "Mutation". on triggering this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, I put that on hold, and have a question about how to store that data on a user if many users will share the same notification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has been updated in #3548
| setTimeout(() => { | ||
| setUpdatedNote("") | ||
| if (editingNoteId === event.id) { | ||
| startEditingNote(null, "") | ||
| } | ||
| }, 50) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is setTimeout necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not really. I was trying to account for a slow refetchEvents where there could be what feels like two "renders" in the UI. I don't imagine it to ever be an issue and when I remove it locally it is pretty unnoticeable. I will remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
| interface ProcessedDatasetEvent { | ||
| id: string | ||
| note: string | ||
| timestamp: string | ||
| user: { | ||
| id: string | ||
| name: string | ||
| email: string | ||
| orcid: string | ||
| } | ||
| event: { | ||
| type: string | ||
| requestId?: string | ||
| status?: string | ||
| } | ||
| success: boolean | ||
| hasBeenRespondedTo?: boolean | ||
| responseStatus?: string | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The event type seems to be defined a few times. It would be good to unify these and import the shared type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| const processedEvents = useMemo(() => { | ||
| const responsesMap = new Map() | ||
| rawEvents.forEach((event) => { | ||
| if (event.event.type === "contributorResponse" && event.event.requestId) { | ||
| responsesMap.set(event.event.requestId, event) | ||
| } | ||
| }) | ||
|
|
||
| const enrichedEvents = rawEvents.map((event) => { | ||
| if (event.event.type === "contributorRequest") { | ||
| const response = responsesMap.get(event.id) | ||
| if (response) { | ||
| return { | ||
| ...event, | ||
| hasBeenRespondedTo: true, | ||
| responseStatus: response.event.status, | ||
| } | ||
| } | ||
| } | ||
| return event | ||
| }) | ||
| return enrichedEvents | ||
| }, [rawEvents]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This step could be done on the API side instead. If the client needs this, the API should just return it or provide an option for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved to the backend
| // Determine if the current user already has any permissions (read, write, admin) | ||
| // If they do, the button should not be displayed. | ||
| const hasPermissions = datasetPermissions?.some((p) => | ||
| p.user.id === currentUserId && | ||
| (p.level === "admin" || p.level === "rw" || p.level === "read") | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check if the user is a contributor, rather than if they have permissions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a perm check for the dataset.contributors array. needs to be reviewed once that field is added.
…vents.tsx add back erroneous removal of sentry Co-authored-by: Nell Hardcastle <[email protected]>
… - needs hook up with #3510(or future update) and datacite.yml
…ts by adding a new UserNotificationStatus model and updating resolvers to populate the status field.
…d code into new dir
Feature/datacite mutation
…m / writing them to the datacite.yml file
…ntributor-onUser Feature/ mutations for event adding contributors and accept/deny adding contributors
App/Contributor tests
Feature/datacite mutation UI
Feat(api) add a user specific virtual notification status to dataset events
Adds EVENTS for managing contributor access to datasets. Allows users to formally request contributor status, and provides dataset administrators with a dedicated UI to view, accept, or deny these requests. All actions are tracked and displayed in a new, unified events log on the dataset and admins get a log in their account notifications.
Actual contributor Roles/Perms are TODO - this PR just handles events resolvers and event related UI
Key Work & Changes
New Contributor Request Workflow
Request Contributor Button: allows users who do not have permissions to a dataset to formally request contributor access.
New GraphQL mutations are used to log these requests as events.
Admin UI: The DatasetEvents component can now managing these requests
Events & Notifications System
New Event Types: contributorRequest and contributorResponse
The DatasetEvents component has been refactored
Detailed Event Items component is responsible for displaying each event
New CSS added for events UI and actions.
tests have been adjusted for various issues related to updates. additional tests needed.