Skip to content

Commit 943f25f

Browse files
globauglobau
committed
Bug 1564166 - moz-phab needs to explicitly request a review of a revision when leaving the WIP state; r=zalun
Remove redundant api call by consolidating transactions, and add `review-request` transaction if required. Differential Revision: https://phabricator.services.mozilla.com/D38160
1 parent 829d0cf commit 943f25f

4 files changed

Lines changed: 75 additions & 164 deletions

File tree

‎moz-phab‎

Lines changed: 37 additions & 81 deletions
Original file line numberDiff line numberDiff line change
@@ -818,42 +818,6 @@ class ConduitAPI:
818818

819819
return diff_dict
820820

821-
def update_phabricator_commit_summary(self, commit):
822-
"""Send a new commit summary and description to Phabricator.
823-
824-
Args:
825-
commit: A VCS commit data dict to use for the call args.
826-
827-
"""
828-
# From https://phabricator.services.mozilla.com/api/differential.revision.edit
829-
#
830-
# Example call format we are aiming for:
831-
#
832-
# $ echo '{
833-
# "transactions": [
834-
# {
835-
# "type": "title",
836-
# "value": "Remove unnecessary branch statement"
837-
# }
838-
# {
839-
# "type": "summary",
840-
# "value": "Blah"
841-
# }
842-
# ],
843-
# "objectIdentifier": "D8095"
844-
# }' | arc call-conduit \
845-
# --conduit-uri https://phabricator.services.mozilla.com/ \
846-
# --conduit-token <conduit-token> differential.revision.edit
847-
logger.debug("updating revision title and summary in Phabricator")
848-
api_call_args = build_api_call_to_update_commit_title_and_summary(commit)
849-
try:
850-
self.call("differential.revision.edit", api_call_args)
851-
except (ConduitAPIError, CommandError) as err:
852-
logger.warn(
853-
"Error attempting to update revision summary and title in Phabricator"
854-
)
855-
logger.warn("Error was: %s", err)
856-
857821
def get_successor_phids(self, phid, include_abandoned=False):
858822
return self.get_related_phids(
859823
phid, relation="child", include_abandoned=include_abandoned
@@ -2892,31 +2856,32 @@ def update_commits_from_args(commits, args):
28922856
update_commit_title_previews(commits)
28932857

28942858

2895-
def build_api_call_to_update_commit_title_and_summary(commit):
2896-
"""Build a Conduit API transaction to update a commit title and summary.
2859+
def update_revision_description(transactions, commit, revision):
2860+
# Appends differential.revision.edit transaction(s) to `transactions` if
2861+
# updating the commit title and/or summary is required.
28972862

2898-
See https://phabricator.services.mozilla.com/api/differential.revision.edit for
2899-
the returned JSON object format.
2863+
if commit["title"] != revision["fields"]["title"]:
2864+
transactions.append(dict(type="title", value=commit["title"]))
29002865

2901-
Args:
2902-
commit: A VCS commit data dict to use for the call args.
2903-
2904-
Returns:
2905-
A JSON dict that can be passed to a Conduit differential.revision.edit API call.
2906-
"""
29072866
# The Phabricator API will refuse the new summary value if we include the
29082867
# "Differential Revision:" keyword in the summary body.
2909-
transactions = [
2910-
dict(type="title", value=commit["title"]),
2911-
dict(
2912-
type="summary",
2913-
value=strip_differential_revision(commit["body"]).encode("utf8"),
2914-
),
2915-
]
2916-
return {"transactions": transactions, "objectIdentifier": commit["rev-id"]}
2868+
local_body = strip_differential_revision(commit["body"]).strip()
2869+
remote_body = strip_differential_revision(revision["fields"]["summary"]).strip()
2870+
if local_body != remote_body:
2871+
transactions.append(dict(type="summary", value=local_body))
2872+
2873+
2874+
def update_revision_bug_id(transactions, commit, revision):
2875+
# Appends differential.revision.edit transaction(s) to `transactions` if
2876+
# updating the commit bug-id is required.
2877+
if commit["bug-id"] != revision["fields"]["bugzilla.bug-id"]:
2878+
transactions.append(dict(type="bugzilla.bug-id", value=commit["bug-id"]))
2879+
29172880

2881+
def update_revision_reviewers(transactions, commit):
2882+
# Appends differential.revision.edit transaction(s) to `transactions` to
2883+
# set the reviewers.
29182884

2919-
def build_transaction_to_update_reviewers(commit):
29202885
# Add reviewers
29212886
all_reviewers = commit["reviewers"]["request"] + commit["reviewers"]["granted"]
29222887
# preload all reviewers
@@ -2928,7 +2893,10 @@ def build_transaction_to_update_reviewers(commit):
29282893
blocking_phid = [
29292894
"blocking(%s)" % user["phid"] for user in conduit.get_users(blocking)
29302895
]
2931-
return [dict(type="reviewers.set", value=reviewers_phid + blocking_phid)]
2896+
2897+
transactions.extend(
2898+
[dict(type="reviewers.set", value=reviewers_phid + blocking_phid)]
2899+
)
29322900

29332901

29342902
def submit(repo, args):
@@ -3104,37 +3072,25 @@ def submit(repo, args):
31043072

31053073
if is_update:
31063074
with wait_message("Updating D%s.." % commit["rev-id"]):
3107-
try:
3108-
conduit.update_phabricator_commit_summary(commit)
3109-
except Exception as e:
3110-
logger.warning(
3111-
"Warning: failed to update commit summary in Phabricator"
3112-
)
3113-
logger.debug(e)
3114-
3115-
# Update bug id if different
3116-
revision = conduit.get_revisions(ids=[int(commit["rev-id"])])[0]
31173075
transactions = []
3118-
if revision["fields"]["bugzilla.bug-id"] != commit["bug-id"]:
3119-
transactions.append(
3120-
dict(type="bugzilla.bug-id", value=commit["bug-id"])
3121-
)
3076+
revision = conduit.get_revisions(ids=[int(commit["rev-id"])])[0]
3077+
3078+
update_revision_description(transactions, commit, revision)
3079+
update_revision_bug_id(transactions, commit, revision)
31223080

31233081
# Add reviewers only if revision lacks them
3124-
if has_commit_reviewers and not args.wip:
3125-
# Add reviewers only if revision lacks them
3126-
if not existing_reviewers:
3127-
transactions.extend(
3128-
build_transaction_to_update_reviewers(commit)
3129-
)
3082+
if not args.wip and has_commit_reviewers and not existing_reviewers:
3083+
update_revision_reviewers(transactions, commit)
3084+
transactions.append(dict(type="request-review"))
31303085

31313086
if transactions:
3132-
api_call_args = {
3133-
"objectIdentifier": "D%s" % commit["rev-id"],
3134-
"transactions": transactions,
3135-
}
31363087
arc_call_conduit(
3137-
"differential.revision.edit", api_call_args, repo.path
3088+
"differential.revision.edit",
3089+
{
3090+
"objectIdentifier": "D%s" % commit["rev-id"],
3091+
"transactions": transactions,
3092+
},
3093+
repo.path,
31383094
)
31393095

31403096
# Append/replace div rev url to/in commit description.

‎tests/conftest.py‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -142,15 +142,13 @@ def in_process(monkeypatch, safe_environ, request):
142142
# to make test debugging easier.
143143
def reraise(*args, **kwargs):
144144
t, v, tb = sys.exc_info()
145-
raise t, v, tb
145+
raise (t, v, tb)
146146

147147
monkeypatch.setattr(sys, "exit", reraise)
148148

149149
# Disable uploading a new commit title and summary to Phabricator. This operation
150150
# is safe to skip and doing so makes it easier to test other conduit call sites.
151-
monkeypatch.setattr(
152-
mozphab.ConduitAPI, "update_phabricator_commit_summary", mock.MagicMock()
153-
)
151+
monkeypatch.setattr(mozphab, "update_revision_description", mock.MagicMock())
154152

155153
def arc_ping(self, *args):
156154
return True

‎tests/test_integration-hg.py‎

Lines changed: 4 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -180,29 +180,14 @@ def test_submit_update_no_new_reviewers(in_process, hg_repo_path):
180180
"differential.revision.edit",
181181
{
182182
"objectIdentifier": "D123",
183-
"transactions": [{"type": "reviewers.set", "value": ["PHID-USER-1"]}],
183+
"transactions": [
184+
{"type": "reviewers.set", "value": ["PHID-USER-1"]},
185+
{"type": "request-review"},
186+
],
184187
},
185188
mock.ANY,
186189
)
187190
check_call_by_line.assert_called_once()
188-
# [
189-
# mock.ANY, # arc command with full path
190-
# '--trace',
191-
# 'diff',
192-
# '--base',
193-
# 'arc:this',
194-
# '--allow-untracked',
195-
# '--no-amend',
196-
# '--no-ansi',
197-
# '--message-file',
198-
# mock.ANY, # temp message file
199-
# '--message',
200-
# 'Revision updated.',
201-
# '--update',
202-
# '123'
203-
# ],
204-
# cwd=mock.ANY,
205-
# never_log=True
206191

207192

208193
def test_submit_update_bug_id(in_process, hg_repo_path):

‎tests/test_submit.py‎

Lines changed: 32 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -661,66 +661,40 @@ def __init__(self, reviewer=None, blocker=None, bug=None):
661661

662662

663663
class TestUpdateCommitSummary(unittest.TestCase):
664-
@mock.patch("mozphab.ConduitAPI.call")
665-
def test_update_summary_conduit_args(self, call_conduit):
666-
mozphab.ARC = ["arc"]
667-
c = commit(rev_id="D123")
668-
call_conduit.return_value = {}
669-
670-
mozphab.conduit.update_phabricator_commit_summary(c)
671-
672-
call_conduit.assert_called_once_with(
673-
"differential.revision.edit",
674-
{
675-
"objectIdentifier": "D123",
676-
"transactions": [
677-
{"type": "title", "value": ""},
678-
{"type": "summary", "value": ""},
679-
],
680-
},
664+
def test_update_revision_description(self):
665+
c = commit(
666+
rev_id="D123",
667+
title="hi!",
668+
body="hello! µ-benchmarks are a thing.\n\n"
669+
"Differential Revision: http://phabricator.test/D123",
681670
)
671+
r = dict(fields=dict(title="", summary=""))
672+
t = []
682673

683-
def test_build_api_call_to_update_title_and_summary(self):
684-
# From https://phabricator.services.mozilla.com/api/differential.revision.edit
685-
#
686-
# Example call format we are aiming for:
687-
#
688-
# $ echo '{
689-
# "transactions": [
690-
# {
691-
# "type": "title",
692-
# "value": "Remove unnecessary branch statement"
693-
# },
694-
# {
695-
# "type": "summary",
696-
# "value": "Blah"
697-
# }
698-
# ],
699-
# "objectIdentifier": "D8095"
700-
# }' | arc call-conduit --conduit-uri \
701-
# https://phabricator.services.mozilla.com/ \
702-
# --conduit-token <conduit-token> differential.revision.edit
674+
expected = [
675+
dict(type="title", value="hi!"),
676+
dict(type="summary", value="hello! µ-benchmarks are a thing."),
677+
]
678+
mozphab.update_revision_description(t, c, r)
703679

680+
self.assertListEqual(t, expected)
681+
682+
def test_update_revision_description_no_op(self):
704683
c = commit(
705684
rev_id="D123",
706685
title="hi!",
707-
body=u"hello! µ-benchmarks are a thing.\n\n"
708-
"Differential Revision: http://phabricator.test/D123",
686+
body="hello!\n\nDifferential Revision: http://phabricator.test/D123",
709687
)
710-
expected_json = {
711-
"transactions": [
712-
{"type": "title", "value": "hi!"},
713-
{"type": "summary", "value": "hello! µ-benchmarks are a thing."},
714-
],
715-
"objectIdentifier": "D123",
716-
}
688+
r = dict(fields=dict(title="hi!", summary="hello!\n\n"))
689+
t = []
717690

718-
api_call_args = mozphab.build_api_call_to_update_commit_title_and_summary(c)
691+
expected = []
692+
mozphab.update_revision_description(t, c, r)
719693

720-
self.assertDictEqual(expected_json, api_call_args)
694+
self.assertListEqual(t, expected)
721695

722696
@mock.patch("mozphab.ConduitAPI.get_users")
723-
def test_biuild_transaction_to_update_reviewers(self, m_get_users):
697+
def test_update_revision_reviewers(self, m_get_users):
724698
# From https://phabricator.services.mozilla.com/api/differential.revision.edit
725699
#
726700
# Example call format we are aiming for:
@@ -746,18 +720,16 @@ def test_biuild_transaction_to_update_reviewers(self, m_get_users):
746720
[dict(phid="PHID-USER-2"), dict(phid="PHID-USER-3")],
747721
)
748722
c = commit(rev_id="123", reviewers=[["alice", "bob!"], ["frankie!"]])
749-
expected_json = [
750-
{
751-
"type": "reviewers.set",
752-
"value": [
753-
"PHID-USER-1",
754-
"blocking(PHID-USER-2)",
755-
"blocking(PHID-USER-3)",
756-
],
757-
}
723+
t = []
724+
725+
expected = [
726+
dict(
727+
type="reviewers.set",
728+
value=["PHID-USER-1", "blocking(PHID-USER-2)", "blocking(PHID-USER-3)"],
729+
)
758730
]
731+
mozphab.update_revision_reviewers(t, c)
759732

760-
api_call_args = mozphab.build_transaction_to_update_reviewers(c)
761733
self.assertEqual(
762734
m_get_users.call_args_list,
763735
[
@@ -766,7 +738,7 @@ def test_biuild_transaction_to_update_reviewers(self, m_get_users):
766738
mock.call(["bob", "frankie"]),
767739
],
768740
)
769-
self.assertEqual(expected_json, api_call_args)
741+
self.assertListEqual(t, expected)
770742

771743
def test_parse_api_response_with_no_problems(self):
772744
# Response comes from running:

0 commit comments

Comments
 (0)