Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug 1583849 - WIP Phabricator revisions not moved out of Draft state #1447

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Bug 1583849 - WIP Phabricator revisions not moved out of Draft state #1447

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 3 additions & 7 deletions extensions/PhabBugz/lib/Feed.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,13 +519,9 @@ sub process_revision_change {
$attachment->update($timestamp);
}

# Set status to request-review if revision is new and
# in draft state and not changes-planned
if ($is_new
&& $revision->status ne 'changes-planned'
&& ($revision->is_draft && !$revision->hold_as_draft))
{
INFO("Moving from draft to needs-review");
# Removing draft status
if ($is_new && $revision->status eq 'draft') {
INFO("Removing draft status");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the revision is currently draft+changes-planned, this will change it to request-review, which isn't what we want.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok after more investigation I still feel like this change is the right way to go.

  1. When a revision is created the status is 'draft'.
  2. When moz-phab creates a WIP revision, the status is set to 'changes-planned'.
  3. 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.
  4. This is why the 'Draft' label is still displayed on a revision that has a status of 'changes-planned' for D1355 for example.
  5. The way to get set the 'draft.broadcast' value to true, is to request review on the revision (set status to 'request-review').
  6. Normally moz-phab will do this automatically with --wip is not provided.
  7. 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.

Copy link
Collaborator

@globau globau Oct 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$revision->set_status('request-review');
}

Expand Down