launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

So on reflection I think the attempt to use a non-DB-backed vocabulary for this was a serious red herring that I should have steered you away from sooner.  I thought it was more of a brief temporary experiment, but it's led you down the path of making quite time-consuming changes all over the place to support it that aren't going to be mergeable.  Similarly, hardcoding IDs is never going to be the right thing to do and I think relying on them has perhaps got in the way of better approaches.  I hope these suggestions get you back on the right path.

Regarding how to do the presentational side of these changes, particularly the parts in SnapAddView and SnapEditView: as I mentioned before, there are a couple of possible approaches.  We do IMO want to keep the approach where store_series and distro_series are only ever edited in combined form in the UI, even if the store_series part of that is mostly vestigial.  That means that we need to figure out a way to represent those combinations in a way that's less confusing.

At the moment, store_distro_series is presented using a LaunchpadRadioWidget, which just renders as a list of radio buttons with choices from the vocabulary.  That's fine - there doesn't seem to be a particular need to change that, so leave it alone.  The vocabulary's terms are "titled": that is, as well as having a value (a SnappyDistroSeries object) and a token (a machine-readable identifier which can be used in HTML forms), they also have a title which is intended for display to humans.  The title seems like the right thing to change in this case.

Now, a vocabulary can either make up its own titles, or it can delegate that to something else.  If you look at SnappyDistroSeriesVocabulary.toTerm, it returns SimpleTerm(obj, token, obj.title), meaning that it's just using the "title" property of SnappyDistroSeries.  That does mean we're leaking presentational details down into the model layer a bit, but that's relatively harmless in this case as IIRC that title isn't used by anything else.  So we probably might as well just decide on what each of the choices should look like in the UI and then make SnappyDistroSeries.title return those.

Diff comments:

> diff --git a/lib/lp/snappy/interfaces/snap.py b/lib/lp/snappy/interfaces/snap.py
> index b408462..c72646b 100644
> --- a/lib/lp/snappy/interfaces/snap.py
> +++ b/lib/lp/snappy/interfaces/snap.py
> @@ -784,7 +784,7 @@ class ISnapEditableAttributes(IHasOwner):
>  
>      store_distro_series = ReferenceChoice(
>          title=_("Store and distro series"),
> -        schema=ISnappyDistroSeries, vocabulary="SnappyDistroSeries",
> +        schema=IDistroSeries, vocabulary="DistroSeries",

No - this needs to keep its previous type and vocabulary.  (If nothing else, changing it like this makes it mostly redundant with ISnap.distro_series.)

We do want to change how the combination of store and distro series are rendered in the UI.  But throughout all of this work, keep in mind that none of the changes should affect the data model layer in any way; this is a presentational change and belongs in the UI layer.  So I'd expect changes to be confined to these parts of the codebase, without implying that you necessarily need to touch all of these:

  lib/lp/snappy/browser/
  lib/lp/snappy/templates/
  lib/lp/snappy/vocabularies.*

(Vocabularies are often used by both the data model layer and the UI layer, but BuildableSnappyDistroSeriesVocabulary is only used by the UI layer so we can count it as part of it for this purpose.)

If you find yourself needing to change lib/lp/snappy/interfaces/ or lib/lp/snappy/model/ very much, that's probably a sign that you're on the wrong path.  Of course sometimes UI changes need extra support from the data model, but they shouldn't involve this kind of interface-breaking change.  In this case, the only model change I can think of that you may need to make is to change SnappyDistroSeries.title, which is somewhat presentational anyway; see my overall comment for more details.

>          required=False, readonly=False)
>  
>      store_name = exported(TextLine(
> diff --git a/lib/lp/snappy/interfaces/snappyseries.py b/lib/lp/snappy/interfaces/snappyseries.py
> index 088806c..1aa0379 100644
> --- a/lib/lp/snappy/interfaces/snappyseries.py
> +++ b/lib/lp/snappy/interfaces/snappyseries.py
> @@ -184,6 +184,17 @@ class ISnappySeriesSet(ISnappySeriesSetEdit):
>          :raises NoSuchSnappySeries: if no snappy series exists with this name.
>          """
>  
> +    @operation_parameters(
> +        id=TextLine(title=_("Snappy series ID"), required=True))
> +    @operation_returns_entry(ISnappySeries)
> +    @export_read_operation()
> +    @operation_for_version("devel")

My suggestions elsewhere should lead to reverting this whole interface addition, as it won't be needed.

However, just while I'm here, it's worth saying that you should avoid being in the habit of exporting things like this on the webservice that exist mainly to support an internal rearrangement.  Webservice users will never have a SnappySeries ID in hand (at least not without guessing it from the URL), as ISnappySeries.id isn't exported.  Also, exporting methods on the webservice creates a commitment from us to support them, while we can rearrange internal interfaces relatively freely.

> +    def getById(id):
> +        """Return the `ISnappySeries` with this ID.
> +
> +        :raises NoSuchSnappySeries: if no snappy series exists with this ID.
> +        """
> +
>      @collection_default_content()
>      def getAll():
>          """Return all `ISnappySeries`."""
> diff --git a/lib/lp/snappy/vocabularies.py b/lib/lp/snappy/vocabularies.py
> index 331088a..910d980 100644
> --- a/lib/lp/snappy/vocabularies.py
> +++ b/lib/lp/snappy/vocabularies.py
> @@ -84,7 +87,8 @@ class SnappyDistroSeriesVocabulary(StormVocabularyBase):
>          LeftJoin(Distribution, DistroSeries.distributionID == Distribution.id),
>          SnappySeries,
>          ]
> -    _clauses = [SnappyDistroSeries.snappy_series_id == SnappySeries.id]
> +    _clauses = [SnappyDistroSeries.snappy_series_id == SnappySeries.id,
> +                SnappySeries.status.is_in(ACTIVE_STATUSES)]

Let's undo this and keep all the changes in BuildableSnappyDistroSeriesVocabulary.  We shouldn't be changing the semantics of this vocabulary.

>  
>      @property
>      def _entries(self):
> @@ -138,12 +142,43 @@ class SnappyDistroSeriesVocabulary(StormVocabularyBase):
>          return self.toTerm(entry)
>  
>  
> -class BuildableSnappyDistroSeriesVocabulary(SnappyDistroSeriesVocabulary):
> +class BuildableSnappyDistroSeriesVocabulary(SimpleVocabulary):
>      """A vocabulary for searching active snappy/distro series combinations."""
>  
> -    _clauses = SnappyDistroSeriesVocabulary._clauses + [
> -        SnappySeries.status.is_in(ACTIVE_STATUSES),
> -        ]
> +    def __init__(self, context=None):
> +
> +        sds_set = getUtility(ISnappyDistroSeriesSet)
> +        store_distro_series = removeSecurityProxy(sds_set.getDistroSeries())
> +        terms = [
> +            self.createTerm(distro, distro.distribution.display_name + ' ' +distro.display_name)
> +            for distro in store_distro_series]
> +
> +        if ISnap.providedBy(context):
> +            # We are editing the Snap
> +            store_series = removeSecurityProxy(context).store_series
> +            if store_series is not None:
> +                if store_series.id == 1:
> +                    # We allow editing to upgrade to
> +                    # Store Series 2 or to remain on the same version
> +                    # but not to downgrade from Store version 2 to 1
> +                    terms = [self.createTerm(
> +                                distro.distribution.display_name
> +                                +' '+
> +                                context.distro_series.display_name +
> +                                ' for Store Series 1')]
> +
> +                    [terms.append(self.createTerm(
> +                        distro.distribution.display_name +
> +                        ' ' +
> +                        distro.display_name +
> +                        ' for Store Series 2'))
> +                    for distro in store_distro_series]
> +
> +        super(BuildableSnappyDistroSeriesVocabulary, self).__init__(terms)
> +
> +    @classmethod
> +    def createTerm(cls, *args):
> +        return SimpleTerm(*args)

OK.  Try reverting all this and instead using something like this, importing Or from storm.locals:

    @property
    def _clauses(self):
        active_clause = SnappySeries.status.is_in(ACTIVE_STATUSES)
        if (ISnap.providedBy(context) and
                context.store_series.status not in ACTIVE_STATUSES):
            active_clause = Or(
                active_clause, SnappySeries.id == context.store_series_id)
        return SnappyDistroSeriesVocabulary._clauses + [active_clause]

Do you see how this works?  Instead of completely rewriting the vocabulary, we make a small surgical change to it that just adjusts the database query it makes to be a little more permissive in the case where the context's current store series is inactive.  You can use the LP_DEBUG_SQL=1 environment variable to see the resulting query, but it should end up looking something like this:

    SELECT SnappyDistroSeries.id, SnappyDistroSeries.snappy_series, SnappyDistroSeries.distro_series, SnappyDistroSeries.preferred
    FROM
        SnappyDistroSeries
        LEFT JOIN DistroSeries ON SnappyDistroSeries.distro_series = DistroSeries.id
        LEFT JOIN Distribution ON DistroSeries.distribution = Distribution.id,
        SnappySeries
    WHERE
        SnappyDistroSeries.snappy_series_id = SnappySeries.id
        AND (
            SnappySeries.status IN (2, 3, 4, 5)
            OR SnappySeries.id = 1);

It's important for maintainability to avoid repeating the common parts of the logic as much as possible; and this approach means that the vocabulary users hardly need to change at all, because it all fits into how the vocabulary is already used.

>  
>  
>  @implementer(IJSONPublishable)


-- 
https://code.launchpad.net/~ilasc/launchpad/+git/launchpad/+merge/375224
Your team Launchpad code reviewers is requested to review the proposed merge of ~ilasc/launchpad:bug-1818755 into launchpad:master.