Merge lp:~lifeless/launchpad/milestones into lp:launchpad

Deryck, Curtis, would appreciate sanity checks from you :)

Hi Rob,

Couple of nitpicks, but otherwise okay. Marking it needs fixing because
of the over-long test (see below) but if we can't split that up I'm
happy for it to land as-is.

> === modified file 'lib/lp/bugs/tests/test_bugtask.py'
> --- lib/lp/bugs/tests/test_bugtask.py 2010-07-14 14:11:15 +0000
> +++ lib/lp/bugs/tests/test_bugtask.py 2010-08-17 09:41:19 +0000
> @@ -792,6 +796,28 @@
> result = target.searchTasks(None, modified_since=date)
> self.assertEqual([task2], list(result))
>
> + def test_private_bug_view_permissions_cached(self):
> + """private bugs from a search know the user can see the bugs."""

Captialise at the start of the sentence ;).

> + target = self.makeBugTarget()
> + person = self.login()
> + self.factory.makeBug(product=target, private=True, owner=person)
> + self.factory.makeBug(product=target, private=True, owner=person)
> + # Search style and parameters taken from the milestone index view where
> + # the issue was discovered.
> + login_person(person)
> + tasks =target.searchTasks(BugTaskSearchParams(person, omit_dupes=True,

Needs a space after the =.

> + orderby=['status', '-importance', 'id']))

> === modified file 'lib/lp/registry/browser/tests/test_milestone.py'
> --- lib/lp/registry/browser/tests/test_milestone.py 2010-08-16 18:39:11 +0000
> +++ lib/lp/registry/browser/tests/test_milestone.py 2010-08-17 09:41:19 +0000
> @@ -105,5 +116,61 @@
> self.assertEqual(0, product.development_focus.all_bugtasks.count())
>
>
> -def test_suite():
> - return unittest.TestLoader().loadTestsFromName(__name__)
> +class TestMilestoneIndex(TestCaseWithFactory):
> +
> + layer = DatabaseFunctionalLayer
> +
> + def test_more_private_bugs_query_count_is_constant(self):

The test should have a comment at the start of it to save me having to
parse the method name.

> + product = self.factory.makeProduct()
> + login_person(product.owner)
> + milestone = self.factory.makeMilestone(
> + productseries=product.development_focus)
> + bug1 = self.factory.makeBug(product=product, private=True,
> + owner=product.owner)
> + bug1.bugtasks[0].transitionToMilestone(milestone, product.owner)
> + # We look at the page as someone who is a member of a team and the team
> + # is subscribed to the bugs, so that we don't get trivial shortcuts
> + # avoiding queries : test the worst case.
> + subscribed_team = self.factory.makeTeam()
> + viewer = self.factory.makePerson(password="test")
> + with person_logged_in(subscribed_team.teamowner):
> + subscribed_team.addMember(viewer, subscribed_team.teamowner)
> + bug1.subscribe(subscribed_team, product.owner)
> + bug1_url = canonical_url(bug1)
> + milestone_url = canonical_url(milestone)
> + browser = self.getUserBrowser(user=viewer)
> + # Seed the cookie cache and any other cross-request state we may gain
> + # in future. See canonical.launchpad.webapp.serssion: _get_secret.
> + browser.open(milestone_url)
> + collector = QueryC...

I'll do the tweaks; I don't see a good way (for now) of shrinking the
test; two separate tests would require a magic constant to be shared
between them rather than being derived on the fly; that would be more
fragile.

-Rob

On 17 August 2010 11:48, Robert Collins <email address hidden> wrote:
> I'll do the tweaks; I don't see a good way (for now) of shrinking the
> test; two separate tests would require a magic constant to be shared
> between them rather than being derived on the fly; that would be more
> fragile.
>

Right, that's pretty much what I figured.

MilestoneView._bugtasks uses precache_permission_for_objects() on the bugtasks and the conjoined bugtasks. Does this change allow us to remove precache_permission_for_objects()? Will your test fail if I remove precache_permission_for_objects() from the view?

MilestoneView._bugtasks calls getConjoinedMaster() with a prepared list (1 query) of expected ids. I think we need to keep behaviour; this branch just lowers the access cost of getConjoinedMaster()

On Wed, Aug 18, 2010 at 2:18 AM, Curtis Hovey
<email address hidden> wrote:
> Review: Needs Information
> MilestoneView._bugtasks uses precache_permission_for_objects() on the bugtasks and the conjoined bugtasks. Does this change allow us to remove precache_permission_for_objects()? Will your test fail if I remove precache_permission_for_objects() from the view?

For objects queries via that search path, the precaching be removed. I
do not know if getConjoinedMaster accesses the related objects via
that code path - and I suspect in fact that it does not.

> MilestoneView._bugtasks calls getConjoinedMaster() with a prepared list (1 query) of expected ids. I think we need to keep behaviour; this branch just lowers the access cost of getConjoinedMaster()

We can refactor the conjoined master stuff too, but its a separate issue IMO.

-Rob

I do not understand the first part of your answer. I see this in MilestoneView._bugtasks:
        ...
        for task in tasks:
            if task.getConjoinedMaster(bugs_and_tasks[task.bug]) is None:
                non_conjoined_slaves.append(task)
        # Checking bug permissions is expensive. We know from the query that
        # the user has at least launchpad.View on the bugtasks and their bugs.
        precache_permission_for_objects(
            self.request, 'launchpad.View', non_conjoined_slaves)
        precache_permission_for_objects(
            self.request, 'launchpad.View',
            [task.bug for task in non_conjoined_slaves])
        return non_conjoined_slaves

Since you modified the permission rules on bugtask, I wonder if the first precache_permission_for_objects() is not needed. I wonder if permission checking is fast enough.

We stopped adding enahncements to the milestone page when the preformance problems started, and we cannot entertain allowing users to edit the bugtasks unless we know the user has launchpad.Edit. I wonder if this permission change allows us to consider enhancements.

I certainly think you can make enchancements, and yes we should be
able to remove the precache permissions for nonconjoinedslaves.

I'd like to do that separately from this branch, as a mitigation : I
know what I've researched works, I'm not sure that there aren't other
gremlins in there.

Yes. I agree that the removal of precache_permissions is a separate branch. Thanks for answering my questions.

Looks good to me, Robert. Thanks for doing this work!

Cheers,
deryck

I hate to ask for a rereview, but I'll ask anyhow. This branch ran into incompatibilities with SQLObjectResultSet so it now converts buildQuery and thing things on it to be less SQLObject and more Storm. This involved finding addressing a bunch of differences; it still has literal SQL and so on, but at least at the surface its now sensible. I'm running it through EC2 test now.

Hi Robert.

Thanks for this refactoring.

Can you place a comment before ``if False and True:`` in BugtaskSet.search() to explain the dagnostic output? I had to think about the old and new guard.

I think you can remove the naked taskset in BugTaskSet.searchBugIds(). I do not think it is being used.

Thank you curtis; I've deleted the naked taskset, and deleted the
print statements I had accidentally left in which had that confusing
if statement.

This is the only failing test ec2 told me about

FAIL: productseries-views_txt
----------------------------------------------------------------------
Text attachment: traceback
------------
Traceback (most recent call last):
  File "/usr/lib/python2.6/unittest.py", line 279, in run
    testMethod()
  File "/usr/lib/python2.6/doctest.py", line 2152, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for productseries-views.txt
  File "lib/lp/registry/browser/tests/productseries-views.txt", line 0

----------------------------------------------------------------------
File "lib/lp/registry/browser/tests/productseries-views.txt", line 80,
in productseries-views.txt
Failed example:
    script = find_tag_by_id(view.render(), 'milestone-script')
Differences (ndiff with -expected +actual):
    + WARNING:root:Memcache set failed for
pt:testrunner:lp/bugs/templates/bugtarget-portlet-latestbugs.pt,10721:16:1,0:1,http_//127.0.0.1?6RKVP3n9fovtv8CxVi8ydF
----------------------------------------------------------------------
File "lib/lp/registry/browser/tests/productseries-views.txt", line
102, in productseries-views.txt
Failed example:
    content = view.render()
Differences (ndiff with -expected +actual):
    + WARNING:root:Memcache set failed for
pt:testrunner:lp/bugs/templates/bugtarget-portlet-latestbugs.pt,10721:243656:1,0:1,http_//127.0.0.1?4Ophjjtmt5aXGqxEwNgYvP
----------------------------------------------------------------------
File "lib/lp/registry/browser/tests/productseries-views.txt", line
117, in productseries-views.txt
Failed example:
    table = find_tag_by_id(view.render(), 'series-simple')
Differences (ndiff with -expected +actual):
    + WARNING:root:Memcache set failed for
pt:testrunner:lp/bugs/templates/bugtarget-portlet-latestbugs.pt,10721:243652:1,0:1,http_//127.0.0.1?IXiDEgHZPmgHvZKrJmNvh

If you have any ideas about it, I'd love to know.

productseries-views.txt is now rendering a template that uses the memcache tales directive. We need to run it on a layer that has the memcached server up.

Add this to special_test_layer in lp/registry/browser/tests/test_views.py
    'productseries-views.txt': LaunchpadFunctionalLayer,