launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #15558
[Merge] lp:~stevenk/launchpad/switch-bmp-to-previewdiff-merge_proposal into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/switch-bmp-to-previewdiff-merge_proposal into lp:launchpad with lp:~stevenk/launchpad/index-previewdiff-merge_proposal as a prerequisite.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/switch-bmp-to-previewdiff-merge_proposal/+merge/162916
Switch everything over to using PreviewDiff.merge_propsal, and remove BranchMergeProposal.preview_diff in preparation for dropping the column.
--
https://code.launchpad.net/~stevenk/launchpad/switch-bmp-to-previewdiff-merge_proposal/+merge/162916
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/switch-bmp-to-previewdiff-merge_proposal into lp:launchpad.
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposal.py'
--- lib/lp/code/browser/tests/test_branchmergeproposal.py 2013-05-08 02:25:38 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposal.py 2013-05-08 02:25:39 +0000
@@ -871,25 +871,23 @@
# librarian content.
text = ''.join(chr(x) for x in range(255))
diff_bytes = ''.join(unified_diff('', text))
- self.setPreviewDiff(diff_bytes)
+ preview_diff = self.setPreviewDiff(diff_bytes)
transaction.commit()
def fake_open(*args):
raise LibrarianServerError
- lfa = removeSecurityProxy(self.bmp.preview_diff).diff.diff_text
+ lfa = preview_diff.diff.diff_text
with monkey_patch(lfa, open=fake_open):
- view = create_initialized_view(self.bmp.preview_diff, '+diff')
+ view = create_initialized_view(preview_diff, '+diff')
self.assertEqual('', view.preview_diff_text)
self.assertFalse(view.diff_available)
markup = view()
self.assertIn('The diff is not available at this time.', markup)
def setPreviewDiff(self, preview_diff_bytes):
- preview_diff = PreviewDiff.create(
+ return PreviewDiff.create(
self.bmp, preview_diff_bytes, u'a', u'b', None, u'')
- removeSecurityProxy(self.bmp).preview_diff = preview_diff
- return preview_diff
def test_linked_bugs_excludes_mutual_bugs(self):
"""List bugs that are linked to the source only."""
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2013-03-15 01:27:19 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2013-05-08 02:25:39 +0000
@@ -207,7 +207,7 @@
with StormStatementRecorder() as recorder:
self.getViewBrowser(
product, '+merges', rootsite='code', user=product.owner)
- self.assertThat(recorder, HasQueryCount(Equals(40)))
+ self.assertThat(recorder, HasQueryCount(Equals(41)))
def test_productseries(self):
target = self.factory.makeBranch()
=== modified file 'lib/lp/code/doc/branch-merge-proposal-notifications.txt'
--- lib/lp/code/doc/branch-merge-proposal-notifications.txt 2013-05-08 02:25:38 +0000
+++ lib/lp/code/doc/branch-merge-proposal-notifications.txt 2013-05-08 02:25:39 +0000
@@ -19,20 +19,16 @@
>>> from lp.code.interfaces.branchmergeproposal import (
... IBranchMergeProposalJobSource)
>>> from lp.code.model.diff import PreviewDiff
- >>> from lp.code.tests.helpers import (
- ... make_merge_proposal_without_reviewers)
+ >>> from lp.code.tests.helpers import make_merge_proposal_without_reviewers
>>> from lp.testing.mail_helpers import pop_notifications
>>> import transaction
>>> login('test@xxxxxxxxxxxxx')
- >>> diff_bytes = ''.join(unified_diff('', 'c'))
- >>> preview_diff = PreviewDiff.create(
- ... None, diff_bytes, u'a', u'b', None, None)
- >>> # Make Librarian happy.
- >>> transaction.commit()
>>> target_owner = factory.makePerson(email='target_owner@xxxxxxxxxxx')
>>> target_branch = factory.makeBranch(owner=target_owner)
>>> bmp = make_merge_proposal_without_reviewers(
... factory, target_branch=target_branch)
+ >>> previewdiff = factory.makePreviewDiff(merge_proposal=bmp)
+ >>> transaction.commit()
>>> source_subscriber = factory.makePerson(
... email='source@xxxxxxxxxxx', displayname='Source Subscriber')
>>> _unused = bmp.source_branch.subscribe(source_subscriber,
@@ -111,7 +107,8 @@
>>> # To avoid needing to access branches, pre-populate diffs.
>>> bmp = source_branch.addLandingTarget(
... registrant, target_branch, needs_review=True)
- >>> removeSecurityProxy(bmp).preview_diff = preview_diff
+ >>> previewdiff = factory.makePreviewDiff(merge_proposal=bmp)
+ >>> transaction.commit()
>>> # Fake the update preview diff as done.
>>> bmp.next_preview_diff_job.start()
>>> bmp.next_preview_diff_job.complete()
@@ -165,10 +162,10 @@
... """)
>>> bmp.deleteProposal()
>>> bmp = source_branch.addLandingTarget(
- ... registrant, target_branch,
- ... description=initial_comment, review_requests=reviewers,
- ... needs_review=True)
- >>> removeSecurityProxy(bmp).preview_diff = preview_diff
+ ... registrant, target_branch, description=initial_comment,
+ ... review_requests=reviewers, needs_review=True)
+ >>> previewdiff = factory.makePreviewDiff(merge_proposal=bmp)
+ >>> transaction.commit()
>>> # Fake the update preview diff as done.
>>> bmp.next_preview_diff_job.start()
>>> bmp.next_preview_diff_job.complete()
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py 2013-02-28 01:02:50 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py 2013-05-08 02:25:39 +0000
@@ -174,6 +174,8 @@
next_preview_diff_job = Attribute(
'The next BranchMergeProposalJob that will update a preview diff.')
+ preview_diffs = Attribute('All preview diffs for this merge proposal.')
+
preview_diff = exported(
Reference(
IPreviewDiff,
=== modified file 'lib/lp/code/mail/tests/test_branchmergeproposal.py'
--- lib/lp/code/mail/tests/test_branchmergeproposal.py 2013-05-08 02:25:38 +0000
+++ lib/lp/code/mail/tests/test_branchmergeproposal.py 2013-05-08 02:25:39 +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 BranchMergeProposal mailings"""
@@ -67,15 +67,12 @@
else:
initial_status = BranchMergeProposalStatus.WORK_IN_PROGRESS
bmp = self.factory.makeBranchMergeProposal(
- registrant=registrant, product=product,
- set_state=initial_status,
+ registrant=registrant, product=product, set_state=initial_status,
prerequisite_branch=prerequisite_branch,
- initial_comment=initial_comment,
- reviewer=reviewer)
- if diff_text is not None:
- removeSecurityProxy(bmp).preview_diff = PreviewDiff.create(
- bmp, diff_text,
- unicode(self.factory.getUniqueString('revid')),
+ initial_comment=initial_comment, reviewer=reviewer)
+ if diff_text:
+ PreviewDiff.create(
+ bmp, diff_text, unicode(self.factory.getUniqueString('revid')),
unicode(self.factory.getUniqueString('revid')), None, None)
transaction.commit()
subscriber = self.factory.makePerson(displayname='Baz Quxx',
=== modified file 'lib/lp/code/mail/tests/test_codehandler.py'
--- lib/lp/code/mail/tests/test_codehandler.py 2013-05-08 02:25:38 +0000
+++ lib/lp/code/mail/tests/test_codehandler.py 2013-05-08 02:25:39 +0000
@@ -381,15 +381,10 @@
def test_reviewer_with_diff(self):
"""Requesting a review with a diff works."""
- diff_text = ''.join(unified_diff('', 'Fake diff'))
- preview_diff = PreviewDiff.create(
- None, diff_text,
- unicode(self.factory.getUniqueString('revid')),
- unicode(self.factory.getUniqueString('revid')), None, None)
+ bmp = make_merge_proposal_without_reviewers(self.factory)
+ preview_diff = self.factory.makePreviewDiff(merge_proposal=bmp)
# To record the diff in the librarian.
transaction.commit()
- bmp = make_merge_proposal_without_reviewers(
- self.factory, preview_diff=preview_diff)
eric = self.factory.makePerson(name="eric", email="eric@xxxxxxxxxxx")
mail = self.factory.makeSignedMessage(body=' reviewer eric')
email_addr = bmp.address
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2013-05-08 02:25:38 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2013-05-08 02:25:39 +0000
@@ -27,10 +27,7 @@
Select,
SQL,
)
-from storm.locals import (
- Int,
- Reference,
- )
+from storm.locals import Reference
from storm.store import Store
from zope.component import getUtility
from zope.event import notify
@@ -104,6 +101,10 @@
from lp.services.job.interfaces.job import JobStatus
from lp.services.job.model.job import Job
from lp.services.mail.sendmail import validate_message
+from lp.services.propertycache import (
+ cachedproperty,
+ get_property_cache,
+ )
def is_valid_transition(proposal, from_state, next_state, user=None):
@@ -226,9 +227,6 @@
else:
return None
- preview_diff_id = Int(name='merge_diff')
- preview_diff = Reference(preview_diff_id, 'PreviewDiff.id')
-
reviewed_revision_id = StringCol(default=None)
commit_message = StringCol(default=None)
@@ -309,6 +307,19 @@
raise WrongBranchMergeProposal
return vote
+ @cachedproperty
+ def preview_diffs(self):
+ return Store.of(self).find(
+ PreviewDiff, PreviewDiff.merge_proposal_id == self.id).order_by(
+ PreviewDiff.date_created)
+
+ @property
+ def preview_diff(self):
+ diffs = list(self.preview_diffs)
+ if diffs:
+ return diffs[-1]
+ return None
+
date_queued = UtcDateTimeCol(notNull=False, default=None)
votes = SQLMultipleJoin(
@@ -706,11 +717,11 @@
comment.destroySelf()
# Delete all jobs referring to the BranchMergeProposal, whether
# or not they have completed.
- from lp.code.model.branchmergeproposaljob import (
- BranchMergeProposalJob)
+ from lp.code.model.branchmergeproposaljob import BranchMergeProposalJob
for job in BranchMergeProposalJob.selectBy(
branch_merge_proposal=self.id):
job.destroySelf()
+ self.preview_diffs.remove()
self.destroySelf()
def getUnlandedSourceBranchRevisions(self):
@@ -860,18 +871,10 @@
target_revision_id, prerequisite_revision_id=None,
conflicts=None):
"""See `IBranchMergeProposal`."""
- # Create the PreviewDiff.
- self.preview_diff = PreviewDiff.create(
+ return PreviewDiff.create(
self, diff_content, source_revision_id, target_revision_id,
prerequisite_revision_id, conflicts)
- # XXX: TimPenhey 2009-02-19 bug 324724
- # Since the branch_merge_proposal attribute of the preview_diff
- # is a on_remote reference, it may not be found unless we flush
- # the storm store.
- Store.of(self).flush()
- return self.preview_diff
-
def getIncrementalDiffRanges(self):
groups = self.getRevisionsSinceReviewStart()
return [
@@ -965,14 +968,15 @@
from lp.registry.model.product import Product
from lp.registry.model.distroseries import DistroSeries
+ ids = set()
source_branch_ids = set()
person_ids = set()
- diff_ids = set()
for mp in branch_merge_proposals:
+ ids.add(mp.id)
source_branch_ids.add(mp.source_branchID)
person_ids.add(mp.registrantID)
person_ids.add(mp.merge_reporterID)
- diff_ids.add(mp.preview_diff_id)
+ get_property_cache(mp).preview_diffs = []
branches = load_related(
Branch, branch_merge_proposals, (
@@ -985,16 +989,13 @@
if len(branches) == 0:
return
- store = IStore(BranchMergeProposal)
-
# Pre-load PreviewDiffs and Diffs.
- preview_diffs_and_diffs = list(store.find(
- (PreviewDiff, Diff),
- PreviewDiff.id.is_in(diff_ids),
- Diff.id == PreviewDiff.diff_id))
- PreviewDiff.preloadData(
- [preview_diff_and_diff[0] for preview_diff_and_diff
- in preview_diffs_and_diffs])
+ preview_diffs = IStore(BranchMergeProposal).find(
+ PreviewDiff, PreviewDiff.merge_proposal_id.is_in(ids))
+ load_related(Diff, preview_diffs, ['diff_id'])
+ for previewdiff in preview_diffs:
+ cache = get_property_cache(previewdiff.merge_proposal)
+ cache.preview_diffs.append(previewdiff)
# Add source branch owners' to the list of pre-loaded persons.
person_ids.update(
=== modified file 'lib/lp/code/model/branchmergeproposaljob.py'
--- lib/lp/code/model/branchmergeproposaljob.py 2012-11-30 05:10:47 +0000
+++ lib/lp/code/model/branchmergeproposaljob.py 2013-05-08 02:25:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 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).
"""Job classes related to BranchMergeProposals are in here.
@@ -371,10 +371,8 @@
"""See `IRunnableJob`."""
self.checkReady()
with server(get_ro_server(), no_replace=True):
- preview = PreviewDiff.fromBranchMergeProposal(
- self.branch_merge_proposal)
- with BranchMergeProposalDelta.monitor(self.branch_merge_proposal):
- self.branch_merge_proposal.preview_diff = preview
+ with BranchMergeProposalDelta.monitor(self.branch_merge_proposal):
+ PreviewDiff.fromBranchMergeProposal(self.branch_merge_proposal)
def getOperationDescription(self):
return ('generating the diff for a merge proposal')
=== modified file 'lib/lp/code/model/diff.py'
--- lib/lp/code/model/diff.py 2013-05-08 02:25:38 +0000
+++ lib/lp/code/model/diff.py 2013-05-08 02:25:39 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 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).
"""Implementation classes for IDiff, etc."""
@@ -48,7 +48,6 @@
)
from lp.codehosting.bzrutils import read_locked
from lp.services.config import config
-from lp.services.database.bulk import load_referencing
from lp.services.database.constants import UTC_NOW
from lp.services.database.datetimecol import UtcDateTimeCol
from lp.services.database.sqlbase import SQLBase
@@ -56,10 +55,7 @@
from lp.services.librarian.interfaces.client import (
LIBRARIAN_SERVER_DEFAULT_TIMEOUT,
)
-from lp.services.propertycache import (
- cachedproperty,
- get_property_cache,
- )
+from lp.services.propertycache import get_property_cache
from lp.services.webapp.adapter import get_request_remaining_seconds
@@ -375,27 +371,9 @@
def has_conflicts(self):
return self.conflicts is not None and self.conflicts != ''
- @staticmethod
- def preloadData(preview_diffs):
- # Circular imports.
- from lp.code.model.branchmergeproposal import BranchMergeProposal
- bmps = load_referencing(
- BranchMergeProposal, preview_diffs, ['preview_diff_id'])
- bmps_preview = dict((bmp.preview_diff_id, bmp) for bmp in bmps)
- for preview_diff in preview_diffs:
- cache = get_property_cache(preview_diff)
- cache.branch_merge_proposal = bmps_preview[preview_diff.id]
-
- _branch_merge_proposal = Reference(
- "PreviewDiff.id", "BranchMergeProposal.preview_diff_id",
- on_remote=True)
-
- @cachedproperty
+ @property
def branch_merge_proposal(self):
- # This can turn into return self.merge_proposal when it's populated.
- if self.merge_proposal:
- return self.merge_proposal
- return self._branch_merge_proposal
+ return self.merge_proposal
@classmethod
def fromBranchMergeProposal(cls, bmp):
@@ -414,9 +392,7 @@
else:
prerequisite_branch = None
diff, conflicts = Diff.mergePreviewFromBranches(
- source_branch, source_revision, target_branch,
- prerequisite_branch)
-
+ source_branch, source_revision, target_branch, prerequisite_branch)
preview = cls()
preview.source_revision_id = source_revision.decode('utf-8')
preview.target_revision_id = target_revision.decode('utf-8')
@@ -424,6 +400,7 @@
preview.diff = diff
preview.conflicts = u''.join(
unicode(conflict) + '\n' for conflict in conflicts)
+ del get_property_cache(bmp).preview_diffs
return preview
@classmethod
=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt 2013-05-08 02:25:38 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposal-listings.txt 2013-05-08 02:25:39 +0000
@@ -173,14 +173,11 @@
After a preview diff is added, it has a line count.
- >>> from lp.code.model.diff import PreviewDiff
>>> login('foo.bar@xxxxxxxxxxxxx')
- >>> removeSecurityProxy(proposal).preview_diff = PreviewDiff.create(
- ... proposal, 'a\n\b\n', u'source-id', u'target-id', u'prereq-id',
- ... u'conflicts')
+ >>> diff = factory.makePreviewDiff(merge_proposal=proposal)
>>> logout()
>>> browser.open(url)
>>> print_tag_with_id(browser.contents, 'proposals')
Other reviews I am not actively reviewing
Branch Merge Proposal Requested By Lines Activity
- lp://dev/... ... 2 None
+ lp://dev/... ... 14 None
=== modified file 'lib/lp/code/stories/branches/xx-branchmergeproposals.txt'
--- lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2013-05-08 02:25:38 +0000
+++ lib/lp/code/stories/branches/xx-branchmergeproposals.txt 2013-05-08 02:25:39 +0000
@@ -609,11 +609,10 @@
>>> from lp.code.model.diff import PreviewDiff
>>> diff_text = ''.join(
... unified_diff('', ['Fake Diff' + u'\u1010'.encode('utf-8')]))
+ >>> bmp = factory.makeBranchMergeProposal()
>>> preview_diff = PreviewDiff.create(
- ... None, diff_text, u'a', u'b', None, u'')
+ ... bmp, diff_text, u'a', u'b', None, u'')
>>> transaction.commit()
- >>> bmp = factory.makeBranchMergeProposal()
- >>> removeSecurityProxy(bmp).preview_diff = preview_diff
>>> url = canonical_url(bmp)
>>> logout()
>>> def get_review_diff():
@@ -642,17 +641,22 @@
If no diff is present, nothing is shown.
- >>> login('admin@xxxxxxxxxxxxx')
- >>> removeSecurityProxy(bmp).preview_diff = None
- >>> logout()
+ >>> from storm.store import Store
+ >>> Store.of(preview_diff).remove(preview_diff)
+ >>> from lp.services.propertycache import get_property_cache
+ >>> del get_property_cache(bmp).preview_diffs
>>> print get_review_diff()
None
If the review diff is empty, then we say it is empty.
>>> login('admin@xxxxxxxxxxxxx')
+<<<<<<< TREE
+ >>> preview_diff = PreviewDiff.create(bmp, '', u'c', u'd', None, u'')
+=======
>>> removeSecurityProxy(bmp).preview_diff = PreviewDiff.create(
... bmp, '', u'c', u'd', None, u'')
+>>>>>>> MERGE-SOURCE
>>> logout()
>>> print extract_text(get_review_diff())
Preview Diff
@@ -662,8 +666,7 @@
Preview diff generation status
------------------------------
- >>> update = find_tag_by_id(
- ... nopriv_browser.contents, 'diff-pending-update')
+ >>> update = find_tag_by_id(nopriv_browser.contents, 'diff-pending-update')
>>> print extract_text(update)
Updating diff...
An updated diff will be available in a few minutes. Reload to see the
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2013-05-08 02:25:38 +0000
+++ lib/lp/testing/factory.py 2013-05-08 02:25:39 +0000
@@ -1483,9 +1483,9 @@
def makeBranchMergeProposal(self, target_branch=None, registrant=None,
set_state=None, prerequisite_branch=None,
product=None, initial_comment=None,
- source_branch=None, preview_diff=None,
- date_created=None, description=None,
- reviewer=None, merged_revno=None):
+ source_branch=None, date_created=None,
+ description=None, reviewer=None,
+ merged_revno=None):
"""Create a proposal to merge based on anonymous branches."""
if target_branch is not None:
target_branch = removeSecurityProxy(target_branch)
@@ -1521,8 +1521,6 @@
unsafe_proposal = removeSecurityProxy(proposal)
unsafe_proposal.merged_revno = merged_revno
- if preview_diff is not None:
- unsafe_proposal.preview_diff = preview_diff
if (set_state is None or
set_state == BranchMergeProposalStatus.WORK_IN_PROGRESS):
# The initial state is work in progress, so do nothing.
