launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18409
Re: [Merge] lp:~cjwatson/launchpad/git-mp-basic-browser into lp:launchpad
Review: Approve code
One bit of useless code, otherwise looks good.
Diff comments:
> === 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:48:46 +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(),
> + })
This isn't useful, as the Same Origin Policy will forbid the request. I'd consider just not implementing the (disabled for all but ~launchpad) feature for Git yet.
> 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:48:46 +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:48:46 +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:48:46 +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:48:46 +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:48:46 +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:48:46 +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:48:46 +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:48:46 +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:48:46 +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:48:46 +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:48:46 +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/testing/factory.py'
> --- lib/lp/testing/factory.py 2015-04-22 16:11:40 +0000
> +++ lib/lp/testing/factory.py 2015-04-28 16:48:46 +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,
>
--
https://code.launchpad.net/~cjwatson/launchpad/git-mp-basic-browser/+merge/257674
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.
References
