Closed Bug 1700712 Opened 3 years ago Closed 3 years ago

Pushing patch to resolve "approval with nits" causes "Approved" status to be lost

Categories

(Conduit :: moz-phab, defect)

defect

Tracking

(Not tracked)

RESOLVED DUPLICATE of bug 1700485

People

(Reporter: mhentges, Unassigned)

References

(Regression)

Details

I discovered this issue when I saw alwu's comment in the Phabricator.

There's a common workflow in-tree where a reviewer approves a patch, but still suggests some nitpicks. However, they're still satisfied with the overall patch, and are content with it landing (with or without the nits being resolved).

Unfortunately, with the new 0.1.96 moz-phab patch, updating an "Approved with nits" patch always overwrites the status with either "Needs Reviewer" or "Changes planned":

  • If a reviewer had been provided in the commit message (r?reviewer), updating an "Approved" patch causes it to change back to "Needs Review".
  • If reviewers had been assigned in Phabricator (the commit message doesn't have reviewers), the patch had been "Accepted" (with nits), then updated, it gets changed to "Changes Planned".

I'm not sure what heuristic should be used here to properly detect when to change the Phabricator patch status, but FWIW the recent changes affect some common workflows.

Regressed by: 1610403
See Also: → 1700179
Summary: Pushing patch to resolve "approval with nits" causes patch status to change to "needs review" or "changes planned" → Pushing patch to resolve "approval with nits" causes "Approved" status to be lost

Yeah, this current flow is... not great for people with a non-trivial number of review requests...

Note that it's not even just "with nits", AIUI this means even rebasing a patch will cause reviewers to be re-flagged.

This seems like a duplicate of bug 1700485.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.