launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

Review: Approve code

8	- # XXX 2010-10-05 gmb bug=655597:
9	- # This line of code keeps the view's query count down,
10	- # possibly using witchcraft. It should be rewritten to be
11	- # useful or removed in favour of making other queries more
12	- # efficient. The witchcraft is because the subscribers are accessed
13	- # in the initial page load, so the data is actually used.
14	- if self.user is not None:
15	- list(bug.getSubscribersForPerson(self.user))

I'd suggest restoring this for now. We know the rest of the changes are probably safe, but about this we're just guessing. It's not detrimental to leave, since we deferred the plan to redesign the method.

153	tzinfo=pytz.UTC)
154	+
155	+def generate_subscription_with(bug, person):

Looks like a missing newline.

155	+def generate_subscription_with(bug, person):
156	+ return [
157	+ With('all_bugsubscriptions', Select(
158	+ (BugSubscription.id, BugSubscription.person_id),
159	+ tables=[BugSubscription, Join(
160	+ Bug, Bug.id == BugSubscription.bug_id)],
161	+ where=Or(Bug.id == bug.id, Bug.duplicateofID == bug.id))),
162	+ With('bugsubscriptions', Select(
163	+ SQL('all_bugsubscriptions.id'),
164	+ tables=[
165	+ SQL('all_bugsubscriptions'), Join(
166	+ TeamParticipation, TeamParticipation.teamID == SQL(
167	+ 'all_bugsubscriptions.person'))],
168	+ where=[TeamParticipation.personID == person.id]))]

Some of the wrapping here is a bit off.

274	+ pillar = self._getTaskPillar(task)

Can you just use task.pillar instead?

292	+ is_muted = Store.of(person).find(
293	+ BugMute, BugMute.bug == bug, BugMute.person == person).one()
294	+ return is_muted is not None

This could even be 'return not Store.of(person).find(BugMute, bug=bug, person=person).is_empty()'
-- 
https://code.launchpad.net/~stevenk/launchpad/no-more-o-n-queries-bug-dupes/+merge/129355
Your team Launchpad code reviewers is subscribed to branch lp:launchpad.