launchpad-reviewers team mailing list archive

[Brad Crittenden <bac@xxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #32618 Swapping jabber IDs violates unique constraint (OOPS-53C33)
  https://bugs.launchpad.net/bugs/32618


= Summary =

Multiple Jabberids can be edited in place with confusing results.  The
UI is confusing.  Simplifying the UI makes the problem impossible to
recreate.

== Proposed fix ==

Only display the jabberids, don't show them in editable fields.  Need to
change a jabberid?  Delete it and add it back correctly.

== Pre-implementation notes ==

Chit chat with Curtis.

== Implementation details ==

None

== Tests ==

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

== Demo and Q/A ==

http://launchpad.dev/~mark/+editjabberids

= Launchpad lint =

Will fix these.

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/registry/stories/person/xx-person-edit-jabber-ids.txt
  lib/lp/registry/templates/person-editjabberids.pt
  lib/lp/registry/browser/person.py

./lib/lp/registry/stories/person/xx-person-edit-jabber-ids.txt
       0: narrative uses a moin header.
       4: narrative uses a moin header.
      32: source exceeds 78 characters.
      45: narrative uses a moin header.
-- 
https://code.launchpad.net/~bac/launchpad/bug-32618/+merge/32145
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~bac/launchpad/bug-32618 into lp:launchpad/devel.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2010-08-05 22:12:44 +0000
+++ lib/lp/registry/browser/person.py	2010-08-09 20:41:11 +0000
@@ -3541,13 +3541,6 @@
         for jabber in self.context.jabberids:
             if form.get('remove_%s' % jabber.jabberid):
                 jabber.destroySelf()
-            else:
-                jabberid = form.get('jabberid_%s' % jabber.jabberid)
-                if not jabberid:
-                    self.request.response.addErrorNotification(
-                        "You cannot save an empty Jabber ID.")
-                    return
-                jabber.jabberid = jabberid
 
         jabberid = form.get('newjabberid')
         if jabberid:
@@ -3600,8 +3593,8 @@
 
     def add_ssh(self):
         sshkey = self.request.form.get('sshkey')
-	try:
-      	    getUtility(ISSHKeySet).new(self.user, sshkey)
+        try:
+            getUtility(ISSHKeySet).new(self.user, sshkey)
         except SSHKeyAdditionError:
             self.error_message = structured('Invalid public key')
         except SSHKeyCompromisedError:

=== modified file 'lib/lp/registry/stories/person/xx-person-edit-jabber-ids.txt'
--- lib/lp/registry/stories/person/xx-person-edit-jabber-ids.txt	2009-11-07 04:30:07 +0000
+++ lib/lp/registry/stories/person/xx-person-edit-jabber-ids.txt	2010-08-09 20:41:11 +0000
@@ -29,36 +29,27 @@
 However, if the user enters a Jabber ID which isn't already registered,
 it will be associated with his account.
 
-    >>> user_browser.getControl(name='newjabberid').value = 'nopriv@xxxxxxxxxx'
-    >>> user_browser.getControl('Save Changes').click()
-
-The new ID can be edited by changing its value, and another ID can
-also be entered.
-
-    >>> user_browser.getControl(name='jabberid_nopriv@xxxxxxxxxx').value = (
-    ...     'no-priv@xxxxxxxxxx')
-    >>> user_browser.getControl(name='newjabberid').value = (
-    ...     'no-priv@xxxxxxxxx')
-    >>> user_browser.getControl('Save Changes').click()
-
-    >>> import re
-    >>> main_content = find_main_content(user_browser.contents)
-    >>> for input in main_content.findAll(
-    ...     'input', {'name': re.compile('jabberid_')}):
-    ...     print input['value']
-    no-priv@xxxxxxxxx
+    >>> user_browser.getControl(name='newjabberid').value = 'no-priv@xxxxxxxxxx'
+    >>> user_browser.getControl('Save Changes').click()
+
+    >>> def show_jabberids(browser):
+    ...     tags = find_tags_by_class(browser.contents, 'jabberid')
+    ...     if tags:
+    ...         for tag in tags:
+    ...             print extract_text(tag)
+    ...     else:
+    ...         print "No jabberids"
+
+    >>> show_jabberids(user_browser)
     no-priv@xxxxxxxxxx
 
 == Removing an ID ==
 
-To remove an existing Jabber ID, the user simply check the 'Remove'
+To remove an existing Jabber ID, the user simply checks the 'Remove'
 checkbox besides the ID:
 
     >>> user_browser.getControl('Remove', index=0).click()
     >>> user_browser.getControl('Save Changes').click()
 
-    >>> main_content = find_main_content(user_browser.contents)
-    >>> for input in main_content.findAll(
-    ...     'input', {'name': re.compile('jabberid_')}):
-    ...     print input['value']
-    no-priv@xxxxxxxxxx
+    >>> show_jabberids(user_browser)
+    No jabberids

=== modified file 'lib/lp/registry/templates/person-editjabberids.pt'
--- lib/lp/registry/templates/person-editjabberids.pt	2009-09-01 19:30:03 +0000
+++ lib/lp/registry/templates/person-editjabberids.pt	2010-08-09 20:41:11 +0000
@@ -20,10 +20,8 @@
         </tr>
 
         <tr tal:repeat="jabber context/jabberids">
-          <td>
-            <input tal:attributes="name string:jabberid_${jabber/jabberid};
-                                   value jabber/jabberid"
-                   type="text" style="margin-bottom: 0.5em;"/>
+          <td tal:content="jabber/jabberid"
+              class="jabberid">
           </td>
 
           <td>