Code review comment for lp:~adeuring/launchpad/bug-333531-bug-status-expired

Revision history for this message
Gavin Panella (allenap) wrote :

Looks good :)

The conversation we had on IRC:

<allenap> adeuring: Is there going to be another branch (or two) to add a date_expired field to IBugTask?
<adeuring> allenap: argh... i forgot completely about that...
<adeuring> yes, there _should_ be a branch...
<allenap> adeuring: I think the bug number in the branch name is wrong.
<adeuring> allenap: right...
<allenap> adeuring: Line 75, could you login(product_owner) to avoid the removeSecurityProxy() call?
<allenap> adeuring: Or even login_person(product_owner)
<adeuring> allenap: in theory, yes. Problem that things become then more complicated: the owner can't assign another person as the supervisor, so I need to create a team, let another person join that team and finally run the test with that other user
<adeuring> allenap: I was too lazy to do all that ;)
<allenap> adeuring: Argh. Leave it then :)
<adeuring> allenap: OK ;)
<allenap> adeuring: Perhaps the removeSecurityProxy() hack could be put in LaunchpadObjectFactory.makeProduct(..., bug_supervisor=None).
<adeuring> allenap: right, good idea!
<allenap> adeuring: Line 124, the bug supervisor or the product owner should be able to call transitionToStatus(), so consider using that rather than removing the proxy.
<adeuring> allenap: right
<allenap> adeuring: 136s/Epired/Expired/
<adeuring> allenap: fixed
<allenap> adeuring: One last thing. There's probably some doc test out there that does a similar thing to TestBugTaskEditViewStatusField. If you know where, consider removing it.
<adeuring> allenap: I did not find anything like that...
<adeuring> that's why I added it ;)

review: Approve

« Back to merge proposal