Code review comment for lp:~edwin-grubbs/launchpad/bug-664828-teammembership-delete-timeout

Hi Maris,

Thanks for the review. An incremental diff is included at the bottom.

> Hi Edwin,
>
> The code in this change looks good to me, and the SQL is nicely documented.
> My first question is, where does the magic number '12' come from as the query
> count in test_deactivate_member_query_count? I know the test is for
> performance testing, but that one test and one number don't detail how the
> query is expected to perform using different data.

I have added a comment explaining the queries. I was able to reduce the number of queries by one by moving create_view() outside of the with-statement.

> On a related note, is there already a test for the case you mentioned to
> Robert? "The only way to tell if the super teams still have participants via
> another path is by recursing the membership tree."

lp/registry/tests/test_teammembership.py already has the TestTeamParticipationMesh, TestTeamParticipationTable, and TestTeamParticipationHierarchy tests that verify that the TeamParticipation table is updated correctly.

Incremental diff:

=== modified file 'lib/lp/registry/browser/tests/test_teammembership.py'
--- lib/lp/registry/browser/tests/test_teammembership.py 2011-01-11 01:40:09 +0000
+++ lib/lp/registry/browser/tests/test_teammembership.py 2011-01-11 20:45:04 +0000
@@ -32,6 +32,25 @@
         self.member = self.factory.makeTeam()

     def test_deactivate_member_query_count(self):
+ # Only these queries should be run, no matter what the
+ # membership tree looks like, although the number of queries
+ # could change slightly if a different user is logged in.
+ # 1. Check whether the user is the team owner.
+ # 2. Deactivate the membership in the TeamMembership table.
+ # 3. Delete from TeamParticipation table.
+ # (Queries #4, #5, #6, #7, and #10 are run because the storm
+ # objects have been invalidated.)
+ # 4. Get the TeamMembership entry.
+ # 5. Verify that the member exists in the db, but don't load
+ # the refresh the rest of its data, since we just need the id.
+ # 6. Verify that the user exists in the db.
+ # 7. Verify that the team exists in the db.
+ # 8. Insert into Job table.
+ # 9. Insert into PersonTransferJob table to schedule sending
+ # email. (This requires the data from queries #5, #6, and
+ # #7.)
+ # 10.Query the rest of the team data for the invalidated
+ # object in order to generate the canonical url.
         self.team.addMember(
             self.member, self.team.teamowner, force_team_add=True)
         form = {
@@ -41,9 +60,10 @@
             }
         membership = self.membership_set.getByPersonAndTeam(
             self.member, self.team)
+ view = create_view(
+ membership, "+index", method='POST', form=form)
         with StormStatementRecorder() as recorder:
- view = create_view(membership, "+index", method='POST', form=form)
             view.processForm()
         self.assertEqual('', view.errormessage)
         self.assertEqual(TeamMembershipStatus.DEACTIVATED, membership.status)
- self.assertThat(recorder, HasQueryCount(LessThan(12)))
+ self.assertThat(recorder, HasQueryCount(LessThan(11)))