Merge lp:~cjwatson/launchpad/git-subscriptions-by-path into lp:launchpad

Proposed by Colin Watson
Status: Rejected
Rejected by: Colin Watson
Proposed branch: lp:~cjwatson/launchpad/git-subscriptions-by-path
Merge into: lp:launchpad
Prerequisite: lp:~cjwatson/launchpad/git-subscription-tests
Diff against target: 743 lines (+287/-49)
13 files modified
lib/lp/code/browser/branchsubscription.py (+2/-2)
lib/lp/code/browser/gitsubscription.py (+107/-24)
lib/lp/code/browser/tests/test_gitsubscription.py (+4/-1)
lib/lp/code/interfaces/gitref.py (+3/-1)
lib/lp/code/interfaces/gitrepository.py (+17/-3)
lib/lp/code/interfaces/gitsubscription.py (+18/-1)
lib/lp/code/model/gitref.py (+3/-3)
lib/lp/code/model/gitrepository.py (+6/-3)
lib/lp/code/model/gitsubscription.py (+28/-4)
lib/lp/code/model/tests/test_branchmergeproposal.py (+28/-0)
lib/lp/code/model/tests/test_gitrepository.py (+33/-4)
lib/lp/code/model/tests/test_gitsubscription.py (+36/-1)
lib/lp/testing/factory.py (+2/-2)
To merge this branch: bzr merge lp:~cjwatson/launchpad/git-subscriptions-by-path
Reviewer Review Type Date Requested Status
Launchpad code reviewers Pending
Review via email: mp+310471@code.launchpad.net

Commit message

Allow subscribing to only particular reference paths within a Git repository.

Description of the change

This is needed as a soft prerequisite for beefing up subscriptions to send revision information, since users may very well want to e.g. subscribe to changes to master without subscribing to random other branches.

It's conceivable that people may want to have separate subscriptions to different ref patterns (e.g. "send me diffs for changes to master, but only tell me summary information elsewhere". I haven't attempted to solve that here (subscriptions are still unique up to person and repository), because it's not especially obvious how to do the UI. That can always be retrofitted later if there's demand.

To post a comment you must log in.
18259. By Colin Watson

Make GitSubscription.paths be a JSON-encoded list instead.

Revision history for this message
Colin Watson (cjwatson) wrote :

Unmerged revisions

18259. By Colin Watson

Make GitSubscription.paths be a JSON-encoded list instead.

18258. By Colin Watson

Allow subscribing to only particular reference paths within a Git repository.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/browser/branchsubscription.py'
--- lib/lp/code/browser/branchsubscription.py 2015-09-28 17:38:45 +0000
+++ lib/lp/code/browser/branchsubscription.py 2016-11-17 15:01:52 +0000
@@ -1,4 +1,4 @@
1# Copyright 2009-2015 Canonical Ltd. This software is licensed under the1# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4__metaclass__ = type4__metaclass__ = type
@@ -232,7 +232,7 @@
232 '%s was already subscribed to this branch with: '232 '%s was already subscribed to this branch with: '
233 % person.displayname,233 % person.displayname,
234 subscription.notification_level, subscription.max_diff_lines,234 subscription.notification_level, subscription.max_diff_lines,
235 review_level)235 subscription.review_level)
236236
237237
238class BranchSubscriptionEditView(LaunchpadEditFormView):238class BranchSubscriptionEditView(LaunchpadEditFormView):
239239
=== modified file 'lib/lp/code/browser/gitsubscription.py'
--- lib/lp/code/browser/gitsubscription.py 2015-09-28 17:38:45 +0000
+++ lib/lp/code/browser/gitsubscription.py 2016-11-17 15:01:52 +0000
@@ -1,4 +1,4 @@
1# Copyright 2015 Canonical Ltd. This software is licensed under the1# Copyright 2015-2016 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4__metaclass__ = type4__metaclass__ = type
@@ -11,10 +11,17 @@
11 'GitSubscriptionEditView',11 'GitSubscriptionEditView',
12 ]12 ]
1313
14from lazr.restful.interface import (
15 copy_field,
16 use_template,
17 )
14from zope.component import getUtility18from zope.component import getUtility
19from zope.formlib.textwidgets import TextWidget
20from zope.interface import Interface
1521
16from lp.app.browser.launchpadform import (22from lp.app.browser.launchpadform import (
17 action,23 action,
24 custom_widget,
18 LaunchpadEditFormView,25 LaunchpadEditFormView,
19 LaunchpadFormView,26 LaunchpadFormView,
20 )27 )
@@ -60,11 +67,62 @@
60 key=lambda subscription: subscription.person.displayname)67 key=lambda subscription: subscription.person.displayname)
6168
6269
70# XXX cjwatson 2016-11-09: We should just use IGitSubscription directly, but
71# we have to modify the description of `paths`: WADL generation demands that
72# the "*" characters be quoted as inline literals to avoid being treated as
73# mismatched emphasis characters, but "``" looks weird in HTML. Perhaps we
74# should arrange for forms to run descriptions through pydoc so that we
75# don't display unprocessed markup?
76# But in any case we need to adjust the description to say that the list is
77# space-separated, given our presentation of it.
78class IGitSubscriptionSchema(Interface):
79
80 use_template(IGitSubscription, include=[
81 "person",
82 "notification_level",
83 "max_diff_lines",
84 "review_level",
85 ])
86 paths = copy_field(IGitSubscription["paths"], description=(
87 u"A space-separated list of patterns matching subscribed reference "
88 u"paths. For example, 'refs/heads/master refs/heads/next' matches "
89 u"just those two branches, while 'refs/heads/releases/*' matches "
90 u"all branches under 'refs/heads/releases/'. Leave this empty to "
91 u"subscribe to the whole repository."))
92
93
94class SpaceDelimitedListWidget(TextWidget):
95 """A widget that represents a list as space-delimited text."""
96
97 def __init__(self, field, value_type, request):
98 # We don't use value_type.
99 super(SpaceDelimitedListWidget, self).__init__(field, request)
100
101 def _toFormValue(self, value):
102 """Convert a list to a space-separated string."""
103 if value == self.context.missing_value or value is None:
104 value = self._missing
105 else:
106 value = u" ".join(value)
107 return super(SpaceDelimitedListWidget, self)._toFormValue(value)
108
109 def _toFieldValue(self, value):
110 """Convert the input string into a list."""
111 value = super(SpaceDelimitedListWidget, self)._toFieldValue(value)
112 if value == self.context.missing_value:
113 return value
114 else:
115 return value.split()
116
117
63class _GitSubscriptionView(LaunchpadFormView):118class _GitSubscriptionView(LaunchpadFormView):
64 """Contains the common functionality of the Add and Edit views."""119 """Contains the common functionality of the Add and Edit views."""
65120
66 schema = IGitSubscription121 schema = IGitSubscriptionSchema
67 field_names = ['notification_level', 'max_diff_lines', 'review_level']122 field_names = [
123 'paths', 'notification_level', 'max_diff_lines', 'review_level',
124 ]
125 custom_widget('paths', SpaceDelimitedListWidget)
68126
69 LEVELS_REQUIRING_LINES_SPECIFICATION = (127 LEVELS_REQUIRING_LINES_SPECIFICATION = (
70 BranchSubscriptionNotificationLevel.DIFFSONLY,128 BranchSubscriptionNotificationLevel.DIFFSONLY,
@@ -83,17 +141,26 @@
83141
84 cancel_url = next_url142 cancel_url = next_url
85143
86 def add_notification_message(self, initial, notification_level,144 def add_notification_message(self, initial, paths, notification_level,
87 max_diff_lines, review_level):145 max_diff_lines, review_level):
146 items = []
147 if paths is not None:
148 items.append("Only paths matching %(paths)s.")
149 items.append("%(notification_level)s")
88 if notification_level in self.LEVELS_REQUIRING_LINES_SPECIFICATION:150 if notification_level in self.LEVELS_REQUIRING_LINES_SPECIFICATION:
89 lines_message = "<li>%s</li>" % max_diff_lines.description151 items.append("%(max_diff_lines)s")
90 else:152 items.append("%(review_level)s")
91 lines_message = ""153 format_str = (
92154 "%(initial)s<ul>" +
93 format_str = "%%s<ul><li>%%s</li>%s<li>%%s</li></ul>" % lines_message155 "".join("<li>%s</li>" % item for item in items) + "</ul>")
94 message = structured(156 message = structured(
95 format_str, initial, notification_level.description,157 format_str, initial=initial,
96 review_level.description)158 paths=(" ".join(paths) if paths is not None else None),
159 notification_level=notification_level.description,
160 max_diff_lines=(
161 max_diff_lines.description
162 if max_diff_lines is not None else None),
163 review_level=review_level.description)
97 self.request.response.addNotification(message)164 self.request.response.addNotification(message)
98165
99 def optional_max_diff_lines(self, notification_level, max_diff_lines):166 def optional_max_diff_lines(self, notification_level, max_diff_lines):
@@ -115,6 +182,7 @@
115 self.request.response.addNotification(182 self.request.response.addNotification(
116 "You are already subscribed to this repository.")183 "You are already subscribed to this repository.")
117 else:184 else:
185 paths = data.get("paths")
118 notification_level = data["notification_level"]186 notification_level = data["notification_level"]
119 max_diff_lines = self.optional_max_diff_lines(187 max_diff_lines = self.optional_max_diff_lines(
120 notification_level, data["max_diff_lines"])188 notification_level, data["max_diff_lines"])
@@ -122,11 +190,11 @@
122190
123 self.context.subscribe(191 self.context.subscribe(
124 self.user, notification_level, max_diff_lines, review_level,192 self.user, notification_level, max_diff_lines, review_level,
125 self.user)193 self.user, paths=paths)
126194
127 self.add_notification_message(195 self.add_notification_message(
128 "You have subscribed to this repository with: ",196 "You have subscribed to this repository with: ",
129 notification_level, max_diff_lines, review_level)197 paths, notification_level, max_diff_lines, review_level)
130198
131199
132class GitSubscriptionEditOwnView(_GitSubscriptionView):200class GitSubscriptionEditOwnView(_GitSubscriptionView):
@@ -146,15 +214,19 @@
146 # This is the case of URL hacking or stale page.214 # This is the case of URL hacking or stale page.
147 return {}215 return {}
148 else:216 else:
149 return {"notification_level": subscription.notification_level,217 return {
150 "max_diff_lines": subscription.max_diff_lines,218 "paths": subscription.paths,
151 "review_level": subscription.review_level}219 "notification_level": subscription.notification_level,
220 "max_diff_lines": subscription.max_diff_lines,
221 "review_level": subscription.review_level,
222 }
152223
153 @action("Change")224 @action("Change")
154 def change_details(self, action, data):225 def change_details(self, action, data):
155 # Be proactive in the checking to catch the stale post problem.226 # Be proactive in the checking to catch the stale post problem.
156 if self.context.hasSubscription(self.user):227 if self.context.hasSubscription(self.user):
157 subscription = self.context.getSubscription(self.user)228 subscription = self.context.getSubscription(self.user)
229 subscription.paths = data.get("paths")
158 subscription.notification_level = data["notification_level"]230 subscription.notification_level = data["notification_level"]
159 subscription.max_diff_lines = self.optional_max_diff_lines(231 subscription.max_diff_lines = self.optional_max_diff_lines(
160 subscription.notification_level,232 subscription.notification_level,
@@ -163,6 +235,7 @@
163235
164 self.add_notification_message(236 self.add_notification_message(
165 "Subscription updated to: ",237 "Subscription updated to: ",
238 subscription.paths,
166 subscription.notification_level,239 subscription.notification_level,
167 subscription.max_diff_lines,240 subscription.max_diff_lines,
168 subscription.review_level)241 subscription.review_level)
@@ -186,7 +259,9 @@
186 """View used to subscribe someone other than the current user."""259 """View used to subscribe someone other than the current user."""
187260
188 field_names = [261 field_names = [
189 "person", "notification_level", "max_diff_lines", "review_level"]262 "person", "paths", "notification_level", "max_diff_lines",
263 "review_level",
264 ]
190 for_input = True265 for_input = True
191266
192 # Since we are subscribing other people, the current user267 # Since we are subscribing other people, the current user
@@ -214,6 +289,7 @@
214 to the repository. Launchpad Admins are special and they can289 to the repository. Launchpad Admins are special and they can
215 subscribe any team.290 subscribe any team.
216 """291 """
292 paths = data.get("paths")
217 notification_level = data["notification_level"]293 notification_level = data["notification_level"]
218 max_diff_lines = self.optional_max_diff_lines(294 max_diff_lines = self.optional_max_diff_lines(
219 notification_level, data["max_diff_lines"])295 notification_level, data["max_diff_lines"])
@@ -223,17 +299,17 @@
223 if subscription is None:299 if subscription is None:
224 self.context.subscribe(300 self.context.subscribe(
225 person, notification_level, max_diff_lines, review_level,301 person, notification_level, max_diff_lines, review_level,
226 self.user)302 self.user, paths=paths)
227 self.add_notification_message(303 self.add_notification_message(
228 "%s has been subscribed to this repository with: "304 "%s has been subscribed to this repository with: "
229 % person.displayname, notification_level, max_diff_lines,305 % person.displayname,
230 review_level)306 paths, notification_level, max_diff_lines, review_level)
231 else:307 else:
232 self.add_notification_message(308 self.add_notification_message(
233 "%s was already subscribed to this repository with: "309 "%s was already subscribed to this repository with: "
234 % person.displayname,310 % person.displayname,
235 subscription.notification_level, subscription.max_diff_lines,311 subscription.paths, subscription.notification_level,
236 review_level)312 subscription.max_diff_lines, subscription.review_level)
237313
238314
239class GitSubscriptionEditView(LaunchpadEditFormView):315class GitSubscriptionEditView(LaunchpadEditFormView):
@@ -243,8 +319,15 @@
243 through the repository action item to edit the user's own subscription.319 through the repository action item to edit the user's own subscription.
244 This is the only current way to edit a team repository subscription.320 This is the only current way to edit a team repository subscription.
245 """321 """
246 schema = IGitSubscription322 schema = IGitSubscriptionSchema
247 field_names = ["notification_level", "max_diff_lines", "review_level"]323 field_names = [
324 "paths", "notification_level", "max_diff_lines", "review_level",
325 ]
326
327 @property
328 def adapters(self):
329 """See `LaunchpadFormView`."""
330 return {IGitSubscriptionSchema: self.context}
248331
249 @property332 @property
250 def page_title(self):333 def page_title(self):
251334
=== modified file 'lib/lp/code/browser/tests/test_gitsubscription.py'
--- lib/lp/code/browser/tests/test_gitsubscription.py 2016-11-09 17:18:21 +0000
+++ lib/lp/code/browser/tests/test_gitsubscription.py 2016-11-17 15:01:52 +0000
@@ -114,7 +114,7 @@
114 with person_logged_in(subscriber):114 with person_logged_in(subscriber):
115 subscription = repository.getSubscription(subscriber)115 subscription = repository.getSubscription(subscriber)
116 self.assertThat(subscription, MatchesStructure.byEquality(116 self.assertThat(subscription, MatchesStructure.byEquality(
117 person=subscriber, repository=repository,117 person=subscriber, repository=repository, paths=None,
118 notification_level=(118 notification_level=(
119 BranchSubscriptionNotificationLevel.ATTRIBUTEONLY),119 BranchSubscriptionNotificationLevel.ATTRIBUTEONLY),
120 max_diff_lines=None,120 max_diff_lines=None,
@@ -145,6 +145,7 @@
145 None, CodeReviewNotificationLevel.FULL, subscriber)145 None, CodeReviewNotificationLevel.FULL, subscriber)
146 browser = self.getViewBrowser(repository, user=subscriber)146 browser = self.getViewBrowser(repository, user=subscriber)
147 browser.getLink('Edit your subscription').click()147 browser.getLink('Edit your subscription').click()
148 browser.getControl('Paths').value = 'refs/heads/master refs/heads/next'
148 browser.getControl('Notification Level').displayValue = [149 browser.getControl('Notification Level').displayValue = [
149 'Branch attribute and revision notifications']150 'Branch attribute and revision notifications']
150 browser.getControl('Generated Diff Size Limit').displayValue = [151 browser.getControl('Generated Diff Size Limit').displayValue = [
@@ -152,6 +153,7 @@
152 browser.getControl('Change').click()153 browser.getControl('Change').click()
153 self.assertTextMatchesExpressionIgnoreWhitespace(154 self.assertTextMatchesExpressionIgnoreWhitespace(
154 'Subscription updated to: '155 'Subscription updated to: '
156 'Only paths matching refs/heads/master refs/heads/next. '
155 'Send notifications for both branch attribute updates and new '157 'Send notifications for both branch attribute updates and new '
156 'revisions added to the branch. '158 'revisions added to the branch. '
157 'Limit the generated diff to 5000 lines. '159 'Limit the generated diff to 5000 lines. '
@@ -161,6 +163,7 @@
161 subscription = repository.getSubscription(subscriber)163 subscription = repository.getSubscription(subscriber)
162 self.assertThat(subscription, MatchesStructure.byEquality(164 self.assertThat(subscription, MatchesStructure.byEquality(
163 person=subscriber, repository=repository,165 person=subscriber, repository=repository,
166 paths=['refs/heads/master', 'refs/heads/next'],
164 notification_level=BranchSubscriptionNotificationLevel.FULL,167 notification_level=BranchSubscriptionNotificationLevel.FULL,
165 max_diff_lines=BranchSubscriptionDiffSize.FIVEKLINES,168 max_diff_lines=BranchSubscriptionDiffSize.FIVEKLINES,
166 review_level=CodeReviewNotificationLevel.FULL))169 review_level=CodeReviewNotificationLevel.FULL))
167170
=== modified file 'lib/lp/code/interfaces/gitref.py'
--- lib/lp/code/interfaces/gitref.py 2016-11-09 17:18:21 +0000
+++ lib/lp/code/interfaces/gitref.py 2016-11-17 15:01:52 +0000
@@ -189,7 +189,7 @@
189 "Persons subscribed to the repository containing this reference.")189 "Persons subscribed to the repository containing this reference.")
190190
191 def subscribe(person, notification_level, max_diff_lines,191 def subscribe(person, notification_level, max_diff_lines,
192 code_review_level, subscribed_by):192 code_review_level, subscribed_by, paths=None):
193 """Subscribe this person to the repository containing this reference.193 """Subscribe this person to the repository containing this reference.
194194
195 :param person: The `Person` to subscribe.195 :param person: The `Person` to subscribe.
@@ -201,6 +201,8 @@
201 cause notification.201 cause notification.
202 :param subscribed_by: The person who is subscribing the subscriber.202 :param subscribed_by: The person who is subscribing the subscriber.
203 Most often the subscriber themselves.203 Most often the subscriber themselves.
204 :param paths: An optional list of patterns matching reference paths
205 to subscribe to.
204 :return: A new or existing `GitSubscription`.206 :return: A new or existing `GitSubscription`.
205 """207 """
206208
207209
=== modified file 'lib/lp/code/interfaces/gitrepository.py'
--- lib/lp/code/interfaces/gitrepository.py 2016-10-14 15:08:36 +0000
+++ lib/lp/code/interfaces/gitrepository.py 2016-11-17 15:01:52 +0000
@@ -415,14 +415,23 @@
415 vocabulary=BranchSubscriptionDiffSize),415 vocabulary=BranchSubscriptionDiffSize),
416 code_review_level=Choice(416 code_review_level=Choice(
417 title=_("The level of code review notification emails."),417 title=_("The level of code review notification emails."),
418 vocabulary=CodeReviewNotificationLevel))418 vocabulary=CodeReviewNotificationLevel),
419 paths=List(
420 title=_("Paths"), required=False,
421 description=_(
422 "A list of patterns matching reference paths to subscribe "
423 "to. For example, ``['refs/heads/master', "
424 "'refs/heads/next']`` matches just those two branches, while "
425 "``['refs/heads/releases/*']`` matches all branches under "
426 "``refs/heads/releases/``. Omit this parameter to subscribe "
427 "to the whole repository.")))
419 # Really IGitSubscription, patched in _schema_circular_imports.py.428 # Really IGitSubscription, patched in _schema_circular_imports.py.
420 @operation_returns_entry(Interface)429 @operation_returns_entry(Interface)
421 @call_with(subscribed_by=REQUEST_USER)430 @call_with(subscribed_by=REQUEST_USER)
422 @export_write_operation()431 @export_write_operation()
423 @operation_for_version("devel")432 @operation_for_version("devel")
424 def subscribe(person, notification_level, max_diff_lines,433 def subscribe(person, notification_level, max_diff_lines,
425 code_review_level, subscribed_by):434 code_review_level, subscribed_by, paths=None):
426 """Subscribe this person to the repository.435 """Subscribe this person to the repository.
427436
428 :param person: The `Person` to subscribe.437 :param person: The `Person` to subscribe.
@@ -434,6 +443,8 @@
434 cause notification.443 cause notification.
435 :param subscribed_by: The person who is subscribing the subscriber.444 :param subscribed_by: The person who is subscribing the subscriber.
436 Most often the subscriber themselves.445 Most often the subscriber themselves.
446 :param paths: An optional list of patterns matching reference paths
447 to subscribe to.
437 :return: A new or existing `GitSubscription`.448 :return: A new or existing `GitSubscription`.
438 """449 """
439450
@@ -469,11 +480,14 @@
469 :return: A `ResultSet`.480 :return: A `ResultSet`.
470 """481 """
471482
472 def getNotificationRecipients():483 def getNotificationRecipients(path=None):
473 """Return a complete INotificationRecipientSet instance.484 """Return a complete INotificationRecipientSet instance.
474485
475 The INotificationRecipientSet instance contains the subscribers486 The INotificationRecipientSet instance contains the subscribers
476 and their subscriptions.487 and their subscriptions.
488
489 :param path: If not None, only consider subscriptions that match
490 this reference path.
477 """491 """
478492
479 landing_targets = exported(CollectionField(493 landing_targets = exported(CollectionField(
480494
=== modified file 'lib/lp/code/interfaces/gitsubscription.py'
--- lib/lp/code/interfaces/gitsubscription.py 2015-04-21 09:29:00 +0000
+++ lib/lp/code/interfaces/gitsubscription.py 2016-11-17 15:01:52 +0000
@@ -1,4 +1,4 @@
1# Copyright 2009-2015 Canonical Ltd. This software is licensed under the1# Copyright 2009-2016 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""Git repository subscription interfaces."""4"""Git repository subscription interfaces."""
@@ -22,6 +22,8 @@
22from zope.schema import (22from zope.schema import (
23 Choice,23 Choice,
24 Int,24 Int,
25 List,
26 TextLine,
25 )27 )
2628
27from lp import _29from lp import _
@@ -58,6 +60,18 @@
58 Reference(60 Reference(
59 title=_("Repository ID"), required=True, readonly=True,61 title=_("Repository ID"), required=True, readonly=True,
60 schema=IGitRepository))62 schema=IGitRepository))
63 paths = List(title=_("Paths"), value_type=TextLine(), required=False)
64 api_paths = exported(
65 List(
66 title=_("Paths"), value_type=TextLine(), required=True,
67 description=_(
68 "A list of patterns matching subscribed reference paths. "
69 "For example, ``['refs/heads/master', 'refs/heads/next']`` "
70 "matches just those two branches, while "
71 "``['refs/heads/releases/*']`` matches all branches under "
72 "``refs/heads/releases/``. Leave this empty to subscribe "
73 "to the whole repository.")),
74 exported_as="paths")
61 notification_level = exported(75 notification_level = exported(
62 Choice(76 Choice(
63 title=_("Notification Level"), required=True,77 title=_("Notification Level"), required=True,
@@ -97,3 +111,6 @@
97 @operation_for_version("devel")111 @operation_for_version("devel")
98 def canBeUnsubscribedByUser(user):112 def canBeUnsubscribedByUser(user):
99 """Can the user unsubscribe the subscriber from the repository?"""113 """Can the user unsubscribe the subscriber from the repository?"""
114
115 def matchesPath(path):
116 """Does this subscription match this reference path?"""
100117
=== modified file 'lib/lp/code/model/gitref.py'
--- lib/lp/code/model/gitref.py 2016-11-09 17:18:21 +0000
+++ lib/lp/code/model/gitref.py 2016-11-17 15:01:52 +0000
@@ -168,11 +168,11 @@
168 return self.repository.subscribers168 return self.repository.subscribers
169169
170 def subscribe(self, person, notification_level, max_diff_lines,170 def subscribe(self, person, notification_level, max_diff_lines,
171 code_review_level, subscribed_by):171 code_review_level, subscribed_by, paths=None):
172 """See `IGitRef`."""172 """See `IGitRef`."""
173 return self.repository.subscribe(173 return self.repository.subscribe(
174 person, notification_level, max_diff_lines, code_review_level,174 person, notification_level, max_diff_lines, code_review_level,
175 subscribed_by)175 subscribed_by, paths=paths)
176176
177 def getSubscription(self, person):177 def getSubscription(self, person):
178 """See `IGitRef`."""178 """See `IGitRef`."""
@@ -184,7 +184,7 @@
184184
185 def getNotificationRecipients(self):185 def getNotificationRecipients(self):
186 """See `IGitRef`."""186 """See `IGitRef`."""
187 return self.repository.getNotificationRecipients()187 return self.repository.getNotificationRecipients(path=self.path)
188188
189 @property189 @property
190 def landing_targets(self):190 def landing_targets(self):
191191
=== modified file 'lib/lp/code/model/gitrepository.py'
--- lib/lp/code/model/gitrepository.py 2016-10-14 15:08:58 +0000
+++ lib/lp/code/model/gitrepository.py 2016-11-17 15:01:52 +0000
@@ -795,7 +795,7 @@
795 person.anyone_can_join())795 person.anyone_can_join())
796796
797 def subscribe(self, person, notification_level, max_diff_lines,797 def subscribe(self, person, notification_level, max_diff_lines,
798 code_review_level, subscribed_by):798 code_review_level, subscribed_by, paths=None):
799 """See `IGitRepository`."""799 """See `IGitRepository`."""
800 if not self.userCanBeSubscribed(person):800 if not self.userCanBeSubscribed(person):
801 raise SubscriptionPrivacyViolation(801 raise SubscriptionPrivacyViolation(
@@ -806,12 +806,13 @@
806 subscription = self.getSubscription(person)806 subscription = self.getSubscription(person)
807 if subscription is None:807 if subscription is None:
808 subscription = GitSubscription(808 subscription = GitSubscription(
809 person=person, repository=self,809 person=person, repository=self, paths=paths,
810 notification_level=notification_level,810 notification_level=notification_level,
811 max_diff_lines=max_diff_lines, review_level=code_review_level,811 max_diff_lines=max_diff_lines, review_level=code_review_level,
812 subscribed_by=subscribed_by)812 subscribed_by=subscribed_by)
813 Store.of(subscription).flush()813 Store.of(subscription).flush()
814 else:814 else:
815 subscription.paths = paths
815 subscription.notification_level = notification_level816 subscription.notification_level = notification_level
816 subscription.max_diff_lines = max_diff_lines817 subscription.max_diff_lines = max_diff_lines
817 subscription.review_level = code_review_level818 subscription.review_level = code_review_level
@@ -869,10 +870,12 @@
869 artifact, [person])870 artifact, [person])
870 store.flush()871 store.flush()
871872
872 def getNotificationRecipients(self):873 def getNotificationRecipients(self, path=None):
873 """See `IGitRepository`."""874 """See `IGitRepository`."""
874 recipients = NotificationRecipientSet()875 recipients = NotificationRecipientSet()
875 for subscription in self.subscriptions:876 for subscription in self.subscriptions:
877 if path is not None and not subscription.matchesPath(path):
878 continue
876 if subscription.person.is_team:879 if subscription.person.is_team:
877 rationale = 'Subscriber @%s' % subscription.person.name880 rationale = 'Subscriber @%s' % subscription.person.name
878 else:881 else:
879882
=== modified file 'lib/lp/code/model/gitsubscription.py'
--- lib/lp/code/model/gitsubscription.py 2015-07-08 16:05:11 +0000
+++ lib/lp/code/model/gitsubscription.py 2016-11-17 15:01:52 +0000
@@ -1,4 +1,4 @@
1# Copyright 2015 Canonical Ltd. This software is licensed under the1# Copyright 2015-2016 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4__metaclass__ = type4__metaclass__ = type
@@ -6,8 +6,11 @@
6 'GitSubscription',6 'GitSubscription',
7 ]7 ]
88
9import fnmatch
10
9from storm.locals import (11from storm.locals import (
10 Int,12 Int,
13 JSON,
11 Reference,14 Reference,
12 )15 )
13from zope.interface import implementer16from zope.interface import implementer
@@ -40,6 +43,8 @@
40 repository_id = Int(name='repository', allow_none=False)43 repository_id = Int(name='repository', allow_none=False)
41 repository = Reference(repository_id, 'GitRepository.id')44 repository = Reference(repository_id, 'GitRepository.id')
4245
46 paths = JSON(name='paths', allow_none=True)
47
43 notification_level = EnumCol(48 notification_level = EnumCol(
44 enum=BranchSubscriptionNotificationLevel, notNull=True,49 enum=BranchSubscriptionNotificationLevel, notNull=True,
45 default=DEFAULT)50 default=DEFAULT)
@@ -52,19 +57,38 @@
52 name='subscribed_by', allow_none=False, validator=validate_person)57 name='subscribed_by', allow_none=False, validator=validate_person)
53 subscribed_by = Reference(subscribed_by_id, 'Person.id')58 subscribed_by = Reference(subscribed_by_id, 'Person.id')
5459
55 def __init__(self, person, repository, notification_level, max_diff_lines,60 def __init__(self, person, repository, paths, notification_level,
56 review_level, subscribed_by):61 max_diff_lines, review_level, subscribed_by):
57 super(GitSubscription, self).__init__()62 super(GitSubscription, self).__init__()
58 self.person = person63 self.person = person
59 self.repository = repository64 self.repository = repository
65 self.paths = paths
60 self.notification_level = notification_level66 self.notification_level = notification_level
61 self.max_diff_lines = max_diff_lines67 self.max_diff_lines = max_diff_lines
62 self.review_level = review_level68 self.review_level = review_level
63 self.subscribed_by = subscribed_by69 self.subscribed_by = subscribed_by
6470
65 def canBeUnsubscribedByUser(self, user):71 def canBeUnsubscribedByUser(self, user):
66 """See `IBranchSubscription`."""72 """See `IGitSubscription`."""
67 if user is None:73 if user is None:
68 return False74 return False
69 permission_check = GitSubscriptionEdit(self)75 permission_check = GitSubscriptionEdit(self)
70 return permission_check.checkAuthenticated(IPersonRoles(user))76 return permission_check.checkAuthenticated(IPersonRoles(user))
77
78 @property
79 def api_paths(self):
80 """See `IGitSubscription`.
81
82 lazr.restful can't represent the difference between a NULL
83 collection and an empty collection, so we simulate the former.
84 """
85 return ["*"] if self.paths is None else self.paths
86
87 def matchesPath(self, path):
88 """See `IGitSubscription`."""
89 if self.paths is None:
90 return True
91 for pattern in self.paths:
92 if fnmatch.fnmatch(path, pattern):
93 return True
94 return False
7195
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposal.py'
--- lib/lp/code/model/tests/test_branchmergeproposal.py 2016-11-09 17:18:21 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposal.py 2016-11-17 15:01:52 +0000
@@ -963,6 +963,34 @@
963 source_ref=source, target_ref=target,963 source_ref=source, target_ref=target,
964 prerequisite_ref=prerequisite, **kwargs)964 prerequisite_ref=prerequisite, **kwargs)
965965
966 def test_getNotificationRecipients_path(self):
967 # If the subscription specifies path patterns, then one of them must
968 # match the reference.
969 bmp = self.makeBranchMergeProposal()
970 source_owner = bmp.merge_source.owner
971 target_owner = bmp.merge_target.owner
972 recipients = bmp.getNotificationRecipients(
973 CodeReviewNotificationLevel.STATUS)
974 subscriber_set = set([source_owner, target_owner])
975 self.assertEqual(subscriber_set, set(recipients.keys()))
976 # Subscribing somebody to a non-matching path has no effect.
977 subscriber = self.factory.makePerson()
978 bmp.merge_source.subscribe(
979 subscriber, BranchSubscriptionNotificationLevel.NOEMAIL, None,
980 CodeReviewNotificationLevel.FULL, subscriber, paths=u"no-match")
981 recipients = bmp.getNotificationRecipients(
982 CodeReviewNotificationLevel.STATUS)
983 self.assertEqual(subscriber_set, set(recipients.keys()))
984 # Subscribing somebody to a matching path is effective.
985 bmp.merge_source.subscribe(
986 subscriber, BranchSubscriptionNotificationLevel.NOEMAIL, None,
987 CodeReviewNotificationLevel.FULL, subscriber,
988 paths=bmp.merge_source.path)
989 recipients = bmp.getNotificationRecipients(
990 CodeReviewNotificationLevel.STATUS)
991 subscriber_set.add(subscriber)
992 self.assertEqual(subscriber_set, set(recipients.keys()))
993
966994
967class TestMergeProposalWebhooksMixin:995class TestMergeProposalWebhooksMixin:
968996
969997
=== modified file 'lib/lp/code/model/tests/test_gitrepository.py'
--- lib/lp/code/model/tests/test_gitrepository.py 2016-10-14 16:16:18 +0000
+++ lib/lp/code/model/tests/test_gitrepository.py 2016-11-17 15:01:52 +0000
@@ -19,7 +19,9 @@
19from storm.exceptions import LostObjectError19from storm.exceptions import LostObjectError
20from storm.store import Store20from storm.store import Store
21from testtools.matchers import (21from testtools.matchers import (
22 ContainsDict,
22 EndsWith,23 EndsWith,
24 Equals,
23 MatchesSetwise,25 MatchesSetwise,
24 MatchesStructure,26 MatchesStructure,
25 )27 )
@@ -2699,9 +2701,34 @@
2699 with person_logged_in(ANONYMOUS):2701 with person_logged_in(ANONYMOUS):
2700 subscription_db = repository_db.getSubscription(subscriber_db)2702 subscription_db = repository_db.getSubscription(subscriber_db)
2701 self.assertIsNotNone(subscription_db)2703 self.assertIsNotNone(subscription_db)
2702 self.assertThat(2704 self.assertThat(response.jsonBody(), ContainsDict({
2703 response.jsonBody()["self_link"],2705 "self_link": EndsWith(api_url(subscription_db)),
2704 EndsWith(api_url(subscription_db)))2706 "paths": Equals([u"*"]),
2707 }))
2708
2709 def test_subscribe_with_paths(self):
2710 # A user can subscribe to some reference paths in a repository.
2711 repository_db = self.factory.makeGitRepository()
2712 subscriber_db = self.factory.makePerson()
2713 webservice = webservice_for_person(
2714 subscriber_db, permission=OAuthPermission.WRITE_PUBLIC)
2715 webservice.default_api_version = "devel"
2716 with person_logged_in(ANONYMOUS):
2717 repository_url = api_url(repository_db)
2718 subscriber_url = api_url(subscriber_db)
2719 response = webservice.named_post(
2720 repository_url, "subscribe", person=subscriber_url,
2721 notification_level=u"Branch attribute notifications only",
2722 max_diff_lines=u"Don't send diffs", code_review_level=u"No email",
2723 paths=[u"refs/heads/master", u"refs/heads/next"])
2724 self.assertEqual(200, response.status)
2725 with person_logged_in(ANONYMOUS):
2726 subscription_db = repository_db.getSubscription(subscriber_db)
2727 self.assertIsNotNone(subscription_db)
2728 self.assertThat(response.jsonBody(), ContainsDict({
2729 "self_link": EndsWith(api_url(subscription_db)),
2730 "paths": Equals([u"refs/heads/master", u"refs/heads/next"]),
2731 }))
27052732
2706 def _makeSubscription(self, repository, subscriber):2733 def _makeSubscription(self, repository, subscriber):
2707 with person_logged_in(subscriber):2734 with person_logged_in(subscriber):
@@ -2747,7 +2774,8 @@
2747 repository_url, "subscribe", person=subscriber_url,2774 repository_url, "subscribe", person=subscriber_url,
2748 notification_level=u"No email",2775 notification_level=u"No email",
2749 max_diff_lines=u"Send entire diff",2776 max_diff_lines=u"Send entire diff",
2750 code_review_level=u"Status changes only")2777 code_review_level=u"Status changes only",
2778 paths=[u"refs/heads/next"])
2751 self.assertEqual(200, response.status)2779 self.assertEqual(200, response.status)
2752 with person_logged_in(subscriber_db):2780 with person_logged_in(subscriber_db):
2753 self.assertThat(2781 self.assertThat(
@@ -2758,6 +2786,7 @@
2758 BranchSubscriptionNotificationLevel.NOEMAIL),2786 BranchSubscriptionNotificationLevel.NOEMAIL),
2759 max_diff_lines=BranchSubscriptionDiffSize.WHOLEDIFF,2787 max_diff_lines=BranchSubscriptionDiffSize.WHOLEDIFF,
2760 review_level=CodeReviewNotificationLevel.STATUS,2788 review_level=CodeReviewNotificationLevel.STATUS,
2789 paths=[u"refs/heads/next"],
2761 ))2790 ))
2762 repository = webservice.get(repository_url).jsonBody()2791 repository = webservice.get(repository_url).jsonBody()
2763 subscribers = webservice.get(2792 subscribers = webservice.get(
27642793
=== modified file 'lib/lp/code/model/tests/test_gitsubscription.py'
--- lib/lp/code/model/tests/test_gitsubscription.py 2015-05-14 13:57:51 +0000
+++ lib/lp/code/model/tests/test_gitsubscription.py 2016-11-17 15:01:52 +0000
@@ -1,4 +1,4 @@
1# Copyright 2010-2015 Canonical Ltd. This software is licensed under the1# Copyright 2010-2016 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).2# GNU Affero General Public License version 3 (see the file LICENSE).
33
4"""Tests for the GitSubscription model object."""4"""Tests for the GitSubscription model object."""
@@ -110,6 +110,41 @@
110 repository.unsubscribe(subscribee, owner)110 repository.unsubscribe(subscribee, owner)
111 self.assertFalse(repository.visibleByUser(subscribee))111 self.assertFalse(repository.visibleByUser(subscribee))
112112
113 def test_matchesPath_none_matches_anything(self):
114 # A paths=None subscription matches any path.
115 subscription = self.factory.makeGitSubscription(paths=None)
116 self.assertTrue(subscription.matchesPath(u"refs/heads/master"))
117 self.assertTrue(subscription.matchesPath(u"refs/heads/next"))
118 self.assertTrue(subscription.matchesPath(u"nonsense"))
119
120 def test_matchesPath_exact(self):
121 # A subscription to a single path matches only that path.
122 subscription = self.factory.makeGitSubscription(
123 paths=[u"refs/heads/master"])
124 self.assertTrue(subscription.matchesPath(u"refs/heads/master"))
125 self.assertFalse(subscription.matchesPath(u"refs/heads/master2"))
126 self.assertFalse(subscription.matchesPath(u"refs/heads/next"))
127
128 def test_matchesPath_fnmatch(self):
129 # A subscription to a path pattern matches anything that fnmatch
130 # accepts.
131 subscription = self.factory.makeGitSubscription(
132 paths=[u"refs/heads/*"])
133 self.assertTrue(subscription.matchesPath(u"refs/heads/master"))
134 self.assertTrue(subscription.matchesPath(u"refs/heads/next"))
135 self.assertTrue(subscription.matchesPath(u"refs/heads/foo/bar"))
136 self.assertFalse(subscription.matchesPath(u"refs/tags/1.0"))
137
138 def test_matchesPath_multiple(self):
139 # A subscription to multiple path patterns matches any of them.
140 subscription = self.factory.makeGitSubscription(
141 paths=[u"refs/heads/*", u"refs/tags/1.0"])
142 self.assertTrue(subscription.matchesPath(u"refs/heads/master"))
143 self.assertTrue(subscription.matchesPath(u"refs/heads/next"))
144 self.assertTrue(subscription.matchesPath(u"refs/heads/foo/bar"))
145 self.assertTrue(subscription.matchesPath(u"refs/tags/1.0"))
146 self.assertFalse(subscription.matchesPath(u"refs/tags/1.0a"))
147
113148
114class TestGitSubscriptionCanBeUnsubscribedbyUser(TestCaseWithFactory):149class TestGitSubscriptionCanBeUnsubscribedbyUser(TestCaseWithFactory):
115 """Tests for GitSubscription.canBeUnsubscribedByUser."""150 """Tests for GitSubscription.canBeUnsubscribedByUser."""
116151
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2016-11-03 15:07:36 +0000
+++ lib/lp/testing/factory.py 2016-11-17 15:01:52 +0000
@@ -1779,7 +1779,7 @@
1779 return repository1779 return repository
17801780
1781 def makeGitSubscription(self, repository=None, person=None,1781 def makeGitSubscription(self, repository=None, person=None,
1782 subscribed_by=None):1782 subscribed_by=None, paths=None):
1783 """Create a GitSubscription."""1783 """Create a GitSubscription."""
1784 if repository is None:1784 if repository is None:
1785 repository = self.makeGitRepository()1785 repository = self.makeGitRepository()
@@ -1789,7 +1789,7 @@
1789 subscribed_by = person1789 subscribed_by = person
1790 return repository.subscribe(removeSecurityProxy(person),1790 return repository.subscribe(removeSecurityProxy(person),
1791 BranchSubscriptionNotificationLevel.NOEMAIL, None,1791 BranchSubscriptionNotificationLevel.NOEMAIL, None,
1792 CodeReviewNotificationLevel.NOEMAIL, subscribed_by)1792 CodeReviewNotificationLevel.NOEMAIL, subscribed_by, paths=paths)
17931793
1794 def makeGitRefs(self, repository=None, paths=None, **repository_kwargs):1794 def makeGitRefs(self, repository=None, paths=None, **repository_kwargs):
1795 """Create and return a list of new, arbitrary GitRefs."""1795 """Create and return a list of new, arbitrary GitRefs."""