Launchpad itself

Merge lp://staging/~julian-edwards/launchpad/rename-account-with-ppas into lp://staging/launchpad

rename-account-with-ppas
Merge into devel

Proposed by Julian Edwards on 2010-05-11

Status:	Merged
Approved by:	Данило Шеган on 2010-05-11
Approved revision:	no longer in the source branch.
Merged at revision:	10848
Proposed branch:	lp://staging/~julian-edwards/launchpad/rename-account-with-ppas
Merge into:	lp://staging/launchpad
Diff against target:	205 lines (+79/-40) 4 files modified lib/lp/registry/browser/person.py (+10/-4) lib/lp/registry/browser/tests/test_person_view.py (+59/-1) lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt (+6/-32) lib/lp/soyuz/templates/archive-activate.pt (+4/-3)
To merge this branch:	bzr merge lp://staging/~julian-edwards/launchpad/rename-account-with-ppas
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Данило Шеган (community)		2010-05-11	Approve on 2010-05-11
Review via email: mp+25042@code.staging.launchpad.net

Description of the change

= Summary =
Allow account renaming when all PPAs are deleted

== Proposed fix ==
Change the account +edit form so that the account can be renamed if:
a) the user has no PPAs
b) the user has a PPA but it has no packages
c) the user had PPAs but they are all deleted

== Pre-implementation notes ==
This was pair-programmed with noodles!

== Implementation details ==
Fairly simple steps:
1. The browser setupWidgets() was changed to disabled/enable the name field
appropriately
2. I added a new TestPersonEditView that unit tests setupWidgets.
3. Fixed the ppa-workflow doctest to remove unit test stuff therein.
4. Changed the activation template so that it has a slightly different warning
message.

== Tests ==
bin/test -cvvt xx-ppa-workflow.txt -t TestPersonEditView

== Demo and Q/A ==
I can Q/A this on dogfood before it lands using some dummy accounts and PPAs.
It's a pretty high-profile bug so I don't want to QA it on edge after it lands
and the world sees it.

Revision history for this message

Данило Шеган (danilo) wrote on 2010-05-11:

"if has_packages" should probably be "if has_packages > 0", and the variable name should better reflect that it's a count of published packages for the person

In test_can_rename_with_deleted_PPA, I assume ppa.delete() sets the status to DELETING, so you should probably test that case separately (since DELETING is still not DELETED :): keep the existing test, but add another one for 'deleting' state if that makes sense.

For the existing 'deleted' test you most likely don't even need to call ppa.delete() since you hard-code the status to DELETED anyway, right? If I am smoking crack, just ignore this comment.

Nice job switching to a unit test!

review: Needs Fixing

Revision history for this message

Julian Edwards (julian-edwards) wrote on 2010-05-11:

Thanks for the review!

> "if has_packages" should probably be "if has_packages > 0", and the
> variable name should better reflect that it's a count of published
> packages for the person

Okay, I changed it.

> In test_can_rename_with_deleted_PPA, I assume ppa.delete() sets the status
> to DELETING, so you should probably test that case separately (since
> DELETING is still not DELETED :): keep the existing test, but add another
> one for 'deleting' state if that makes sense.

Good catch! I've added an additional test for this.

> For the existing 'deleted' test you most likely don't even need to call
> ppa.delete() since you hard-code the status to DELETED anyway, right? If
> I am smoking crack, just ignore this comment.

You're smoking crack :)

Deleting a PPA also sets all its publications to DELETED, which is also
necessary to rename your account. I changed a comment to make that clearer in
the test.

> Nice job switching to a unit test!

Thanks!

Partial diff attached.

Cheers
J

Revision history for this message

Данило Шеган (danilo) wrote on 2010-05-11:

Looks good, thanks for all the fish (and crack).

review: Approve