Launchpad itself

Merge lp://qastaging/~brian-murray/launchpad/595124 into lp://qastaging/launchpad

595124
Merge into devel

Proposed by Brian Murray on 2010-06-25

Status:

Merged

Merged at revision:

11108

Proposed branch:

lp://qastaging/~brian-murray/launchpad/595124

Merge into:

lp://qastaging/launchpad

Diff against target:

443 lines (+164/-41)

12 files modified

lib/lp/bugs/browser/bugtask.py (+7/-4)
lib/lp/bugs/configure.zcml (+2/-1)
lib/lp/bugs/doc/bug.txt (+6/-6)
lib/lp/bugs/doc/bugtask-expiration.txt (+30/-7)
lib/lp/bugs/interfaces/bug.py (+18/-2)
lib/lp/bugs/model/bug.py (+36/-9)
lib/lp/bugs/model/bugtask.py (+1/-1)
lib/lp/bugs/stories/bugs/xx-incomplete-bugs.txt (+16/-6)
lib/lp/bugs/stories/webservice/xx-bug.txt (+42/-0)
lib/lp/bugs/templates/bug-listing-expirable.pt (+3/-3)
lib/lp/bugs/templates/bugtask-index.pt (+1/-1)
lib/lp/bugs/tests/bug.py (+2/-1)

To merge this branch:

bzr merge lp://qastaging/~brian-murray/launchpad/595124

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Leonard Richardson (community)		2010-06-25	Approve on 2010-07-01
Review via email: mp+28543@code.qastaging.launchpad.net

Commit message

unexport IBug.can_expire which is confusing instead create and export IBug.isExpirable() which can use a custom number of days.

Description of the change

A bug report's can_expire attribute has lead to some confusion as it shows if a bug may expire not whether or not it should expire. To remove any confusion I've modified can_expire to call findExpirableTasks with days_old equal to config.malone.days_before_expiration, (60 by the way), so it will reflect whether or not a bug should expire.

This also changes the results displayed at +expirable-bugs so that only bugs that are expirable will show up not ones that match the criteria and have a days_old of less than 60. Deryck and I discussed this and are in agreement that this makes the most sense. However, individual bug pages will still display "will be marked for expiration in XYZ days" as isExpirable is called with days_old=0.

I've also created IBug.isExpirable() which accepts a quantity of days as an argument we can call findExpirableTasks with a different quantity of days_old - for example 0. This is also exported in the API as it may be useful for a project that wanted a different expiration period than the default or if a package maintainer wanted to be particularly aggressive in expiring bugs.

Tests include:

bin/test -cvvt xx-bug.txt -t bugtask-expiration.txt -t xx-incomplete-bugs.txt

Revision history for this message

Leonard Richardson (leonardr) wrote on 2010-06-29:

This all looks good except for one minor point: you are changing the behavior of the web service's can_expire attribute in a backwards-incompatible way. Bugs that used to have can_expire=False will now have can_expire=True without the bugs themselves changing.

I personally don't think the change is big enough to warrant backwards-compatibility, but here's how to do it if Francis disagrees.

Declare two attributes in IBug, can_expire (which uses your current implementation) and can_expire_beta:

can_expire = exported(Bool(...), ('devel', dict(exported=True),
'beta', dict(exported=False)))

can_expire_beta = exported(Bool(...), exported_as='can_expire',
('devel', dict(exported=False))

This will make can_expire_beta show up as 'can_expire' in 'beta' and '1.0', but will replace it with can_expire in 'devel'.

I may not have the syntax right; refer to lazr.restful src/lazr/restful/examples/multiversion/resource.py and src/lazr/restful/docs/multiversion.txt for help.

review: Needs Fixing

Revision history for this message

Francis J. Lacoste (flacoste) wrote on 2010-06-29:

I don't mind the backward incompatibility either. I think Brian is one of the heavy user of that API and if that's fine by him, I wouldn't second guess him here.

I have another concern which isn't introduce by this branch, which is that this means that sending a bugs representation makes an additional query to get the value of the can_expire attribute. I think this is very bad from a performance point of view. Especially when navigating through bug searches.

A better API would be to turn this into a collection exposed on the context and return the expirable bug tasks.

And just remove can_expire from the model. Maybe that's what we should do now? Mark it as removed, export only isExpirable() which can be used to retrieve the information if really needed. Exporting the set of expirable bugs might be done at another time (not sure if the IBugTarget has findExpirableBugs in it already).

Revision history for this message

Brian Murray (brian-murray) wrote on 2010-06-30:

This made sense to me so I've removed can_expire from being exported in the development version, and kept it in previous versions, of the API and will report a bug about exporting findExirableBugs.

Revision history for this message

Leonard Richardson (leonardr) wrote on 2010-07-01:

This now looks good. I have one advisory comment: only make this change if it makes sense.

isExpirable() uses the janitor to do a search because it doesn't have access to the user making the request. If you make that method take a user argument, you can have the web service fill in the current user automatically:

from lazr.restful.declarations import call_with, REQUEST_USER
@call_with(who=REQUEST_USER)
def isExpirable(who, days_old=None):
...

review: Approve