Merge lp:~wallyworld/launchpad/disabled-project-bugs-shown into lp:launchpad

Looks good. But could you use bugtask.pillar.active instead of the disjunctions? Also, I don't understand why you set both series' statuses to EXPERIMENTAL.

So there are more issues here.

Firstly, code & structure: Much better to write up these changes as unit tests not doctests, and there isn't any need really to add to the doctest.

More importantly, doing a filter in the list comprehension is inefficient and leaves waste in the system: note that the getBugTaskAndNominationViews gets bugtasks from self.bugtasks - and that that set of bugtasks is eager loaded and used for other things such as target calculations and milestone lookups. The filtering should, as always, be done on data retrieval or at minimum at the first poin where we might eager load.

Testing: Corner cases; what if all the tasks on the bug are hidden? That may be 'cannot be traversed to'.

Lastly: Elsewhere in the system disabled things are only hidden for non-admin, non-registry-experts; that seems appropriate to apply here and would resolve the testing case.

Lastly lastly: I suspect that the bugtask search for a given bug would resolve all of these issues at once (vs using a bugtasks attribute on bug).

The reason for the series status -> experimental is that the "active" on
product|distro series is a read only property based on the value of the
status.

I'll look at changing to using bugtask.pillar.active

On 12/04/11 11:52, William Grant wrote:
> Review: Approve code*
> Looks good. But could you use bugtask.pillar.active instead of the disjunctions? Also, I don't understand why you set both series' statuses to EXPERIMENTAL.

I've changed the implementation as per the suggestion to use a BugTaskSet search to load the bug tasks. I've also moved the tests from doc tests to unit tests as requested.

This looks pretty good. Normal users are unable to see a bug with all its tasks deactivated, but that's because of a long-standing bug in Bug.default_bugtask (it always uses the earliest task, regardless of whether it's visible).

I note that BugTaskSearchParams always excludes inactive projects, regardless of privilege. This means that registry admins will see an empty task table, which probably isn't too bad.

32 + if len(self.bugtasks):
33 + self.milestones = list(
34 + bugtask_set.getBugTaskTargetMilestones(self.bugtasks))

Does it break if self.bugtasks is empty? If not, why the guard?

72 - self.assertThat(recorder, HasQueryCount(LessThan(67)))
73 + self.assertThat(recorder, HasQueryCount(LessThan(69)))

I only see one new query. What's the second?

103 + foo_bug = self.factory.makeBug(product=product_foo)
104 + bugtask_set = getUtility(IBugTaskSet)
105 + bugtask_set.createTask(
106 + bug=foo_bug, owner=foo_bug.owner, product=product_bar)

Consider Bug.addTask or LOF.makeBugTask for the second task.

108 + removeSecurityProxy(product_bar).active = False
132 + removeSecurityProxy(product_foo).active = False

Consider letting LOF.makeProduct take an "active" parameter.

> 32 + if len(self.bugtasks):
> 33 + self.milestones = list(
> 34 + bugtask_set.getBugTaskTargetMilestones(self.bugtasks))
>
> Does it break if self.bugtasks is empty? If not, why the guard?
>

Yes. getBugTaskTargetMilestones() breaks.

> 72 - self.assertThat(recorder, HasQueryCount(LessThan(67)))
> 73 + self.assertThat(recorder, HasQueryCount(LessThan(69)))
>
> I only see one new query. What's the second?
>

From what I saw, the section of code with the extra query is actually
called twice when the page loads. I've not yet tracked down why.

> 103 + foo_bug = self.factory.makeBug(product=product_foo)
> 104 + bugtask_set = getUtility(IBugTaskSet)
> 105 + bugtask_set.createTask(
> 106 + bug=foo_bug, owner=foo_bug.owner, product=product_bar)
>
> Consider Bug.addTask or LOF.makeBugTask for the second task.
>

Ok. I copied from what was in the doctest. Bad move perhaps :-)

> 108 + removeSecurityProxy(product_bar).active = False
> 132 + removeSecurityProxy(product_foo).active = False
>
> Consider letting LOF.makeProduct take an "active" parameter.

Considered it but there's already a lot of parameters there and the
number of times we need to change active to false didn't seem to justify
the additional clutter.

self.milestones = list()

is unidiomatic -
self.milestones = []
is the usual way this is written.
It might be clearer to do
if self.bugtasks:
 assign here
else:
 self.milestones = []

Oh, and you might want to file a bug about registry + admins having the search results unconditionally filtered. Not sure what we should do there, but it will lead to time wasting at some future date.