Merge lp://staging/~rockstar/launchpad/merge-queue-index into lp://staging/launchpad/db-devel

Proposed by Paul Hummer
Status: Merged
Approved by: Gavin Panella
Approved revision: no longer in the source branch.
Merged at revision: 9943
Proposed branch: lp://staging/~rockstar/launchpad/merge-queue-index
Merge into: lp://staging/launchpad/db-devel
Diff against target: 433 lines (+302/-3)
11 files modified
lib/lp/code/browser/branch.py (+6/-1)
lib/lp/code/browser/branchmergequeue.py (+79/-0)
lib/lp/code/browser/configure.zcml (+26/-0)
lib/lp/code/browser/tests/test_branchmergequeue.py (+127/-0)
lib/lp/code/model/branchmergequeue.py (+3/-0)
lib/lp/code/templates/branch-pending-merges.pt (+15/-1)
lib/lp/code/templates/branchmergequeue-index.pt (+39/-0)
lib/lp/testing/__init__.py (+2/-0)
lib/lp/testing/factory.py (+1/-1)
setup.py (+2/-0)
versions.cfg (+2/-0)
To merge this branch: bzr merge lp://staging/~rockstar/launchpad/merge-queue-index
Reviewer Review Type Date Requested Status
Gavin Panella (community) Approve
Review via email: mp+39488@code.staging.launchpad.net

Commit message

Add branch merge queue index page. Add soupmatchers library dependency.

Description of the change

This branch adds a (really basic) index page for branch merge queues, as long as a flow for creating a merge queue and linking a branch to it. This branch also adds some more things, including:

 - The addition of james_w's soupmatchers library for HTML page matchers. This makes pagetest-type unit tests that we write in code a little nicer.
 - Fixed a bug in getUserBrowser where the authentication didn't support any passwords but 'test'. I fixed this by setting the password for auth to the sooper sekrit attribute that holds the cleartext password.
 - I also fixed a bug in the factory where the branch merge queues owner and registrant were being specified backwards.

This work is hiding behind a feature flag, and the UI is really raw, but I need to get this (and the next branch) landed so that others can also start working on branch merge queues.

To post a comment you must log in.
Revision history for this message
Gavin Panella (allenap) wrote :

Merge queues are cool. A few pretty minor comments.

[1]

+ def enable_queue_flag(self):
+ getFeatureStore().add(FeatureFlag(
+ scope=u'default', flag=u'code.branchmergequeue',
+ value=u'on', priority=1))

There's a neat context manager in lp.testing that you might like. I
think the flag only needs to be enabled on the branch index page, so
something like this might work:

    from lp.testing import feature_flags, set_feature_flag

    with feature_flags():
        set_feature_flag(u'code.branchmergequeue', u'on')
        browser = self.getUserBrowser(
            canonical_url(branch), user=rockstar)

Okay, that's not actually much clearer. Anyway, it's there.

[2]

I think you should ad a short test to show that the +create-queue link
is not available when the feature flag is not set.

[3]

+ if configuration is None:
+ configuration = unicode(simplejson.dumps({}))

The docstring for unicode says:

  Create a new Unicode object from the given encoded string.
  encoding defaults to the current default string encoding.

So I think this would be more correct as:

  configuration = simplejson.dumps({}).decode('ascii')

Or:

  configuration = simplejson.dumps({}, ensure_ascii=False)

In practice it makes bugger all difference though because I doubt
Launchpad will ever run with a default encoding that isn't a superset
of ASCII.

I should just shut up :)

[4]

+ <h4>Merge Queue</h4>
+ This branch is not managed by a queue.

Maybe a test for this too.

[5]

+ password = naked_user._password_cleartext_cached

This clobbers the password argument. Can you remove the argument? (In
a follow-on branch is fine if you need to get this landed.)

review: Approve
Revision history for this message
Gavin Panella (allenap) wrote :

> [5]
>
> + password = naked_user._password_cleartext_cached
>
> This clobbers the password argument. Can you remove the argument? (In
> a follow-on branch is fine if you need to get this landed.)

Erm, not true, as rockstar just pointed out to me on IRC.

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: