Merge lp:~bac/launchpad/bug-32618 into lp:launchpad

[Brad Crittenden]

Do not allow editing of jabberids in place. Convert to be a real LaunchpadFormView.

= 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.

[Curtis Hovey (sinzui)]

Hi Brad.

I love your proposed soltution that made this issue a trival problem. I see your editor converted a tab into a spaces :).

I have an optional request. I see that the save() method is doing validation /after/ it starts making changes to the jabber ids. I think we should move the validation code to a validate mthod and make the save() method only do changes. I see there are already tests showing the expected behaviour, and I expect this proposed refactoring to pass:

    def validate(self, data):
        jabberid = data.get('newjabberid')
        if jabberid is not None:
            jabberset = getUtility(IJabberIDSet)
            existingjabber = jabberset.getByJabberID(jabberid)
            if existingjabber.person != self.context:
                self.request.response.addErrorNotification(
                    structured(
                        'The Jabber ID %s is already registered by '
                        '<a href="%s">%s</a>.',
                        jabberid, canonical_url(existingjabber.person),
                        existingjabber.person.displayname))
            else:
                self.request.response.addErrorNotification(
                    'The Jabber ID %s already belongs to you.' % jabberid)
...
    @action(_("Save Changes"), name="save")
    def save(self, action, data):
...
        jabberid = form.get('newjabberid')
        if jabberid is not None:
            jabberset.new(self.context, jabberid)

review: Approve (code)

[Brad Crittenden (bac)]

Thanks for the comments, Curtis. I converted the form to be a proper LaunchpadFormView with the validation method. This also fixed bug 615872.

Incremental at http://pastebin.ubuntu.com/475950/

[Curtis Hovey (sinzui)]

I did not mean to cause any consternation. I really appreciate your fix to this old code and that this also addresses another bug.

=== modified file 'lib/canonical/launchpad/utilities/gpghandler.py'
--- lib/canonical/launchpad/utilities/gpghandler.py	2010-05-15 17:43:59 +0000
+++ lib/canonical/launchpad/utilities/gpghandler.py	2010-08-10 16:19:43 +0000
@@ -163,10 +163,10 @@
                 # XXX: 2010-04-26, Salgado, bug=570244: This hack is needed
                 # for python2.5 compatibility. We should remove it when we no
                 # longer need to run on python2.5.
-                if hasattr(e, 'message'):
+                if hasattr(e, 'strerror'):
+                    msg = e.strerror
+                else:
                     msg = e.message
-                else:
-                    msg = e.strerror
                 raise GPGVerificationError(msg)
         else:
             # store clearsigned signature
@@ -180,10 +180,10 @@
                 # XXX: 2010-04-26, Salgado, bug=570244: This hack is needed
                 # for python2.5 compatibility. We should remove it when we no
                 # longer need to run on python2.5.
-                if hasattr(e, 'message'):
+                if hasattr(e, 'strerror'):
+                    msg = e.strerror
+                else:
                     msg = e.message
-                else:
-                    msg = e.strerror
                 raise GPGVerificationError(msg)
 
         # XXX jamesh 2006-01-31:

=== 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-10 16:19:43 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0211,E0213,C0322
@@ -156,7 +156,7 @@
 from lp.registry.interfaces.codeofconduct import ISignedCodeOfConductSet
 from lp.registry.interfaces.gpg import IGPGKeySet
 from lp.registry.interfaces.irc import IIrcIDSet
-from lp.registry.interfaces.jabber import IJabberIDSet
+from lp.registry.interfaces.jabber import IJabberID, IJabberIDSet
 from lp.registry.interfaces.mailinglist import (
     CannotUnsubscribe, IMailingListSet)
 from lp.registry.interfaces.mailinglistsubscription import (
@@ -3520,7 +3520,18 @@
 
 
 class PersonEditJabberIDsView(LaunchpadFormView):
-    schema = Interface
+
+    schema = IJabberID
+    field_names = ['jabberid']
+
+    def setUpFields(self):
+        super(PersonEditJabberIDsView, self).setUpFields()
+        if self.context.jabberids.count() > 0:
+            # Make the jabberid entry optional on the edit page if one or more
+            # ids already exist, which allows the removal of ids without
+            # filling out the new jabberid field.
+            jabber_field = self.form_fields['jabberid']
+            jabber_field.field.required = False
 
     @property
     def page_title(self):
@@ -3529,42 +3540,42 @@
     label = page_title
 
     @property
-    def cancel_url(self):
+    def next_url(self):
         return canonical_url(self.context)
 
+    cancel_url = next_url
+
+    def validate(self, data):
+        """Ensure the edited data does not already exist."""
+        jabberid = data.get('jabberid')
+        if jabberid is not None:
+            jabberset = getUtility(IJabberIDSet)
+            existingjabber = jabberset.getByJabberID(jabberid)
+            if existingjabber is not None:
+                if existingjabber.person != self.context:
+                    self.setFieldError(
+                        'jabberid',
+                        structured(
+                            'The Jabber ID %s is already registered by '
+                            '<a href="%s">%s</a>.',
+                            jabberid, canonical_url(existingjabber.person),
+                            existingjabber.person.displayname))
+                else:
+                    self.setFieldError(
+                        'jabberid',
+                        'The Jabber ID %s already belongs to you.' % jabberid)
+
     @action(_("Save Changes"), name="save")
     def save(self, action, data):
         """Process the Jabber ID form."""
-        # XXX: EdwinGrubbs 2009-09-01 bug=422784
-        # This view should use schema and form validation.
         form = self.request.form
         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:
+        jabberid = data.get('jabberid')
+        if jabberid is not None:
             jabberset = getUtility(IJabberIDSet)
-            existingjabber = jabberset.getByJabberID(jabberid)
-            if existingjabber is None:
-                jabberset.new(self.context, jabberid)
-            elif existingjabber.person != self.context:
-                self.request.response.addErrorNotification(
-                    structured(
-                        'The Jabber ID %s is already registered by '
-                        '<a href="%s">%s</a>.',
-                        jabberid, canonical_url(existingjabber.person),
-                        existingjabber.person.displayname))
-            else:
-                self.request.response.addErrorNotification(
-                    'The Jabber ID %s already belongs to you.' % jabberid)
+            jabberset.new(self.context, jabberid)
 
 
 class PersonEditSSHKeysView(LaunchpadView):
@@ -3600,8 +3611,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/interfaces/jabber.py'
--- lib/lp/registry/interfaces/jabber.py	2009-07-17 00:26:05 +0000
+++ lib/lp/registry/interfaces/jabber.py	2010-08-10 16:19:43 +0000
@@ -33,7 +33,7 @@
         Reference(
             title=_("Owner"), required=True, schema=Interface, readonly=True))
     jabberid = exported(
-        TextLine(title=_("Jabber user ID"), required=True))
+        TextLine(title=_("New Jabber user ID"), required=True))
 
     def destroySelf():
         """Delete this JabberID from the database."""
@@ -50,4 +50,3 @@
 
     def getByPerson(person):
         """Return all JabberIDs for the given person."""
-

=== 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-10 16:19:43 +0000
@@ -1,8 +1,12 @@
-= Jabber IDs =
+==========
+Jabber IDs
+==========
+
 
 In their Launchpad profile, users can register their Jabber IDs.
 
-== Adding and editing an ID ==
+Adding and editing an ID
+------------------------
 
 To register a Jabber ID with his account, the user visits his
 profile page and uses the 'Update Jabber IDs' link.
@@ -15,50 +19,46 @@
 The user enters the Jabber ID in the text field and clicks on the
 'Save Changes' button.
 
-    >>> user_browser.getControl(name='newjabberid').value = '[e-mail]'
+    >>> user_browser.getControl(name='field.jabberid').value = (
+    ...     '[e-mail]')
     >>> user_browser.getControl('Save Changes').click()
 
 In this case, the user tried registering a jabber ID that was already
 registered by someone else. Since only one person can use a Jabber ID,
 an error is displayed and the user can enter another one:
 
-    >>> for error in find_tags_by_class(user_browser.contents, 'error'):
-    ...     print extract_text(error)
+    >>> def show_errors(browser):
+    ...     for error in find_tags_by_class(browser.contents, 'error'):
+    ...         print extract_text(error)
+    >>> show_errors(user_browser)
+    There is 1 error.
+    New Jabber user ID:
     The Jabber ID [e-mail] is already registered by Jeff Waugh.
 
 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 = '[e-mail]'
-    >>> 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='[e-mail]').value = (
+    >>> user_browser.getControl(name='field.jabberid').value = (
     ...     '[e-mail]')
-    >>> user_browser.getControl(name='newjabberid').value = (
-    ...     '[e-mail]')
     >>> 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']
-    [e-mail]
+    >>> show_errors(user_browser)
+
+    >>> def show_jabberids(browser):
+    ...     tags = find_tag_by_id(browser.contents, 'jabber-ids')
+    ...     for dd in tags.findAll('dd'):
+    ...         print extract_text(dd)
+
+    >>> show_jabberids(user_browser)
     [e-mail]
 
-== Removing an ID ==
+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.getLink('Update Jabber IDs').click()
     >>> 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']
-    [e-mail]
+    >>> show_jabberids(user_browser)
+    No Jabber IDs registered.

=== 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-10 16:19:43 +0000
@@ -13,17 +13,15 @@
 
     <table>
 
-      <div tal:condition="context/jabberids">
+      <tal:existing_jabber condition="context/jabberids">
 
         <tr>
           <td><label>Existing Jabber IDs:</label></td>
         </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>
@@ -34,23 +32,16 @@
             </label>
           </td>
         </tr>
-
-      </div>
-
-    <tr tal:condition="context/jabberids">
-      <td><label>New Jabber ID:</label></td>
-    </tr>
-
-    <tr>
-      <td>
-        <input name="newjabberid" type="text" />
-      </td>
-    </tr>
-
-    <tr>
-      <td class="formHelp">Example: [e-mail]</td>
-    </tr>
-  </table>
+      </tal:existing_jabber>
+
+      <tal:widget define="widget nocall:view/widgets/jabberid">
+        <metal:block use-macro="context/@@launchpad_form/widget_row" />
+      </tal:widget>
+
+      <tr>
+        <td class="formHelp">Example: [e-mail]</td>
+      </tr>
+    </table>
 
   </div>
 </div>