launchpad-reviewers team mailing list archive

[Abel Deuring <abel.deuring@xxxxxxxxxxxxx>]

Review: Needs Information code

Overall, this looks good. But I don't understand how the change in lines 8 to 32 of the diff is related to the bug. As I understand it, this "just" changes the behaviour of the job: people keep their subscription if they can access the bug. This might make sense or ot might be bad -- what is the exact purpose of this job?

Also, filtering in a list comprehension might be quite slow. OK, this is a job, so it does not need to be blindingly fast, and I know that copying for example the check "bug.private == True" from userCanView() into an SQL filter would duplicate code, but nevertheless: Do we expect that len(self.bug_ids) in the first hunk of this diff could be as larga as, let's say, 10000?
-- 
https://code.launchpad.net/~wallyworld/launchpad/RemoveGranteeSubscriptionsJob-admins-1009283/+merge/109279
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.