Disallow assigning a project author as the editor #2290
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
added 8 commits
September 17, 2024 11:40
The previous test case in test_assign_editor did not check that the project was assigned to the correct editor (it asserted that project.editor was true, not that project.editor equalled editor.) The previous test case in test_reassign_editor likewise did not check that the project was correctly reassigned. And indeed, test_reassign_editor was *not* correctly reassigning the project, because it did not include the required 'reassign_editor' parameter. Moreover, test_reassign_editor was not logging in as the assigned editor before attempting reassignment - in fact this currently works, but it isn't supposed to be allowed. Additionally, rearrange the code a little for readability, use refresh_from_db instead of duplicating earlier queries, and add additional precondition checks.
Reassigning a project should only be permitted for the current editor, not for everyone who has the project.change_activeproject permission. This function already implicitly assumes that request.user is the current editor: request.user is the one who is excluded from the query in ReassignEditorForm, and the log message implies that request.user is the current editor.
Assigning one of the project's authors as the editor should not be allowed. This form class uses separate 'project' and 'editor' fields (a single instance of the form is used in the submitted_projects page) so the two fields must be validated together in 'clean'.
Assigning one of the project's authors as the editor should not be allowed. Additionally, reassigning the project to its current editor shouldn't be allowed (the previous logic was "the project can't be reassigned to the person making the request", but what was really meant was "the project can't be reassigned to its current editor".) Handle both of these restrictions by adding a 'project' argument to the form constructor (keyword-only, to avoid confusion with 'user', which is being removed.)
AssignEditorForm (in submitted_projects), and ReassignEditorForm (in submission_info), should not allow an author of the project to be assigned as its editor, even if that person has the can_edit_activeprojects permission.
In this page, the editor assignment form is hidden inside a "modal box", so unlike most forms in the site, we don't display errors alongside the form (a new AssignEditorForm is instantated later in this function.) Instead, display the errors as messages at the top of the page.
The reassignment form is hidden in an "inactive tab", so any form error messages are not immediately obvious. Add a message at the top of the page so it is clear that the form did not take effect.
Adding the project's editor as an author should not be allowed. (This would normally only be possible if the project is in NEEDS_RESUBMISSION state.) It would be better to have a way of completely un-assigning the editor, but the current system doesn't have a way to handle that (AssignEditorForm requires the project to be in NEEDS_ASSIGNMENT state.)
bemoody
force-pushed
the
bm/editor-assignment
branch
from
9416117
to
cba2580
Compare
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.
We want to avoid assigning a project author as the editor, to avoid the appearance of a conflict of interest, and to ensure that there's always another set of eyes on the project before it's published.
I think these changes should cover all the possibilities, but it's been a couple of months since I wrote this, so take a careful look. :)
When project is "awaiting editor assignment", trying to assign it to one of the authors should give an error.
When project is "awaiting decision", trying to reassign it to one of the authors should give an error.
When project is "awaiting author revisions", if the submitting author sends an author invitation to the editor, and they try to accept that invitation, that should give an error. (They should be able to reassign to another editor and then accept the invitation.)