On Tue, 2009-09-15 at 20:45 +0000, Curtis Hovey wrote:
> Review: Needs Fixing code
> Hi Salgado.
> 
> This branch is great. I think we need to add a lot of links to the info section
> and because of this I have not marked this branch approved. We already discussed
> adding the oauth link on IRC, as well as moving content out of the heading slot.
> 
> > === modified file 'lib/lp/registry/browser/person.py'
> > --- lib/lp/registry/browser/person.py	2009-09-15 01:13:15 +0000
> > +++ lib/lp/registry/browser/person.py	2009-09-15 14:32:37 +0000
> 
> 
> ...
> 
> 
> > @@ -5670,6 +5654,17 @@
> >              return 'Contact this user'
> >  
> >  
> > +class IPersonIndexMenu(Interface):
> > +    """A marker interface for the +index navigation menu."""
> > +
> > +
> > +class PersonIndexMenu(NavigationMenu, PersonMenuMixin):
> > +    usedfor = IPersonIndexMenu
> > +    facet = 'overview'
> > +    title = 'Change person'
> > +    links = ('edit', 'administer')
> 
> 
> Add Change password to this. Any edit form we cannot work into the
> page should be in here. Change branding too. I think we can inline all
> others

Good catch.  Both added.

> 
> 
> > === modified file 'lib/lp/registry/templates/person-index.pt'
> > --- lib/lp/registry/templates/person-index.pt	2009-07-20 15:27:26 +0000
> > +++ lib/lp/registry/templates/person-index.pt	2009-09-15 18:02:40 +0000

> > +<div metal:fill-slot="heading"
> > +     tal:condition="context/is_valid_person_or_team">
> > +  <div
> > +    class="description"
> > +    tal:condition="context/homepage_content"
> > +    tal:content="structure context/homepage_content/fmt:text-to-html"
> > +  />
> 
> I wonder if we should put a link to edit the homepage content? No I think
> not, I expect this link to be off of the edit page...near where I set
> the description.

Actually, the homepage content can be edited from the +edit page, which
makes +edithomepage redundant: <https://bugs.launchpad.net/bugs/430680>

> 
> > +  <ul class="horizontal">
> > +    <li
> > +      tal:define="link context/menu:overview/projects"
> > +      tal:condition="link/enabled"
> > +      tal:content="structure link/fmt:link" />
> > +    <li
> > +      tal:define="link context/menu:overview/maintained"
> > +      tal:condition="link/enabled"
> > +      tal:content="structure link/fmt:link" />
> > +    <li
> > +      tal:define="link context/menu:overview/ppa"
> > +      tal:condition="link/enabled"
> > +      tal:content="structure link/fmt:link" />
> > +  </ul>
> 
> Please add a link for the user to manage his oauth-tokens. This will fix
> Bug #316731: [provide a link to the +oauth-tokens page on users page]

Added. Shown only when the user has launchpad.Edit and there are any
claimed/unclaimed tokens for the context.

> 
> 
> > +</div>
> > +
> > +<div metal:fill-slot="main"
> >    tal:define="overview_menu context/menu:overview">
> 
> ...
> 
> 
> The heading-slot is deprecated since barry has solved the header issue. We
> want to remove it. I don't think it should be reused for non-heading content.
> 
> Move the person/team narrative and list of links here, as the first
> content in the main slot, above the first <div class="yui-g">

Right, I did that yesterday after we discussed it on IRC.

> 
> 
> +    <div class="yui-g">
> 
> +      <div tal:content="structure context/@@+portlet-contact-details"/>
> 
> +    </div>
> 
> +
> 
> +    <div tal:content="structure context/@@+portlet-currentfocus"/>
> 
> +
> 
> +    <div class="yui-g">
> 
> +      <!-- First portlet column. -->
> 
> +      <div class="first yui-u">
> 
> +        <div tal:content="structure context/@@+portlet-ppas"/>
> 
> +        <div tal:content="structure context/@@+portlet-memberships"/>
> 
> +      </div>
> 
> +      <div class="yui-u">
> 
> +        <div tal:content="structure context/@@+portlet-contributions"/>
> 
> +
> 
> +      </div>
> 
> +    </div>
> 
> 
> I don't think placing ppas before memberships is ideal because when all
> three porlets show up, they will never align most users will have 1 paa,
> but will have more than one contribution.
> 
> Oh, bother. We can land this as is. I think this is easy to solve after
> we see this with real content.

Heh, okay.

> 
> 
> > === modified file 'lib/lp/registry/templates/person-portlet-contact-details.pt'
> > --- lib/lp/registry/templates/person-portlet-contact-details.pt	2009-07-17 17:59:07 +0000
> > +++ lib/lp/registry/templates/person-portlet-contact-details.pt	2009-09-15 18:02:40 +0000
> ...
> 
> 
> > +     tal:define="overview_menu context/menu:overview">
> > +
> > +  <h2>User information</h2>
> 
> 
> The links to edit these bits should be inline. I think that means the links
> in the edit menu need to be shared

Yeah, I've moved branding() and editpassword() to PersonMenuMixin so
that they can be used on the edit menu.

> 
> I am not certain if we can put this at the end of the list or we need to
> do something like we do for sshkeys
> 
>     <tal:first condition="repeat/sshkey/start">
>         <make link>
>     </tal:first>

I'd never noticed this, but I don't think having the edit icon beside
only one (the first) entry is a good idea.  I'd be in favor of rendering
the link (with its text) below all entries, but even better might be to
render just the icon inside the <dt>, with some explanatory text for
when there are no entries to show in that section:
<http://people.canonical.com/~salgado/person4.png>

Also, inlining the edit links here means we'll need to change the logic
to decide when to render such sections -- currently they're only
rendered when there are items to show but now we'll have to render them
when the logged in user has launchpad.Edit on the context.

The diff below does all that. Let me know what you think of it.

=== modified file 'lib/lp/registry/browser/person.py'
--- lib/lp/registry/browser/person.py	2009-09-15 14:32:37 +0000
+++ lib/lp/registry/browser/person.py	2009-09-16 13:12:48 +0000
@@ -902,6 +902,18 @@
 class PersonMenuMixin(CommonMenuLinks):
 
     @enabled_with_permission('launchpad.Edit')
+    def branding(self):
+        target = '+branding'
+        text = 'Change branding'
+        return Link(target, text, icon='edit')
+
+    @enabled_with_permission('launchpad.Edit')
+    def editpassword(self):
+        target = '+changepassword'
+        text = 'Change your password'
+        return Link(target, text, icon='edit')
+
+    @enabled_with_permission('launchpad.Edit')
     def edit(self):
         target = '+edit'
         text = 'Change details'
@@ -924,13 +936,16 @@
              'editsshkeys', 'editpgpkeys', 'editlocation', 'memberships',
              'mentoringoffers', 'codesofconduct', 'karma',
              'administer', 'projects', 'activate_ppa', 'maintained',
-             'view_ppa_subscriptions', 'ppa']
+             'view_ppa_subscriptions', 'ppa', 'oauth_tokens']
 
     @enabled_with_permission('launchpad.Edit')
-    def branding(self):
-        target = '+branding'
-        text = 'Change branding'
-        return Link(target, text, icon='edit')
+    def oauth_tokens(self):
+        target = '+oauth-tokens'
+        text = 'Authorized applications'
+        access_tokens = self.context.oauth_access_tokens
+        request_tokens = self.context.oauth_request_tokens
+        enabled = bool(access_tokens or request_tokens)
+        return Link(target, text, enabled=enabled, icon='info')
 
     @enabled_with_permission('launchpad.Edit')
     def editlanguages(self):
@@ -962,12 +977,6 @@
         text = 'Update Jabber IDs'
         return Link(target, text, icon='edit')
 
-    @enabled_with_permission('launchpad.Edit')
-    def editpassword(self):
-        target = '+changepassword'
-        text = 'Change your password'
-        return Link(target, text, icon='edit')
-
     @enabled_with_permission('launchpad.EditLocation')
     def editlocation(self):
         target = '+editlocation'
@@ -2575,6 +2584,56 @@
 class PersonView(LaunchpadView, FeedsMixin, TeamJoinMixin):
     """A View class used in almost all Person's pages."""
 
+    @property
+    def should_show_ircnicknames_section(self):
+        """Should the 'IRC nicknames' section be shown?
+
+        It's shown when the person has IRC nicknames registered or has rights
+        to register new ones.
+        """
+        return bool(self.context.ircnicknames) or (
+            check_permission('launchpad.Edit', self.context))
+
+    @property
+    def should_show_wikinames_section(self):
+        """Should the 'Wiki names' section be shown?
+
+        It's shown when the person has Wiki names registered or has rights
+        to register new ones.
+        """
+        return not self.context.wiki_names.is_empty() or (
+            check_permission('launchpad.Edit', self.context))
+
+    @property
+    def should_show_jabberids_section(self):
+        """Should the 'Jabber IDs' section be shown?
+
+        It's shown when the person has Jabber IDs registered or has rights
+        to register new ones.
+        """
+        return bool(self.context.jabberids) or (
+            check_permission('launchpad.Edit', self.context))
+
+    @property
+    def should_show_sshkeys_section(self):
+        """Should the 'SSH keys' section be shown?
+
+        It's shown when the person has SSH keys registered or has rights
+        to register new ones.
+        """
+        return bool(self.context.sshkeys) or (
+            check_permission('launchpad.Edit', self.context))
+
+    @property
+    def should_show_gpgkeys_section(self):
+        """Should the 'OpenPGP keys' section be shown?
+
+        It's shown when the person has OpenPGP keys registered or has rights
+        to register new ones.
+        """
+        return bool(self.context.gpgkeys) or (
+            check_permission('launchpad.Edit', self.context))
+
     @cachedproperty
     def recently_approved_members(self):
         members = self.context.getMembersByStatus(
@@ -2908,7 +2967,7 @@
 
     @cachedproperty
     def languages(self):
-        """The user's preferred languages, or English is none are set."""
+        """The user's preferred languages, or English if none are set."""
         languages = list(self.context.languages)
         if len(languages) > 0:
             englishnames = [language.englishname for language in languages]
@@ -5662,7 +5721,7 @@
     usedfor = IPersonIndexMenu
     facet = 'overview'
     title = 'Change person'
-    links = ('edit', 'administer')
+    links = ('edit', 'administer', 'branding', 'editpassword')
 
 
 class ITeamIndexMenu(Interface):

=== modified file 'lib/lp/registry/templates/person-index.pt'
--- lib/lp/registry/templates/person-index.pt	2009-09-15 18:02:40 +0000
+++ lib/lp/registry/templates/person-index.pt	2009-09-15 20:55:11 +0000
@@ -82,33 +82,34 @@
   </div>
 </div>
 
-<div metal:fill-slot="heading"
-     tal:condition="context/is_valid_person_or_team">
-  <div
-    class="description"
-    tal:condition="context/homepage_content"
-    tal:content="structure context/homepage_content/fmt:text-to-html"
-  />
-  <ul class="horizontal">
-    <li
-      tal:define="link context/menu:overview/projects"
-      tal:condition="link/enabled"
-      tal:content="structure link/fmt:link" />
-    <li
-      tal:define="link context/menu:overview/maintained"
-      tal:condition="link/enabled"
-      tal:content="structure link/fmt:link" />
-    <li
-      tal:define="link context/menu:overview/ppa"
-      tal:condition="link/enabled"
-      tal:content="structure link/fmt:link" />
-  </ul>
-</div>
-
 <div metal:fill-slot="main"
   tal:define="overview_menu context/menu:overview">
   <tal:is-valid-person tal:condition="context/is_valid_person">
 
+    <div
+      class="description"
+      tal:condition="context/homepage_content"
+      tal:content="structure context/homepage_content/fmt:text-to-html"
+    />
+    <ul class="horizontal">
+      <li
+        tal:define="link context/menu:overview/projects"
+        tal:condition="link/enabled"
+        tal:content="structure link/fmt:link" />
+      <li
+        tal:define="link context/menu:overview/maintained"
+        tal:condition="link/enabled"
+        tal:content="structure link/fmt:link" />
+      <li
+        tal:define="link context/menu:overview/ppa"
+        tal:condition="link/enabled"
+        tal:content="structure link/fmt:link" />
+      <li
+        tal:define="link context/menu:overview/oauth_tokens"
+        tal:condition="link/enabled"
+        tal:content="structure link/fmt:link" />
+    </ul>
+
     <div class="yui-g">
       <div tal:content="structure context/@@+portlet-contact-details"/>
     </div>

=== modified file 'lib/lp/registry/templates/person-portlet-contact-details.pt'
--- lib/lp/registry/templates/person-portlet-contact-details.pt	2009-09-15 18:02:40 +0000
+++ lib/lp/registry/templates/person-portlet-contact-details.pt	2009-09-16 13:19:13 +0000
@@ -11,7 +11,9 @@
 
   <div class="first yui-u">
     <dl>
-      <dt>Email:</dt>
+      <dt>Email:
+        <a tal:replace="structure overview_menu/editemailaddresses/fmt:icon" />
+      </dt>
       <dd
         tal:attributes="title view/visible_email_address_description">
         <tal:not_logged_in
@@ -45,13 +47,18 @@
       </dd>
     </dl>
 
-    <dl tal:condition="context/jabberids">
-      <dt>Jabber:</dt>
+    <dl tal:condition="view/should_show_jabberids_section">
+      <dt>Jabber:
+        <a tal:replace="structure overview_menu/editjabberids/fmt:icon" />
+      </dt>
       <dd>
         <tal:block repeat="jabberid context/jabberids">
           <span tal:replace="jabberid/jabberid/fmt:obfuscate-email"
             /><span tal:condition="not: repeat/jabberid/end">,</span>
         </tal:block>
+        <div tal:condition="not: context/jabberids">
+          No Jabber IDs registered.
+        </div>
       </dd>
     </dl>
 
@@ -109,30 +116,41 @@
   </div>
 
   <div class="yui-u two-column-list">
-    <dl tal:condition="not: context/wiki_names/is_empty">
-      <dt>Wiki:</dt>
+    <dl tal:condition="view/should_show_wikinames_section">
+      <dt>Wiki:
+        <a tal:replace="structure overview_menu/editwikinames/fmt:icon" />
+      </dt>
       <dd>
-        <span tal:repeat="wiki context/wiki_names">
+        <div tal:repeat="wiki context/wiki_names">
           <a tal:content="wiki/url"
              tal:attributes="href wiki/url"
-           >WikiName</a><tal:block condition="not: repeat/wiki/end">,
-           </tal:block>
-        </span>
+           >WikiName</a>
+        </div>
+        <div tal:condition="not: context/wiki_names">
+          No Wiki names registered.
+        </div>
       </dd>
     </dl>
-    <dl tal:condition="context/ircnicknames">
-      <dt>IRC:</dt>
+    <dl tal:condition="view/should_show_ircnicknames_section">
+      <dt>IRC:
+        <a tal:replace="structure overview_menu/editircnicknames/fmt:icon" />
+      </dt>
       <dd>
         <div tal:repeat="ircnick context/ircnicknames">
           <strong tal:content="ircnick/nickname"/>
           <span class="discreet">on network</span>
           <strong tal:content="ircnick/network" />
         </div>
+        <div tal:condition="not: context/ircnicknames">
+          No IRC nicknames registered.
+        </div>
       </dd>
     </dl>
 
-    <dl tal:condition="context/gpgkeys">
-      <dt>OpenPGP keys:</dt>
+    <dl tal:condition="view/should_show_gpgkeys_section">
+      <dt>OpenPGP keys:
+        <a tal:replace="structure overview_menu/editpgpkeys/fmt:icon" />
+      </dt>
       <dd>
         <tal:keys repeat="key context/gpgkeys">
           <a tal:attributes="href key/keyserverURL"
@@ -140,45 +158,42 @@
             <tal:keyid replace="key/keyid"
               /></a><span tal:condition="not: repeat/key/end">,</span>
         </tal:keys>
+        <div tal:condition="not: context/gpgkeys">
+          No OpenPGP keys registered.
+        </div>
       </dd>
     </dl>
 
-    <dl id="sshkeys">
-      <dt>SSH Keys:</dt>
+    <dl id="sshkeys" tal:condition="view/should_show_sshkeys_section">
+      <dt>SSH Keys:
+        <a tal:replace="structure overview_menu/editsshkeys/fmt:icon" />
+      </dt>
       <dd tal:define="sshkeys context/sshkeys">
-        <tal:no_keys condition="not: sshkeys">
-          <tal:is_user condition="view/viewing_own_page">
-            <a tal:replace="structure overview_menu/editsshkeys/fmt:icon-link" />
-          </tal:is_user>
-          <tal:other_user condition="not: view/viewing_own_page">
-            No keys registered
-          </tal:other_user>
-        </tal:no_keys>
-
-        <tal:has_keys condition="sshkeys">
-          <tal:keys repeat="sshkey sshkeys">
-            <a href="+sshkeys"><tal:key replace="sshkey/comment" /></a>
-            <tal:first condition="repeat/sshkey/start">
-              <a tal:replace="structure overview_menu/editsshkeys/fmt:icon" />
-            </tal:first>
-            <br />
-          </tal:keys>
-        </tal:has_keys>
+        <tal:keys repeat="sshkey sshkeys">
+          <a href="+sshkeys" tal:content="sshkey/comment">foo@bar</a>
+          <br />
+        </tal:keys>
+        <div tal:condition="not: context/sshkeys">
+          No SSH keys registered.
+        </div>
       </dd>
     </dl>
 
     <dl id="languages">
-      <dt>Languages:</dt>
+      <dt>Languages:
+        <a tal:replace="structure overview_menu/editlanguages/fmt:icon" />
+      </dt>
       <dd>
         <tal:languages content="view/languages">
           English
         </tal:languages>
-        <a tal:replace="structure overview_menu/editlanguages/fmt:icon" />
       </dd>
     </dl>
 
     <dl tal:condition="context/time_zone">
-      <dt>Time zone:</dt>
+      <dt>Time zone:
+        <a tal:replace="structure overview_menu/editlocation/fmt:icon" />
+      </dt>
       <dd tal:content="context/time_zone">UTC</dd>
     </dl>
 

=== modified file 'lib/lp/registry/templates/team-index.pt'
--- lib/lp/registry/templates/team-index.pt	2009-09-09 19:37:48 +0000
+++ lib/lp/registry/templates/team-index.pt	2009-09-15 19:38:05 +0000
@@ -22,7 +22,6 @@
 
 <body>
 
-<!-- side slot -->
 <div metal:fill-slot="side"
      tal:define="overview_menu context/menu:overview">
 
@@ -47,44 +46,40 @@
 
 </div>
 
-<!-- heading slot. -->
-<div metal:fill-slot="heading">
-    <div tal:attributes="class view/public_private_css">
-      <tal:comment condition="nothing">
-        Display striped image.
-      </tal:comment>
-    </div>
-
-    <div
-      class="description"
-      tal:condition="context/homepage_content"
-      tal:content="structure context/homepage_content/fmt:text-to-html"
-    />
-
-    <div
-      class="description"
-      tal:content="structure context/teamdescription/fmt:text-to-html"
-    />
-    <ul class="horizontal">
-      <li
-        tal:define="link context/menu:overview/projects"
-        tal:condition="link/enabled"
-        tal:content="structure link/fmt:link" />
-      <li
-        tal:define="link context/menu:overview/maintained"
-        tal:condition="link/enabled"
-        tal:content="structure link/fmt:link" />
-      <li
-        tal:define="link context/menu:overview/ppa"
-        tal:condition="link/enabled"
-        tal:content="structure link/fmt:link" />
-    </ul>
-</div>
-
-<!-- main slot. -->
 <div metal:fill-slot="main"
      tal:define="overview_menu context/menu:overview">
 
+  <div tal:attributes="class view/public_private_css">
+    <tal:comment condition="nothing">
+      Display striped image.
+    </tal:comment>
+  </div>
+
+  <div
+    class="description"
+    tal:condition="context/homepage_content"
+    tal:content="structure context/homepage_content/fmt:text-to-html"
+  />
+
+  <div
+    class="description"
+    tal:content="structure context/teamdescription/fmt:text-to-html"
+  />
+  <ul class="horizontal">
+    <li
+      tal:define="link context/menu:overview/projects"
+      tal:condition="link/enabled"
+      tal:content="structure link/fmt:link" />
+    <li
+      tal:define="link context/menu:overview/maintained"
+      tal:condition="link/enabled"
+      tal:content="structure link/fmt:link" />
+    <li
+      tal:define="link context/menu:overview/ppa"
+      tal:condition="link/enabled"
+      tal:content="structure link/fmt:link" />
+  </ul>
+
   <div class="yui-g">
     <div class="first yui-u">
       <metal:details use-macro="context/@@+person-macros/team-details" />


-- 
Guilherme Salgado <email address hidden>