Launchpad itself

Merge lp://staging/~benji/launchpad/bug-669701 into lp://staging/launchpad

bug-669701
Merge into devel

Proposed by Benji York on 2011-01-04

Status:

Merged

Approved by:

Curtis Hovey on 2011-01-04

Approved revision:

no longer in the source branch.

Merged at revision:

12169

Proposed branch:

lp://staging/~benji/launchpad/bug-669701

Merge into:

lp://staging/launchpad

Diff against target:

273 lines (+114/-28)

6 files modified

lib/lp/registry/browser/tests/test_projectgroup.py (+1/-3)
lib/lp/services/features/browser/edit.py (+35/-18)
lib/lp/services/features/browser/tests/test_feature_editor.py (+64/-2)
lib/lp/services/features/rulesource.py (+4/-1)
lib/lp/services/features/templates/feature-rules.pt (+7/-1)
lib/lp/services/features/tests/test_flags.py (+3/-3)

To merge this branch:

bzr merge lp://staging/~benji/launchpad/bug-669701

Related bugs:

Bug #651852: NameError: Unauthorized: when ~registry user tries to edit feature	Critical	Fix Released
Bug #669701: Feature Flag control gives no feedback on successful application of new rules	High	Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Edwin Grubbs (community)	code		Approve on 2011-01-05
Curtis Hovey (community)	ui		Approve on 2011-01-04
Leonard Richardson (community)		2011-01-04	Approve on 2011-01-04
Review via email: mp+45171@code.staging.launchpad.net

Commit message

[r=edwin-grubbs,leonardr][ui=sinzui][bug=651852,669701] [r=leonardr][ui=sinzui][bug=669701]

Description of the change

When saving the features flag rules the user is given no indication that
anything actually happened (bug 669701). This branch fixes that by
adding a message and displaying the changes applied in the form of a
diff.

This branch also fixes a bug discovered during development: if a user
who is not authorized to make feature flag changes attempts to do so a
NameError will be raised because of a missing import in untested code.
Both the import and a test were added.

The tests for the modified code can be run with:

bin/test -c lp.services.features.browser

The feature can be interactively tested by starting a dev Launchpad
instance and logging in as an admin user (<email address hidden>/test)
will work and then interacting with https://launchpad.dev/+feature-rules.

Several bits of lint were also fixed on this branch.

Revision history for this message

Leonard Richardson (leonardr) wrote on 2011-01-04:

r=me with minor changes. Around line 151 you have some line breaks that violate the style guide. diff_rules() should be a method of the view so that it's clear which code it belongs to.

review: Approve

Revision history for this message

Benji York (benji) wrote on 2011-01-04:

Leonard's suggestions have been integrated into the branch.

Revision history for this message

Curtis Hovey (sinzui) wrote on 2011-01-04:

We talk about why this does not use class="informational message" like other forms. The reason is because the placing a coloured diff looks bad. We do not want to introduce a blue, yellow, red, and green splotch to a page and risk giving a beloved losa a seizure.

review: Approve (ui)

Revision history for this message

Martin Pool (mbp) wrote on 2011-01-05:

The text diff seems nice. It might be a bit nicer if you did a diff against the value read back by getAllRulesAsText(), which will avoid spurious diffs due to text changes that the database doesn't care about.

The NameError you fixed in passing was https://bugs.launchpad.net/launchpad/+bug/651852 -- well done!

Revision history for this message

Benji York (benji) wrote on 2011-01-05:

> The text diff seems nice. It might be a bit nicer if you did a diff against
> the value read back by getAllRulesAsText(), which will avoid spurious diffs
> due to text changes that the database doesn't care about.

Done.

Revision history for this message

Benji York (benji) wrote on 2011-01-05:

I made a couple of further changes. One address Martin's suggestion above. The other two address bug 670019 and bug 636193. I'm going to get someone to review the incremental diff (at http://pastebin.ubuntu.com/550823/) before landing.

Revision history for this message

Edwin Grubbs (edwin-grubbs) wrote on 2011-01-05:

>+ # Why re-fetch the rules here? This way we get them reformatted
>+ # (whitespace normalized) and ordered consistently so the diff is
>+ # minimal.
>+ diff = '\n'.join(self.diff_rules(original_rules,
>+ self.request.features.rule_source.getAllRulesAsText()))

This formatting is hard to read and nonstandard. You could reformat like the following to comply with the style guide, but it would probably be more readable if you put the rules into a variable, so that the join() fits on one line.

        diff = '\n'.join(self.diff_rules(
            original_rules,
            self.request.features.rule_source.getAllRulesAsText()))

Everything else looks good.

review: Approve (code)

Revision history for this message

Benji York (benji) wrote on 2011-01-05:

> This formatting is hard to read and nonstandard. You could reformat like the
> following to comply with the style guide, but it would probably be more
> readable if you put the rules into a variable, so that the join() fits on one
> line.

Fixed. I added a variable.