Code review comment for lp:~bac/launchpad/lep-projconfig

Hi Brad.

This change looks great. I see I see extra white space between the download
portlet and announcements.

I think we need a model change to support this :( I an configure Answers to
not be official and my application may not be translatable. So the progress
bar will never be complete. Consider Launchpad. It does not have a po file,
so it cannot be translated, but the UI requires me to set translations to be
official to make the bar be complete. I think we need to None, True, or False
to make this work. I am seeing this same issue my the feature I am working
on. I propose we switch from bool to a vocab or UNKNOWN, LAUNCHPAD, EXTERNAL.
We may only need this some of the apps.

I have a lot of concern about the implementation. I do not think ILink, its
model, and template needed to change. I certainly do not think we should
be making invalid HTML on all Launchpad pages for a single portlet that
appears on one page. I think the fix is easy, but I have not spent a lot
of time looking at the issue.

> === modified file 'lib/canonical/launchpad/icing/style-3-0.css.in'
> --- lib/canonical/launchpad/icing/style-3-0.css.in 2010-07-26 14:48:43 +0000
> +++ lib/canonical/launchpad/icing/style-3-0.css.in 2010-07-26 22:01:07 +0000
> @@ -686,6 +686,13 @@
> margin: 0;
> padding: 0;
> }
> +div.centered {
> + text-align: center;
> + }
> +div.centered table {
> + margin: 0 auto;
> + text-align: left;
> + }

We used to have rules like this for the 1.0 layouts. I guess It was naive
of me to remove them.

> === modified file 'lib/canonical/launchpad/templates/launchpad-inline-link.pt'
> --- lib/canonical/launchpad/templates/launchpad-inline-link.pt 2009-08-27 02:22:07 +0000
> +++ lib/canonical/launchpad/templates/launchpad-inline-link.pt 2010-07-26 22:01:07 +0000
> @@ -2,8 +2,10 @@
> xmlns:tal="http://xml.zope.org/namespaces/tal"
> condition="context/enabled">
> <tal:link-linked
> - condition="context/linked"><a
> - href="" class="" title=""
> + condition="context/linked">
> + <table width="100%"><tr>
> + <td width="90%">
> + <a href="" class="" title=""

This looks scary. I think it is wrong. This file is used to adapt every link,
and most links are inline text.

/me runs lp

This cannot land. It is create tables in paragraphs and inline markup that
cannot contain tables.

Since we are designing a presentation that will appear on exactly one page
and we have not discussed reusing it. I expected to see either a subclass
or some form of delegates so that the portlet's needs did not step on any
other part of launchpad. The Involvement portlet is already a bundle of
exceptions for menus and does its own layout. I think the view and existing
template could be doing all the work.

Maybe we should have looked closer at why the Involvement used CSS to place
a right aligned background image.

...

> === modified file 'lib/canonical/launchpad/webapp/interfaces.py'
> --- lib/canonical/launchpad/webapp/interfaces.py 2010-07-21 08:15:57 +0000
> +++ lib/canonical/launchpad/webapp/interfaces.py 2010-07-26 22:01:07 +0000
> @@ -238,6 +238,10 @@
> icon_url = Attribute(
> "The full URL for this link's associated icon, if it has one.")
>
> + configured = Attribute(
> + "Whether the item has been configured yet. If None, no indicator is"
> + "used. Otherwise it is a boolean.")
> +
> def render():
> """Return a HTML representation of the link."""

We have one use case for this.
/me thinks the 'menu' arg is not used in Lp anymore.

> === modified file 'lib/canonical/launchpad/webapp/menu.py'
> --- lib/canonical/launchpad/webapp/menu.py 2010-06-15 19:35:41 +0000
> +++ lib/canonical/launchpad/webapp/menu.py 2010-07-26 22:01:07 +0000

...

> @@ -121,6 +121,9 @@
> 'blueprint' for a specific site.
>
> :param menu: The sub menu used by the page that the link represents.
> +
> + :parem configured: Whether the item has been configured. If not None,
> + then show a boolean indicator for configured or not configured.

grammar: s/parem/param/

...

> @@ -171,6 +175,13 @@
> else:
> return cgi.escape(text)
>
> + def set_configured(self, value):
> + self._linkdata.configured = value
> + def get_configured(self):
> + return self._linkdata.configured
> +
> + configured = property(get_configured, set_configured)

PEP 8 will remind you to have a blank line between methods.

I do not think these changes are needed.

...

> === modified file 'lib/lp/registry/browser/product.py'
> --- lib/lp/registry/browser/product.py 2010-07-09 10:22:32 +0000
> +++ lib/lp/registry/browser/product.py 2010-07-26 22:01:07 +0000
> @@ -349,7 +349,33 @@
> """Encourage configuration of involvement links for projects."""
>
> has_involvement = True
> - visible_disabled_link_names = ['submit_code']

Why was this removed? Does this relate to the issue that we were not
seeing a strong indication that no one every configured translations, etc...?

> + show_progress = True
> +
> + @property
> + def visible_disabled_link_names(self):
> + """Show all disabled links...except blueprints"""
> + involved_menu = MenuAPI(self).navigation
> + all_links = involved_menu.keys()
> + # The register blueprints link should not be shown since its use is
> + # not encouraged.
> + all_links.remove('register_blueprint')
> + return all_links
> +
> + @cachedproperty
> + def configuration_states(self):
> + """Create a dictionary indicating the configuration statuses.
> +
> + Each app area will be represented in the return dictionary, except
> + blueprints which we are not currently promoting.
> + """
> + states = {}
> + states['configure_bugtracker'] = (
> + self.official_malone or self.context.bugtracker is not None)
> + states['configure_answers'] = self.official_answers
> + states['configure_translations'] = self.official_rosetta
> + states['configure_codehosting'] = (
> + self.context.development_focus.branch is not None)
> + return states

I think this work shows that the view is really doing the work and since
it has a special template for rendering itself, ILink and models and templates
do not need to change.

...

> === modified file 'lib/lp/registry/stories/product/xx-product-files.txt'
> --- lib/lp/registry/stories/product/xx-product-files.txt 2010-01-15 21:44:23 +0000
> +++ lib/lp/registry/stories/product/xx-product-files.txt 2010-07-26 22:01:07 +0000

Revert this change

> === modified file 'lib/lp/registry/templates/pillar-involvement-portlet.pt'
> --- lib/lp/registry/templates/pillar-involvement-portlet.pt 2010-06-15 19:37:37 +0000
> +++ lib/lp/registry/templates/pillar-involvement-portlet.pt 2010-07-26 22:01:07 +0000
> @@ -31,6 +31,29 @@
> </tal:disabled>
> </ul>
>
> + <tal:editor tal:condition="context/required:launchpad.Edit">
> + <tal:show_configuration
> + tal:condition="registration"
> + tal:define="registration view/registration_completeness">
> + <h2>Configuration Progress</h2>
> + <div class="centered">
> + <table width="80%" border="1">

Width and border attributes are deprecated. Use style.

I think this is the one template that needs to be making these special links:
I see:

  <ul tal:condition="view/configuration_links" style="padding-top: 1em">
    <li tal:repeat="link view/configuration_links">
      <a tal:replace="structure link/fmt:link" />
    </li>
  </ul>

If view/configuration_links returned a dict of link and state then:

  <ul tal:condition="view/configuration_links" style="padding-top: 1em">
    <li tal:repeat="configured_link view/configuration_links">
      <table>
        <tr>
          <td>
            <a tal:replace="structure configured_link/link/fmt:link" />
          </td>
          <td>
            <img src="/@@/${configured_link/configured}" />
          </td>
    </li>
  </ul>

Or using css:

  <ul tal:condition="view/configuration_links" style="padding-top: 1em">
    <li tal:repeat="configured_link view/configuration_links"
      attributes="class configured_link/css_state">
      <a tal:replace="structure configured_link/link/fmt:link" />
    </li>
  </ul>

...

> === modified file 'lib/lp/registry/tests/test_product.py'
> --- lib/lp/registry/tests/test_product.py 2010-04-21 16:15:36 +0000
> +++ lib/lp/registry/tests/test_product.py 2010-07-26 22:01:07 +0000

Revert this.