Launchpad itself

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

lp-253509
Merge into 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://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

High

Fix Released

Link a bug report

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.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.

Revision history for this message

Jelmer Vernooij (jelmer) wrote on 2010-05-20:

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 on 2010-05-20:

> 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 on 2010-05-21:

Thanks for the warm welcome.

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

Revision history for this message

Jelmer Vernooij (jelmer) on 2010-05-25:

review: Approve (code*)

Revision history for this message

Michael Nelson (michael.nelson) wrote on 2010-05-26:

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 :)