Code review comment for lp:~adiroiban/launchpad/bug-146178

Revision history for this message
Abel Deuring (adeuring) wrote :

This is my branch to fix from trivial issues while my brain is stuck in
first gear.

    lp:~sinzui/launchpad/oil-and-pigment
    Diff size: 489
    Launchpad bug: https://bugs.launchpad.net/bugs/436299
                   https://bugs.launchpad.net/bugs/568336
                   https://bugs.launchpad.net/bugs/130285
                   https://bugs.launchpad.net/bugs/557544
    Test command: ./bin/test -vv lp.registry.tests.test_person
        ./bin/test -t site-search -t xx-team-home -t xx-private-membership \
            -t person-karma
    Pre-implementation: no one
    Target release: 10.04

Fix from trivial issues while my brain is stuck in first gear
--------------------------------------------------------------------

Bug #436299 [Search results give wrong "Registered by" information]
    The maintainer is listed as the registrant. This is wrong. The
    problem is that distributions do not have a registrant.

Bug #568336 [Typo on profile workload page]
    There is a double 'or'

Bug #130285 [Disregard deleted projects from Most active in]
    Deactivated projects are listed in "Most active in" in the profile page
    and the links are a 404.

Bug #557544 [Bad plural on team page ("1 active members")]
    What: "1 active members" can appear on team page. This is incorrect use
    of a plural.

Rules
-----

Bug #436299 [Search results give wrong "Registered by" information]
    Verify there is a registrant using tales before appending the clause.
    tal:define="registrant view/pillar/registrant|nothing"
    tal:condition="registrant"

Bug #568336 [Typo on profile workload page]
    Remove the second 'or'.

Bug #130285 [Disregard deleted projects from lists]
    This issue in on the cusp of *not* trivial because a good understanding
    of pillar name behaviour and karma testing is needed. The fix can be
    easily placed into an existing doc test, but that is not the correct
    location.
    * pillar names (used by karma cache) have no concept of active/inactive.
      The method must filter deactivate pillars /after/ the query to get the
      names. The callsite must request more than is needed because of the
      filtering.
    * Generate the karma for a unit test of the method *and* the private
      methods that were never directly tested.
    * Test that deactivated pillars are not included.
    * ADDENDUM: While I knew how to generate the karma, the reason why
      the karma looks like it is inserted twice was not obvious from existing
      tests. I took an extra hour learning this and adding comments for
      future developers.

Bug #557544 [Bad plural on team page ("1 active members")]
    * Use the plural-message macro to switch between member and members

QA
--

Bug #436299 [Search results give wrong "Registered by" information]
    * Visit https://edge.launchpad.net/+search?field.text=wesnoth
    * Verify that Karianne Fog Heen is listed as the registrant
    * Visit https://edge.launchpad.net/+search?field.text=ubuntu
    * Verify that no registrant is listed:
      Registered on <date>

Bug #568336 [Typo on profile workload page]
    * Visit https://blueprints.edge.launchpad.net/~humphreybc/+specworkload
    * Verify text:
      Benjamin Humphrey is expected to work on, or is its creator

Bug #130285 [Disregard deleted projects from lists]
    * Visit https://edge.launchpad.net/~simon-zoellner
    * Verify that PHProute is not in "Most active in"

Bug #557544 [Bad plural on team page ("1 active members")]
    * Visit https://launchpad.net/~halfgeek.org-ixan
    * Verify the page says 1 active member

Lint
----

Linting changed files:
  lib/lp/app/stories/launchpad-root/site-search.txt
  lib/lp/app/templates/base-layout-macros.pt
  lib/lp/app/templates/launchpad-search.pt
  lib/lp/blueprints/templates/person-specworkload.pt
  lib/lp/registry/doc/person-karma.txt
  lib/lp/registry/model/person.py
  lib/lp/registry/stories/team/xx-team-home.txt
  lib/lp/registry/stories/teammembership/xx-private-membership.txt
  lib/lp/registry/templates/team-portlet-membership.pt
  lib/lp/registry/tests/test_person.py

Test
----

    * lib/lp/app/stories/launchpad-root/site-search.txt
      * Verified that distributions do not get a "registered by" line.
    * lib/lp/registry/doc/person-karma.txt
      * Moved test from doctest to unittest.
    * lib/lp/registry/stories/team/xx-team-home.txt
      * Verified singural and plural cases.
    * lib/lp/registry/stories/teammembership/xx-private-membership.txt
      * Verified singural and plural cases.
    * lib/lp/registry/tests/test_person.py
      * Lint hated this file. I removed a lot of unused imports and vars.
      * I switched the tests to run on the faster DatabaseFunctionalLayer.
      * Added a test for Person.getProjectsAndCategoriesContributedTo()
        and its private helpers. This also tests the new rule to not include
        deactivate projects.

Implementation
--------------

    * lib/lp/app/templates/base-layout-macros.pt
      * Discovered that the macro generated white-space that is inappropirate
        for anchors and punctuation.
    * lib/lp/app/templates/launchpad-search.pt
      * Updated the template to use registrant, not owner, and only print
        the "registered by" line if the pillar has a registrant.
    * lib/lp/blueprints/templates/person-specworkload.pt
      * Removed redundant "or".
    * lib/lp/registry/model/person.py
      * Request 5 extra pillar names and built a list of 5 active pillars.
    * lib/lp/registry/templates/team-portlet-membership.pt
      * Fixed plural rule for active, proposed, and pending membership.
        Sorry that this makes the template hard to read.

review: Needs Information (code)

« Back to merge proposal