Merge lp:~bac/launchpad/bug-419930 into lp:launchpad
| Status: | Merged |
|---|---|
| Merged at revision: | not available |
| Proposed branch: | lp:~bac/launchpad/bug-419930 |
| Merge into: | lp:launchpad |
| Diff against target: |
500 lines (+219/-105) 9 files modified
lib/canonical/launchpad/doc/tales.txt (+1/-2) lib/canonical/launchpad/interfaces/launchpad.py (+1/-1) lib/canonical/launchpad/pagetitles.py (+0/-4) lib/lp/bugs/doc/externalbugtracker-bug-imports.txt (+2/-2) lib/lp/registry/browser/peoplemerge.py (+48/-30) lib/lp/registry/interfaces/person.py (+8/-7) lib/lp/registry/stories/person/merge-people.txt (+63/-6) lib/lp/registry/templates/people-requestmerge-multiple.pt (+95/-52) lib/lp/soyuz/browser/sourcepackagerelease.py (+1/-1) |
| To merge this branch: | bzr merge lp:~bac/launchpad/bug-419930 |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| Martin Albisetti (community) | ui | 2010-01-08 | Approve on 2010-01-12 |
| Guilherme Salgado (community) | 2010-01-08 | Approve on 2010-01-09 | |
|
Review via email:
|
|||
Commit Message
Rework merge request pages
| Brad Crittenden (bac) wrote : | # |
| Guilherme Salgado (salgado) wrote : | # |
Hi Brad,
This looks pretty good. I have only a few comments/
request for a missing test.
review needsfixing
On Fri, 2010-01-08 at 13:36 +0000, Brad Crittenden wrote:
> Brad Crittenden has proposed merging lp:~bac/launchpad/bug-419930 into
> lp:launchpad/devel.
>
> Requested reviews:
> Canonical Launchpad Engineering (launchpad)
> Related bugs:
> #419930 non-public email address shown after merge request
> https:/
>
>
> = Summary =
>
> Users can choose to hide their email addresses and we should always
> honor that
> request. Currently anyone who attempts to merge a person with hidden
> email addresses
> is shown all addresses for that user.
>
> Also did a drive-by fix to the person picker vocabulary that was not
> catching an
> Unauthorized exception which prevented people with hidden emails from
> being selected
> in the picker.
Does that mean we can't merge people with hidden email addresses unless
we know their Launchpad ID or email address?
>
> == Proposed fix ==
>
> Rather than displaying the addresses and letting the user choose which
> ones to
> contact we should just show a count of addresses and send notification
> to them all.
>
> == Pre-implementation notes ==
>
> Quick call with Curtis as a sanity check of the approach.
>
> == Implementation details ==
>
> As above.
>
> == Tests ==
>
> bin/test -vvm lp.registry -t merge-people.txt
>
> === modified file 'lib/canonical/
> --- lib/canonical/
> +++ lib/canonical/
> @@ -82,7 +82,7 @@
> def person_
> """Adapts IPerson to IPickerEntry."""
> extra = default_
> - if person.
> + if person.
I didn't see a test for this; care to add one?
> extra.description = person.
> return extra
>
>
> === modified file 'lib/lp/
> --- lib/lp/
> +++ lib/lp/
> @@ -359,8 +359,7 @@
>
>
> class RequestPeopleMe
> - """A view for the page where the user asks a merge and the dupe account
> - have more than one email address."""
> + """Merge request view when dupe account has multiple email addresses."""
>
> def __init__(self, context, request):
> self.context = context
> @@ -390,29 +389,38 @@
> login = getUtility(
> logintokenset = getUtility(
>
> - emails = self.request.
> - if emails is not None:
> - # We can have multiple email adressess selected, and in this case
> - # emails will be a list. Otherwise it will be a string and we need
> - # to make a list with that value to use in the for loop.
> - if not isinstance(emails, list):
> - em...
| Brad Crittenden (bac) wrote : | # |
> Hi Brad,
>
> This looks pretty good. I have only a few comments/
> request for a missing test.
Thanks for the review.
> > Also did a drive-by fix to the person picker vocabulary that was not
> > catching an
> > Unauthorized exception which prevented people with hidden emails from
> > being selected
> > in the picker.
>
> Does that mean we can't merge people with hidden email addresses unless
> we know their Launchpad ID or email address?
Currently we cannot use the picker to select people with hidden email addresses. But you can merge them by typing in the ID into the field on the form.
> > === modified file 'lib/canonical/
> > --- lib/canonical/
> +0000
> > +++ lib/canonical/
> +0000
> > @@ -82,7 +82,7 @@
> > def person_
> > """Adapts IPerson to IPickerEntry."""
> > extra = default_
> > - if person.
> > + if person.
> person.
>
> I didn't see a test for this; care to add one?
I have discovered Edwin is working to redo the way these pickers are working and my drive-by change would soon be replaced by his work. Therefore I'm going to back out this portion of the change and just wait for his more thorough fix.
> > === modified file 'lib/lp/
> > + # We can have multiple email adressess selected, and in
> this
>
> Typo: adressess
This typo was pre-existing. I looked to see if it existed elsewhere and found and fixed six other occurrences.
> > === modified file 'lib/lp/
> > --- lib/lp/
> +0000
> > +++ lib/lp/
> +0000
> > @@ -172,3 +172,56 @@
> > #>>> browser.open('http://
> sent?dupe=55')
> > #>>> browser.url
> > #'http://
> > +
> > +If the duplicate account has multiple email addresses and has chosen
> > +to hide them the process is slightly different. We cannot display the
> > +hidden addresses so instead we just inform the user to check all of
> > +them (and hope they know which ones) and we send merge request
> > +messages to them all.
> > +
> > + >>> login('<email address hidden>')
> > + >>> dupe = factory.
> <email address hidden>",
> > + ... hide_email_
> > + >>> email = factory.
> > + >>> logout()
> > +
> > +Ensure there are no leftover emails.
> > +
> > + >>> len(stub.
> > + 0
> > +
> > +Open the merge page and merge our duplicate account with hidden email
> > +addresses.
> > +
> > + >>> browser.open('http://
> > + >>> browser.getControl(
> > + ... 'Duplicated Account').value = 'duplicate-person'
> > + >>> browser.getC...
| Brad Crittenden (bac) wrote : | # |
| Brad Crittenden (bac) wrote : | # |
Added screenshots to the bug 419930 for a UI review.
| Guilherme Salgado (salgado) wrote : | # |
On Fri, 2010-01-08 at 21:37 +0000, Brad Crittenden wrote:
> > > === modified file 'lib/canonical/
> > > --- lib/canonical/
> > +0000
> > > +++ lib/canonical/
> > +0000
> > > @@ -82,7 +82,7 @@
> > > def person_
> > > """Adapts IPerson to IPickerEntry."""
> > > extra = default_
> > > - if person.
> > > + if person.
> > person.
> >
> > I didn't see a test for this; care to add one?
>
> I have discovered Edwin is working to redo the way these pickers are
> working and my drive-by change would soon be replaced by his work.
> Therefore I'm going to back out this portion of the change and just
> wait for his more thorough fix.
Ok, do you mind filing a bug and assigning it to him, just to make sure
it's not forgotten?
>
>
> > > === modified file 'lib/lp/
>
> > > + # We can have multiple email adressess selected, and in
> > this
> >
> > Typo: adressess
>
> This typo was pre-existing. I looked to see if it existed elsewhere and found and fixed six other occurrences.
>
Nice, thanks!
review approve
--
Guilherme Salgado <email address hidden>

= Summary =
Users can choose to hide their email addresses and we should always honor that
request. Currently anyone who attempts to merge a person with hidden email addresses
is shown all addresses for that user.
Also did a drive-by fix to the person picker vocabulary that was not catching an
Unauthorized exception which prevented people with hidden emails from being selected
in the picker.
== Proposed fix ==
Rather than displaying the addresses and letting the user choose which ones to
contact we should just show a count of addresses and send notification to them all.
== Pre-implementation notes ==
Quick call with Curtis as a sanity check of the approach.
== Implementation details ==
As above.
== Tests ==
bin/test -vvm lp.registry -t merge-people.txt
== Demo and Q/A ==
= Launchpad lint =
Checking for conflicts. and issues in doctests and templates.
Running jslint, xmllint, pyflakes, and pylint.
Using normal rules.
Linting changed files: registry/ browser/ peoplemerge. py registry/ stories/ person/ merge-people. txt registry/ interfaces/ person. py registry/ templates/ people- requestmerge- multiple. pt /launchpad/ browser/ vocabulary. py
lib/lp/
lib/lp/
lib/lp/
lib/lp/
lib/canonical
== Pylint notices ==
Non-issues:
lib/lp/ registry/ browser/ peoplemerge. py iew.deactivate_ members_ and_merge_ action] Operator not 'deactivate_ members_ and_merge' ) members_ and_merge_ action( self, action, data):
250: [C0322, AdminTeamMergeV
preceded by a space
name=
^
def deactivate_
lib/lp/ registry/ interfaces/ person. py .snapshot' (No module named lifecycle) interface' (No module named restful) declarations' (No module named restful) fields' (No module named restful) ._validate] Use super on an old style class ricted. addMember] Operator not preceded by a space copy_field( ITeamMembership ['status' ]), Text(required= False)) write_operation () TeamMembershipS tatus.APPROVED, add=False, subscribe_ to_list= True): ricted. acceptInvitatio nToBeMemberOf] Operator not write_operation () nToBeMemberOf( team, comment): ricted. declineInvitati onToBeMemberOf] Operator not write_operation () onToBeMemberOf( team, comment): ershipperiod= 'default_ membership_ period' , walperiod= 'default_ renewal_ period' ) parameters( npolicy= Choice( _('Subscription policy'), vocabulary= TeamSubscriptio nPolicy, TeamSubscriptio nPolicy. MODERATED) ) factory_ oper...
52: [F0401] Unable to import 'lazr.enum' (No module named enum)
53: [F0401] Unable to import 'lazr.lifecycle
54: [F0401] Unable to import 'lazr.restful.
55: [F0401] Unable to import 'lazr.restful.
62: [F0401] Unable to import 'lazr.restful.
407: [E1002, PersonNameField
1405: [C0322, IPersonEditRest
status=
^
comment=
@export_
def addMember(person, reviewer, status=
comment=None, force_team_
may_
1444: [C0322, IPersonEditRest
preceded by a space
comment=Text())
^
@export_
def acceptInvitatio
1456: [C0322, IPersonEditRest
preceded by a space
comment=Text())
^
@export_
def declineInvitati
1754: [C0322, IPersonSet.newTeam] Operator not preceded by a space
defaultmemb
^
defaultrene
@operation_
subscriptio
title=
required=False, default=
@export_