launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

Brad Crittenden has proposed merging lp:~bac/launchpad/bug-633926 into lp:launchpad/devel.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #633926 Changing profile name results in error: 'Not allowed here'
  https://bugs.launchpad.net/bugs/633926


= Summary =

A permission change resulted in users not being able to change their name.

== Proposed fix ==

Since the form is protected by launchpad.Edit, the fix is to use
removeSecurityProxy in the form action to allow users to set the name.

== Pre-implementation notes ==

None.

== Implementation details ==

It should be noted that the use of Foo Bar, who holds administrator
privileges, in the xx-person-edit.txt test hid the regression.  That
test has been changed to not use sample data and instead creates a
normal user.

== Tests ==

bin/test -vvt xx-person-edit.txt

== Demo and Q/A ==

On staging as a normal person try to change your name.

= Launchpad lint =

I'll fix these.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/stories/person/xx-person-edit.txt
  lib/lp/registry/browser/person.py

./lib/lp/registry/stories/person/xx-person-edit.txt
       1: narrative uses a moin header.
./lib/lp/registry/browser/person.py
     211: 'safe_hasattr' imported but unused
-- 
https://code.launchpad.net/~bac/launchpad/bug-633926/+merge/35040
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-633926 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-09-03 03:12:39 +0000
+++ lib/lp/registry/browser/person.py	2010-09-09 21:26:00 +0000
@@ -209,7 +209,6 @@
     PersonFormatterAPI,
     )
 from canonical.lazr.utils import (
-    safe_hasattr,
     smartquote,
     )
 from canonical.widgets import (
@@ -4167,6 +4166,14 @@
 
     @action(_("Save Changes"), name="save")
     def action_save(self, action, data):
+        # Users should be able to change their name, but the permission
+        # setting for the attribute is launchpad.Moderate, which only allows
+        # admins and registry.  A user must have launchpad.Edit to access this
+        # page.
+        if 'name' in data:
+            new_name = data['name']
+            removeSecurityProxy(self.context).name = new_name
+            del data['name']
         self.updateContextFromData(data)
 
 

=== modified file 'lib/lp/registry/stories/person/xx-person-edit.txt'
--- lib/lp/registry/stories/person/xx-person-edit.txt	2010-03-24 21:45:05 +0000
+++ lib/lp/registry/stories/person/xx-person-edit.txt	2010-09-09 21:26:00 +0000
@@ -1,15 +1,22 @@
-== Changing personal details ==
+Changing personal details
+=========================
 
-Foo Bar wants to check if his personal information is up to date in
+A user wants to check if his personal information is up to date in
 Launchpad.
 
-    >>> browser.addHeader('Authorization', 'Basic foo.bar@xxxxxxxxxxxxx:test')
-    >>> browser.open('http://launchpad.dev/~name16')
+    >>> from lp.testing.sampledata import ADMIN_EMAIL
+    >>> login(ADMIN_EMAIL)
+    >>> user = factory.makePerson(name='ray', displayname='Ray Ray',
+    ...                           email='ray@xxxxxxxxxxx',
+    ...                           password='test')
+    >>> logout()
+    >>> browser.addHeader('Authorization', 'Basic ray@xxxxxxxxxxx:test')
+    >>> browser.open('http://launchpad.dev/~ray')
     >>> browser.getLink('Change details').click()
     >>> browser.getControl('Display Name').value
-    'Foo Bar'
+    'Ray Ray'
     >>> browser.getControl('Name', index=1).value
-    'name16'
+    'ray'
     >>> browser.getControl(
     ...     'Hide my email addresses from other Launchpad users').selected
     False
@@ -25,46 +32,46 @@
 (If he leaves leading or trailing whitespace in the displayname it'll
 be stripped)
 
-    >>> browser.getControl('Name', index=1).value = 'foobar'
-    >>> browser.getControl('Display Name').value = '  Foo Bar2  '
+    >>> browser.getControl('Name', index=1).value = 'rayjay'
+    >>> browser.getControl('Display Name').value = '   Mr Ray Jay '
     >>> browser.getControl(
     ...     'Hide my email addresses from other Launchpad users').click()
     >>> browser.getControl('Save').click()
 
     >>> browser.url
-    'http://launchpad.dev/~foobar'
+    'http://launchpad.dev/~rayjay'
 
 Now we check to make sure the displayname was changed and stripped.
 
-    >>> browser.open('http://launchpad.dev/~foobar/+edit')
+    >>> browser.open('http://launchpad.dev/~rayjay/+edit')
     >>> browser.getControl('Display Name').value
-    'Foo Bar2'
+    'Mr Ray Jay'
 
 And the email addresses are not shown on Foo Bar's home page to any random
 user.
 
-    >>> user_browser.open('http://launchpad.dev/~foobar')
-    >>> 'foo.bar@xxxxxxxxxxxxx' in user_browser.contents
+    >>> user_browser.open('http://launchpad.dev/~rayjay')
+    >>> 'ray@xxxxxxxxxxx' in user_browser.contents
     False
 
 He will decide to make all bug notifications that are sent to him
-verbose.
+not include descriptions.
 
     >>> browser.getControl("Include bug descriptions when sending me "
     ...     "bug notifications").selected
-    False
+    True
 
     >>> browser.getControl("Include bug descriptions when sending me "
     ...     "bug notifications").click()
     >>> browser.getControl('Save').click()
 
     >>> browser.url
-    'http://launchpad.dev/~foobar'
+    'http://launchpad.dev/~rayjay'
 
 We now check to ensure that the verbose bug notifications option was
 changed:
 
-    >>> browser.open('http://launchpad.dev/~foobar/+edit')
+    >>> browser.open('http://launchpad.dev/~rayjay/+edit')
     >>> browser.getControl("Include bug descriptions when sending me "
     ...     "bug notifications").selected
-    True
+    False