Code review comment for lp:~rockstar/launchpad/merge-queue-index

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

« Back to merge proposal