Merge lp:~bryce/launchpad/lp-253509 into lp:launchpad/db-devel

Proposed by Bryce Harrington
Status: Merged
Approved by: Jelmer Vernooij
Approved revision: no longer in the source branch.
Merged at revision: 9398
Proposed branch: lp:~bryce/launchpad/lp-253509
Merge into: lp:launchpad/db-devel
Diff against target: 82 lines (+32/-5)
2 files modified
lib/canonical/launchpad/mailnotification.py (+4/-1)
lib/lp/bugs/doc/security-teams.txt (+28/-4)
To merge this branch: bzr merge lp:~bryce/launchpad/lp-253509
Reviewer Review Type Date Requested Status
Michael Nelson (community) code Approve
Jelmer Vernooij (community) code* Approve
Review via email: mp+25655@code.launchpad.net

Commit message

When transitioning a bug from one project to another, only subscribe the new project's security team if the bug was marked as a security issue.

Description of the change

Modifies update_security_contact_subscriptions() to only subscribe new project's security team if the bug was marked as a security issue.

To post a comment you must log in.
Revision history for this message
Jelmer Vernooij (jelmer) wrote :

Hi Bryce,

Welcome to Launchpad!

Docstrings should start on the same line as the opening quotes (line 8 of your patch).

It would be great to have a test that ensures update_security_contact_subscriptions works as expected and doesn't subscribe the security contact for non-security issues but does for security issues. Otherwise, seems good.

review: Needs Fixing (code*)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

> Hi Bryce,

Hi Bryce and Jelmer.

Both great points Jelmer :)

>
> Welcome to Launchpad!
>
> Docstrings should start on the same line as the opening quotes (line 8 of your
> patch).

If it's too long, just break it up like in other examples:

"""Subscribe the new security contact when a bugtask's product changes.

Note: we only subscribe the security contact if the bug was marked a
security issue originally.
"""

>
> It would be great to have a test that ensures
> update_security_contact_subscriptions works as expected and doesn't subscribe
> the security contact for non-security issues but does for security issues.

+1... it might even be worth grabbing some time with Deryck to chat about the process they use when fixing bugs. Where possible, it's great to write the test first, watch it fail, fix the code and watch it pass, but when starting out it can all be a bit much :)

> Otherwise, seems good.

Revision history for this message
Bryce Harrington (bryce) wrote :

Thanks for the warm welcome.

Guess I'll have to bone up how to write tests.

Revision history for this message
Jelmer Vernooij (jelmer) :
review: Approve (code*)
Revision history for this message
Michael Nelson (michael.nelson) wrote :

Thanks for the documentation Bryce.

Approved as is. Not really worth mentioning, but some people find it more readable to do:

       >>> print subscriber_names(bug)
       name12
       name16

instead of:

> + >>> subscriber_names(bug)
> + [u'name12', u'name16']

See https://dev.launchpad.net/TestsStyleGuide#When%20to%20print%20and%20when%20to%20return%20values if you're interested. You'll also see there that we're gradually moving towards using doctests just for documentation, and testing everything with unit tests, but a long way to go there yet :)

review: Approve (code)
Revision history for this message
Bryce Harrington (bryce) wrote :

Ok thanks for the note; I was just cargo culting off some of the other code in the file in this case.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== added directory 'lib/canonical/launchpad/apidoc'
=== removed directory 'lib/canonical/launchpad/apidoc'
=== modified file 'lib/canonical/launchpad/mailnotification.py'
--- lib/canonical/launchpad/mailnotification.py 2010-02-11 14:04:16 +0000
+++ lib/canonical/launchpad/mailnotification.py 2010-05-25 22:32:33 +0000
@@ -344,6 +344,9 @@
344def update_security_contact_subscriptions(modified_bugtask, event):344def update_security_contact_subscriptions(modified_bugtask, event):
345 """Subscribe the new security contact when a bugtask's product changes.345 """Subscribe the new security contact when a bugtask's product changes.
346346
347 Only subscribes the new security contact if the bug was marked a
348 security issue originally.
349
347 No change is made for private bugs.350 No change is made for private bugs.
348 """351 """
349 if event.object.bug.private:352 if event.object.bug.private:
@@ -358,7 +361,7 @@
358 if (bugtask_before_modification.product !=361 if (bugtask_before_modification.product !=
359 bugtask_after_modification.product):362 bugtask_after_modification.product):
360 new_product = bugtask_after_modification.product363 new_product = bugtask_after_modification.product
361 if new_product.security_contact:364 if bugtask_before_modification.bug.security_related and new_product.security_contact:
362 bugtask_after_modification.bug.subscribe(365 bugtask_after_modification.bug.subscribe(
363 new_product.security_contact, IPerson(event.user))366 new_product.security_contact, IPerson(event.user))
364367
365368
=== modified file 'lib/lp/bugs/doc/security-teams.txt'
--- lib/lp/bugs/doc/security-teams.txt 2010-05-18 21:24:27 +0000
+++ lib/lp/bugs/doc/security-teams.txt 2010-05-25 22:32:33 +0000
@@ -207,8 +207,9 @@
207 [u'name12', u'name16']207 [u'name12', u'name16']
208208
209Finally, reassigning a public bug to a different product will subscribe209Finally, reassigning a public bug to a different product will subscribe
210the new security contact, if present. Let's set stub to the security210the new security contact, if present and if the original bug was marked
211contact for thunderbird to see how this works.211as a security issue. Let's set stub to the security contact for
212thunderbird to see how this works.
212213
213 >>> thunderbird = productset.get(8)214 >>> thunderbird = productset.get(8)
214 >>> print thunderbird.name215 >>> print thunderbird.name
@@ -241,17 +242,40 @@
241 >>> subscriber_names(bug)242 >>> subscriber_names(bug)
242 [u'name12', u'name16']243 [u'name12', u'name16']
243244
244But publish the same event again, when the bug is marked public, and245Also verify the same event again, when marked public does cause
245stub will be added to the subscriber list:246stub to get subscribed:
246247
247 >>> bug.setPrivate(False, getUtility(ILaunchBag).user)248 >>> bug.setPrivate(False, getUtility(ILaunchBag).user)
248 True249 True
249250
251 >>> bug.security_related
252 True
253
250 >>> notify(bug_product_changed)254 >>> notify(bug_product_changed)
251255
252 >>> subscriber_names(bug)256 >>> subscriber_names(bug)
253 [u'name12', u'name16', u'stub']257 [u'name12', u'name16', u'stub']
254258
259But if it is not a security issue originally, stub does not get
260subscribed when moving it to the new project.
261
262 >>> bug.unsubscribe(stub, stub)
263
264 >>> subscriber_names(bug)
265 [u'name12', u'name16']
266
267 >>> bug.setSecurityRelated(False)
268 True
269
270 >>> bug.security_related
271 False
272
273 >>> notify(bug_product_changed)
274
275 >>> subscriber_names(bug)
276 [u'name12', u'name16']
277
278
255When a bug becomes security-related, the security contacts for the pillars it279When a bug becomes security-related, the security contacts for the pillars it
256affects are subscribed to it.280affects are subscribed to it.
257281

Subscribers

People subscribed via source and target branches

to status/vote changes: