Let reviewers correct a task's completion status and edit its bundle - #1283
Open
CollinBeczak wants to merge 1 commit into
Open
CollinBeczak wants to merge 1 commit into
CollinBeczak wants to merge 1 commit into
Conversation
The Completion widget was added to the Review workspace on the frontend
(maproulette3#2846) but nothing landed here to support it, so every attempt
failed. The frontend reused PUT /task/:id/review/:status with newTaskStatus,
which routes through TaskDAL.setTaskStatus. That only permits a status reset
when completedBy == user.id, so a reviewer always tripped "Invalid task status
supplied". Forcing it through anyway would have been wrong regardless: that
path sets completed_by to the caller and credits their score and leaderboard,
making the reviewer the task's mapper, and reusing the review endpoint writes
a task_review_history row and resets reviewed_by for what is only a status
correction.
Add a dedicated PUT /task/:id/completionStatus/:status instead. It requires
the caller to hold the review claim (or be a super user), writes only
tasks.status, and leaves completed_by, mapped_on and the whole review row
alone. The status_actions row and the score rollback/credit are attributed to
the original mapper so leaderboard standings and user metrics stay with them,
while the reviewer is recorded in the actions audit log by the controller. The
review lock is deliberately not released so reviewing continues, and bundle
membership is resolved server-side so every member moves together.
Reviewers also could not edit the bundle of a task they were reviewing. Two
things blocked it:
- lockBundle always refreshed with is_review_claim = false, demoting the
reviewer's claim to an ordinary edit lock. That subjected them to the
one-edit-lock-per-user invariant - so an unrelated lock elsewhere returned
409 Conflict - and destroyed the claim the review endpoints key off of.
A refresh of a row we already hold as a review claim now stays one.
- TaskBundleService required super user or bundle owner. The holder of the
review claim is now accepted too, for update/unbundle/delete alike, since
the client deletes the bundle when the last member is removed.
|
CollinBeczak
marked this pull request as ready for review
September 30, 2026 19:03
This branch has not been deployed
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
The Completion widget was added to the Review workspace on the frontend (maproulette3#2846) but nothing landed here to support it, so every attempt failed. The frontend reused PUT /task/:id/review/:status with newTaskStatus, which routes through TaskDAL.setTaskStatus. That only permits a status reset when completedBy == user.id, so a reviewer always tripped "Invalid task status supplied". Forcing it through anyway would have been wrong regardless: that path sets completed_by to the caller and credits their score and leaderboard, making the reviewer the task's mapper, and reusing the review endpoint writes a task_review_history row and resets reviewed_by for what is only a status correction.
Add a dedicated PUT /task/:id/completionStatus/:status instead. It requires the caller to hold the review claim (or be a super user), writes only tasks.status, and leaves completed_by, mapped_on and the whole review row alone. The status_actions row and the score rollback/credit are attributed to the original mapper so leaderboard standings and user metrics stay with them, while the reviewer is recorded in the actions audit log by the controller. The review lock is deliberately not released so reviewing continues, and bundle membership is resolved server-side so every member moves together.
Reviewers also could not edit the bundle of a task they were reviewing. Two things blocked it:
lockBundle always refreshed with is_review_claim = false, demoting the reviewer's claim to an ordinary edit lock. That subjected them to the one-edit-lock-per-user invariant - so an unrelated lock elsewhere returned 409 Conflict - and destroyed the claim the review endpoints key off of. A refresh of a row we already hold as a review claim now stays one.
TaskBundleService required super user or bundle owner. The holder of the review claim is now accepted too, for update/unbundle/delete alike, since the client deletes the bundle when the last member is removed.