-
Notifications
You must be signed in to change notification settings - Fork 212
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
Enable soft-locking functionality for media reports to assist simultaneous moderators #4374
Conversation
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.
Requesting changes due to the keys
issue. Otherwise, this LGTM, and generally I am keen on the HTTP request approach! At first, I was sceptical of using HTTP requests to modify the lock, but I see how that accomplishes some interesting things, namely allowing us to detect navigation away even if the moderator closes the Django admin without navigating to another admin page first. It's cool to release the lock in this way.
However, in addition to the keys
issue, outlined in my other comment, I think if we used a polling-send approach, we can also fix the tab switching issue, make it possible for a moderator to have a lock on multiple reports (if they have them open), and remove the need to ever manually release the locks.
Basically, this approach would use a very short expiration (lets say 10 seconds), and have the JavaScript send a polling request every 5 seconds. Each request sets the expiration for that work a further 10 seconds into the future. We can prune at this point as well. Setting a lock on one work does not release locks on other works for that same moderator, we only prune expired locks.
I want to emphasise that I think the HTTP request approach is really cool, and it's not one I would have thought of, I think. The use of keys
and the improvements to be had by using a poll-send-expire approach instead of a lock/release approach notwithstanding, I was ready to "LGTM, let's merge this despite some nitpicks" on this PR!
Closing in favour of a much more holistic implementation in #4386. |
Fixes
Fixes #3639 by @sarayourfriend
Description
This PR
/lock
and/unlock
, in response to events (including but not limited to loading the change page, switching between tabs or closing the browser)Testing Instructions
This is best tested using two browsers, or two profiles in separate windows of the same browser, or one regular and one incognito session in the same profile of the same browser.
deploy
anddeploy
.moderator
anddeploy
. (This was weird when I first learned about it, I expected the password to be the same as the username.)Test list highlights
...continued
Test other moderator message
...continued
Test page visibility changes
...continued
Checklist
Update index.md
).main
) or a parent feature branch.just catalog/generate-docs
for catalogPRs) or the media properties generator (
just catalog/generate-docs media-props
for the catalog or
just api/generate-docs
for the API) where applicable.Developer Certificate of Origin
Developer Certificate of Origin