Issue-1469/notifications

[cayb0rg]

Starts the notification UI for #1469

[clpetersonucf]

It's mostly functional, a few quick comments:
Bugs:

• Dismissing a notification message will clear the message inbox but the envelope icon persists. Ideally the icon should be dismissed completely when the message queue is empty. Right now, the envelope icon continues to persist, even after logging out and logging back in.
• Actions performed through notifications are non functional. There's only one right now: granting access to another user if they request access via the LTI pre-embed placeholder. An easy way to test this:

1. Visit /lti/test/provider and select LTI Picker Launch -> As Instructor
2. Select a widget to get the pre-embed confirmation.
3. Select LTI Picker Launch -> As New Instructor
4. Request access.
5. Launch via As Instructor again, then select the option to create a new widget in Materia.

You should have the notification requesting access. For me, granting access yields a console error. On production, it loads the widget in My Widgets, opens the collaborator dialog, and pre-populates requesting user as a collaborator. This may require some additional engineering to those components, so let me know if you want a hand with getting it working.

Design feedback:

• Font scaling in the notification shade is a little all over the place. I would reduce all font sizes a bit, including the action button:

• The close button is a PNG file that hasn't scaled well on high resolution displays. I would like to replace it with an SVG or font glyph that isn't rasterized.

Overall, good work so far 👍

[cayb0rg]

Thanks for the feedback!

New changes:

• “Grant access” now redirects user to my widgets and adds the requesting user in collaborator
• Notification shows error message on server failure
• Adds sent timestamp to notifications
• Fixes request access success message showing for all owners in LTI view
• Fixes widget link href in notifications
• Sender avatar is no longer clickable
• Changes delete button to an SVG
• Clicking outside notifications closes notifications
• Notification symbol no longer persists after notifications have all been dismissed

[clpetersonucf]

This is awesome, everything works, only a few final bits of feedback:

• The grant access button in notifications is explicitly updating a user's access as soon as it's selected, in addition to opening the collaborator window. This is a slight change from the current behavior on prod, where the pending collaborator is added in the collaboration window and highlighted, but the update (and actual API call) is not performed until they hit save. I think your implementation is reasonable, but it also assumes a level of access (full) and I'm not sure which option is more intuitive. I'm leaning towards using the behavior we have on prod, but I'm open to your alternative, especially if the original is more complicated to implement.
• Most recent notifications should probably be at the top of the list, since that's the order in which the notifications will likely be read.
• It'd be nice to have a hover state for the notification button, just for consistency with other interactive elements.

[cayb0rg]

It does make more sense to perform the update after the player hits save. I've changed it to update the query data instead, but this may need further testing.

Edit: Not functioning correctly upon further testing. Cancelling and attempting to grant access to another user doesn't add them to the collaborator.

Edit 2: Reverted to the previous method.

[clpetersonucf]

LGTM. I really like the new flow for granting access, great job!

The only remaining requested change: clear out the remaining console logs, I would also update the console.log associated with the onError in the useSetUserInstancePerms hook:

onError: () => console.log('failed to update user perms')

with a console.warn or console.error

[clpetersonucf]

I was ready to merge this in but did a bit of final testing after resolving the merge conflict and found a few additional issues:

• The collab dialog is duplicating the last user in the list when receiving the data from user_get when in the context of the instance admin panel. The collab dialog in My Widgets doesn't appear to have this issue.
• Adding someone as a collaborator from either My Widgets or the instance admin panel causes the page to error out as soon as a user is selected from the search results drop-down.

I'm not sure if these behaviors were present prior to resolving the merge conflict - resolving it had to do with renaming the state variable updatedOtherUserPerms to updatedAllUserPerms in the my-widgets-collaborate-dialog component.

[clpetersonucf]

A follow-up to my last comment: the issues relating to duplicate collaborators is related to loose management of variable types, particularly with user perms.

In the my-widgets-page component, we're explicitly setting the user ID key of the other user perms' object to an int:

Materia/src/components/my-widgets-page.jsx

Line 128 in 0d139fa

othersPerms.set(parseInt(i), rawPermsToObj(permUsers.widget_user_perms[i], isEditable))

However, in the support-selected-instance component, that same value is not being explicitly set. It ends up being cast as a string, which is something that (iirc) changed with the PHP 8.1 update. That causes comparisons downstream to fail, like this one, which is why collaborators end up duplicated in one version of the collaborator dialog but not another:
https://github.com/cayb0rg/Materia/blob/69f311cd0e1ea44381f9575bc1cbd6d4252f5c6a/src/components/my-widgets-collaborate-dialog.jsx#L250

I would wrap the user perm key in a parseInt() in the support-selected-instance component as well, because IMO, ints are preferable to strings for these numeric IDs. That also means I'd probably remove this line:
https://github.com/cayb0rg/Materia/blob/69f311cd0e1ea44381f9575bc1cbd6d4252f5c6a/src/components/my-widgets-collaborate-dialog.jsx#LL83C3-L83C3
Since it's trying to perform the opposite. Unfortunately, the looseness of type definitions in javascript is biting us here a little bit 😬

[cayb0rg]

New changes:

• Deletes notifications optimistically (instead of calling notification_get every time a notification is deleted)
• Support users can be added as collaborators even if they are students
• Adds null check for newCollabUser
• Fixes an issue with added users not appearing in collaborator
• Disabled option to remove yourself as a collaborator if you have view access (quick fix to this issue)
• Adds parseInt() to support selected instance to fix duplicated users

[clpetersonucf]

It looks good to go, for realsies now. I'd like to re-implement allowing users with View Scores access to revoke their own access, because a user should always be able to remove themselves from a widget, but that can be done outside of this PR.