Skip to content
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

Add blocking notification to EditPage #3382

Merged
merged 48 commits into from
Jun 7, 2024

Conversation

G-Ambatte
Copy link
Collaborator

@G-Ambatte G-Ambatte commented Mar 29, 2024

This PR contributes towards Issue #3326.

This PR adds a notification to the EditPage which must be acknowledged before the brew itself will become editable.

EditPage:
image

MongoDB entry:
image

@G-Ambatte G-Ambatte self-assigned this Mar 29, 2024
@G-Ambatte G-Ambatte added P2 - minor feature or not urgent Minor bugs or less-popular features 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels Mar 29, 2024
@G-Ambatte G-Ambatte mentioned this pull request Mar 29, 2024
4 tasks
Copy link
Member

@calculuschild calculuschild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some thoughts:

  1. Seeing this popup in action, it occurs to me that it behaves as kind of a mini error page. Would it make more sense to use the common "Error Page" for this message? And put the "click to unlock" button on there?
  2. The error should probably explain what being "locked" means. I.e. the brew is not visible via the share page, etc.
  3. So "Click to unlock" just takes you to the editor as normal, but doesn't actually lift the lock. Is there a better name for that button, like "CLICK TO CONTINUE TO THE EDITOR".
  4. I think a better use for this kind of "popup" thing might be for a "submit to admins" thing. I'm picturing this:
  • User tries to open their edit page, and hits the error page instead. This is where all the details are. "your brew is locked. Here are all the details, and what this means for you. etc. Click here to continue to the editor"
  • User enters the editor, where this popup now appears with a smaller message: "Your brew is locked. (locking message). Once you have resolved this issue, click here to notify the administrators for review. (BUTTON "REQUEST UNLOCK"). Click (OTHER BUTTON) to temporarily hide this notification (will reappear when the page is next reloaded).
  1. It might make sense to reuse (and maybe update) the "notificationPopup.jsx" component we already have instead of making a separate component element.

This might need some more discussion but I would like to hear what you think.

@calculuschild calculuschild added 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging and removed 🔍 R0 - Needs first review 👀 PR ready but has not been reviewed labels Apr 4, 2024
@ericscheid
Copy link
Collaborator

ericscheid commented Apr 4, 2024

Some thoughts:

  1. This covers the case of initially loading /edit/:id .. but what happens when an author has opened a non-locked brew, but that brew has since had the lock applied?

This can happen as authors do have a habit of opening a brew to edit and leaving it open in their browser for long periods of time (e.g. weeks).

  1. Does this click-to-acknowledge persist? That is, having obtained edit access for this session, if they close the brew and later go to edit it again will they see this message again? Or does click-to-acknowledge update the mongo record for the brew (e.g. editor-ack: <some-date> .. I would recommend a date rather than simply true, btw).

@calculuschild
Copy link
Member

calculuschild commented Apr 10, 2024

  1. This covers the case of initially loading /edit/:id .. but what happens when an author has opened a non-locked brew, but that brew has since had the lock applied?

I don't think we have any way to push notifications out like that without creating a live socket connection for every editing session, which adds some complexity. Something like that would be a whole separate project, but eventually that might be something we add.

  1. Does this click-to-acknowledge persist? That is, having obtained edit access for this session, if they close the brew and later go to edit it again will they see this message again?

I am imagining whatever notice we send should reappear every time the edit page is re-opened. If they are actively in violation of something I would rather keep reminding them than let them mute it. That said, I'm not really sure how lax or strict we should be in this type of situation.

@5e-Cleric
Copy link
Member

That said, I'm not really sure how lax or strict we should be in this type of situation.

I think the question here is what is the locking function here to prevent? what type of content are we expecting to lock? confidential doc leaks? an alien cult manifesto? communist propaganda? (its a joke but you get what i mean).

It is a locking function, for content the user can rewrite or already has in their system. If the content of the brew is so bad it is deserving to lock the document, why not delete it entirely?

I understand the idea behind the locking, not so sure of its usefulness. Better to have it and not use it than need it and not have it, for sure.

@ericscheid
Copy link
Collaborator

I don't think we have any way to push notifications out like that without creating a live socket connection for every editing session, which adds some complexity. Something like that would be a whole separate project, but eventually that might be something we add.

We could throw the notification upon the first (auto)save of that brew, based on info received then.

@calculuschild
Copy link
Member

That's a good point. Autosave creates regular periodic requests which could be used this way.

@ericscheid
Copy link
Collaborator

It is a locking function, for content the user can rewrite or already has in their system. If the content of the brew is so bad it is deserving to lock the document, why not delete it entirely?

There might be a copyright claim against an image in a brew. Deleting the entire brew would be an overreaction.

@5e-Cleric
Copy link
Member

It is a locking function, for content the user can rewrite or already has in their system. If the content of the brew is so bad it is deserving to lock the document, why not delete it entirely?

There might be a copyright claim against an image in a brew. Deleting the entire brew would be an overreaction.

locking the brew for a copyright claim seems too much for me, however, the "get that image out or we will lock it" would seem more appropiate to me. IDK

@calculuschild
Copy link
Member

calculuschild commented Apr 11, 2024

It is a locking function, for content the user can rewrite or already has in their system. If the content of the brew is so bad it is deserving to lock the document, why not delete it entirely?

There might be a copyright claim against an image in a brew. Deleting the entire brew would be an overreaction.

locking the brew for a copyright claim seems too much for me, however, the "get that image out or we will lock it" would seem more appropiate to me. IDK

In response to a DMCA notice, we are required by law to remove the offending content from public access until the issue is resolved. We as the hosting service are not liable for our users' copyright infringement unless we knowingly allow infringing content to remain accessible after receiving a notice.

@G-Ambatte
Copy link
Collaborator Author

1. Seeing this popup in action, it occurs to me that it behaves as kind of a mini error page. Would it make more sense to use the common "Error Page" for this message? And put the "click to unlock" button on there?

Rather than creating a new LockNotification component, we could instead throw, and include in that error some function to disable the lock temporarily.

2. The error should probably explain what being "locked" means. I.e. the brew is not visible via the share page, etc.

Currently the only fixed text is "BREW LOCKED", everything else comes from the lock.message field. It should be trivial to add some explanatory text.

3. So "Click to unlock" just takes you to the editor as normal, but doesn't actually lift the lock. Is there a better name for that button, like "CLICK TO CONTINUE TO THE EDITOR".

Should be a fairly simple change.

4. I think a better use for this kind of "popup" thing might be for a "submit to admins" thing. I'm picturing this:

* User tries to open their edit page, and hits the error page instead. This is where all the details are. "your brew is locked. Here are all the details, and what this means for you. etc. Click here to continue to the editor"

* User enters the editor, where this popup now appears with a smaller message: "Your brew is locked. (locking message). Once you have resolved this issue, click here to notify the administrators for review. (BUTTON "REQUEST UNLOCK"). Click (OTHER BUTTON) to temporarily hide this notification (will reappear when the page is next reloaded).

A method for requesting locks be removed will need to be created, which is intended to be added in a later PR, once functionality to actually add/remove/otherwise update locks has been created.

5. It might make sense to reuse (and maybe update) the "notificationPopup.jsx" component we already have instead of making a separate component element.

As NotificationPopup is a non-blocking and permanently clearable message, it has a different purpose to this notification; although I suspect it might work with some modifications.

@naturalcrit naturalcrit deleted a comment from github-actions bot Apr 20, 2024
@G-Ambatte
Copy link
Collaborator Author

image
"This is a test message from the database." is the lock.message property. All other text in the notification is currently fixed text and will appear in every lock notification.

The "REQUEST LOCK REMOVAL" button is currently attached to a stub function.
image

@G-Ambatte
Copy link
Collaborator Author

It occurs to me that we may want two messages - one for the ErrorPage, and one for the LockNotification. The general public needs only to know that the brew is locked, while the author needs to know the specifics of why it is locked... As such, an editMessage and a shareMessage might be appropriate.

@naturalcrit naturalcrit deleted a comment from github-actions bot Apr 20, 2024
@naturalcrit naturalcrit deleted a comment from github-actions bot Apr 20, 2024
@naturalcrit naturalcrit deleted a comment from github-actions bot Apr 20, 2024
@G-Ambatte G-Ambatte added 🔍 R4 - Reviewed - Fixed! Awaiting final review⭐ PR review comments addressed and removed 🔍 R3 - Reviewed - Awaiting Fixes 🔧 PR is okayed but needs fixes before merging labels Jun 6, 2024
@5e-Cleric
Copy link
Member

Looks good to me.

@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3382 June 6, 2024 22:51 Inactive
Stop the notification from covering the renderWarning when both are present
@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3382 June 6, 2024 23:02 Inactive
@G-Ambatte
Copy link
Collaborator Author

FYI the Lock Notification has both onClick and onCancel methods, so it can be cleared by pressing the ESC key. This calls the same function as clicking the "CONTINUE TO EDITOR" button.

@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3382 June 6, 2024 23:25 Inactive
@G-Ambatte
Copy link
Collaborator Author

G-Ambatte commented Jun 6, 2024

Final state of the PR:

When a modal (blocking notification) is shown, the rest of the page dims to indicate that it cannot be interacted with.
image

Regular pop ups (like Render Warning or Notification Popup) do not cause this dimming.

Currently, clicking "REQUEST LOCK REMOVAL" pops a "NYI" alert. Implementing this functionality is work for a future PR.
Clicking "CONTINUE TO EDITOR" (or pressing ESC) removes the modal pop up and allows normal access to the editor. However, this pop up will return every time the page is refreshed.
Possible work for a future PR: cause the Lock notification to reappear as a non-blocking notification on a relatively long timer (30 minutes, 60 minutes)

The content of the notification is fixed, with the exception of the LOCK REASON message pulled from the lock object on the brew itself (in the image, "This is the private message") which is only intended to only be visible to authors.

@5e-Cleric
Copy link
Member

5e-Cleric commented Jun 6, 2024

Possible work for a future PR: cause the Lock notification to reappear as a non-blocking notification on a relatively long timer (30 minutes, 60 minutes)

I disagree with this, if we lock a brew, the user first priority should be removing such content. This encourages that behaviour. Making it wait longer a longer time gives 0 advantages.

@G-Ambatte
Copy link
Collaborator Author

G-Ambatte commented Jun 6, 2024

Possible work for a future PR: cause the Lock notification to reappear as a non-blocking notification on a relatively long timer (30 minutes, 60 minutes)

I disagree with this, if we lock a brew, the user first priority should be removing such content. This encourages that behaviour. Making it wait longer a longer time gives 0 advantages.

This would be in addition to the existing blocking notification, so authors can't just clear the message and then never refresh the Edit page, and thus never be presented with the notification again. The timed notification would serve as an additional reminder, and potentially also provide an easier way to access the "REQUEST REVIEW" button once the required changes have been made.

@G-Ambatte G-Ambatte temporarily deployed to homebrewery-pr-3382 June 7, 2024 04:05 Inactive
@G-Ambatte
Copy link
Collaborator Author

I've run this through the Linter now, the only thing it picked up from these changes is an unused useState, which I have corrected in the latest commit.

@5e-Cleric
Copy link
Member

5e-Cleric commented Jun 7, 2024

This would be in addition to the existing blocking notification, so authors can't just clear the message and then never refresh the Edit page, and thus never be presented with the notification again. The timed notification would serve as an additional reminder, and potentially also provide an easier way to access the "REQUEST REVIEW" button once the required changes have been made.

Oh, then yeah, sure, great
Actually we might want to keep a floating button somewhere, with the "REQUEST REVIEW" option. I can imagine users complaining: i fixed the brew but i can't request review!

@calculuschild calculuschild temporarily deployed to homebrewery-pr-3382 June 7, 2024 15:29 Inactive
@calculuschild calculuschild merged commit f68af55 into naturalcrit:master Jun 7, 2024
2 checks passed
@calculuschild
Copy link
Member

Thanks @G-Ambatte ! 💯 ⭐ 🎉

@G-Ambatte G-Ambatte mentioned this pull request Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 - minor feature or not urgent Minor bugs or less-popular features 🔍 R4 - Reviewed - Fixed! Awaiting final review⭐ PR review comments addressed sub-epic Sub-task of an Epic
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

6 participants