launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15234
[Merge] lp:~stevenk/launchpad/less-public-bmp into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/less-public-bmp into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/less-public-bmp/+merge/150731
Split IBranchMergeProposal into four classes -- IBranchMergeProposal{Public,View,Edit,AnyPerson}, and these are used in the ZCML rather than listing attributes.
--
https://code.launchpad.net/~stevenk/launchpad/less-public-bmp/+merge/150731
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/less-public-bmp into lp:launchpad.
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2013-01-07 02:40:55 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2013-02-27 07:42:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
@@ -224,19 +224,6 @@
return private_team1, public_person1
- def testPrivateVotesNotVisibleIfBranchNotVisible(self):
- # User can't see votes for private teams if they can't see the branch.
- private_team1, public_person1 = self._createPrivateVotes(False)
- login_person(self.factory.makePerson())
- view = BranchMergeProposalVoteView(self.bmp, LaunchpadTestRequest())
-
- # Check the requested reviews.
- requested_reviews = view.requested_reviews
- self.assertEqual(1, len(requested_reviews))
- self.assertContentEqual(
- [public_person1],
- [review.reviewer for review in requested_reviews])
-
def testPrivateVotesVisibleIfBranchVisible(self):
# User can see votes for private teams if they can see the branch.
private_team1, public_person1 = self._createPrivateVotes()
@@ -805,7 +792,8 @@
browser = self.getViewBrowser(bmp, '+resubmit')
browser.getControl('Description').value = 'flibble'
browser.getControl('Resubmit').click()
- self.assertEqual('flibble', bmp.superseded_by.description)
+ with person_logged_in(self.user):
+ self.assertEqual('flibble', bmp.superseded_by.description)
class TestBranchMergeProposalView(TestCaseWithFactory):
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2013-02-25 05:23:12 +0000
+++ lib/lp/code/configure.zcml 2013-02-27 07:42:24 +0000
@@ -218,75 +218,23 @@
<!-- Branch Merge Proposal -->
<class class="lp.code.model.branchmergeproposal.BranchMergeProposal">
- <allow attributes="
- address
- id
- registrant
- source_branch
- source_branchID
- target_branch
- prerequisite_branch
- prerequisite_branchID
- description
- whiteboard
- queue_status
- private
- reviewer
- reviewed_revision_id
- queuer
- queue_position
- queued_revision_id
- commit_message
- date_queued
- merged_revno
- date_merged
- merge_reporter
- supersedes
- superseded_by
- title
- date_created
- date_review_requested
- date_reviewed
- next_preview_diff_job
- preview_diff
+ <allow
+ interface="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalPublic"
+ attributes="generateIncrementalDiff
getIncrementalDiffs
- votes
- all_comments
- getRelatedBugTasks
- revision_end_date
- isMergable
- getComment
- getRevisionsSinceReviewStart
- getNotificationRecipients
- getVoteReference
- isValidTransition
- getUnlandedSourceBranchRevisions
- root_message_id
- getUsersVoteReference
- generateIncrementalDiff"/>
+ revision_end_date"/>
<allow interface="lp.code.interfaces.branchtarget.IHasBranchTarget"/>
<require
+ permission="launchpad.View"
+ interface="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalView"/>
+ <require
permission="launchpad.Edit"
+ interface="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalEdit"
set_attributes="description whiteboard merged_revno commit_message
- root_message_id prerequisite_branch"
- attributes="
- deleteProposal
- setStatus
- setAsWorkInProgress
- requestReview
- approveBranch
- rejectBranch
- markAsMerged
- resubmit
- enqueue
- dequeue
- moveToFrontOfQueue
- nominateReviewer
- updatePreviewDiff"/>
+ root_message_id prerequisite_branch"/>
<require
permission="launchpad.AnyPerson"
- attributes="createComment
- createCommentFromMessage"/>
+ interface="lp.code.interfaces.branchmergeproposal.IBranchMergeProposalAnyPerson"/>
</class>
<!-- Branch Merge Proposal Jobs -->
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py 2013-02-05 22:56:33 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py 2013-02-27 07:42:24 +0000
@@ -94,20 +94,24 @@
)
-class IBranchMergeProposal(IPrivacy):
- """Branch merge proposals show intent of landing one branch on another."""
-
- export_as_webservice_entry()
+class IBranchMergeProposalPublic(IPrivacy):
id = Int(
title=_('DB ID'), required=True, readonly=True,
description=_("The tracking number for this merge proposal."))
+ source_branchID = Int(
+ title=_('Source branch ID'), required=True, readonly=True)
+ prerequisite_branchID = Int(
+ title=_('Prerequisite branch ID'), required=True, readonly=True)
- registrant = exported(
- PublicPersonChoice(
- title=_('Person'), required=True,
- vocabulary='ValidPersonOrTeam', readonly=True,
- description=_('The person who registered the merge proposal.')))
+ # This is redefined from IPrivacy.private because the attribute is
+ # read-only. The value is determined by the involved branches.
+ private = exported(
+ Bool(
+ title=_("Proposal is confidential"), required=False,
+ readonly=True, default=False,
+ description=_(
+ "If True, this proposal is visible only to subscribers.")))
source_branch = exported(
ReferenceChoice(
@@ -131,14 +135,14 @@
"If this branch is the same as the target branch, then "
"leave this field blank.")))
- # This is redefined from IPrivacy.private because the attribute is
- # read-only. The value is determined by the involved branches.
- private = exported(
- Bool(
- title=_("Proposal is confidential"), required=False,
- readonly=True, default=False,
- description=_(
- "If True, this proposal is visible only to subscribers.")))
+
+class IBranchMergeProposalView(Interface):
+
+ registrant = exported(
+ PublicPersonChoice(
+ title=_('Person'), required=True,
+ vocabulary='ValidPersonOrTeam', readonly=True,
+ description=_('The person who registered the merge proposal.')))
description = exported(
Text(title=_('Description'), required=False,
@@ -312,19 +316,63 @@
notified.
"""
- # Cannot specify value type without creating a circular dependency
votes = exported(
CollectionField(
title=_('The votes cast or expected for this proposal'),
# Really ICodeReviewVoteReference.
value_type=Reference(schema=Interface),
- readonly=True,
- )
- )
+ readonly=True))
def isValidTransition(next_state, user=None):
"""True if it is valid for user update the proposal to next_state."""
+ def isMergable():
+ """Is the proposal in a state that allows it to being merged?
+
+ As long as the proposal isn't in one of the end states, it is valid
+ to be merged.
+ """
+
+ def getUnlandedSourceBranchRevisions():
+ """Return a sequence of `BranchRevision` objects.
+
+ Returns up to 10 revisions that are in the revision history for the
+ source branch that are not in the revision history of the target
+ branch. These are the revisions that have been committed to the
+ source branch since it branched off the target branch.
+ """
+
+ def getUsersVoteReference(user):
+ """Get the existing vote reference for the given user.
+
+ :return: A `CodeReviewVoteReference` or None.
+ """
+
+
+class IBranchMergeProposalEdit(Interface):
+
+ def deleteProposal():
+ """Delete the proposal to merge."""
+
+ def updatePreviewDiff(diff_content, source_revision_id,
+ target_revision_id, prerequisite_revision_id=None,
+ conflicts=None):
+ """Update the preview diff for this proposal.
+
+ If there is not an existing preview diff, one will be created.
+
+ :param diff_content: The raw bytes of the diff content to be put in
+ the librarian.
+ :param diff_stat: Text describing the files added, remove or modified.
+ :param source_revision_id: The revision id that was used from the
+ source branch.
+ :param target_revision_id: The revision id that was used from the
+ target branch.
+ :param prerequisite_revision_id: The revision id that was used from
+ the prerequisite branch.
+ :param conflicts: Text describing the conflicts if any.
+ """
+
@call_with(user=REQUEST_USER)
@rename_parameters_as(revision_id='revid')
@operation_parameters(
@@ -385,27 +433,6 @@
the branch reviewer for the target branch.
"""
- def enqueue(queuer, revision_id):
- """Put the proposal into the merge queue for the target branch.
-
- If the proposal is not in the Approved state before this method
- is called, approveBranch is called with the reviewer and revision_id
- specified.
-
- If None is supplied as the revision_id, the proposals
- reviewed_revision_id is used.
- """
-
- def dequeue():
- """Take the proposal out of the merge queue of the target branch.
-
- :raises: BadStateTransition if the proposal is not in the queued
- state.
- """
-
- def moveToFrontOfQueue():
- """Move the queue proposal to the front of the queue."""
-
def markAsMerged(merged_revno=None, date_merged=None,
merge_reporter=None):
"""Mark the branch merge proposal as merged.
@@ -451,21 +478,26 @@
the current description).
"""
- def isMergable():
- """Is the proposal in a state that allows it to being merged?
-
- As long as the proposal isn't in one of the end states, it is valid
- to be merged.
- """
-
- def getUnlandedSourceBranchRevisions():
- """Return a sequence of `BranchRevision` objects.
-
- Returns up to 10 revisions that are in the revision history for the
- source branch that are not in the revision history of the target
- branch. These are the revisions that have been committed to the
- source branch since it branched off the target branch.
- """
+ def enqueue(queuer, revision_id):
+ """Put the proposal into the merge queue for the target branch.
+
+ If the proposal is not in the Approved state before this method
+ is called, approveBranch is called with the reviewer and revision_id
+ specified.
+
+ If None is supplied as the revision_id, the proposals
+ reviewed_revision_id is used.
+ """
+
+ def dequeue():
+ """Take the proposal out of the merge queue of the target branch.
+
+ :raises: BadStateTransition if the proposal is not in the queued
+ state.
+ """
+
+ def moveToFrontOfQueue():
+ """Move the queue proposal to the front of the queue."""
@operation_parameters(
reviewer=Reference(
@@ -482,11 +514,8 @@
the details are updated.
"""
- def getUsersVoteReference(user):
- """Get the existing vote reference for the given user.
- :return: A `CodeReviewVoteReference` or None.
- """
+class IBranchMergeProposalAnyPerson(Interface):
@operation_parameters(
subject=Text(), content=Text(),
@@ -517,27 +546,13 @@
:param original_email: Original email message.
"""
- def deleteProposal():
- """Delete the proposal to merge."""
-
- def updatePreviewDiff(diff_content, source_revision_id,
- target_revision_id, prerequisite_revision_id=None,
- conflicts=None):
- """Update the preview diff for this proposal.
-
- If there is not an existing preview diff, one will be created.
-
- :param diff_content: The raw bytes of the diff content to be put in
- the librarian.
- :param diff_stat: Text describing the files added, remove or modified.
- :param source_revision_id: The revision id that was used from the
- source branch.
- :param target_revision_id: The revision id that was used from the
- target branch.
- :param prerequisite_revision_id: The revision id that was used from
- the prerequisite branch.
- :param conflicts: Text describing the conflicts if any.
- """
+
+class IBranchMergeProposal(IBranchMergeProposalPublic,
+ IBranchMergeProposalView, IBranchMergeProposalEdit,
+ IBranchMergeProposalAnyPerson):
+ """Branch merge proposals show intent of landing one branch on another."""
+
+ export_as_webservice_entry()
class IBranchMergeProposalJob(Interface):
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2013-01-04 05:42:01 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2013-02-27 07:42:24 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd. This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd. This software is licensed under the
# GNU Affero General Public License version 3 (see the file LICENSE).
"""Tests for BranchMergeProposals."""
@@ -953,31 +953,37 @@
def test_getNotificationRecipients_privacy(self):
# If a user can see only one of the source and target branches, then
# they do not get email about the proposal.
- bmp = self.factory.makeBranchMergeProposal()
+ owner = self.factory.makePerson()
+ product = self.factory.makeProduct()
+ source = self.factory.makeBranch(owner=owner, product=product)
+ target = self.factory.makeBranch(owner=owner, product=product)
+ bmp = self.factory.makeBranchMergeProposal(
+ source_branch=source, target_branch=target)
# Subscribe eric to the source branch only.
eric = self.factory.makePerson()
- bmp.source_branch.subscribe(
+ source.subscribe(
eric, BranchSubscriptionNotificationLevel.NOEMAIL, None,
CodeReviewNotificationLevel.FULL, eric)
# Subscribe bob to the target branch only.
bob = self.factory.makePerson()
- bmp.target_branch.subscribe(
+ target.subscribe(
bob, BranchSubscriptionNotificationLevel.NOEMAIL, None,
CodeReviewNotificationLevel.FULL, bob)
# Subscribe charlie to both.
charlie = self.factory.makePerson()
- bmp.source_branch.subscribe(
+ source.subscribe(
charlie, BranchSubscriptionNotificationLevel.NOEMAIL, None,
CodeReviewNotificationLevel.FULL, charlie)
- bmp.target_branch.subscribe(
+ target.subscribe(
charlie, BranchSubscriptionNotificationLevel.NOEMAIL, None,
CodeReviewNotificationLevel.FULL, charlie)
# Make both branches private.
- for branch in (bmp.source_branch, bmp.target_branch):
+ for branch in (source, target):
removeSecurityProxy(branch).transitionToInformationType(
InformationType.USERDATA, branch.owner, verify_policy=False)
- recipients = bmp.getNotificationRecipients(
- CodeReviewNotificationLevel.FULL)
+ with person_logged_in(owner):
+ recipients = bmp.getNotificationRecipients(
+ CodeReviewNotificationLevel.FULL)
self.assertNotIn(bob, recipients)
self.assertNotIn(eric, recipients)
self.assertIn(charlie, recipients)
@@ -2038,12 +2044,13 @@
def test_getMergeProposals_with_merged_revnos(self):
"""Specifying merged revnos selects the correct merge proposal."""
- mp = self.factory.makeBranchMergeProposal()
+ registrant = self.factory.makePerson()
+ mp = self.factory.makeBranchMergeProposal(registrant=registrant)
launchpad = launchpadlib_for(
- 'test', mp.registrant,
+ 'test', registrant,
service_root=self.layer.appserver_root_url('api'))
- with person_logged_in(mp.registrant):
+ with person_logged_in(registrant):
mp.markAsMerged(merged_revno=123)
transaction.commit()
target = ws_object(launchpad, mp.target_branch)
=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2012-12-10 13:43:47 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2013-02-27 07:42:24 +0000
@@ -667,8 +667,9 @@
Updating diff...
An updated diff will be available in a few minutes. Reload to see the
changes.
- >>> bmp.next_preview_diff_job.start()
- >>> bmp.next_preview_diff_job.complete()
+ >>> job = removeSecurityProxy(bmp).next_preview_diff_job
+ >>> job.start()
+ >>> job.complete()
>>> transaction.commit()
>>> nopriv_browser.open(url)
>>> print find_tag_by_id(nopriv_browser.contents, 'diff-pending-update')
