launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18400
[Merge] lp:~cjwatson/launchpad/git-mp-basic-browser into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/git-mp-basic-browser into lp:launchpad.
Commit message:
Add Git support to most of the merge proposal views.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1445017 in Launchpad itself: "Support for Launchpad Git merge proposals "
https://bugs.launchpad.net/launchpad/+bug/1445017
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-mp-basic-browser/+merge/257671
Add Git support to most of the merge proposal views.
GitRef:+register-merge is about the same size again, and will be in a separate MP.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-mp-basic-browser into lp:launchpad.
=== modified file 'lib/lp/code/browser/branchmergeproposal.py'
--- lib/lp/code/browser/branchmergeproposal.py 2015-04-22 16:41:41 +0000
+++ lib/lp/code/browser/branchmergeproposal.py 2015-04-28 16:44:03 +0000
@@ -86,6 +86,7 @@
InvalidBranchMergeProposal,
WrongBranchMergeProposal,
)
+from lp.code.interfaces.branch import IBranch
from lp.code.interfaces.branchmergeproposal import IBranchMergeProposal
from lp.code.interfaces.codereviewcomment import ICodeReviewComment
from lp.code.interfaces.codereviewinlinecomment import (
@@ -355,7 +356,7 @@
@property
def codebrowse_url(self):
"""Return the link to codebrowse for this branch."""
- return self.context.source_branch.getCodebrowseUrl()
+ return self.context.merge_source.getCodebrowseUrl()
class BranchMergeProposalRevisionIdMixin:
@@ -373,6 +374,9 @@
return None
# If the source branch is REMOTE, then there won't be any ids.
source_branch = self.context.source_branch
+ if source_branch is None:
+ # Git doesn't have revision numbers. Just use the ids.
+ return revision_id
if source_branch.branch_type == BranchType.REMOTE:
return revision_id
else:
@@ -586,12 +590,18 @@
def initialize(self):
super(BranchMergeProposalView, self).initialize()
cache = IJSONRequestCache(self.request)
- cache.objects.update({
- 'branch_diff_link':
- 'https://%s/+loggerhead/%s/diff/' % (
- config.launchpad.code_domain,
- self.context.source_branch.unique_name),
- })
+ if IBranch.providedBy(self.context.merge_source):
+ cache.objects.update({
+ 'branch_diff_link':
+ 'https://%s/+loggerhead/%s/diff/' % (
+ config.launchpad.code_domain,
+ self.context.source_branch.unique_name),
+ })
+ else:
+ cache.objects.update({
+ 'repository_diff_link':
+ '%s/diff/' % self.context.merge_source.getCodebrowseUrl(),
+ })
if getFeatureFlag("longpoll.merge_proposals.enabled"):
cache.objects['merge_proposal_event_key'] = subscribe(
self.context).event_key
@@ -618,7 +628,9 @@
# Sort the comments by date order.
merge_proposal = self.context
groups = list(merge_proposal.getRevisionsSinceReviewStart())
- source = DecoratedBranch(merge_proposal.source_branch)
+ source = merge_proposal.merge_source
+ if IBranch.providedBy(source):
+ source = DecoratedBranch(source)
comments = []
if getFeatureFlag('code.incremental_diffs.enabled'):
ranges = [
@@ -685,6 +697,8 @@
@property
def pending_diff(self):
+ if self.context.source_branch is None:
+ return False
return (
self.context.next_preview_diff_job is not None or
self.context.source_branch.pending_writes)
@@ -705,6 +719,10 @@
@property
def spec_links(self):
+ if self.context.source_branch is None:
+ # XXX cjwatson 2015-04-16: Implement once Git refs have linked
+ # blueprints.
+ return []
return list(
self.context.source_branch.getSpecificationLinks(self.user))
@@ -748,12 +766,16 @@
@property
def status_config(self):
"""The config to configure the ChoiceSource JS widget."""
+ if IBranch.providedBy(self.context.merge_source):
+ source_revid = self.context.merge_source.last_scanned_id
+ else:
+ source_revid = self.context.merge_source.commit_sha1
return simplejson.dumps({
'status_widget_items': vocabulary_to_choice_edit_items(
self._createStatusVocabulary(),
css_class_prefix='mergestatus'),
'status_value': self.context.queue_status.title,
- 'source_revid': self.context.source_branch.last_scanned_id,
+ 'source_revid': source_revid,
'user_can_edit_status': check_permission(
'launchpad.Edit', self.context),
})
@@ -965,19 +987,35 @@
schema = ResubmitSchema
for_input = True
page_title = label = "Resubmit proposal to merge"
- field_names = [
- 'source_branch',
- 'target_branch',
- 'prerequisite_branch',
- 'description',
- 'break_link',
- ]
def initialize(self):
self.cancel_url = canonical_url(self.context)
super(BranchMergeProposalResubmitView, self).initialize()
@property
+ def field_names(self):
+ if IBranch.providedBy(self.context.merge_source):
+ field_names = [
+ 'source_branch',
+ 'target_branch',
+ 'prerequisite_branch',
+ ]
+ else:
+ field_names = [
+ 'source_git_repository',
+ 'source_git_path',
+ 'target_git_repository',
+ 'target_git_path',
+ 'prerequisite_git_repository',
+ 'prerequisite_git_path',
+ ]
+ field_names.extend([
+ 'description',
+ 'break_link',
+ ])
+ return field_names
+
+ @property
def initial_values(self):
UNSET = object()
items = ((key, getattr(self.context, key, UNSET)) for key in
@@ -988,10 +1026,24 @@
def resubmit_action(self, action, data):
"""Resubmit this proposal."""
try:
+ if IBranch.providedBy(self.context.merge_source):
+ merge_source = data['source_branch']
+ merge_target = data['target_branch']
+ merge_prerequisite = data['prerequisite_branch']
+ else:
+ merge_source = data['source_git_repository'].getRefByPath(
+ data['source_git_path'])
+ merge_target = data['target_git_repository'].getRefByPath(
+ data['target_git_path'])
+ if data['prerequisite_git_repository']:
+ merge_prerequisite = (
+ data['prerequisite_git_repository'].getRefByPath(
+ data['prerequisite_git_path']))
+ else:
+ merge_prerequisite = None
proposal = self.context.resubmit(
- self.user, data['source_branch'], data['target_branch'],
- data['prerequisite_branch'], data['description'],
- data['break_link'])
+ self.user, merge_source, merge_target, merge_prerequisite,
+ data['description'], data['break_link'])
except BranchMergeProposalExists as e:
message = structured(
'Cannot resubmit because <a href="%(url)s">a similar merge'
@@ -1072,18 +1124,31 @@
"""The view to mark a merge proposal as merged."""
schema = IBranchMergeProposal
page_title = label = "Edit branch merge proposal"
- field_names = ["merged_revno"]
for_input = True
@property
+ def field_names(self):
+ if IBranch.providedBy(self.context.merge_target):
+ return ["merged_revno"]
+ else:
+ return ["merged_revision_id"]
+
+ @property
def initial_values(self):
# Default to the tip of the target branch, on the assumption that the
# source branch has just been merged into it.
- if self.context.merged_revno is not None:
- revno = self.context.merged_revno
+ if IBranch.providedBy(self.context.merge_target):
+ if self.context.merged_revno is not None:
+ revno = self.context.merged_revno
+ else:
+ revno = self.context.merge_target.revision_count
+ return {'merged_revno': revno}
else:
- revno = self.context.target_branch.revision_count
- return {'merged_revno': revno}
+ if self.context.merged_revision_id is not None:
+ revision_id = self.context.merged_revision_id
+ else:
+ revision_id = self.context.merge_target.commit_sha1
+ return {'merged_revision_id': revision_id}
@property
def next_url(self):
@@ -1095,13 +1160,16 @@
@notify
def mark_merged_action(self, action, data):
"""Update the whiteboard and go back to the source branch."""
- revno = data['merged_revno']
+ if IBranch.providedBy(self.context.merge_target):
+ kwargs = {'merged_revno': data['merged_revno']}
+ else:
+ kwargs = {'merged_revision_id': data['merged_revision_id']}
if self.context.queue_status == BranchMergeProposalStatus.MERGED:
- self.context.markAsMerged(merged_revno=revno)
+ self.context.markAsMerged(**kwargs)
self.request.response.addNotification(
'The proposal\'s merged revision has been updated.')
else:
- self.context.markAsMerged(revno, merge_reporter=self.user)
+ self.context.markAsMerged(merge_reporter=self.user, **kwargs)
self.request.response.addNotification(
'The proposal has now been marked as merged.')
=== modified file 'lib/lp/code/browser/configure.zcml'
--- lib/lp/code/browser/configure.zcml 2015-04-22 16:11:40 +0000
+++ lib/lp/code/browser/configure.zcml 2015-04-28 16:44:03 +0000
@@ -824,6 +824,9 @@
<browser:page
name="++ref-commits"
template="../templates/gitref-commits.pt"/>
+ <browser:page
+ name="++ref-pending-merges"
+ template="../templates/gitref-pending-merges.pt"/>
</browser:pages>
<browser:page
for="lp.code.interfaces.gitref.IGitRef"
=== modified file 'lib/lp/code/browser/gitref.py'
--- lib/lp/code/browser/gitref.py 2015-04-22 16:11:40 +0000
+++ lib/lp/code/browser/gitref.py 2015-04-28 16:44:03 +0000
@@ -10,12 +10,19 @@
'GitRefView',
]
+from lp.code.browser.branchmergeproposal import (
+ latest_proposals_for_each_branch,
+ )
from lp.code.interfaces.gitref import IGitRef
+from lp.code.model.gitrepository import GitRepository
+from lp.services.database.bulk import load_related
+from lp.services.propertycache import cachedproperty
from lp.services.webapp import (
LaunchpadView,
Navigation,
stepthrough,
)
+from lp.services.webapp.authorization import check_permission
class GitRefNavigation(Navigation):
@@ -49,3 +56,57 @@
"author_date": self.context.author_date,
"commit_message": self.context.commit_message,
}
+
+ @property
+ def show_merge_links(self):
+ """Return whether or not merge proposal links should be shown.
+
+ Merge proposal links should not be shown if there is only one
+ reference in the entire target.
+ """
+ if not self.context.namespace.supports_merge_proposals:
+ return False
+ repositories = self.context.namespace.collection.getRepositories()
+ if repositories.count() > 1:
+ return True
+ repository = repositories.one()
+ if repository is None:
+ return False
+ return repository.refs.count() > 1
+
+ @cachedproperty
+ def landing_targets(self):
+ """Return a filtered list of landing targets."""
+ return latest_proposals_for_each_branch(self.context.landing_targets)
+
+ @cachedproperty
+ def landing_candidates(self):
+ """Return a decorated list of landing candidates."""
+ candidates = list(self.context.landing_candidates)
+ load_related(
+ GitRepository, candidates,
+ ["source_git_repositoryID", "prerequisite_git_repositoryID"])
+ return [proposal for proposal in candidates
+ if check_permission("launchpad.View", proposal)]
+
+ def _getBranchCountText(self, count):
+ """Help to show user friendly text."""
+ if count == 0:
+ return 'No branches'
+ elif count == 1:
+ return '1 branch'
+ else:
+ return '%s branches' % count
+
+ @cachedproperty
+ def landing_candidate_count_text(self):
+ return self._getBranchCountText(len(self.landing_candidates))
+
+ @cachedproperty
+ def dependent_landings(self):
+ return [proposal for proposal in self.context.dependent_landings
+ if check_permission("launchpad.View", proposal)]
+
+ @cachedproperty
+ def dependent_landing_count_text(self):
+ return self._getBranchCountText(len(self.dependent_landings))
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2015-04-19 12:56:32 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2015-04-28 16:44:03 +0000
@@ -46,6 +46,7 @@
BranchMergeProposalStatus,
CodeReviewVote,
)
+from lp.code.interfaces.gitrepository import GIT_FEATURE_FLAG
from lp.code.model.diff import PreviewDiff
from lp.code.tests.helpers import (
add_revision_to_branch,
@@ -55,6 +56,7 @@
PersonVisibility,
TeamMembershipPolicy,
)
+from lp.services.features.testing import FeatureFixture
from lp.services.librarian.interfaces.client import LibrarianServerError
from lp.services.messages.model.message import MessageSet
from lp.services.webapp import canonical_url
@@ -107,8 +109,8 @@
self.assertTrue(d.can_change_review)
-class TestBranchMergeProposalMergedView(TestCaseWithFactory):
- """Tests for `BranchMergeProposalMergedView`."""
+class TestBranchMergeProposalMergedViewBzr(TestCaseWithFactory):
+ """Tests for `BranchMergeProposalMergedView` for Bazaar."""
layer = DatabaseFunctionalLayer
@@ -129,6 +131,30 @@
view.initial_values)
+class TestBranchMergeProposalMergedViewGit(TestCaseWithFactory):
+ """Tests for `BranchMergeProposalMergedView` for Git."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ # Use an admin so we don't have to worry about launchpad.Edit
+ # permissions on the merge proposals for adding comments, or
+ # nominating reviewers.
+ TestCaseWithFactory.setUp(self, user="admin@xxxxxxxxxxxxx")
+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+ self.bmp = self.factory.makeBranchMergeProposalForGit()
+
+ def test_initial_values(self):
+ # The default merged_revision_id is the head commit SHA-1 of the
+ # target ref.
+ view = BranchMergeProposalMergedView(self.bmp, LaunchpadTestRequest())
+ removeSecurityProxy(self.bmp.source_git_ref).commit_sha1 = "0" * 40
+ removeSecurityProxy(self.bmp.target_git_ref).commit_sha1 = "1" * 40
+ self.assertEqual(
+ {'merged_revision_id': self.bmp.target_git_ref.commit_sha1},
+ view.initial_values)
+
+
class TestBranchMergeProposalAddVoteView(TestCaseWithFactory):
"""Test the AddVote view."""
@@ -661,14 +687,14 @@
self.assertEqual(BrowserNotificationLevel.INFO, notification.level)
-class TestBranchMergeProposalResubmitView(TestCaseWithFactory):
+class TestBranchMergeProposalResubmitViewMixin:
"""Test BranchMergeProposalResubmitView."""
layer = DatabaseFunctionalLayer
def createView(self):
"""Create the required view."""
- context = self.factory.makeBranchMergeProposal()
+ context = self._makeBranchMergeProposal()
self.useContext(person_logged_in(context.registrant))
view = BranchMergeProposalResubmitView(
context, LaunchpadTestRequest())
@@ -679,36 +705,34 @@
"""resubmit_action resubmits the proposal."""
view = self.createView()
context = view.context
- new_proposal = view.resubmit_action.success(
- {'source_branch': context.source_branch,
- 'target_branch': context.target_branch,
- 'prerequisite_branch': context.prerequisite_branch,
- 'description': None,
- 'break_link': False,
- })
+ new_proposal = view.resubmit_action.success(self._getFormValues(
+ context.merge_source, context.merge_target,
+ context.merge_prerequisite, {
+ 'description': None,
+ 'break_link': False,
+ }))
self.assertEqual(new_proposal.supersedes, context)
- self.assertEqual(new_proposal.source_branch, context.source_branch)
- self.assertEqual(new_proposal.target_branch, context.target_branch)
+ self.assertEqual(new_proposal.merge_source, context.merge_source)
+ self.assertEqual(new_proposal.merge_target, context.merge_target)
self.assertEqual(
- new_proposal.prerequisite_branch, context.prerequisite_branch)
+ new_proposal.merge_prerequisite, context.merge_prerequisite)
def test_resubmit_action_change_branches(self):
"""Changing the branches changes the branches in the new proposal."""
view = self.createView()
- target = view.context.source_branch.target
- new_source = self.factory.makeBranchTargetBranch(target)
- new_target = self.factory.makeBranchTargetBranch(target)
- new_prerequisite = self.factory.makeBranchTargetBranch(target)
- new_proposal = view.resubmit_action.success(
- {'source_branch': new_source, 'target_branch': new_target,
- 'prerequisite_branch': new_prerequisite,
- 'description': 'description',
- 'break_link': False,
- })
+ new_source = self._makeBranchWithSameTarget(view.context.merge_source)
+ new_target = self._makeBranchWithSameTarget(view.context.merge_source)
+ new_prerequisite = self._makeBranchWithSameTarget(
+ view.context.merge_source)
+ new_proposal = view.resubmit_action.success(self._getFormValues(
+ new_source, new_target, new_prerequisite, {
+ 'description': 'description',
+ 'break_link': False,
+ }))
self.assertEqual(new_proposal.supersedes, view.context)
- self.assertEqual(new_proposal.source_branch, new_source)
- self.assertEqual(new_proposal.target_branch, new_target)
- self.assertEqual(new_proposal.prerequisite_branch, new_prerequisite)
+ self.assertEqual(new_proposal.merge_source, new_source)
+ self.assertEqual(new_proposal.merge_target, new_target)
+ self.assertEqual(new_proposal.merge_prerequisite, new_prerequisite)
def test_resubmit_action_break_link(self):
"""Enabling break_link prevents linking the old and new proposals."""
@@ -716,24 +740,22 @@
new_proposal = self.resubmitDefault(view, break_link=True)
self.assertIs(None, new_proposal.supersedes)
- @staticmethod
- def resubmitDefault(view, break_link=False, prerequisite_branch=None):
+ @classmethod
+ def resubmitDefault(cls, view, break_link=False, merge_prerequisite=None):
context = view.context
- if prerequisite_branch is None:
- prerequisite_branch = context.prerequisite_branch
- return view.resubmit_action.success(
- {'source_branch': context.source_branch,
- 'target_branch': context.target_branch,
- 'prerequisite_branch': prerequisite_branch,
- 'description': None,
- 'break_link': break_link,
- })
+ if merge_prerequisite is None:
+ merge_prerequisite = context.merge_prerequisite
+ return view.resubmit_action.success(cls._getFormValues(
+ context.merge_source, context.merge_target, merge_prerequisite, {
+ 'description': None,
+ 'break_link': break_link,
+ }))
def test_resubmit_existing(self):
"""Resubmitting a proposal when another is active is a user error."""
view = self.createView()
first_bmp = view.context
- with person_logged_in(first_bmp.target_branch.owner):
+ with person_logged_in(first_bmp.merge_target.owner):
first_bmp.resubmit(first_bmp.registrant)
self.resubmitDefault(view)
(notification,) = view.request.response.notifications
@@ -742,19 +764,86 @@
' <a href=.*>a similar merge proposal</a> is already active.'))
self.assertEqual(BrowserNotificationLevel.ERROR, notification.level)
+
+class TestBranchMergeProposalResubmitViewBzr(
+ TestBranchMergeProposalResubmitViewMixin, TestCaseWithFactory):
+ """Test BranchMergeProposalResubmitView for Bazaar."""
+
+ def _makeBranchMergeProposal(self):
+ return self.factory.makeBranchMergeProposal()
+
+ @staticmethod
+ def _getFormValues(source_branch, target_branch, prerequisite_branch,
+ extras):
+ values = {
+ 'source_branch': source_branch,
+ 'target_branch': target_branch,
+ 'prerequisite_branch': prerequisite_branch,
+ }
+ values.update(extras)
+ return values
+
+ def _makeBranchWithSameTarget(self, branch):
+ return self.factory.makeBranchTargetBranch(branch.target)
+
def test_resubmit_same_target_prerequisite(self):
"""User error if same branch is target and prerequisite."""
view = self.createView()
first_bmp = view.context
- self.resubmitDefault(
- view, prerequisite_branch=first_bmp.target_branch)
+ self.resubmitDefault(view, merge_prerequisite=first_bmp.merge_target)
self.assertEqual(
view.errors,
['Target and prerequisite branches must be different.'])
-class TestResubmitBrowser(BrowserTestCase):
- """Browser tests for resubmitting branch merge proposals."""
+class TestBranchMergeProposalResubmitViewGit(
+ TestBranchMergeProposalResubmitViewMixin, TestCaseWithFactory):
+ """Test BranchMergeProposalResubmitView for Git."""
+
+ def setUp(self):
+ super(TestBranchMergeProposalResubmitViewGit, self).setUp()
+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+ def _makeBranchMergeProposal(self):
+ return self.factory.makeBranchMergeProposalForGit()
+
+ @staticmethod
+ def _getFormValues(source_branch, target_branch, prerequisite_branch,
+ extras):
+ values = {
+ 'source_git_repository': source_branch.repository,
+ 'source_git_path': source_branch.path,
+ 'target_git_repository': target_branch.repository,
+ 'target_git_path': target_branch.path,
+ }
+ if prerequisite_branch is not None:
+ values.update({
+ 'prerequisite_git_repository': prerequisite_branch.repository,
+ 'prerequisite_git_path': prerequisite_branch.path,
+ })
+ else:
+ values.update({
+ 'prerequisite_git_repository': None,
+ 'prerequisite_git_path': '',
+ })
+ values.update(extras)
+ return values
+
+ def _makeBranchWithSameTarget(self, branch):
+ return self.factory.makeGitRefs(target=branch.target)[0]
+
+ def test_resubmit_same_target_prerequisite(self):
+ """User error if same branch is target and prerequisite."""
+ view = self.createView()
+ first_bmp = view.context
+ self.resubmitDefault(view, merge_prerequisite=first_bmp.merge_target)
+ self.assertEqual(
+ view.errors,
+ ['Target and prerequisite references must be different.'])
+
+
+class TestResubmitBrowserBzr(BrowserTestCase):
+ """Browser tests for resubmitting branch merge proposals for Bazaar."""
layer = DatabaseFunctionalLayer
@@ -781,6 +870,41 @@
self.assertEqual('flibble', bmp.superseded_by.description)
+class TestResubmitBrowserGit(BrowserTestCase):
+ """Browser tests for resubmitting branch merge proposals for Git."""
+
+ layer = DatabaseFunctionalLayer
+
+ def setUp(self):
+ super(TestResubmitBrowserGit, self).setUp()
+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+ def test_resubmit_text(self):
+ """The text of the resubmit page is as expected."""
+ bmp = self.factory.makeBranchMergeProposalForGit(registrant=self.user)
+ text = self.getMainText(bmp, '+resubmit')
+ expected = (
+ 'Resubmit proposal to merge.*'
+ 'Source Git Repository:.*'
+ 'Source Git branch path:.*'
+ 'Target Git Repository:.*'
+ 'Target Git branch path:.*'
+ 'Prerequisite Git Repository:.*'
+ 'Prerequisite Git branch path:.*'
+ 'Description.*'
+ 'Start afresh.*')
+ self.assertTextMatchesExpressionIgnoreWhitespace(expected, text)
+
+ def test_resubmit_controls(self):
+ """Proposals can be resubmitted using the browser."""
+ bmp = self.factory.makeBranchMergeProposalForGit(registrant=self.user)
+ browser = self.getViewBrowser(bmp, '+resubmit')
+ browser.getControl('Description').value = 'flibble'
+ browser.getControl('Resubmit').click()
+ with person_logged_in(self.user):
+ self.assertEqual('flibble', bmp.superseded_by.description)
+
+
class TestBranchMergeProposalView(TestCaseWithFactory):
layer = LaunchpadFunctionalLayer
@@ -1248,17 +1372,17 @@
self.assertEqual(mp_url, browser.url)
-class TestLatestProposalsForEachBranch(TestCaseWithFactory):
+class TestLatestProposalsForEachBranchMixin:
"""Confirm that the latest branch is returned."""
layer = DatabaseFunctionalLayer
def test_newest_first(self):
# If each proposal targets a different branch, each will be returned.
- bmp1 = self.factory.makeBranchMergeProposal(
+ bmp1 = self._makeBranchMergeProposal(
date_created=(
datetime(year=2008, month=9, day=10, tzinfo=pytz.UTC)))
- bmp2 = self.factory.makeBranchMergeProposal(
+ bmp2 = self._makeBranchMergeProposal(
date_created=(
datetime(year=2008, month=10, day=10, tzinfo=pytz.UTC)))
self.assertEqual(
@@ -1266,27 +1390,57 @@
def test_visible_filtered_out(self):
# If the proposal is not visible to the user, they are not returned.
- bmp1 = self.factory.makeBranchMergeProposal(
+ bmp1 = self._makeBranchMergeProposal(
date_created=(
datetime(year=2008, month=9, day=10, tzinfo=pytz.UTC)))
- bmp2 = self.factory.makeBranchMergeProposal(
+ bmp2 = self._makeBranchMergeProposal(
date_created=(
datetime(year=2008, month=10, day=10, tzinfo=pytz.UTC)))
- removeSecurityProxy(bmp2.source_branch).transitionToInformationType(
- InformationType.USERDATA, bmp2.source_branch.owner,
- verify_policy=False)
+ self._setBranchInvisible(bmp2.merge_source)
self.assertEqual(
[bmp1], latest_proposals_for_each_branch([bmp1, bmp2]))
def test_same_target(self):
# If the proposals target the same branch, then the most recent is
# returned.
- bmp1 = self.factory.makeBranchMergeProposal(
+ bmp1 = self._makeBranchMergeProposal(
date_created=(
datetime(year=2008, month=9, day=10, tzinfo=pytz.UTC)))
- bmp2 = self.factory.makeBranchMergeProposal(
- target_branch=bmp1.target_branch,
+ bmp2 = self._makeBranchMergeProposal(
+ merge_target=bmp1.merge_target,
date_created=(
datetime(year=2008, month=10, day=10, tzinfo=pytz.UTC)))
self.assertEqual(
[bmp2], latest_proposals_for_each_branch([bmp1, bmp2]))
+
+
+class TestLatestProposalsForEachBranchBzr(
+ TestLatestProposalsForEachBranchMixin, TestCaseWithFactory):
+ """Confirm that the latest branch is returned for Bazaar."""
+
+ def _makeBranchMergeProposal(self, merge_target=None, **kwargs):
+ return self.factory.makeBranchMergeProposal(
+ target_branch=merge_target, **kwargs)
+
+ @staticmethod
+ def _setBranchInvisible(branch):
+ removeSecurityProxy(branch).transitionToInformationType(
+ InformationType.USERDATA, branch.owner, verify_policy=False)
+
+
+class TestLatestProposalsForEachBranchGit(
+ TestLatestProposalsForEachBranchMixin, TestCaseWithFactory):
+ """Confirm that the latest branch is returned for Bazaar."""
+
+ def setUp(self):
+ super(TestLatestProposalsForEachBranchGit, self).setUp()
+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+ def _makeBranchMergeProposal(self, merge_target=None, **kwargs):
+ return self.factory.makeBranchMergeProposalForGit(
+ target_ref=merge_target, **kwargs)
+
+ @staticmethod
+ def _setBranchInvisible(branch):
+ removeSecurityProxy(branch.repository).transitionToInformationType(
+ InformationType.USERDATA, branch.owner, verify_policy=False)
=== modified file 'lib/lp/code/errors.py'
--- lib/lp/code/errors.py 2015-04-19 12:56:32 +0000
+++ lib/lp/code/errors.py 2015-04-28 16:44:03 +0000
@@ -231,11 +231,18 @@
"""Raised if there is already a matching BranchMergeProposal."""
def __init__(self, existing_proposal):
+ # Circular import.
+ from lp.code.interfaces.branch import IBranch
+ # display_name is the newer style, but IBranch uses the older style.
+ if IBranch.providedBy(existing_proposal.merge_source):
+ display_name = "displayname"
+ else:
+ display_name = "display_name"
super(BranchMergeProposalExists, self).__init__(
'There is already a branch merge proposal registered for '
'branch %s to land on %s that is still active.' %
- (existing_proposal.source_branch.displayname,
- existing_proposal.target_branch.displayname))
+ (getattr(existing_proposal.merge_source, display_name),
+ getattr(existing_proposal.merge_target, display_name)))
self.existing_proposal = existing_proposal
=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py 2015-04-22 16:11:40 +0000
+++ lib/lp/code/interfaces/gitrepository.py 2015-04-28 16:44:03 +0000
@@ -49,6 +49,7 @@
Choice,
Datetime,
Int,
+ List,
Text,
TextLine,
)
@@ -607,6 +608,39 @@
:return: A collection of `IGitRepository` objects.
"""
+ @call_with(user=REQUEST_USER)
+ @operation_parameters(
+ person=Reference(
+ title=_("The person whose repository visibility is being "
+ "checked."),
+ schema=IPerson),
+ repository_names=List(value_type=Text(),
+ title=_('List of repository unique names'), required=True),
+ )
+ @export_read_operation()
+ @operation_for_version("devel")
+ def getRepositoryVisibilityInfo(user, person, repository_names):
+ """Return the named repositories visible to both user and person.
+
+ Anonymous requesters don't get any information.
+
+ :param user: The user requesting the information. If the user is
+ None then we return an empty dict.
+ :param person: The person whose repository visibility we wish to
+ check.
+ :param repository_names: The unique names of the repositories to
+ check.
+
+ Return a dict with the following values:
+ person_name: the displayname of the person.
+ visible_repositories: a list of the unique names of the repositories
+ which the requester and specified person can both see.
+
+ This API call is provided for use by the client Javascript. It is
+ not designed to efficiently scale to handle requests for large
+ numbers of repositories.
+ """
+
@operation_parameters(
target=Reference(
title=_("Target"), required=True, schema=IHasGitRepositories))
=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py 2015-04-24 12:58:46 +0000
+++ lib/lp/code/model/gitref.py 2015-04-28 16:44:03 +0000
@@ -376,3 +376,6 @@
def __ne__(self, other):
return not self == other
+
+ def __hash__(self):
+ return hash(self.repository) ^ hash(self.path) ^ hash(self.commit_sha1)
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2015-04-22 16:42:57 +0000
+++ lib/lp/code/model/gitrepository.py 2015-04-28 16:44:03 +0000
@@ -38,6 +38,7 @@
from storm.store import Store
from zope.component import getUtility
from zope.interface import implements
+from zope.security.interfaces import Unauthorized
from zope.security.proxy import removeSecurityProxy
from lp.app.enums import (
@@ -774,6 +775,27 @@
collection = IGitCollection(target).visibleByUser(user)
return collection.getRepositories(eager_load=True)
+ def getRepositoryVisibilityInfo(self, user, person, repository_names):
+ """See `IGitRepositorySet`."""
+ if user is None:
+ return dict()
+ lookup = getUtility(IGitLookup)
+ visible_repositories = []
+ for name in repository_names:
+ repository = lookup.getByUniqueName(name)
+ try:
+ if (repository is not None
+ and repository.visibleByUser(user)
+ and repository.visibleByUser(person)):
+ visible_repositories.append(repository.unique_name)
+ except Unauthorized:
+ # We don't include repositories user cannot see.
+ pass
+ return {
+ 'person_name': person.displayname,
+ 'visible_repositories': visible_repositories,
+ }
+
def getDefaultRepository(self, target):
"""See `IGitRepositorySet`."""
clauses = [GitRepository.target_default == True]
=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py 2015-04-21 23:05:48 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py 2015-04-28 16:44:03 +0000
@@ -1216,6 +1216,80 @@
public_repositories + [private_repository],
self.repository_set.getRepositories(other_person, project))
+ def test_getRepositoryVisibilityInfo_empty_repository_names(self):
+ # If repository_names is empty, getRepositoryVisibilityInfo returns
+ # an empty visible_repositories list.
+ person = self.factory.makePerson(name="fred")
+ info = self.repository_set.getRepositoryVisibilityInfo(
+ person, person, repository_names=[])
+ self.assertEqual("Fred", info["person_name"])
+ self.assertEqual([], info["visible_repositories"])
+
+ def test_getRepositoryVisibilityInfo(self):
+ person = self.factory.makePerson(name="fred")
+ owner = self.factory.makePerson()
+ visible_repository = self.factory.makeGitRepository()
+ invisible_repository = self.factory.makeGitRepository(
+ owner=owner, information_type=InformationType.USERDATA)
+ invisible_name = removeSecurityProxy(invisible_repository).unique_name
+ repositories = [visible_repository.unique_name, invisible_name]
+
+ with person_logged_in(owner):
+ info = self.repository_set.getRepositoryVisibilityInfo(
+ owner, person, repository_names=repositories)
+ self.assertEqual("Fred", info["person_name"])
+ self.assertEqual(
+ [visible_repository.unique_name], info["visible_repositories"])
+
+ def test_getRepositoryVisibilityInfo_unauthorised_user(self):
+ # If the user making the API request cannot see one of the
+ # repositories, that repository is not included in the results.
+ person = self.factory.makePerson(name="fred")
+ owner = self.factory.makePerson()
+ visible_repository = self.factory.makeGitRepository()
+ invisible_repository = self.factory.makeGitRepository(
+ owner=owner, information_type=InformationType.USERDATA)
+ invisible_name = removeSecurityProxy(invisible_repository).unique_name
+ repositories = [visible_repository.unique_name, invisible_name]
+
+ someone = self.factory.makePerson()
+ with person_logged_in(someone):
+ info = self.repository_set.getRepositoryVisibilityInfo(
+ someone, person, repository_names=repositories)
+ self.assertEqual("Fred", info["person_name"])
+ self.assertEqual(
+ [visible_repository.unique_name], info["visible_repositories"])
+
+ def test_getRepositoryVisibilityInfo_anonymous(self):
+ # Anonymous users are not allowed to see any repository visibility
+ # information, even if the repository they are querying about is
+ # public.
+ person = self.factory.makePerson(name="fred")
+ owner = self.factory.makePerson()
+ visible_repository = self.factory.makeGitRepository(owner=owner)
+ repositories = [visible_repository.unique_name]
+
+ with person_logged_in(owner):
+ info = self.repository_set.getRepositoryVisibilityInfo(
+ None, person, repository_names=repositories)
+ self.assertEqual({}, info)
+
+ def test_getRepositoryVisibilityInfo_invalid_repository_name(self):
+ # If an invalid repository name is specified, it is not included.
+ person = self.factory.makePerson(name="fred")
+ owner = self.factory.makePerson()
+ visible_repository = self.factory.makeGitRepository(owner=owner)
+ repositories = [
+ visible_repository.unique_name,
+ "invalid_repository_name"]
+
+ with person_logged_in(owner):
+ info = self.repository_set.getRepositoryVisibilityInfo(
+ owner, person, repository_names=repositories)
+ self.assertEqual("Fred", info["person_name"])
+ self.assertEqual(
+ [visible_repository.unique_name], info["visible_repositories"])
+
def test_setDefaultRepository_refuses_person(self):
# setDefaultRepository refuses if the target is a person.
person = self.factory.makePerson()
=== modified file 'lib/lp/code/templates/branchmergeproposal-resubmit.pt'
--- lib/lp/code/templates/branchmergeproposal-resubmit.pt 2010-11-04 16:56:35 +0000
+++ lib/lp/code/templates/branchmergeproposal-resubmit.pt 2015-04-28 16:44:03 +0000
@@ -24,7 +24,7 @@
</div>
</div>
- <div id="source-revisions">
+ <div id="source-revisions" tal:condition="context/source_branch">
<tal:history-available condition="context/source_branch/revision_count"
define="branch context/source_branch;
revisions view/unlanded_revisions">
=== modified file 'lib/lp/code/templates/gitref-index.pt'
--- lib/lp/code/templates/gitref-index.pt 2015-03-19 17:04:22 +0000
+++ lib/lp/code/templates/gitref-index.pt 2015-04-28 16:44:03 +0000
@@ -17,6 +17,13 @@
<div metal:fill-slot="main">
<div class="yui-g">
+ <div id="ref-relations" class="portlet">
+ <tal:ref-pending-merges
+ replace="structure context/@@++ref-pending-merges" />
+ </div>
+ </div>
+
+ <div class="yui-g">
<div id="ref-info" class="portlet">
<h2>Branch information</h2>
<div class="two-column-list">
=== added file 'lib/lp/code/templates/gitref-pending-merges.pt'
--- lib/lp/code/templates/gitref-pending-merges.pt 1970-01-01 00:00:00 +0000
+++ lib/lp/code/templates/gitref-pending-merges.pt 2015-04-28 16:44:03 +0000
@@ -0,0 +1,43 @@
+<div
+ xmlns:tal="http://xml.zope.org/namespaces/tal";
+ xmlns:metal="http://xml.zope.org/namespaces/metal";
+ xmlns:i18n="http://xml.zope.org/namespaces/i18n";
+ tal:define="
+ context_menu view/context/menu:context;
+ features request/features"
+ tal:condition="view/show_merge_links">
+
+ <h3>Branch merges</h3>
+ <div id="merge-links"
+ class="actions">
+ <div id="merge-summary">
+
+ <div id="landing-candidates"
+ tal:condition="view/landing_candidates">
+ <img src="/@@/merge-proposal-icon" />
+ <a href="+activereviews" tal:content="structure view/landing_candidate_count_text">
+ 1 branch
+ </a>
+ proposed for merging into this one.
+
+ </div>
+
+ <div id="dependent-landings" tal:condition="view/dependent_landings">
+ <img src="/@@/merge-proposal-icon" />
+ <a href="+merges" tal:content="structure view/dependent_landing_count_text">
+ 1 branch
+ </a>
+ dependent on this one.
+ </div>
+
+ <div id="landing-targets" tal:condition="view/landing_targets">
+ <tal:landing-candidates repeat="mergeproposal view/landing_targets">
+ <tal:merge-fragment
+ tal:replace="structure mergeproposal/@@+summary-fragment"/>
+ </tal:landing-candidates>
+ </div>
+
+ </div>
+ </div>
+
+</div>
=== modified file 'lib/lp/code/vocabularies/gitrepository.py'
--- lib/lp/code/vocabularies/gitrepository.py 2015-04-13 19:02:15 +0000
+++ lib/lp/code/vocabularies/gitrepository.py 2015-04-28 16:44:03 +0000
@@ -19,17 +19,17 @@
from lp.services.webapp.vocabulary import (
CountableIterator,
IHugeVocabulary,
- SQLObjectVocabularyBase,
+ StormVocabularyBase,
)
-class GitRepositoryVocabulary(SQLObjectVocabularyBase):
+class GitRepositoryVocabulary(StormVocabularyBase):
"""A vocabulary for searching Git repositories."""
implements(IHugeVocabulary)
_table = GitRepository
- _orderBy = ['name', 'id']
+ _order_by = ['name', 'id']
displayname = 'Select a Git repository'
step_title = 'Search'
=== modified file 'lib/lp/services/webapp/marshallers.py'
--- lib/lp/services/webapp/marshallers.py 2010-03-26 19:18:31 +0000
+++ lib/lp/services/webapp/marshallers.py 2015-04-28 16:44:03 +0000
@@ -10,12 +10,13 @@
def choiceMarshallerError(field, request, vocabulary=None):
# We don't support marshalling a normal Choice field with a
- # SQLObjectVocabularyBase-based vocabulary.
+ # SQLObjectVocabularyBase/StormVocabularyBase-based vocabulary.
# Normally for this kind of use case, one returns None and
# lets the Zope machinery alert the user that the lookup has gone wrong.
# However, we want to be more helpful, so we make an assertion,
# with a comment on how to make things better.
raise AssertionError("You exported %s as an IChoice based on an "
- "SQLObjectVocabularyBase, you should use "
- "lazr.restful.fields.ReferenceChoice instead."
+ "SQLObjectVocabularyBase/StormVocabularyBase; you "
+ "should use lazr.restful.fields.ReferenceChoice "
+ "instead."
% field.__name__)
=== modified file 'lib/lp/services/webapp/vocabulary.py'
--- lib/lp/services/webapp/vocabulary.py 2013-01-07 02:40:55 +0000
+++ lib/lp/services/webapp/vocabulary.py 2015-04-28 16:44:03 +0000
@@ -10,14 +10,15 @@
__metaclass__ = type
__all__ = [
+ 'BatchedCountableIterator',
+ 'CountableIterator',
'FilteredVocabularyBase',
'ForgivingSimpleVocabulary',
'IHugeVocabulary',
+ 'NamedSQLObjectHugeVocabulary',
+ 'NamedSQLObjectVocabulary',
'SQLObjectVocabularyBase',
- 'NamedSQLObjectVocabulary',
- 'NamedSQLObjectHugeVocabulary',
- 'CountableIterator',
- 'BatchedCountableIterator',
+ 'StormVocabularyBase',
'VocabularyFilter',
]
@@ -28,6 +29,8 @@
AND,
CONTAINSSTRING,
)
+from storm.base import Storm
+from storm.store import EmptyResultSet
from zope.interface import (
Attribute,
implements,
@@ -43,6 +46,7 @@
)
from zope.security.proxy import isinstance as zisinstance
+from lp.services.database.interfaces import IStore
from lp.services.database.sqlbase import SQLBase
from lp.services.helpers import ensure_unicode
@@ -464,3 +468,120 @@
clause = AND(clause, self._filter)
results = self._table.select(clause, orderBy=self._orderBy)
return self.iterator(results.count(), results, self.toTerm)
+
+
+class StormVocabularyBase(FilteredVocabularyBase):
+ """A base class for widgets that are rendered to collect values
+ for attributes that are Storm references.
+
+ So if a content class behind some form looks like:
+
+ class Foo(StormBase):
+ id = Int(...)
+ bar_id = Int(...)
+ bar = Reference(bar_id, ...)
+ ...
+
+ Then the vocabulary for the widget that captures a value for bar
+ should derive from StormVocabularyBase.
+ """
+ implements(IVocabulary, IVocabularyTokenized)
+ _order_by = None
+ _clauses = []
+
+ def __init__(self, context=None):
+ self.context = context
+
+ # XXX kiko 2007-01-16: note that the method searchForTerms is part of
+ # IHugeVocabulary, and so should not necessarily need to be
+ # implemented here; however, many of our vocabularies depend on
+ # searchForTerms for popup functionality so I have chosen to just do
+ # that. It is possible that a better solution would be to have the
+ # search functionality produce a new vocabulary restricted to the
+ # desired subset.
+ def searchForTerms(self, query=None, vocab_filter=None):
+ results = self.search(query, vocab_filter)
+ return CountableIterator(results.count(), results, self.toTerm)
+
+ def search(self, query, vocab_filter=None):
+ # This default implementation of searchForTerms glues together
+ # the legacy API of search() with the toTerm method. If you
+ # don't reimplement searchForTerms you will need to at least
+ # provide your own search() method.
+ raise NotImplementedError
+
+ def toTerm(self, obj):
+ # This default implementation assumes that your object has a
+ # title attribute. If it does not you will need to reimplement
+ # toTerm, or reimplement the whole searchForTerms.
+ return SimpleTerm(obj, obj.id, obj.title)
+
+ @property
+ def _entries(self):
+ entries = IStore(self._table).find(self._table, *self._clauses)
+ if self._order_by is not None:
+ entries = entries.order_by(self._order_by)
+ return entries
+
+ def __iter__(self):
+ """Return an iterator which provides the terms from the vocabulary."""
+ for obj in self._entries:
+ yield self.toTerm(obj)
+
+ def __len__(self):
+ return self._entries.count()
+
+ def __contains__(self, obj):
+ # Sometimes this method is called with a Storm instance, but z3 form
+ # machinery sends through integer ids. This might be due to a bug
+ # somewhere.
+ if zisinstance(obj, Storm):
+ clauses = [self._table.id == obj.id]
+ if self._clauses:
+ # XXX kiko 2007-01-16: this code is untested.
+ clauses.extend(self._clauses)
+ found_obj = IStore(self._table).find(self._table, *clauses).one()
+ return found_obj is not None and found_obj == obj
+ else:
+ clauses = [self._table.id == int(obj)]
+ if self._clauses:
+ # XXX kiko 2007-01-16: this code is untested.
+ clauses.extend(self._clauses)
+ found_obj = IStore(self._table).find(self._table, *clauses).one()
+ return found_obj is not None
+
+ def getTerm(self, value):
+ # Short circuit. There is probably a design problem here since we
+ # sometimes get the id and sometimes a Storm instance.
+ if zisinstance(value, Storm):
+ return self.toTerm(value)
+
+ try:
+ value = int(value)
+ except ValueError:
+ raise LookupError(value)
+
+ clauses = [self._table.id == value]
+ if self._clauses:
+ clauses.extend(self._clauses)
+ try:
+ obj = IStore(self._table).find(self._table, *clauses).one()
+ except ValueError:
+ raise LookupError(value)
+
+ if obj is None:
+ raise LookupError(value)
+
+ return self.toTerm(obj)
+
+ def getTermByToken(self, token):
+ return self.getTerm(token)
+
+ def emptySelectResults(self):
+ """Return a SelectResults object without any elements.
+
+ This is to be used when no search string is given to the search()
+ method of subclasses, in order to be consistent and always return
+ a SelectResults object.
+ """
+ return EmptyResultSet()
=== modified file 'lib/lp/services/webservice/configure.zcml'
--- lib/lp/services/webservice/configure.zcml 2012-01-31 01:19:45 +0000
+++ lib/lp/services/webservice/configure.zcml 2015-04-28 16:44:03 +0000
@@ -64,12 +64,26 @@
factory="lp.services.webapp.marshallers.choiceMarshallerError"
/>
<adapter
+ for="zope.schema.interfaces.IChoice
+ zope.publisher.interfaces.http.IHTTPRequest
+ lp.services.webapp.vocabulary.StormVocabularyBase"
+ provides="lazr.restful.interfaces.IFieldMarshaller"
+ factory="lp.services.webapp.marshallers.choiceMarshallerError"
+ />
+ <adapter
for="lazr.restful.interfaces.IReferenceChoice
zope.publisher.interfaces.http.IHTTPRequest
lp.services.webapp.vocabulary.SQLObjectVocabularyBase"
provides="lazr.restful.interfaces.IFieldMarshaller"
factory="lazr.restful.marshallers.ObjectLookupFieldMarshaller"
/>
+ <adapter
+ for="lazr.restful.interfaces.IReferenceChoice
+ zope.publisher.interfaces.http.IHTTPRequest
+ lp.services.webapp.vocabulary.StormVocabularyBase"
+ provides="lazr.restful.interfaces.IFieldMarshaller"
+ factory="lazr.restful.marshallers.ObjectLookupFieldMarshaller"
+ />
<!-- The API documentation -->
<browser:page
=== modified file 'lib/lp/services/webservice/doc/webservice-marshallers.txt'
--- lib/lp/services/webservice/doc/webservice-marshallers.txt 2011-12-24 17:49:30 +0000
+++ lib/lp/services/webservice/doc/webservice-marshallers.txt 2015-04-28 16:44:03 +0000
@@ -99,7 +99,7 @@
Traceback (most recent call last):
...
AssertionError: You exported some_person as an IChoice based on an
- SQLObjectVocabularyBase, you should use
+ SQLObjectVocabularyBase/StormVocabularyBase; you should use
lazr.restful.fields.ReferenceChoice instead.
Cleanup.
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2015-04-22 16:11:40 +0000
+++ lib/lp/testing/factory.py 2015-04-28 16:44:03 +0000
@@ -1747,7 +1747,8 @@
u"type": GitObjectType.COMMIT,
}
for path in paths}
- return repository.createOrUpdateRefs(refs_info, get_objects=True)
+ return removeSecurityProxy(repository).createOrUpdateRefs(
+ refs_info, get_objects=True)
def makeBug(self, target=None, owner=None, bug_watch_url=None,
information_type=None, date_closed=None, title=None,
