Bug 1583849 - WIP Phabricator revisions not moved out of Draft state #1447
Conversation
| INFO("Moving from draft to needs-review"); | ||
| # Removing draft status | ||
| if ($is_new && $revision->status eq 'draft') { | ||
| INFO("Removing draft status"); |
There was a problem hiding this comment.
if the revision is currently draft+changes-planned, this will change it to request-review, which isn't what we want.
There was a problem hiding this comment.
Ok after more investigation I still feel like this change is the right way to go.
- When a revision is created the status is 'draft'.
- When moz-phab creates a WIP revision, the status is set to 'changes-planned'.
- When a revision is created with 'changes-planned' without ever being in the 'request-review' state, it an additional property on the revision, separate from the revision status, called '{"draft.broadcast":false}'. This means that no email notifications will be send as long as that is false.
- This is why the 'Draft' label is still displayed on a revision that has a status of 'changes-planned' for D1355 for example.
- The way to get set the 'draft.broadcast' value to true, is to request review on the revision (set status to 'request-review').
- Normally moz-phab will do this automatically with --wip is not provided.
- The BMO change on this PR will therefore only update the status if the current status is 'draft'. If moz-phab sets the status to 'changes-planned' using --wip, BMO feed daemon will just leave it as is and it will be up to the user or moz-phab to transistion it to 'request-review' status.
There was a problem hiding this comment.
The problem with this change is it doesn't address the bug in question.
As we've changed the meaning of Draft from Phabricator's default of work-in-progress to not-yet-processed-by-bugzilla we need to ensure the draft badge is never displayed once phabbugz has processed a revision.
Changing the revision's status to request-review it isn't a reflection of the user's intent that a WIP revision should be in a changes-planned state. I understand that right now that's the only way to clear the draft.broadcast property, but it's bound to be more confusing than leaving the draft badge visible.
I suspect the right thing to do is to remove the code inDifferentialRevisionViewController.php which displays the draft badge/tag when shouldBroadcast is false.
As the draft.broadcast property impacts a revision's visibility in the feed, if we don't set it to true then a revision won't be updated as the bug changes (eg. if a bug is made secure).
We might have to add a way to directly set draft.broadcast to true without setting the status to request-review.
| INFO("Moving from draft to needs-review"); | ||
| # Removing draft status | ||
| if ($is_new && $revision->status eq 'draft') { | ||
| INFO("Removing draft status"); |
There was a problem hiding this comment.
The problem with this change is it doesn't address the bug in question.
As we've changed the meaning of Draft from Phabricator's default of work-in-progress to not-yet-processed-by-bugzilla we need to ensure the draft badge is never displayed once phabbugz has processed a revision.
Changing the revision's status to request-review it isn't a reflection of the user's intent that a WIP revision should be in a changes-planned state. I understand that right now that's the only way to clear the draft.broadcast property, but it's bound to be more confusing than leaving the draft badge visible.
I suspect the right thing to do is to remove the code inDifferentialRevisionViewController.php which displays the draft badge/tag when shouldBroadcast is false.
As the draft.broadcast property impacts a revision's visibility in the feed, if we don't set it to true then a revision won't be updated as the bug changes (eg. if a bug is made secure).
We might have to add a way to directly set draft.broadcast to true without setting the status to request-review.
No description provided.