Merge lp://staging/~bryce/launchpad/lp-253509 into lp://staging/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://staging/~bryce/launchpad/lp-253509
Merge into: lp://staging/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://staging/~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.staging.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
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to status/vote changes: