launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Hi Abel

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

The job is run when the +sharing page or +sharingdetails page is used to revoke access for a user - either revoke access to a specific bug or branch or to all artifacts of a given information type eg embargoed security. When access is revoked, the job run to remove subscriptions for the now invisible bugs/branches or artifacts of a given info type. 
 
The job runs as a result of an action on a grantee on the sharing views as mentioned above. The grantee may be an admin who can see the bug regardless of any grant or may have been given access after the job was queued. So lines 8-32 simply check that the specified grantee really cannot see the bug or branch etc before removing subscriptions.

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

The size of the list of bugs/branches is currently 0 or 1 when invoked from the ui, and will be small when run from the api, maybe of the order of 10s at the very most, since the new sharing paradigm means that large numbers of grants to individual bugs or branches is not required.

-- 
https://code.launchpad.net/~wallyworld/launchpad/RemoveGranteeSubscriptionsJob-admins-1009283/+merge/109279
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.