Launchpad itself

Merge lp://staging/~wallyworld/launchpad/person-mergequeue-listview into lp://staging/launchpad/db-devel

  1. person-mergequeue-listview
  2. Merge into db-devel
Proposed by Ian Booth
Status: Superseded
Proposed branch: lp://staging/~wallyworld/launchpad/person-mergequeue-listview
Merge into: lp://staging/launchpad/db-devel
Prerequisite: lp://staging/~rockstar/launchpad/merge-queue-index
Diff against target: 1125 lines (+922/-8)
14 files modified
lib/lp/code/browser/branchlisting.py (+4/-2)
lib/lp/code/browser/branchmergequeuelisting.py (+105/-0)
lib/lp/code/browser/configure.zcml (+18/-0)
lib/lp/code/browser/tests/test_branchmergequeuelisting.py (+227/-0)
lib/lp/code/configure.zcml (+11/-0)
lib/lp/code/interfaces/branchmergequeue.py (+14/-0)
lib/lp/code/interfaces/branchmergequeuecollection.py (+64/-0)
lib/lp/code/model/branchmergequeue.py (+4/-2)
lib/lp/code/model/branchmergequeuecollection.py (+174/-0)
lib/lp/code/model/tests/test_branchmergequeuecollection.py (+201/-0)
lib/lp/code/templates/branchmergequeue-listing.pt (+68/-0)
lib/lp/code/templates/branchmergequeue-macros.pt (+20/-0)
lib/lp/code/templates/person-codesummary.pt (+8/-1)
lib/lp/testing/factory.py (+4/-3)
To merge this branch: bzr merge lp://staging/~wallyworld/launchpad/person-mergequeue-listview
Reviewer Review Type Date Requested Status
Paul Hummer (community) ui Approve
Tim Penhey (community) mentor Approve
Steve Kowalik (community) code* Approve
Review via email: mp+39933@code.staging.launchpad.net

This proposal has been superseded by a proposal from 2010-11-18.

Commit message

Add person merge queue listing functionality. Work in progress merge queue development.

Description of the change

This branch delivers functionality for the merge queue development project. It adds a person merge queue list view and associated model functionality.

= Implementation =

The usual suspects were developed:
- view implementation class
- page template
- zcml changes

To supply data for the view, an IBranchMergeQueueCollection implementation was developed. The implementation is similar to IBranchCollection. A key difference is that the main collection getter, in this case getMergeQueues(), returns a list() rather than a ResultSet(). This is due to how the filtering is done in the VisibleBranchMergeQueueCollection subclass - it's done by iterating over the queue branches in memory rather than as a Storm SQL query. The required query will be non-trivial and can be implemented during another coding iteration if required. The IBranchMergeQueueCollection implementation doesn't have any search() APIs yet - this can be added later if required.

The merge queue list view shows:
- queue name
- queue size
- queue branches

NB the queue size is hard coded pending the required API being developed for IBranchMergeQueue

The menu to access the merge queue list is including alongside the other person branch menu links.

A feature flag is used to hide this functionality as the whole development effort is still WIP. This branch is sufficiently complete in what it delivers and needs to allow collaboration between separate development efforts.

= Screenshot =

Here's a screenshot of the list page:

http://people.canonical.com/~ianb/mergequeuelist.png

= Tests =

bin/test -vvt test_branchmergequeuecollection
bin/test -vvt test_branchmergequeuelisting

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  setup.py
  versions.cfg
  lib/lp/code/configure.zcml
  lib/lp/code/browser/branch.py
  lib/lp/code/browser/branchlisting.py
  lib/lp/code/browser/branchmergequeue.py
  lib/lp/code/browser/branchmergequeuelisting.py
  lib/lp/code/browser/configure.zcml
  lib/lp/code/browser/tests/test_branchmergequeue.py
  lib/lp/code/browser/tests/test_branchmergequeuelisting.py
  lib/lp/code/interfaces/branchmergequeue.py
  lib/lp/code/interfaces/branchmergequeuecollection.py
  lib/lp/code/model/branchmergequeue.py
  lib/lp/code/model/branchmergequeuecollection.py
  lib/lp/code/model/tests/test_branchmergequeuecollection.py
  lib/lp/code/templates/branch-pending-merges.pt
  lib/lp/code/templates/branchmergequeue-index.pt
  lib/lp/code/templates/branchmergequeue-listing.pt
  lib/lp/code/templates/branchmergequeue-macros.pt
  lib/lp/code/templates/person-codesummary.pt
  lib/lp/testing/__init__.py
  lib/lp/testing/factory.py

./setup.py
     131: E202 whitespace before ']'
./lib/lp/testing/__init__.py
     129: 'anonymous_logged_in' imported but unused
     129: 'with_anonymous_login' imported but unused
     129: 'is_logged_in' imported but unused
     148: 'launchpadlib_for' imported but unused
     148: 'launchpadlib_credentials_for' imported but unused
     129: 'person_logged_in' imported but unused
     148: 'oauth_access_token_for' imported but unused
     129: 'login_celebrity' imported but unused
     129: 'with_celebrity_logged_in' imported but unused
     147: 'test_tales' imported but unused
     129: 'celebrity_logged_in' imported but unused
     129: 'run_with_login' imported but unused
     129: 'with_person_logged_in' imported but unused
     129: 'login_team' imported but unused
     129: 'login_person' imported but unused
     129: 'login_as' imported but unused
     429: E301 expected 1 blank line, found 0
     861: E301 expected 1 blank line, found 0
     887: E302 expected 2 blank lines, found 1
     963: E302 expected 2 blank lines, found 1

Process finished with exit code 0

To post a comment you must log in.
Revision history for this message
Ian Booth (wallyworld) wrote :

Adding back comments from old incorrect merge proposal:

SteveK wrote:

Hi Ian,

Firstly, thanks for this awesome work! I've been looking forward to Merge Queues for a while.

I have some comments below:

* 1,500 lines? Why!? I'd suggest in future you look at splitting up work into two separate branches for plumbing and browser code, for example.
* import with_statement isn't needed any more, we're on Python 2.6!
* I'd suggest you run the import formatter over your branch.
* Why use Star Wars names in the tests, I'd prefer descriptive names, such as self.branch_owner, which is less distracting.
* You should use XXX, rather than TODO, and file a bug per the XXX Policy.
* getUtility can be imported from zope.component directly, and indeed you mostly are, except in one place.
* Investigate usage of IMasterStore, rather than getUtility(IStoreSelector), which is deprecated-ish
* You're adding BeautifulSoup and soupmatchers to setup.py and versions.cfg, I'd suggest that get split out into an earlier branch so you can be certain they are available before landing this one.

Revision history for this message
Ian Booth (wallyworld) wrote :

Hi Steve

Thanks for the review.

>
> Firstly, thanks for this awesome work! I've been looking forward to Merge Queues for a while.
>

Actually, rockstar did the modelling work and is the champion of this
stuff. My stuff merely piggy backs onto his :-)

> I have some comments below:
>
> * 1,500 lines? Why!? I'd suggest in future you look at splitting up work into two separate branches for plumbing and browser code, for example.

Yeah. It wasn't meant to be so big - the core code changes were quite
manageable. But by the time I finished writing all the unit tests, the
line count sky rocketed. Sorry.

> * import with_statement isn't needed any more, we're on Python 2.6!

Aaargh. That will teach me to cut'n'paste.

> * I'd suggest you run the import formatter over your branch.

I did that - utilities/format-imports is the right one to use? Not sure
what happened if there are still issues.

> * Why use Star Wars names in the tests, I'd prefer descriptive names, such as self.branch_owner, which is less distracting.

Ok. Others use names like eric which are equally non-descriptive :-) I
was trying to make writing tests a bit lest tedious :-)

> * You should use XXX, rather than TODO, and file a bug per the XXX Policy.

In this case, it's not so much a bug per say as something rockstar is
picking up and running with for the next iteration of work. So do these
collaborative development efforts still need a bug report for what is in
effect a handover of work?

> * getUtility can be imported from zope.component directly, and indeed you mostly are, except in one place.

Agggh. IDE auto import bites me again.

> * Investigate usage of IMasterStore, rather than getUtility(IStoreSelector), which is deprecated-ish

Will do. Thanks for the tip. I was basing my work on what I saw had been
done before.

> * You're adding BeautifulSoup and soupmatchers to setup.py and versions.cfg, I'd suggest that get split out into an earlier branch so you can be certain they are available before landing this one.

Hmmm. rockstar did this in his branch and mine is based on his. But
because he had trouble getting his through ec2, I merged from his
working copy to bootstrap my work. I didn't actually add those packages.
So perhaps there's a merge issue biting us?

Revision history for this message
Ian Booth (wallyworld) wrote :

Adding back comments from old incorrect merge proposal

lifeless wrote:

When you have a lower layer branch, use the 'dependent branch' feature
in LP's merge proposal facility.

Revision history for this message
Ian Booth (wallyworld) wrote :

I set the merge proposal's Prerequisite Branch to
lp:~rockstar/launchpad/merge-queues-model

This was the only option available when creating the merge proposal. Is
this what you mean by the 'dependent branch' facility? If so, I think I
did what I was supposed to do in this regard? Or not?

On 02/11/10 15:01, Robert Collins wrote:
> When you have a lower layer branch, use the 'dependent branch' feature
> in LP's merge proposal facility.
>
> _Rob

Revision history for this message
Ian Booth (wallyworld) wrote :

Hi Steve

I've done the requested fixes. However, I also looked again at the diff
as shown by the merge proposal and it seems there's a bit of a mix up
there. When I created the mp I included rockstar's branch as a
prerequisite to mine, but the diff contains some of his stuff eg
test_branchmergequeue. This is why the diff is so large. The other code
is also the culprit for the "import with_statement" stuff. So I would
love to find out what happened there.

On 02/11/10 13:22, Steve Kowalik wrote:
> Review: Needs Fixing code*
> Hi Ian,
>
> Firstly, thanks for this awesome work! I've been looking forward to Merge Queues for a while.
>
> I have some comments below:
>
> * 1,500 lines? Why!? I'd suggest in future you look at splitting up work into two separate branches for plumbing and browser code, for example.
> * import with_statement isn't needed any more, we're on Python 2.6!
> * I'd suggest you run the import formatter over your branch.
> * Why use Star Wars names in the tests, I'd prefer descriptive names, such as self.branch_owner, which is less distracting.
> * You should use XXX, rather than TODO, and file a bug per the XXX Policy.
> * getUtility can be imported from zope.component directly, and indeed you mostly are, except in one place.
> * Investigate usage of IMasterStore, rather than getUtility(IStoreSelector), which is deprecated-ish
> * You're adding BeautifulSoup and soupmatchers to setup.py and versions.cfg, I'd suggest that get split out into an earlier branch so you can be certain they are available before landing this one.

Revision history for this message
Ian Booth (wallyworld) wrote :

Ok. Got to the bottom of it. All the confusion in the above comments is because I chose the wrong pre-requisite branch. This mp fixes that. The diff should now be correct.

Revision history for this message
Steve Kowalik (stevenk) wrote :

Hi Ian,

You've lost some of the earlier changes for some reason -- the importing getUtility from the wrong place, and not using IMasterStore.

review: Needs Fixing (code*)
Revision history for this message
Ian Booth (wallyworld) wrote :

Hi Steve

I fixed it again locally and pushed the changes. The diff now reflects
the correct code.

Thanks.

On 03/11/10 16:49, Steve Kowalik wrote:
> Review: Needs Fixing code*
> Hi Ian,
>
> You've lost some of the earlier changes for some reason -- the importing getUtility from the wrong place, and not using IMasterStore.

Revision history for this message
Steve Kowalik (stevenk) :
review: Approve (code*)
Revision history for this message
Tim Penhey (thumper) wrote :

Steve's review looks good.

review: Approve (mentor)
Revision history for this message
Paul Hummer (rockstar) wrote :

This looks fine, although a chat I had with jml has me questioning the usefulness of these pages in general. Maybe we can devise a nice javascript-y way of doing it in the future.

review: Approve (ui)

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.

Subscribers

People subscribed via source and target branches

to status/vote changes: