launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #22251
[Merge] lp:~cjwatson/launchpad/revamp-register-merge into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/revamp-register-merge into lp:launchpad.
Commit message:
Clean up the {Branch,GitRef}:+register-merge UI slightly.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/revamp-register-merge/+merge/341551
The commit message is now offered before the description, and filling in a description that duplicates the commit message is gently discouraged; the arguably rather pointless "Extra options" expander is gone; and I've cleaned up some hard-to-understand text.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/revamp-register-merge into lp:launchpad.
=== modified file 'lib/lp/code/browser/branch.py'
--- lib/lp/code/browser/branch.py 2017-11-06 09:32:45 +0000
+++ lib/lp/code/browser/branch.py 2018-03-16 21:32:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Branch views."""
@@ -1142,23 +1142,25 @@
class RegisterProposalSchema(Interface):
"""The schema to define the form for registering a new merge proposal."""
target_branch = Choice(
- title=_('Target Branch'),
+ title=_('Target branch'),
vocabulary='Branch', required=True, readonly=True,
description=_(
"The branch that the source branch will be merged into."))
prerequisite_branch = Choice(
- title=_('Prerequisite Branch'),
+ title=_('Prerequisite branch'),
vocabulary='Branch', required=False, readonly=False,
description=_(
'A branch that should be merged before this one. (Its changes'
' will not be shown in the diff.)'))
comment = Text(
- title=_('Description of the Change'), required=False,
+ title=_('Description of the change'), required=False,
description=_('Describe what changes your branch introduces, '
'what bugs it fixes, or what features it implements. '
- 'Ideally include rationale and how to test.'))
+ 'Ideally include rationale and how to test. '
+ 'You do not need to repeat information from the commit '
+ 'message here.'))
reviewer = copy_field(
ICodeReviewVoteReference['reviewer'], required=False)
@@ -1182,6 +1184,7 @@
for_input = True
custom_widget('target_branch', TargetBranchWidget)
+ custom_widget('commit_message', TextAreaWidget, cssClass='comment-text')
custom_widget('comment', TextAreaWidget, cssClass='comment-text')
page_title = label = 'Propose branch for merging'
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2017-11-06 09:32:45 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2018-03-16 21:32:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Views, navigation and actions for BranchMergeProposals."""
@@ -777,7 +777,7 @@
"""The description as widget HTML."""
mp = self.context
description = IBranchMergeProposal['description']
- title = "Description of the Change"
+ title = "Description of the change"
return TextAreaEditorWidget(
mp, description, title, edit_view='+edit-description')
@@ -793,7 +793,7 @@
"""The commit message as widget HTML."""
mp = self.context
commit_message = IBranchMergeProposal['commit_message']
- title = "Commit Message"
+ title = "Commit message"
return TextAreaEditorWidget(
mp, commit_message, title, edit_view='+edit-commit-message')
=== modified file 'lib/lp/code/browser/gitref.py'
--- lib/lp/code/browser/gitref.py 2016-11-11 14:57:42 +0000
+++ lib/lp/code/browser/gitref.py 2018-03-16 21:32:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2015-2016 Canonical Ltd. This software is licensed under the
+# Copyright 2015-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Git reference views."""
@@ -198,27 +198,32 @@
description=_("The repository that the source will be merged into."))
target_git_path = TextLine(
- title=_("Target reference path"), required=True, readonly=True,
+ title=_("Target branch"), required=True, readonly=True,
description=_(
- "The reference within the target repository that the source will "
+ "The branch within the target repository that the source will "
"be merged into."))
prerequisite_git_repository = Choice(
title=_("Prerequisite repository"),
vocabulary="GitRepository", required=False, readonly=True,
- description=_("The repository that the source will be merged into."))
+ description=_(
+ "A repository containing a branch that should be merged before "
+ "this one. (Its changes will not be shown in the diff.)"))
prerequisite_git_path = TextLine(
- title=_("Prerequisite reference path"), required=False, readonly=True,
+ title=_("Prerequisite branch"), required=False, readonly=True,
description=_(
- "The reference within the target repository that the source will "
- "be merged into."))
+ "A branch within the prerequisite repository that should be "
+ "merged before this one. (Its changes will not be shown in the "
+ "diff.)"))
comment = Text(
- title=_('Description of the Change'), required=False,
+ title=_('Description of the change'), required=False,
description=_('Describe what changes your branch introduces, '
'what bugs it fixes, or what features it implements. '
- 'Ideally include rationale and how to test.'))
+ 'Ideally include rationale and how to test. '
+ 'You do not need to repeat information from the commit '
+ 'message here.'))
reviewer = copy_field(
ICodeReviewVoteReference['reviewer'], required=False)
@@ -243,6 +248,7 @@
for_input = True
custom_widget('target_git_repository', TargetGitRepositoryWidget)
+ custom_widget('commit_message', TextAreaWidget, cssClass='comment-text')
custom_widget('comment', TextAreaWidget, cssClass='comment-text')
page_title = label = 'Propose for merging'
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2018-02-01 18:38:24 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2018-03-16 21:32:39 +0000
@@ -232,7 +232,7 @@
"""Tests for `BranchMergeProposalMergedView` for Bazaar."""
arbitrary_revisions = (1, 2, 42)
- merged_revision_text = 'Merged Revision Number'
+ merged_revision_text = 'Merged revision number'
def makeBranchMergeProposal(self):
return self.factory.makeBranchMergeProposal()
@@ -250,7 +250,7 @@
"""Tests for `BranchMergeProposalMergedView` for Git."""
arbitrary_revisions = ("0" * 40, "1" * 40, "2" * 40)
- merged_revision_text = 'Merged Revision ID'
+ merged_revision_text = 'Merged revision ID'
def makeBranchMergeProposal(self):
return self.factory.makeBranchMergeProposalForGit()
@@ -1261,9 +1261,9 @@
text = self.getMainText(bmp, '+resubmit')
expected = (
'Resubmit proposal to merge.*'
- 'Source Branch:.*'
- 'Target Branch:.*'
- 'Prerequisite Branch:.*'
+ 'Source branch:.*'
+ 'Target branch:.*'
+ 'Prerequisite branch:.*'
'Description.*'
'Start afresh.*')
self.assertTextMatchesExpressionIgnoreWhitespace(expected, text)
@@ -1289,11 +1289,11 @@
text = self.getMainText(bmp, '+resubmit')
expected = (
'Resubmit proposal to merge.*'
- 'Source Git Repository:.*'
+ 'Source Git repository:.*'
'Source Git branch path:.*'
- 'Target Git Repository:.*'
+ 'Target Git repository:.*'
'Target Git branch path:.*'
- 'Prerequisite Git Repository:.*'
+ 'Prerequisite Git repository:.*'
'Prerequisite Git branch path:.*'
'Description.*'
'Start afresh.*')
@@ -1303,7 +1303,7 @@
"""Proposals can be resubmitted using the browser."""
bmp = self.factory.makeBranchMergeProposalForGit(registrant=self.user)
browser = self.getViewBrowser(bmp, '+resubmit')
- browser.getControl('Commit Message').value = 'dribble'
+ browser.getControl('Commit message').value = 'dribble'
browser.getControl('Description').value = 'flibble'
browser.getControl('Resubmit').click()
with person_logged_in(self.user):
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py 2017-11-24 17:22:34 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py 2018-03-16 21:32:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2017 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""The interface for branch merge proposals."""
@@ -136,12 +136,12 @@
source_branch = exported(
ReferenceChoice(
- title=_('Source Branch'), schema=IBranch, vocabulary='Branch',
+ title=_('Source branch'), schema=IBranch, vocabulary='Branch',
required=False, readonly=True,
description=_("The branch that has code to land.")))
source_git_repository = exported(
ReferenceChoice(
- title=_('Source Git Repository'), schema=IGitRepository,
+ title=_('Source Git repository'), schema=IGitRepository,
vocabulary='GitRepository', required=False, readonly=True,
description=_("The Git repository that has code to land.")))
source_git_path = exported(
@@ -157,13 +157,13 @@
target_branch = exported(
ReferenceChoice(
- title=_('Target Branch'),
+ title=_('Target branch'),
schema=IBranch, vocabulary='Branch', required=False, readonly=True,
description=_(
"The branch that the source branch will be merged into.")))
target_git_repository = exported(
ReferenceChoice(
- title=_('Target Git Repository'), schema=IGitRepository,
+ title=_('Target Git repository'), schema=IGitRepository,
vocabulary='GitRepository', required=False, readonly=True,
description=_(
"The Git repository that the source branch will be merged "
@@ -182,7 +182,7 @@
prerequisite_branch = exported(
ReferenceChoice(
- title=_('Prerequisite Branch'),
+ title=_('Prerequisite branch'),
schema=IBranch, vocabulary='Branch', required=False,
readonly=True, description=_(
"The branch that the source branch branched from. "
@@ -190,7 +190,7 @@
"leave this field blank.")))
prerequisite_git_repository = exported(
ReferenceChoice(
- title=_('Prerequisite Git Repository'), schema=IGitRepository,
+ title=_('Prerequisite Git repository'), schema=IGitRepository,
vocabulary='GitRepository', required=False, readonly=True,
description=_(
"The Git repository containing the branch that the source "
@@ -284,14 +284,14 @@
commit_message = exported(
Summary(
- title=_("Commit Message"), required=False,
+ title=_("Commit message"), required=False,
description=_("The commit message that should be used when "
"merging the source branch."),
strip_text=True))
merged_revno = exported(
Int(
- title=_("Merged Revision Number"), required=False,
+ title=_("Merged revision number"), required=False,
readonly=True,
description=_(
"The revision number on the target branch which contains the "
@@ -299,7 +299,7 @@
merged_revision_id = exported(
TextLine(
- title=_("Merged Revision ID"), required=False, readonly=True,
+ title=_("Merged revision ID"), required=False, readonly=True,
description=_(
"The revision ID on the target branch which contains the "
"merge from the source branch (currently Git only).")))
@@ -310,7 +310,7 @@
date_merged = exported(
Datetime(
- title=_('Date Merged'), required=False,
+ title=_('Date merged'), required=False,
readonly=True,
description=_("The date that the source branch was merged into "
"the target branch")))
@@ -322,7 +322,7 @@
merge_reporter = exported(
PublicPersonChoice(
- title=_("Merge Reporter"), vocabulary="ValidPerson",
+ title=_("Merge reporter"), vocabulary="ValidPerson",
required=False, readonly=True,
description=_("The user that marked the branch as merged.")))
@@ -334,20 +334,20 @@
"supersedes.")))
superseded_by = exported(
Reference(
- title=_("Superseded By"), schema=Interface,
+ title=_("Superseded by"), schema=Interface,
required=False, readonly=True,
description=_(
"The branch merge proposal that supersedes this one.")))
date_created = exported(
Datetime(
- title=_('Date Created'), required=True, readonly=True))
+ title=_('Date created'), required=True, readonly=True))
date_review_requested = exported(
Datetime(
- title=_('Date Review Requested'), required=False, readonly=True))
+ title=_('Date review requested'), required=False, readonly=True))
date_reviewed = exported(
Datetime(
- title=_('Date Reviewed'), required=False, readonly=True))
+ title=_('Date reviewed'), required=False, readonly=True))
root_message_id = Text(
title=_('The email message id from the first message'),
required=False)
=== modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
--- lib/lp/code/mail/tests/test_branchmergeproposal.py 2015-09-11 12:20:23 +0000
+++ lib/lp/code/mail/tests/test_branchmergeproposal.py 2018-03-16 21:32:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2015 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2018 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for BranchMergeProposal mailings"""
@@ -502,7 +502,7 @@
"""Ensure the right delta is filled out if there is a change."""
job, subscriber = self.makeProposalUpdatedEmailJob()
self.assertEqual(
- 'Commit Message changed to:\n\nnew commit message\n\n'
+ 'Commit message changed to:\n\nnew commit message\n\n'
'Description changed to:\n\nchange description',
job.delta_text)
@@ -528,7 +528,7 @@
expected = dedent("""\
The proposal to merge %(source)s into %(target)s has been updated.
- Commit Message changed to:
+ Commit message changed to:
new commit message
=== modified file 'lib/lp/code/stories/branches/package-branch-merges-with-product-branches.txt'
--- lib/lp/code/stories/branches/package-branch-merges-with-product-branches.txt 2012-04-10 14:01:17 +0000
+++ lib/lp/code/stories/branches/package-branch-merges-with-product-branches.txt 2018-03-16 21:32:39 +0000
@@ -30,7 +30,7 @@
>>> print_errors(browser.contents)
There is 1 error.
- Target Branch:
+ Target branch:
...
This branch is not mergeable into lp://dev/~eric/...
...
=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2016-06-25 08:05:06 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2018-03-16 21:32:39 +0000
@@ -104,7 +104,8 @@
>>> nopriv_browser.getControl('Update').click()
>>> print_tag_with_id(nopriv_browser.contents, 'edit-commit_message')
- Edit Commit Message
+ Edit
+ Commit message
Add more <b>mojo</b>
A commit message can be removed deleting the text when editing.
=== modified file 'lib/lp/code/stories/branches/xx-claiming-team-code-reviews.txt'
--- lib/lp/code/stories/branches/xx-claiming-team-code-reviews.txt 2015-10-06 06:48:01 +0000
+++ lib/lp/code/stories/branches/xx-claiming-team-code-reviews.txt 2018-03-16 21:32:39 +0000
@@ -21,7 +21,7 @@
>>> eric_browser = setupBrowser(auth="Basic eric@xxxxxxxxxxx:test")
>>> eric_browser.open(branch_url)
>>> eric_browser.getLink('Propose for merging').click()
- >>> eric_browser.getControl('Description of the Change').value = (
+ >>> eric_browser.getControl('Description of the change').value = (
... 'This fix is awesome!')
>>> eric_browser.getControl('Propose Merge').click()
=== modified file 'lib/lp/code/templates/branch-register-merge.pt'
--- lib/lp/code/templates/branch-register-merge.pt 2012-09-10 03:45:21 +0000
+++ lib/lp/code/templates/branch-register-merge.pt 2018-03-16 21:32:39 +0000
@@ -16,6 +16,10 @@
<metal:block use-macro="context/@@launchpad_form/widget_row" />
</tal:widget>
+ <tal:widget define="widget nocall:view/widgets/commit_message">
+ <metal:block use-macro="context/@@launchpad_form/widget_row" />
+ </tal:widget>
+
<tal:widget define="widget nocall:view/widgets/comment">
<metal:block use-macro="context/@@launchpad_form/widget_row" />
</tal:widget>
@@ -28,27 +32,13 @@
<metal:block use-macro="context/@@launchpad_form/widget_row" />
</tal:widget>
- <td colspan="2">
- <fieldset id="mergeproposal-extra-options"
- class="collapsible">
- <legend>Extra options</legend>
- <div class="hide-on-load"><!-- hidden by default -->
- <table class="extra-options">
- <tal:widget define="widget nocall:view/widgets/commit_message">
- <metal:block use-macro="context/@@launchpad_form/widget_row" />
- </tal:widget>
-
- <tal:widget define="widget nocall:view/widgets/needs_review">
- <metal:block use-macro="context/@@launchpad_form/widget_row" />
- </tal:widget>
-
- <tal:widget define="widget nocall:view/widgets/prerequisite_branch">
- <metal:block use-macro="context/@@launchpad_form/widget_row" />
- </tal:widget>
- </table>
- </div>
- </fieldset>
- </td>
+ <tal:widget define="widget nocall:view/widgets/needs_review">
+ <metal:block use-macro="context/@@launchpad_form/widget_row" />
+ </tal:widget>
+
+ <tal:widget define="widget nocall:view/widgets/prerequisite_branch">
+ <metal:block use-macro="context/@@launchpad_form/widget_row" />
+ </tal:widget>
</table>
</metal:formbody>
=== modified file 'lib/lp/code/templates/gitref-register-merge.pt'
--- lib/lp/code/templates/gitref-register-merge.pt 2015-04-28 16:48:22 +0000
+++ lib/lp/code/templates/gitref-register-merge.pt 2018-03-16 21:32:39 +0000
@@ -20,6 +20,10 @@
<metal:block use-macro="context/@@launchpad_form/widget_row" />
</tal:widget>
+ <tal:widget define="widget nocall:view/widgets/commit_message">
+ <metal:block use-macro="context/@@launchpad_form/widget_row" />
+ </tal:widget>
+
<tal:widget define="widget nocall:view/widgets/comment">
<metal:block use-macro="context/@@launchpad_form/widget_row" />
</tal:widget>
@@ -32,31 +36,17 @@
<metal:block use-macro="context/@@launchpad_form/widget_row" />
</tal:widget>
- <td colspan="2">
- <fieldset id="mergeproposal-extra-options"
- class="collapsible">
- <legend>Extra options</legend>
- <div class="hide-on-load"><!-- hidden by default -->
- <table class="extra-options">
- <tal:widget define="widget nocall:view/widgets/commit_message">
- <metal:block use-macro="context/@@launchpad_form/widget_row" />
- </tal:widget>
-
- <tal:widget define="widget nocall:view/widgets/needs_review">
- <metal:block use-macro="context/@@launchpad_form/widget_row" />
- </tal:widget>
-
- <tal:widget define="widget nocall:view/widgets/prerequisite_git_repository">
- <metal:block use-macro="context/@@launchpad_form/widget_row" />
- </tal:widget>
-
- <tal:widget define="widget nocall:view/widgets/prerequisite_git_path">
- <metal:block use-macro="context/@@launchpad_form/widget_row" />
- </tal:widget>
- </table>
- </div>
- </fieldset>
- </td>
+ <tal:widget define="widget nocall:view/widgets/needs_review">
+ <metal:block use-macro="context/@@launchpad_form/widget_row" />
+ </tal:widget>
+
+ <tal:widget define="widget nocall:view/widgets/prerequisite_git_repository">
+ <metal:block use-macro="context/@@launchpad_form/widget_row" />
+ </tal:widget>
+
+ <tal:widget define="widget nocall:view/widgets/prerequisite_git_path">
+ <metal:block use-macro="context/@@launchpad_form/widget_row" />
+ </tal:widget>
</table>
</metal:formbody>
Follow ups
