Launchpad itself

Merge lp://qastaging/~bac/launchpad/bug-618148 into lp://qastaging/launchpad

bug-618148
Merge into devel

Proposed by Brad Crittenden on 2010-08-24

Status:

Merged

Approved by:

Edwin Grubbs on 2010-08-25

Approved revision:

no longer in the source branch.

Merged at revision:

11443

Proposed branch:

lp://qastaging/~bac/launchpad/bug-618148

Merge into:

lp://qastaging/launchpad

Diff against target:

419 lines (+127/-67)

9 files modified

lib/canonical/launchpad/pagetests/standalone/xx-form-layout.txt (+6/-10)
lib/lp/app/interfaces/launchpad.py (+6/-6)
lib/lp/registry/browser/product.py (+26/-9)
lib/lp/registry/stories/product/xx-product-launchpad-usage.txt (+56/-12)
lib/lp/testing/sampledata.py (+2/-1)
lib/lp/translations/stories/standalone/xx-product-translations.txt (+13/-13)
lib/lp/translations/stories/translationgroups/15-product-translation-group.txt (+1/-3)
lib/lp/translations/stories/translationgroups/46-test-distro-structured-permissions.txt (+7/-5)
lib/lp/translations/stories/translations/55-rosetta-potemplates.txt (+10/-8)

To merge this branch:

bzr merge lp://qastaging/~bac/launchpad/bug-618148

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Edwin Grubbs (community)	code		Approve on 2010-08-25
Steve Kowalik (community)	code*	2010-08-24	Needs Information on 2010-08-25
Tim Penhey	code	2010-08-25	Pending
Review via email: mp+33600@code.qastaging.launchpad.net

Commit message

Make +configure-{answers,blueprints,translations} use the ServiceUsage enums rather than a Boolean.

Description of the change

= Summary =

The project configuration progress bar and configuration indicators are
currently based on the official_* booleans and usage of Launchpad apps
is controlled by a boolean. This branch changes the
+configure{answers|blueprints|translations} pages to use the
ServiceUsage enums for multi-state selection.

== Proposed fix ==

As above.

== Pre-implementation notes ==

Talks with Curtis and Jon.

== Implementation details ==

As above.

== Tests ==

bin/test -vvt xx-product-launchpad-usage.txt

== Demo and Q/A ==

Go to https://launchpad.dev/firefox and click on the four configure
links. For blueprints you must go to the app and then click on the
configure link.

= Launchpad lint =

I'll fix these now.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/translations/stories/standalone/xx-product-translations.txt
  lib/lp/testing/sampledata.py
  lib/lp/registry/browser/product.py
  lib/lp/app/interfaces/launchpad.py
  lib/lp/translations/stories/translations/55-rosetta-potemplates.txt
  lib/lp/registry/stories/product/xx-product-launchpad-usage.txt
  lib/canonical/launchpad/pagetests/standalone/xx-form-layout.txt

lib/lp/translations/stories/translationgroups/46-test-distro-structured-permissions.txt

lib/lp/translations/stories/translationgroups/15-product-translation-group.txt

./lib/lp/translations/stories/standalone/xx-product-translations.txt
       1: narrative uses a moin header.
     169: narrative uses a moin header.
./lib/lp/app/interfaces/launchpad.py
      80: E231 missing whitespace after ','
./lib/lp/translations/stories/translations/55-rosetta-potemplates.txt
       1: narrative uses a moin header.
      70: narrative uses a moin header.
      98: narrative uses a moin header.
     109: source exceeds 78 characters.
     128: want exceeds 78 characters.
     140: want exceeds 78 characters.
     153: want exceeds 78 characters.
     166: want exceeds 78 characters.
./lib/lp/registry/stories/product/xx-product-launchpad-usage.txt
      63: source exceeds 78 characters.
     100: source exceeds 78 characters.

Revision history for this message

Steve Kowalik (stevenk) wrote on 2010-08-25:

Hi,

This looks like a well-thought out change, however, I have a few questions:

* You have lots of use of sampledata in the doctests changed, would you consider not using it?
* You have three classes that look exceedingly similar (ProductConfigureBlueprintsView and so on), can field_names and custom_widget be moved to the base class?

review: Needs Information (code*)

Revision history for this message

Brad Crittenden (bac) wrote on 2010-08-25:

Hi Steven,

Thanks for the review.

Your idea about refactoring the classes was a good one. It was a little more complicated due to the use of class variables and how custom_widget works but I did get it working and it'll be better long term. Oh, there is some extra funny business there too since the bug tracker set extends this class but then does some funniness.

Incremental diff at http://pastebin.ubuntu.com/483495/

As to sample data, you'll note all of the tests were just changes to existing tests that rely on sample data. I assiduously avoid sample data when constructing new tests but when only modifying old ones it often doesn't make sense to invest the time to rewrite the hole thing. This approach seems to be the one adopted by most teams.

Revision history for this message

Brad Crittenden (bac) wrote on 2010-08-25:

Thanks for your comments on IRC, Edwin.

Here is a diff to copy the field before modifying and to make 'field_names' a property. Thanks for the improvements.
http://pastebin.ubuntu.com/483575/

Revision history for this message

Edwin Grubbs (edwin-grubbs) wrote on 2010-08-25:

Brad, You changes look good.

Steve, I feel I should give you some feedback as a reviewer mentee. I think your review's comment about moving more of the common elements into the base class was very good. You should definitely take a look at Brad's latest changes using copy_field(), since the description modifications would have propagated to other forms if the field isn't copied. Of course, that's a hard one to catch. Eliminating sample data is also a tough call. If a pre-existing test only requires a few lines to be changed, and the branch isn't urgent, then it would be reasonable to force the drive-by changes to made immediately. So, I think your review was good, and you got to learn about some of the esoteric behavior of LaunchpadFormView.

review: Approve (code)