Merge lp:~wallyworld/launchpad/person-mergequeue-listview into lp:launchpad/db-devel
| Status: | Superseded |
|---|---|
| Proposed branch: | lp:~wallyworld/launchpad/person-mergequeue-listview |
| Merge into: | lp:launchpad/db-devel |
| Prerequisite: | lp:~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:~wallyworld/launchpad/person-mergequeue-listview |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Paul Hummer (community) | ui | 2010-11-05 | Approve on 2010-11-08 |
| Tim Penhey (community) | mentor | Approve on 2010-11-05 | |
| Steve Kowalik (community) | code* | 2010-11-03 | Approve on 2010-11-05 |
|
Review via email:
|
|||
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 IBranchMergeQue
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://
= Tests =
bin/test -vvt test_branchmerg
bin/test -vvt test_branchmerg
= Launchpad lint =
Checking for conflicts and issues in changed files.
Linting changed files:
setup.py
versions.cfg
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/lp/
./setup.py
131: E202 whitespace before ']'
./lib/lp/
129: 'anonymous_
129: 'with_anonymous
129: 'is_logged_in' imported but unused
148: 'launchpadlib_for' imported but unused
148: 'launchpadlib_
129: 'person_logged_in' imported but unused
148: 'oauth_
129: 'login_celebrity' imported but unused
129: 'with_celebrity
147: 'test_tales' imported but unused
129: 'celebrity_
129: 'run_with_login' imported but unused
129: 'with_person_
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
| Ian Booth (wallyworld) wrote : | # |
| 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/
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(
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?
| 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.
| 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
| 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_branchmerg
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(
> * 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.
| 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.
| 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.
| 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.
| 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.

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. IStoreSelector) , which is deprecated-ish
* 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(
* 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.