launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #18303
[Merge] lp:~cjwatson/launchpad/git-mail into lp:launchpad
Colin Watson has proposed merging lp:~cjwatson/launchpad/git-mail into lp:launchpad with lp:~cjwatson/launchpad/git-subscriptions as a prerequisite.
Commit message:
Add basic mail infrastructure for Git.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #1444591 in Launchpad itself: "Allow Git repository subscriptions"
https://bugs.launchpad.net/launchpad/+bug/1444591
For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/git-mail/+merge/256523
Add basic mail infrastructure for Git.
This is really just about enough to send mail for attribute changes to Git repositories, and to support near-future work on merge proposals. It mostly consists of shims to make IBranch and IGitRef look sufficiently alike, and some initial preparation of BranchMergeProposal for being able to handle both Bazaar and Git.
--
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/git-mail into lp:launchpad.
=== modified file 'lib/lp/code/adapters/branch.py'
--- lib/lp/code/adapters/branch.py 2012-05-18 04:14:08 +0000
+++ lib/lp/code/adapters/branch.py 2015-04-16 15:40:22 +0000
@@ -53,17 +53,16 @@
self.whiteboard = whiteboard
self.lifecycle_status = lifecycle_status
- @staticmethod
- def construct(old_branch, new_branch, user):
+ @classmethod
+ def construct(klass, old_branch, new_branch, user):
"""Return a BranchDelta instance that encapsulates the changes.
This method is primarily used by event subscription code to
determine what has changed during an ObjectModifiedEvent.
"""
delta = ObjectDelta(old_branch, new_branch)
- delta.recordNewValues(("summary", "whiteboard"))
- delta.recordNewAndOld(("name", "lifecycle_status",
- "title", "url"))
+ delta.recordNewValues(klass.new_values)
+ delta.recordNewAndOld(klass.delta_values)
# delta.record_list_added_and_removed()
# XXX thumper 2006-12-21: Add in bugs and specs.
if delta.changes:
=== added file 'lib/lp/code/adapters/gitrepository.py'
--- lib/lp/code/adapters/gitrepository.py 1970-01-01 00:00:00 +0000
+++ lib/lp/code/adapters/gitrepository.py 2015-04-16 15:40:22 +0000
@@ -0,0 +1,52 @@
+# Copyright 2009-2011 Canonical Ltd. This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Components related to Git repositories."""
+
+__metaclass__ = type
+__all__ = [
+ "GitRepositoryDelta",
+ ]
+
+from lazr.lifecycle.objectdelta import ObjectDelta
+from zope.interface import implements
+
+from lp.code.interfaces.gitrepository import (
+ IGitRepository,
+ IGitRepositoryDelta,
+ )
+
+
+class GitRepositoryDelta:
+ """See `IGitRepositoryDelta`."""
+
+ implements(IGitRepositoryDelta)
+
+ delta_values = ('name', 'identity')
+
+ interface = IGitRepository
+
+ def __init__(self, repository, user, name=None, identity=None):
+ self.repository = repository
+ self.user = user
+
+ self.name = name
+ self.identity = identity
+
+ @classmethod
+ def construct(klass, old_repository, new_repository, user):
+ """Return a GitRepositoryDelta instance that encapsulates the changes.
+
+ This method is primarily used by event subscription code to
+ determine what has changed during an ObjectModifiedEvent.
+ """
+ delta = ObjectDelta(old_repository, new_repository)
+ delta.recordNewAndOld(klass.delta_values)
+ if delta.changes:
+ changes = delta.changes
+ changes["repository"] = new_repository
+ changes["user"] = user
+
+ return GitRepositoryDelta(**changes)
+ else:
+ return None
=== modified file 'lib/lp/code/configure.zcml'
--- lib/lp/code/configure.zcml 2015-04-16 15:40:21 +0000
+++ lib/lp/code/configure.zcml 2015-04-16 15:40:22 +0000
@@ -839,6 +839,12 @@
<allow interface="lp.code.interfaces.gitrepository.IGitRepositorySet" />
</securedutility>
+ <!-- GitRepositoryDelta -->
+
+ <class class="lp.code.adapters.gitrepository.GitRepositoryDelta">
+ <allow interface="lp.code.interfaces.gitrepository.IGitRepositoryDelta"/>
+ </class>
+
<!-- GitSubscription -->
<class class="lp.code.model.gitsubscription.GitSubscription">
=== modified file 'lib/lp/code/doc/branch-notifications.txt'
--- lib/lp/code/doc/branch-notifications.txt 2010-05-27 02:04:21 +0000
+++ lib/lp/code/doc/branch-notifications.txt 2015-04-16 15:40:22 +0000
@@ -41,8 +41,8 @@
... BranchSubscriptionNotificationLevel.FULL,
... BranchSubscriptionDiffSize.WHOLEDIFF,
... CodeReviewNotificationLevel.NOEMAIL, branch.owner)
- >>> BranchMailer.forRevision(branch, 1, 'foo@xxxxxxxxxxxxx',
- ... 'The contents.', None, 'Subject line').sendAll()
+ >>> BranchMailer.forRevision(branch, 'foo@xxxxxxxxxxxxx',
+ ... 'The contents.', None, 'Subject line', revno=1).sendAll()
>>> notifications = pop_notifications()
>>> len(notifications)
@@ -213,8 +213,8 @@
Send the revision notifications.
>>> BranchMailer.forRevision(
- ... branch, 1234, 'no-reply@xxxxxxxxxxxxx', message, diff,
- ... None).sendAll()
+ ... branch, 'no-reply@xxxxxxxxxxxxx', message, diff,
+ ... None, revno=1234).sendAll()
>>> notifications = pop_notifications()
>>> len(notifications)
6
@@ -287,8 +287,8 @@
>>> diff = '\n'.join([str(value) for value in xrange(800)])
>>> BranchMailer.forRevision(
- ... branch, 1234, 'no-reply@xxxxxxxxxxxxx', message, diff,
- ... None).sendAll()
+ ... branch, 'no-reply@xxxxxxxxxxxxx', message, diff,
+ ... None, revno=1234).sendAll()
>>> notifications = pop_notifications()
>>> len(notifications)
6
=== modified file 'lib/lp/code/interfaces/branch.py'
--- lib/lp/code/interfaces/branch.py 2015-04-15 13:41:13 +0000
+++ lib/lp/code/interfaces/branch.py 2015-04-16 15:40:22 +0000
@@ -701,6 +701,10 @@
'lp:product/series. Otherwise the result is '
'lp:~user/product/branch-name.')))
+ identity = Attribute(
+ "The identity of this branch: a VCS-independent synonym for "
+ "bzr_identity.")
+
def addToLaunchBag(launchbag):
"""Add information about this branch to `launchbag'.
@@ -1513,6 +1517,8 @@
identity, context = self.branchIdentities()[0]
return identity
+ identity = bzr_identity
+
def branchIdentities(self):
"""See `IBranch`."""
lp_prefix = config.codehosting.bzr_lp_prefix
=== modified file 'lib/lp/code/interfaces/branchmergeproposal.py'
--- lib/lp/code/interfaces/branchmergeproposal.py 2015-01-30 18:24:07 +0000
+++ lib/lp/code/interfaces/branchmergeproposal.py 2015-04-16 15:40:22 +0000
@@ -136,6 +136,16 @@
"If this branch is the same as the target branch, then "
"leave this field blank.")))
+ merge_source = Attribute(
+ "The branch that has code to land (VCS-agnostic).")
+
+ merge_target = Attribute(
+ "The branch that the source branch will be merged into "
+ "(VCS-agnostic).")
+
+ merge_prerequisite = Attribute(
+ "The branch that the source branch branched from (VCS-agnostic).")
+
class IBranchMergeProposalView(Interface):
=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py 2015-04-13 19:02:15 +0000
+++ lib/lp/code/interfaces/gitref.py 2015-04-16 15:40:22 +0000
@@ -87,6 +87,51 @@
title=_("The first line of the commit message."),
required=True, readonly=True)
+ identity = Attribute(
+ "The identity of this reference. This will be the shortened path to "
+ "the containing repository, plus a colon, plus the reference path "
+ "with any leading refs/heads/ removed; for example, launchpad:master.")
+
+ unique_name = Attribute(
+ "The unique name of this reference. This will be the unique name of "
+ "the containing repository, plus a colon, plus the reference path "
+ "with any leading refs/heads/ removed; for example, "
+ "~launchpad-pqm/launchpad:master.")
+
+ owner = Attribute("The owner of the repository containing this reference.")
+
+ target = Attribute(
+ "The target of the repository containing this reference.")
+
+ subscribers = Attribute(
+ "Persons subscribed to the repository containing this reference.")
+
+ def subscribe(person, notification_level, max_diff_lines,
+ code_review_level, subscribed_by):
+ """Subscribe this person to the repository containing this reference.
+
+ :param person: The `Person` to subscribe.
+ :param notification_level: The kinds of repository changes that
+ cause notification.
+ :param max_diff_lines: The maximum number of lines of diff that may
+ appear in a notification.
+ :param code_review_level: The kinds of code review activity that
+ cause notification.
+ :param subscribed_by: The person who is subscribing the subscriber.
+ Most often the subscriber themselves.
+ :return: A new or existing `GitSubscription`.
+ """
+
+ def getSubscription(person):
+ """Return the `GitSubscription` for this person."""
+
+ def getNotificationRecipients():
+ """Return a complete INotificationRecipientSet instance.
+
+ The INotificationRecipientSet instance contains the subscribers
+ and their subscriptions.
+ """
+
class IGitRefBatchNavigator(ITableBatchNavigator):
pass
=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py 2015-04-16 15:40:21 +0000
+++ lib/lp/code/interfaces/gitrepository.py 2015-04-16 15:40:22 +0000
@@ -11,6 +11,7 @@
'GIT_REPOSITORY_NAME_VALIDATION_ERROR_MESSAGE',
'git_repository_name_validator',
'IGitRepository',
+ 'IGitRepositoryDelta',
'IGitRepositorySet',
'user_has_special_git_repository_access',
]
@@ -194,6 +195,10 @@
"'lp:' plus a shortcut version of the path via that target. "
"Otherwise it is simply 'lp:' plus the unique name.")))
+ identity = Attribute(
+ "The identity of this repository: a VCS-independent synonym for "
+ "git_identity.")
+
refs = exported(CollectionField(
title=_("The references present in this repository."),
readonly=True,
@@ -417,6 +422,13 @@
:return: A `ResultSet`.
"""
+ def getNotificationRecipients():
+ """Return a complete INotificationRecipientSet instance.
+
+ The INotificationRecipientSet instance contains the subscribers
+ and their subscriptions.
+ """
+
class IGitRepositoryModerateAttributes(Interface):
"""IGitRepository attributes that can be edited by more than one community.
@@ -635,6 +647,19 @@
"""
+class IGitRepositoryDelta(Interface):
+ """The quantitative changes made to a Git repository that was edited or
+ altered.
+ """
+
+ repository = Attribute("The IGitRepository, after it's been edited.")
+ user = Attribute("The IPerson that did the editing.")
+
+ # fields on the repository itself, we provide just the new changed value
+ name = Attribute("Old and new names or None.")
+ identity = Attribute("Old and new identities or None.")
+
+
class GitIdentityMixin:
"""This mixin class determines Git repository paths.
@@ -654,6 +679,8 @@
"""See `IGitRepository`."""
return "lp:" + self.shortened_path
+ identity = git_identity
+
def getRepositoryDefaults(self):
"""See `IGitRepository`."""
defaults = []
=== modified file 'lib/lp/code/mail/branch.py'
--- lib/lp/code/mail/branch.py 2012-01-01 02:58:52 +0000
+++ lib/lp/code/mail/branch.py 2015-04-16 15:40:22 +0000
@@ -8,12 +8,16 @@
from zope.security.proxy import removeSecurityProxy
from lp.code.adapters.branch import BranchDelta
+from lp.code.adapters.gitrepository import GitRepositoryDelta
from lp.code.enums import (
BranchSubscriptionDiffSize,
BranchSubscriptionNotificationLevel,
CodeReviewNotificationLevel,
)
+from lp.code.interfaces.branch import IBranch
+from lp.code.interfaces.gitref import IGitRef
from lp.registry.interfaces.person import IPerson
+from lp.registry.interfaces.product import IProduct
from lp.services.mail import basemailer
from lp.services.mail.basemailer import BaseMailer
from lp.services.mail.sendmail import format_address
@@ -31,6 +35,17 @@
mailer.sendAll()
+def send_git_repository_modified_notifications(repository, event):
+ """Notify the related people that a Git repository has been modifed."""
+ user = IPerson(event.user)
+ repository_delta = GitRepositoryDelta.construct(
+ event.object_before_modification, repository, user)
+ if repository_delta is None:
+ return
+ mailer = BranchMailer.forBranchModified(repository, user, repository_delta)
+ mailer.sendAll()
+
+
class RecipientReason(basemailer.RecipientReason):
def __init__(self, subscriber, recipient, branch, mail_header,
@@ -55,15 +70,15 @@
except KeyError:
# Don't bother trying to remember the cache, as the cache only
# makes sense across multiple instances of this type of object.
- return branch.bzr_identity
+ return branch.identity
@classmethod
def forBranchSubscriber(
- cls, subscription, recipient, rationale, merge_proposal=None,
+ cls, subscription, branch, recipient, rationale, merge_proposal=None,
branch_identity_cache=None):
"""Construct RecipientReason for a branch subscriber."""
return cls(
- subscription.person, recipient, subscription.branch, rationale,
+ subscription.person, recipient, branch, rationale,
'%(entity_is)s subscribed to branch %(branch_name)s.',
merge_proposal, subscription.max_diff_lines,
branch_identity_cache=branch_identity_cache,
@@ -76,14 +91,13 @@
The reviewer will be the sole recipient.
"""
- branch = branch_merge_proposal.source_branch
if pending_review:
reason_template = (
'%(entity_is)s requested to review %(merge_proposal)s.')
else:
reason_template = (
'%(entity_is)s reviewing %(merge_proposal)s.')
- return cls(reviewer, reviewer, branch,
+ return cls(reviewer, reviewer, branch_merge_proposal.merge_source,
cls.makeRationale('Reviewer', reviewer),
reason_template, branch_merge_proposal,
branch_identity_cache=branch_identity_cache)
@@ -96,7 +110,7 @@
"""
reason_template = 'You proposed %(branch_name)s for merging.'
return cls(merge_proposal.registrant, merge_proposal.registrant,
- merge_proposal.source_branch,
+ merge_proposal.merge_source,
'Registrant', reason_template, merge_proposal,
branch_identity_cache=branch_identity_cache)
@@ -107,7 +121,7 @@
The owner of the source branch will be the sole recipient. If the
source branch owner is a team, None is returned.
"""
- branch = merge_proposal.source_branch
+ branch = merge_proposal.merge_source
owner = branch.owner
if owner.is_team:
return None
@@ -134,10 +148,8 @@
template_values = super(RecipientReason, self)._getTemplateValues()
template_values['branch_name'] = self._getBranchIdentity(self.branch)
if self.merge_proposal is not None:
- source = self._getBranchIdentity(
- self.merge_proposal.source_branch)
- target = self._getBranchIdentity(
- self.merge_proposal.target_branch)
+ source = self._getBranchIdentity(self.merge_proposal.merge_source)
+ target = self._getBranchIdentity(self.merge_proposal.merge_target)
template_values['merge_proposal'] = (
'the proposed merge of %s into %s' % (source, target))
return template_values
@@ -150,7 +162,8 @@
def __init__(self, subject, template_name, recipients, from_address,
delta=None, contents=None, diff=None, message_id=None,
- revno=None, notification_type=None, **kwargs):
+ revno=None, revision_id=None, notification_type=None,
+ **kwargs):
BaseMailer.__init__(self, subject, template_name, recipients,
from_address, delta, message_id,
notification_type)
@@ -161,6 +174,7 @@
else:
self.diff_size = self.diff.count('\n') + 1
self.revno = revno
+ self.revision_id = revision_id
self.extra_template_params = kwargs
@classmethod
@@ -193,7 +207,7 @@
else:
actual_recipients[recipient] = \
RecipientReason.forBranchSubscriber(
- subscription, recipient, rationale)
+ subscription, branch, recipient, rationale)
from_address = format_address(
user.displayname, removeSecurityProxy(user).preferredemail.email)
return cls(
@@ -202,8 +216,8 @@
notification_type='branch-updated')
@classmethod
- def forRevision(cls, db_branch, revno, from_address, contents, diff,
- subject):
+ def forRevision(cls, db_branch, from_address, contents, diff, subject,
+ revno=None, revision_id=None):
"""Construct a BranchMailer for mail about branch revisions.
:param branch: The db_branch that was modified.
@@ -223,20 +237,27 @@
subscription, rationale = recipients.getReason(recipient)
if subscription.notification_level in interested_levels:
subscriber_reason = RecipientReason.forBranchSubscriber(
- subscription, recipient, rationale)
+ subscription, db_branch, recipient, rationale)
recipient_dict[recipient] = subscriber_reason
return cls('%(full_subject)s', 'branch-modified.txt', recipient_dict,
from_address, contents=contents, diff=diff, revno=revno,
+ revision_id=revision_id,
notification_type='branch-revision', full_subject=subject)
def _getHeaders(self, email):
headers = BaseMailer._getHeaders(self, email)
reason, rationale = self._recipients.getReason(email)
headers['X-Launchpad-Branch'] = reason.branch.unique_name
- if reason.branch.product is not None:
- headers['X-Launchpad-Project'] = reason.branch.product.name
+ if IGitRef.providedBy(reason.branch):
+ if IProduct.providedBy(reason.branch.target):
+ headers['X-Launchpad-Project'] = reason.branch.target.name
+ elif IBranch.providedBy(reason.branch):
+ if reason.branch.product is not None:
+ headers['X-Launchpad-Project'] = reason.branch.product.name
if self.revno is not None:
headers['X-Launchpad-Branch-Revision-Number'] = str(self.revno)
+ if self.revision_id is not None:
+ headers['X-Launchpad-Branch-Revision-ID'] = self.revision_id
return headers
def _getTemplateParams(self, email, recipient):
@@ -244,13 +265,19 @@
reason, rationale = self._recipients.getReason(email)
branch = reason.branch
params['unique_name'] = branch.unique_name
- params['branch_identity'] = branch.bzr_identity
+ params['branch_identity'] = branch.identity
params['branch_url'] = canonical_url(branch)
if reason.recipient in branch.subscribers:
# Give subscribers a link to unsubscribe.
+ # XXX cjwatson 2015-04-15: Perhaps GitRef:+edit-subscription
+ # should be made to work?
+ if IGitRef.providedBy(branch):
+ unsubscribe_url = canonical_url(branch.repository)
+ else:
+ unsubscribe_url = canonical_url(branch)
params['unsubscribe'] = (
"\nTo unsubscribe from this branch go to "
- "%s/+edit-subscription" % canonical_url(branch))
+ "%s/+edit-subscription" % unsubscribe_url)
else:
params['unsubscribe'] = ''
params['diff'] = self.contents or ''
=== modified file 'lib/lp/code/mail/branchmergeproposal.py'
--- lib/lp/code/mail/branchmergeproposal.py 2012-09-18 21:49:22 +0000
+++ lib/lp/code/mail/branchmergeproposal.py 2015-04-16 15:40:22 +0000
@@ -139,8 +139,8 @@
proposal = self.merge_proposal
params = {
'proposal_registrant': proposal.registrant.displayname,
- 'source_branch': proposal.source_branch.bzr_identity,
- 'target_branch': proposal.target_branch.bzr_identity,
+ 'source_branch': proposal.merge_source.identity,
+ 'target_branch': proposal.merge_target.identity,
'prerequisite': '',
'proposal_title': proposal.title,
'proposal_url': canonical_url(proposal),
@@ -154,8 +154,8 @@
if self.delta_text is not None:
params['delta'] = self.delta_text
- if proposal.prerequisite_branch is not None:
- prereq_url = proposal.prerequisite_branch.bzr_identity
+ if proposal.merge_prerequisite is not None:
+ prereq_url = proposal.merge_prerequisite.identity
params['prerequisite'] = ' with %s as a prerequisite' % prereq_url
requested_reviews = []
=== modified file 'lib/lp/code/mail/tests/test_branch.py'
--- lib/lp/code/mail/tests/test_branch.py 2012-01-01 02:58:52 +0000
+++ lib/lp/code/mail/tests/test_branch.py 2015-04-16 15:40:22 +0000
@@ -10,11 +10,14 @@
BranchSubscriptionNotificationLevel,
CodeReviewNotificationLevel,
)
+from lp.code.interfaces.gitrepository import GIT_FEATURE_FLAG
from lp.code.mail.branch import (
BranchMailer,
RecipientReason,
)
from lp.code.model.branch import Branch
+from lp.code.model.gitref import GitRef
+from lp.services.features.testing import FeatureFixture
from lp.testing import (
login_person,
TestCaseWithFactory,
@@ -22,38 +25,25 @@
from lp.testing.layers import DatabaseFunctionalLayer
-class TestRecipientReason(TestCaseWithFactory):
+class TestRecipientReasonMixin:
"""Test the RecipientReason class."""
layer = DatabaseFunctionalLayer
def setUp(self):
- # Need to set target_branch.date_last_modified.
+ # Need to set merge_target.date_last_modified.
TestCaseWithFactory.setUp(self, user='test@xxxxxxxxxxxxx')
- def makeProposalWithSubscription(self, subscriber=None):
- """Test fixture."""
- if subscriber is None:
- subscriber = self.factory.makePerson()
- source_branch = self.factory.makeProductBranch(title='foo')
- target_branch = self.factory.makeProductBranch(
- product=source_branch.product, title='bar')
- merge_proposal = source_branch.addLandingTarget(
- source_branch.owner, target_branch)
- subscription = merge_proposal.source_branch.subscribe(
- subscriber, BranchSubscriptionNotificationLevel.NOEMAIL, None,
- CodeReviewNotificationLevel.FULL, subscriber)
- return merge_proposal, subscription
-
def test_forBranchSubscriber(self):
"""Test values when created from a branch subscription."""
merge_proposal, subscription = self.makeProposalWithSubscription()
subscriber = subscription.person
reason = RecipientReason.forBranchSubscriber(
- subscription, subscriber, '', merge_proposal)
+ subscription, merge_proposal.merge_source, subscriber, '',
+ merge_proposal)
self.assertEqual(subscriber, reason.subscriber)
self.assertEqual(subscriber, reason.recipient)
- self.assertEqual(merge_proposal.source_branch, reason.branch)
+ self.assertEqual(merge_proposal.merge_source, reason.branch)
def makeReviewerAndSubscriber(self):
"""Return a tuple of vote_reference, subscriber."""
@@ -74,7 +64,7 @@
self.assertEqual(subscriber, reason.subscriber)
self.assertEqual(subscriber, reason.recipient)
self.assertEqual(
- vote_reference.branch_merge_proposal.source_branch, reason.branch)
+ vote_reference.branch_merge_proposal.merge_source, reason.branch)
def test_forReview_individual_pending(self):
bmp = self.factory.makeBranchMergeProposal()
@@ -83,8 +73,7 @@
self.assertEqual('Reviewer', reason.mail_header)
self.assertEqual(
'You are requested to review the proposed merge of %s into %s.'
- % (bmp.source_branch.bzr_identity,
- bmp.target_branch.bzr_identity),
+ % (bmp.merge_source.identity, bmp.merge_target.identity),
reason.getReason())
def test_forReview_individual_in_progress(self):
@@ -94,8 +83,7 @@
self.assertEqual('Reviewer', reason.mail_header)
self.assertEqual(
'You are reviewing the proposed merge of %s into %s.'
- % (bmp.source_branch.bzr_identity,
- bmp.target_branch.bzr_identity),
+ % (bmp.merge_source.identity, bmp.merge_target.identity),
reason.getReason())
def test_forReview_team_pending(self):
@@ -106,18 +94,18 @@
self.assertEqual(
'Your team Vikings is requested to review the proposed merge'
' of %s into %s.'
- % (bmp.source_branch.bzr_identity,
- bmp.target_branch.bzr_identity),
+ % (bmp.merge_source.identity, bmp.merge_target.identity),
reason.getReason())
def test_getReasonPerson(self):
"""Ensure the correct reason is generated for individuals."""
merge_proposal, subscription = self.makeProposalWithSubscription()
reason = RecipientReason.forBranchSubscriber(
- subscription, subscription.person, '', merge_proposal)
+ subscription, merge_proposal.merge_source, subscription.person, '',
+ merge_proposal)
self.assertEqual(
'You are subscribed to branch %s.'
- % merge_proposal.source_branch.bzr_identity, reason.getReason())
+ % merge_proposal.merge_source.identity, reason.getReason())
def test_getReasonTeam(self):
"""Ensure the correct reason is generated for teams."""
@@ -126,10 +114,28 @@
team = self.factory.makeTeam(team_member, displayname='Qux')
bmp, subscription = self.makeProposalWithSubscription(team)
reason = RecipientReason.forBranchSubscriber(
- subscription, team_member, '', bmp)
+ subscription, bmp.merge_source, team_member, '', bmp)
self.assertEqual(
'Your team Qux is subscribed to branch %s.'
- % bmp.source_branch.bzr_identity, reason.getReason())
+ % bmp.merge_source.identity, reason.getReason())
+
+
+class TestRecipientReasonBzr(TestRecipientReasonMixin, TestCaseWithFactory):
+ """Test RecipientReason for Bazaar branches."""
+
+ def makeProposalWithSubscription(self, subscriber=None):
+ """Test fixture."""
+ if subscriber is None:
+ subscriber = self.factory.makePerson()
+ source_branch = self.factory.makeProductBranch(title='foo')
+ target_branch = self.factory.makeProductBranch(
+ product=source_branch.product, title='bar')
+ merge_proposal = source_branch.addLandingTarget(
+ source_branch.owner, target_branch)
+ subscription = merge_proposal.merge_source.subscribe(
+ subscriber, BranchSubscriptionNotificationLevel.NOEMAIL, None,
+ CodeReviewNotificationLevel.FULL, subscriber)
+ return merge_proposal, subscription
def test_usesBranchIdentityCache(self):
"""Ensure that the cache is used for branches if provided."""
@@ -139,30 +145,78 @@
def blowup(self):
raise AssertionError('boom')
- patched = Branch.bzr_identity
- Branch.bzr_identity = property(blowup)
+ patched = Branch.identity
+ Branch.identity = property(blowup)
def cleanup():
- Branch.bzr_identity = patched
+ Branch.identity = patched
self.addCleanup(cleanup)
- self.assertRaises(AssertionError, getattr, branch, 'bzr_identity')
+ self.assertRaises(AssertionError, getattr, branch, 'identity')
reason = RecipientReason.forBranchSubscriber(
- subscription, subscription.person, '',
+ subscription, branch, subscription.person, '',
branch_identity_cache=branch_cache)
self.assertEqual(
'You are subscribed to branch lp://fake.',
reason.getReason())
-class TestBranchMailerHeaders(TestCaseWithFactory):
- """Check the headers are correct for Branch email."""
+# XXX cjwatson 2015-04-15: Enable this (by adding TestCaseWithFactory) once
+# BranchMergeProposal supports Git.
+class TestRecipientReasonGit(TestRecipientReasonMixin):
+ """Test RecipientReason for Git references."""
+
+ def setUp(self):
+ super(TestRecipientReasonGit, self).setUp()
+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+ def makeProposalWithSubscription(self, subscriber=None):
+ """Test fixture."""
+ if subscriber is None:
+ subscriber = self.factory.makePerson()
+ source_repository = self.factory.makeGitRepository()
+ source_ref = self.factory.makeGitRefs(repository=source_repository)
+ target_repository = self.factory.makeGitRepository(
+ target=source_repository.target)
+ target_ref = self.factory.makeGitRefs(repository=target_repository)
+ merge_proposal = source_ref.addLandingTarget(
+ source_repository.owner, target_ref)
+ subscription = merge_proposal.merge_source.subscribe(
+ subscriber, BranchSubscriptionNotificationLevel.NOEMAIL, None,
+ CodeReviewNotificationLevel.FULL, subscriber)
+ return merge_proposal, subscription
+
+ def test_usesBranchIdentityCache(self):
+ """Ensure that the cache is used for Git references if provided."""
+ [ref] = self.factory.makeGitRefs()
+ subscription = ref.getSubscription(ref.owner)
+ branch_cache = {ref: 'fake:master'}
+
+ def blowup(self):
+ raise AssertionError('boom')
+ patched = GitRef.identity
+ GitRef.identity = property(blowup)
+
+ def cleanup():
+ GitRef.identity = patched
+ self.addCleanup(cleanup)
+ self.assertRaises(AssertionError, getattr, ref, 'identity')
+ reason = RecipientReason.forBranchSubscriber(
+ subscription, ref, subscription.person, '',
+ branch_identity_cache=branch_cache)
+ self.assertEqual(
+ 'You are subscribed to branch fake:master.',
+ reason.getReason())
+
+
+class TestBranchMailerHeadersMixin:
+ """Check the headers are correct."""
layer = DatabaseFunctionalLayer
def test_branch_modified(self):
# Test the email headers for a branch modified email.
bob = self.factory.makePerson(email='bob@xxxxxxxxxxx')
- branch = self.factory.makeProductBranch(owner=bob)
+ branch = self.makeBranch(owner=bob)
branch.getSubscription(bob).notification_level = (
BranchSubscriptionNotificationLevel.FULL)
mailer = BranchMailer.forBranchModified(branch, branch.owner, None)
@@ -172,18 +226,19 @@
{'X-Launchpad-Branch': branch.unique_name,
'X-Launchpad-Message-Rationale': 'Subscriber',
'X-Launchpad-Notification-Type': 'branch-updated',
- 'X-Launchpad-Project': branch.product.name,
+ 'X-Launchpad-Project': self.getBranchProjectName(branch),
'Message-Id': '<foobar-example-com>'},
ctrl.headers)
def test_branch_revision(self):
# Test the email headers for new revision email.
bob = self.factory.makePerson(email='bob@xxxxxxxxxxx')
- branch = self.factory.makeProductBranch(owner=bob)
+ branch = self.makeBranch(owner=bob)
branch.getSubscription(bob).notification_level = (
BranchSubscriptionNotificationLevel.FULL)
mailer = BranchMailer.forRevision(
- branch, 1, 'from@xxxxxxxxxxx', contents='', diff=None, subject='')
+ branch, 'from@xxxxxxxxxxx', contents='', diff=None, subject='',
+ revno=1)
mailer.message_id = '<foobar-example-com>'
ctrl = mailer.generateEmail('bob@xxxxxxxxxxx', branch.owner)
self.assertEqual(
@@ -191,26 +246,54 @@
'X-Launchpad-Message-Rationale': 'Subscriber',
'X-Launchpad-Notification-Type': 'branch-revision',
'X-Launchpad-Branch-Revision-Number': '1',
- 'X-Launchpad-Project': branch.product.name,
+ 'X-Launchpad-Project': self.getBranchProjectName(branch),
'Message-Id': '<foobar-example-com>'},
ctrl.headers)
-class TestBranchMailerDiff(TestCaseWithFactory):
- """Check the diff is an attachment for Branch email."""
+class TestBranchMailerHeadersBzr(
+ TestBranchMailerHeadersMixin, TestCaseWithFactory):
+ """Check the headers are correct for Branch email."""
+
+ def makeBranch(self, owner):
+ return self.factory.makeProductBranch(owner=owner)
+
+ def getBranchProjectName(self, branch):
+ return branch.product.name
+
+
+class TestBranchMailerHeadersGit(
+ TestBranchMailerHeadersMixin, TestCaseWithFactory):
+ """Check the headers are correct for GitRef email."""
+
+ def setUp(self):
+ super(TestBranchMailerHeadersGit, self).setUp()
+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+ def makeBranch(self, owner):
+ repository = self.factory.makeGitRepository(owner=owner)
+ return self.factory.makeGitRefs(repository=repository)[0]
+
+ def getBranchProjectName(self, branch):
+ return branch.target.name
+
+
+class TestBranchMailerDiffMixin:
+ """Check the diff is an attachment."""
layer = DatabaseFunctionalLayer
def makeBobMailController(self, diff=None,
max_lines=BranchSubscriptionDiffSize.WHOLEDIFF):
bob = self.factory.makePerson(email='bob@xxxxxxxxxxx')
- branch = self.factory.makeProductBranch(owner=bob)
+ branch = self.makeBranch(owner=bob)
subscription = branch.getSubscription(bob)
subscription.max_diff_lines = max_lines
subscription.notification_level = (
BranchSubscriptionNotificationLevel.FULL)
mailer = BranchMailer.forRevision(
- branch, 1, 'from@xxxxxxxxxxx', contents='', diff=diff, subject='')
+ branch, 'from@xxxxxxxxxxx', contents='', diff=diff, subject='',
+ revno=1)
return mailer.generateEmail('bob@xxxxxxxxxxx', branch.owner)
def test_generateEmail_with_no_diff(self):
@@ -247,21 +330,60 @@
self.assertNotIn('larger than your specified limit', ctrl.body)
-class TestBranchMailerSubject(TestCaseWithFactory):
+class TestBranchMailerDiffBzr(TestBranchMailerDiffMixin, TestCaseWithFactory):
+ """Check the diff is an attachment for Branch email."""
+
+ def makeBranch(self, owner):
+ return self.factory.makeProductBranch(owner=owner)
+
+
+class TestBranchMailerDiffGit(TestBranchMailerDiffMixin, TestCaseWithFactory):
+ """Check the diff is an attachment for GitRef email."""
+
+ def setUp(self):
+ super(TestBranchMailerDiffGit, self).setUp()
+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+ def makeBranch(self, owner):
+ repository = self.factory.makeGitRepository(owner=owner)
+ return self.factory.makeGitRefs(repository=repository)[0]
+
+
+class TestBranchMailerSubjectMixin:
"""The subject for a BranchMailer is returned verbatim."""
layer = DatabaseFunctionalLayer
def test_subject(self):
# No string interpolation should occur on the subject.
- branch = self.factory.makeAnyBranch()
+ branch = self.makeBranch()
# Subscribe the owner to get revision email.
branch.getSubscription(branch.owner).notification_level = (
BranchSubscriptionNotificationLevel.FULL)
mailer = BranchMailer.forRevision(
- branch, 1, 'test@xxxxxxxxxxx', 'content', 'diff',
- 'Testing %j foo')
+ branch, 'test@xxxxxxxxxxx', 'content', 'diff', 'Testing %j foo',
+ revno=1)
branch_owner_email = removeSecurityProxy(
branch.owner).preferredemail.email
self.assertEqual('Testing %j foo', mailer._getSubject(
branch_owner_email, branch.owner))
+
+
+class TestBranchMailerSubjectBzr(
+ TestBranchMailerSubjectMixin, TestCaseWithFactory):
+ """The subject for a BranchMailer is returned verbatim for Branch."""
+
+ def makeBranch(self):
+ return self.factory.makeAnyBranch()
+
+
+class TestBranchMailerSubjectGit(
+ TestBranchMailerSubjectMixin, TestCaseWithFactory):
+ """The subject for a BranchMailer is returned verbatim for GitRef."""
+
+ def setUp(self):
+ super(TestBranchMailerSubjectGit, self).setUp()
+ self.useFixture(FeatureFixture({GIT_FEATURE_FLAG: u"on"}))
+
+ def makeBranch(self):
+ return self.factory.makeGitRefs()[0]
=== modified file 'lib/lp/code/model/branchjob.py'
--- lib/lp/code/model/branchjob.py 2014-06-10 16:13:03 +0000
+++ lib/lp/code/model/branchjob.py 2015-04-16 15:40:22 +0000
@@ -463,8 +463,8 @@
def getMailer(self):
"""Return a BranchMailer for this job."""
return BranchMailer.forRevision(
- self.branch, self.revno, self.from_address, self.body,
- None, self.subject)
+ self.branch, self.from_address, self.body, None, self.subject,
+ revno=self.revno)
def run(self):
"""See `IRevisionMailJob`."""
@@ -601,8 +601,8 @@
else:
diff_text = None
return BranchMailer.forRevision(
- self.branch, revno, self.from_address, message, diff_text,
- subject)
+ self.branch, self.from_address, message, diff_text, subject,
+ revno=revno)
def getMergedRevisionIDs(self, revision_id, graph):
"""Determine which revisions were merged by this revision.
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2015-04-15 13:41:13 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2015-04-16 15:40:22 +0000
@@ -185,6 +185,18 @@
prerequisite_branch = ForeignKey(
dbName='dependent_branch', foreignKey='Branch', notNull=False)
+ @property
+ def merge_source(self):
+ return self.source_branch
+
+ @property
+ def merge_target(self):
+ return self.target_branch
+
+ @property
+ def merge_prerequisite(self):
+ return self.prerequisite_branch
+
description = StringCol(default=None)
whiteboard = StringCol(default=None)
@@ -352,7 +364,7 @@
if (subscription.review_level < min_level):
continue
recipients[recipient] = RecipientReason.forBranchSubscriber(
- subscription, recipient, rationale, self,
+ subscription, branch, recipient, rationale, self,
branch_identity_cache=branch_identity_cache)
# Add in all the individuals that have been asked for a review,
# or who have reviewed. These people get added to the recipients
=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py 2015-03-19 17:04:22 +0000
+++ lib/lp/code/model/gitref.py 2015-04-16 15:40:22 +0000
@@ -4,6 +4,7 @@
__metaclass__ = type
__all__ = [
'GitRef',
+ 'GitRefFrozen',
]
import pytz
@@ -11,17 +12,70 @@
DateTime,
Int,
Reference,
+ Store,
Unicode,
)
from zope.interface import implements
+from lp.app.errors import NotFoundError
from lp.code.enums import GitObjectType
from lp.code.interfaces.gitref import IGitRef
from lp.services.database.enumcol import EnumCol
from lp.services.database.stormbase import StormBase
-class GitRef(StormBase):
+class GitRefMixin:
+ """Methods and properties common to GitRef and GitRefFrozen.
+
+ These can be derived solely from the repository and path, and so do not
+ require a database record.
+ """
+
+ @property
+ def display_name(self):
+ return self.path.split("/", 2)[-1]
+
+ @property
+ def _branch_name(self):
+ if self.path.startswith("refs/heads/"):
+ return self.path[len("refs/heads/"):]
+ else:
+ return self.path
+
+ @property
+ def identity(self):
+ return "%s:%s" % (self.repository.shortened_path, self._branch_name)
+
+ @property
+ def unique_name(self):
+ return "%s:%s" % (self.repository.unique_name, self._branch_name)
+
+ @property
+ def owner(self):
+ return self.repository.owner
+
+ @property
+ def target(self):
+ return self.repository.target
+
+ @property
+ def subscribers(self):
+ return self.repository.subscribers
+
+ def subscribe(self, person, notification_level, max_diff_lines,
+ code_review_level, subscribed_by):
+ return self.repository.subscribe(
+ person, notification_level, max_diff_lines, code_review_level,
+ subscribed_by)
+
+ def getSubscription(self, person):
+ return self.repository.getSubscription(person)
+
+ def getNotificationRecipients(self):
+ return self.repository.getNotificationRecipients()
+
+
+class GitRef(StormBase, GitRefMixin):
"""See `IGitRef`."""
__storm_table__ = 'GitRef'
@@ -51,9 +105,40 @@
commit_message = Unicode(name='commit_message', allow_none=True)
@property
- def display_name(self):
- return self.path.split("/", 2)[-1]
-
- @property
def commit_message_first_line(self):
return self.commit_message.split("\n", 1)[0]
+
+
+class GitRefFrozen(GitRefMixin):
+ """A frozen Git reference.
+
+ This is like a GitRef, but is frozen at a particular commit, even if the
+ real reference has moved on or has been deleted. It isn't necessarily
+ backed by a real database object, and will retrieve columns from the
+ database when required. Use this when you have a
+ repository/path/commit_sha1 that you want to pass around as a single
+ object, but don't necessarily know that the ref still exists.
+ """
+
+ implements(IGitRef)
+
+ def __init__(self, repository, path, commit_sha1):
+ self.repository = repository
+ self.path = path
+ self.commit_sha1 = commit_sha1
+
+ @property
+ def _self_in_database(self):
+ """Return the equivalent database-backed record of self."""
+ ref = Store.of(GitRef).get(GitRef, (self.repository, self.path))
+ if ref is None:
+ raise NotFoundError(
+ "Repository '%s' does not currently contain a reference named "
+ "'%s'" % (self.repository, self.path))
+ return ref
+
+ def __getattr__(self, name):
+ return getattr(self._self_in_database, name)
+
+ def __setattr__(self, name, value):
+ return setattr(self._self_in_database, name, value)
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2015-04-16 15:40:21 +0000
+++ lib/lp/code/model/gitrepository.py 2015-04-16 15:40:22 +0000
@@ -75,6 +75,7 @@
user_has_special_git_repository_access,
)
from lp.code.interfaces.revision import IRevisionSet
+from lp.code.mail.branch import send_git_repository_modified_notifications
from lp.code.model.gitref import GitRef
from lp.code.model.gitsubscription import GitSubscription
from lp.registry.enums import PersonVisibility
@@ -116,6 +117,7 @@
Values,
)
from lp.services.features import getFeatureFlag
+from lp.services.mail.notificationrecipientset import NotificationRecipientSet
from lp.services.propertycache import cachedproperty
from lp.services.webapp.authorization import available_with_permission
@@ -135,6 +137,7 @@
events on Git repositories.
"""
repository.date_last_modified = UTC_NOW
+ send_git_repository_modified_notifications(repository, event)
class GitRepository(StormBase, GitIdentityMixin):
@@ -681,6 +684,17 @@
artifact, [person])
store.flush()
+ def getNotificationRecipients(self):
+ """See `IGitRepository`."""
+ recipients = NotificationRecipientSet()
+ for subscription in self.subscriptions:
+ if subscription.person.is_team:
+ rationale = 'Subscriber @%s' % subscription.person.name
+ else:
+ rationale = 'Subscriber'
+ recipients.add(subscription.person, subscription, rationale)
+ return recipients
+
def destroySelf(self):
raise NotImplementedError
=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py 2015-04-16 15:40:21 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py 2015-04-16 15:40:22 +0000
@@ -304,7 +304,7 @@
date_created=datetime(2015, 02, 04, 17, 42, 0, tzinfo=pytz.UTC))
notify(ObjectModifiedEvent(
removeSecurityProxy(repository), repository,
- [IGitRepository["name"]]))
+ [IGitRepository["name"]], user=repository.owner))
self.assertSqlAttributeEqualsDate(
repository, "date_last_modified", UTC_NOW)
Follow ups
