Merge lp:~adeuring/launchpad/bug-834303 into lp:launchpad

Proposed by Abel Deuring
Status: Merged
Approved by: j.c.sackett
Approved revision: no longer in the source branch.
Merged at revision: 14731
Proposed branch: lp:~adeuring/launchpad/bug-834303
Merge into: lp:launchpad
Diff against target: 217 lines (+88/-13)
6 files modified
lib/lp/soyuz/browser/archivesubscription.py (+18/-7)
lib/lp/soyuz/doc/archivesubscriber.txt (+5/-2)
lib/lp/soyuz/model/archivesubscriber.py (+9/-3)
lib/lp/soyuz/stories/ppa/xx-private-ppa-subscription-stories.txt (+2/-0)
lib/lp/soyuz/templates/archive-subscribers.pt (+5/-1)
lib/lp/soyuz/tests/test_archivesubscriptionview.py (+49/-0)
To merge this branch: bzr merge lp:~adeuring/launchpad/bug-834303
Reviewer Review Type Date Requested Status
j.c.sackett (community) Approve
Richard Harding (community) code* Approve
Review via email: mp+90304@code.launchpad.net

Commit message

[r=jcsackett,rharding][bug=834303] use batching in the +subscription view of PPPAs; sort subscribers by Person.name

Description of the change

This branch fixes bug 834303: Archive:+subscriptions times out with
many subscribers.

The cause of the timeout is Storm needing really much time to load
15000 or 20000 ArchiveSubscriber records.

As lifeless noted in a bug comment, the fix is obvious: To show the
data in batches.

This leads to another problem, noted by Anthony Lenton in another comment
on the bug: The data was sorted by the creation time of a subscription,
while an important use case is to change the subscription status of
a subscriber. Looking them up by sifting through more than 100 pages
to look up a subscriber by name would be really painful.

The fix is obvious: Sort by name. Anthony suggested to use the display
name; after an IRC chat with him I switched instead to sorting by LP
user name. This allows us to use StormRangeFactory, which uses the
sort value of the first/last displayed result row as the "batch value"
in URLs. This allows to manually edit a URL to quickly look up the
subscription status of a given user. Not as nice as a decent search
form, but at least a convenient work-around, as long as we don't have
a search form and the related result page...

In theory, we could use Person.displayname for batching with
StormRangeFactory too, but the related URLs would look even more
insane than the URLs for sorting by LP user name: We would need
Person.id as an additional sort parameter, and the sort value
-- which appears in the URL -- may contain spaces, and when you
change a URL manually, you should be aware of the meaning of the
second sort parameter, Person.id. To compare the URLs:

sort by Person.name:

https://launchpad.dev/~cprov/+archive/p3a/+subscriptions?batch=5&memo=[%22stevea%22]&start=5

sort by Person.displayname, Person.id

https://launchpad.dev/~cprov/+archive/p3a/+subscriptions?batch=5&memo=[%22Steve+Alexander%22,1234]&start=5

tests:

./bin/test soyuz -vvt test_archivesubscriptionview
./bin/test soyuz -vvt archivesubscriber.txt

no lint

To post a comment you must log in.
Revision history for this message
Richard Harding (rharding) wrote :

Looks good, my only comment is that I don't think you need the set() in line 30 since I don't think the subscriber ids should have dupes in there.

review: Approve (code*)
Revision history for this message
j.c.sackett (jcsackett) wrote :

Abel--

I concur with Rick's assessment, but set or list is a good cast, and there isn't really any reason to change it from set at this point.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.