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

Proposed by Bryce Harrington on 2010-05-20
Status: Merged
Approved by: Jelmer Vernooij on 2010-05-26
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 2010-05-20 Approve on 2010-05-26
Jelmer Vernooij (community) code* 2010-05-20 Approve on 2010-05-25
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.
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*)
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.

Bryce Harrington (bryce) wrote :

Thanks for the warm welcome.

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

Jelmer Vernooij (jelmer) :
review: Approve (code*)
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)
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
1=== added directory 'lib/canonical/launchpad/apidoc'
2=== removed directory 'lib/canonical/launchpad/apidoc'
3=== modified file 'lib/canonical/launchpad/mailnotification.py'
4--- lib/canonical/launchpad/mailnotification.py 2010-02-11 14:04:16 +0000
5+++ lib/canonical/launchpad/mailnotification.py 2010-05-25 22:32:33 +0000
6@@ -344,6 +344,9 @@
7 def update_security_contact_subscriptions(modified_bugtask, event):
8 """Subscribe the new security contact when a bugtask's product changes.
9
10+ Only subscribes the new security contact if the bug was marked a
11+ security issue originally.
12+
13 No change is made for private bugs.
14 """
15 if event.object.bug.private:
16@@ -358,7 +361,7 @@
17 if (bugtask_before_modification.product !=
18 bugtask_after_modification.product):
19 new_product = bugtask_after_modification.product
20- if new_product.security_contact:
21+ if bugtask_before_modification.bug.security_related and new_product.security_contact:
22 bugtask_after_modification.bug.subscribe(
23 new_product.security_contact, IPerson(event.user))
24
25
26=== modified file 'lib/lp/bugs/doc/security-teams.txt'
27--- lib/lp/bugs/doc/security-teams.txt 2010-05-18 21:24:27 +0000
28+++ lib/lp/bugs/doc/security-teams.txt 2010-05-25 22:32:33 +0000
29@@ -207,8 +207,9 @@
30 [u'name12', u'name16']
31
32 Finally, reassigning a public bug to a different product will subscribe
33-the new security contact, if present. Let's set stub to the security
34-contact for thunderbird to see how this works.
35+the new security contact, if present and if the original bug was marked
36+as a security issue. Let's set stub to the security contact for
37+thunderbird to see how this works.
38
39 >>> thunderbird = productset.get(8)
40 >>> print thunderbird.name
41@@ -241,17 +242,40 @@
42 >>> subscriber_names(bug)
43 [u'name12', u'name16']
44
45-But publish the same event again, when the bug is marked public, and
46-stub will be added to the subscriber list:
47+Also verify the same event again, when marked public does cause
48+stub to get subscribed:
49
50 >>> bug.setPrivate(False, getUtility(ILaunchBag).user)
51 True
52
53+ >>> bug.security_related
54+ True
55+
56 >>> notify(bug_product_changed)
57
58 >>> subscriber_names(bug)
59 [u'name12', u'name16', u'stub']
60
61+But if it is not a security issue originally, stub does not get
62+subscribed when moving it to the new project.
63+
64+ >>> bug.unsubscribe(stub, stub)
65+
66+ >>> subscriber_names(bug)
67+ [u'name12', u'name16']
68+
69+ >>> bug.setSecurityRelated(False)
70+ True
71+
72+ >>> bug.security_related
73+ False
74+
75+ >>> notify(bug_product_changed)
76+
77+ >>> subscriber_names(bug)
78+ [u'name12', u'name16']
79+
80+
81 When a bug becomes security-related, the security contacts for the pillars it
82 affects are subscribed to it.
83

Subscribers

People subscribed via source and target branches

to status/vote changes: