Fix deleted work prevents reverting a user's works #9013
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cdrini
approved these changes
Apr 3, 2024
There was a problem hiding this comment.
One small comment! We discussed the possible performance implications of this.
Although this will take longer, we think it shouldn't have a huge impact on most reversion since they are unlikely to be very large. In the larger case, it should I believe just be slower.
We'll monitor and ask for feedback from librarians; if they observe this is erroring when the revert set is large, we have two mitigations:
- Wrap the revert() in a try/except ; this will delay the problem, but possible enough for it to effectively not be a problem
- Do the revert in batches.
cdrini
reviewed
May 1, 2024
… work or edition from raising an error.
cdrini
force-pushed
the
4045/bug/deleted-work-prevents-edit-reversion
branch
from
246e0cd to
605246a
Compare
Also moved the main revert methods into helpers for easier testing.
cdrini
force-pushed
the
4045/bug/deleted-work-prevents-edit-reversion
branch
from
605246a to
e8b8b78
Compare
…ents-edit-reversion
Adjusted the 'action-type' of adding/editing the books, to accurately reflect the actions being done.
cdrini
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #4045
Currently, there is an exploit in our mass-revert feature that spammers are abusing. When a user is blocked and all of their edits are reverted en masse, the process will stall if any of their edits reference deleted works. This R will causes the reversion process to instead skip past these defunct edits, allowing the overall process to succeed.
Technical
Currently, the problem arises in
infogami.infobase.writequery.SaveProcesser.process_value, where type-checking occurs for the various keys involved in saving changes. Rather than altering infogami's functionality in this manner, the implemented solution instead checks each key referenced in each changeset for deleted keys earlier in the process, back inopenlibrary.plugins.admin.code.people_view.POST_block_account_and_revert, and only passes the changeset ids without these references to be reverted.This could theoretically result in a problem, if a user with a sufficiently large edit history is being reverted, due to the large amount of information being fetched. Currently, it is being investigated if this is likely to happen.
Testing
The first step for verifying this PR is to create two separate user accounts. Each user account should create a work, and an edition. Then, each user should edit the other user's work. Lastly, each user should then be blocked, and have their edits reverted.
Stakeholders
@cdrini