Merge lp:~sinzui/launchpad/dsp-vocab-ui into lp:launchpad

Proposed by Curtis Hovey
Status: Merged
Merged at revision: 14104
Proposed branch: lp:~sinzui/launchpad/dsp-vocab-ui
Merge into: lp:launchpad
Prerequisite: lp:~sinzui/launchpad/dsp-historic-attributes
Diff against target: 0 lines
To merge this branch: bzr merge lp:~sinzui/launchpad/dsp-vocab-ui
Reviewer Review Type Date Requested Status
Steve Kowalik (community) code Needs Information
Review via email: mp+78194@code.launchpad.net

Description of the change

Update the DSP picker UI.

    Launchpad bug: https://bugs.launchpad.net/bugs/867718
    Pre-implementation: jcsackett

The DSP picker contains not one but two redundant title bug two. eg:

    ubuntu/commercialpackage (commercialpackage) (commercialpackage)
    > commercialpackage

--------------------------------------------------------------------

RULES

    * The vocab and the picker entry are constructing the same data. The data
      is redundant with the actual package title because the title comes
      from Lp ids. Both parenthetical names can be removed.
    * There is often an extra blank line between the entry details and the
      maintainer. This is because empty strings/lists are being appended
      even though there is nothing to show.
      * ADDENDUM: While writing a test, I discovered that the entry can oops
        trying to get the maintainer for an unbuilt package.
    * The vocabulary has already retrieved the binary package names and it
      would be nice to include them in the entry without a DB call. In the
      case of official package branches, the vocabulary knows more about the
      binary names than the publishing history.

QA

    See http://people.canonical.com/~curtis/fixed-package-picker.png
    to compare the final result with the picture in the reported bug.

    To verify that the vocab, enable the feature:
    disclosure.dsp_picker.enabled default 0 on

    * Visit an ubuntu bug on qastaging.
    * Expand the Ubuntu package task.
    * Open the package field's picker using the choose link.
    * Search for 'mozilla'.
    * Verify each entry does *not* have a parenthetical package name; only
      the ubuntu/<package> text is used for the title.
    * Expand the details for the entries.
    * Verify there is not a blank link between the binary package names
      and the maintainer.
    * Visit a charm bug on on qastaging.
    * Search for 'mysql'
    * The details *may* say the package is unbuilt.
    * Expand the details.
    * Verify that there is no maintainer text.

    Looking at a dev instance also requires the feature flag to be enabled.
    Search requires an update to the sample db because the data is incomplete.
        Insert into distributionsourcepackage(sourcepackagename, distribution)
        select sourcepackagename, distribution
        from distributionsourcepackagecache;

LINT

    lib/lp/app/browser/tests/test_vocabulary.py
    lib/lp/app/browser/vocabulary.py
    lib/lp/registry/configure.zcml
    lib/lp/registry/interfaces/distributionsourcepackage.py
    lib/lp/registry/model/distributionsourcepackage.py
    lib/lp/registry/tests/test_distributionsourcepackage.py
    lib/lp/registry/tests/test_dsp_vocabularies.py
    lib/lp/registry/vocabularies.py

TEST

    ./bin/test -vvc lp.app.browser.tests.test_vocabulary
    ./bin/test -vvc lp.registry.tests.test_dsp_vocabularies
    ./bin/test -vvc -t TestDistributionSourcePackage.test \
        lp.registry.tests.test_distributionsourcepackage

IMPLEMENTATION

Redefined the DSP picker adapter's getPickerEntries() to not set the alt_title
property because it is redundant with the token that is used for the title.
Added a rule to only append the second_line tot he details if it is not empty.
Add a rule return None for the maintainer when there is no current release.
    lib/lp/app/browser/tests/test_vocabulary.py
    lib/lp/app/browser/vocabulary.py

Moved the rules to get the binary package names for a DSP into the model.
Created a cachedproperty called binary_names. The intent is to permit other
processes to prefill the cache so that extra db calls are not needed.
    lib/lp/registry/configure.zcml
    lib/lp/registry/interfaces/distributionsourcepackage.py
    lib/lp/registry/model/distributionsourcepackage.py
    lib/lp/registry/tests/test_distributionsourcepackage.py

Removed the summary rule used to make the term.title because it is redundant
with the token. Updated the DSP vocabulary's toTerm() method to accept a tuple
as well as a package. The tuple is the package and the binary_names to
prepopulate the binary_names cache with. Updated the search method to pass the
DSP and binary_names.
    lib/lp/registry/tests/test_dsp_vocabularies.py
    lib/lp/registry/vocabularies.py

To post a comment you must log in.
Revision history for this message
Steve Kowalik (stevenk) wrote :

I question the usefulness of pulling out Maintainer from built packages -- I suspect a better suggestion is the short description for binary packages, and maybe one or two binary packages names for a source package.

Why do all of the entries say 'package', is the plan to have it distinguish between source and binary packages?

However, this looks loads better than we have currently, great work!

review: Needs Information (code)
Revision history for this message
Curtis Hovey (sinzui) wrote :

We spoke on IRC and we agreed the DSP maintainer role it too ambiguous. I set the method to always return None to remove it from the UI.

The picker entries will always list that distro-name/<package-name>, followed by list of binary package names so that the user can see why the package matched the search.

All target pickers state the kind of thing the user is selection, "package" in this case. Users can search with source and binary package names, but the value is really a distribution source package. Most users do not understand any of this, which is bug 42298 and bug 157602. Both bugs are fixed if this DSP vocab proves to be usable.

Preview Diff

Empty