launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Review: Approve

This looks pretty good now, thanks!  I commented on a few tweaks that I think are needed, and I'm sure we'll find things to adjust once a substantial feature like this hits QA, but you can go ahead with this when you're ready.

Can https://code.launchpad.net/~cjwatson/launchpad/+git/launchpad/+merge/401743 be withdrawn now?  My understanding is that it should be no longer needed following your redesign.

Diff comments:

> diff --git a/lib/lp/app/widgets/launchpadtarget.py b/lib/lp/app/widgets/launchpadtarget.py
> index 00f0b37..e2fe87c 100644
> --- a/lib/lp/app/widgets/launchpadtarget.py
> +++ b/lib/lp/app/widgets/launchpadtarget.py
> @@ -55,15 +55,17 @@ class LaunchpadTargetWidget(BrowserWidget, InputWidget):
>      def getProductVocabulary(self):
>          return 'Product'
>  
> -    def setUpSubWidgets(self):
> -        if self._widgets_set_up:
> -            return
> +    def getPackageVocabulary(self):

Maybe `getPackageVocabularyName`, since it returns a name rather than an actual vocabulary object?

>          if bool(getFeatureFlag('disclosure.dsp_picker.enabled')):
>              # Replace the default field with a field that uses the better
>              # vocabulary.
> -            package_vocab = 'DistributionSourcePackage'
> +            return 'DistributionSourcePackage'
>          else:
> -            package_vocab = 'BinaryAndSourcePackageName'
> +            return 'BinaryAndSourcePackageName'
> +
> +    def setUpSubWidgets(self):
> +        if self._widgets_set_up:
> +            return
>          fields = [
>              Choice(
>                  __name__='product', title=u'Project',
> diff --git a/lib/lp/bugs/stories/bugtask-management/xx-bugtask-edit-forms.txt b/lib/lp/bugs/stories/bugtask-management/xx-bugtask-edit-forms.txt
> index 49341d1..174a261 100644
> --- a/lib/lp/bugs/stories/bugtask-management/xx-bugtask-edit-forms.txt
> +++ b/lib/lp/bugs/stories/bugtask-management/xx-bugtask-edit-forms.txt
> @@ -49,6 +49,7 @@ respective bug task.
>      Target
>      Distribution
>      ...
> +    Package (Find...)
>      Project (Find…)

Is the difference in ellipsis style here accidental?

>      Status  Importance  Milestone
>      New...  Low...      (nothing selected)...
> diff --git a/lib/lp/registry/browser/ociproject.py b/lib/lp/registry/browser/ociproject.py
> index 3f938d8..3fada35 100644
> --- a/lib/lp/registry/browser/ociproject.py
> +++ b/lib/lp/registry/browser/ociproject.py
> @@ -175,8 +178,23 @@ class OCIProjectFacets(StandardLaunchpadFacets):
>      enable_only = [
>          'overview',
>          'branches',
> +        'bugs',
>          ]
>  
> +    def makeLink(self, text, context, view_name, site):
> +        site = 'mainsite' if self.mainsite_only else site
> +        target = canonical_url(context, view_name=view_name, rootsite=site)
> +        return Link(target, text, site=site)
> +
> +    def branches(self):
> +        return self.makeLink('Code', self.context.pillar, '+branches', 'code')

Despite the property name, I think this should probably link to +code rather than the IIRC Bazaar-specific +branches.  (Maybe while you're here you could also make it link to `self.context` rather than `self.context.pillar`, since `OCIProject` has a reasonable implementation of a +code page.)

> +
> +    def bugs(self):
> +        """Override bugs link to show the OCIProject's bug page, instead of
> +        the pillar's bug page.
> +        """
> +        return self.makeLink('Bugs', self.context, '+bugs', 'bugs')
> +
>  
>  class OCIProjectNavigationMenu(NavigationMenu):
>      """Navigation menu for OCI projects."""
> diff --git a/lib/lp/registry/interfaces/ociproject.py b/lib/lp/registry/interfaces/ociproject.py
> index 85fb588..d2cabc8 100644
> --- a/lib/lp/registry/interfaces/ociproject.py
> +++ b/lib/lp/registry/interfaces/ociproject.py
> @@ -67,7 +72,9 @@ from lp.services.fields import (
>  OCI_PROJECT_ALLOW_CREATE = 'oci.project.create.enabled'
>  
>  
> -class IOCIProjectView(IHasGitRepositories, Interface):
> +class IOCIProjectView(IHasGitRepositories, IServiceUsage,

If it's necessary I suppose I don't object, but I wasn't expecting `OCIProject` to implement `IServiceUsage`: `DistributionSourcePackage` doesn't, for comparison.  The service usage should probably be controlled by the pillar.

> +                      IHasOfficialBugTags, IStructuralSubscriptionTarget,
> +                      Interface):
>      """IOCIProject attributes that require launchpad.View permission."""
>  
>      id = Int(title=_("ID"), required=True, readonly=True)
> diff --git a/lib/lp/registry/vocabularies.py b/lib/lp/registry/vocabularies.py
> index 02e3158..e59028e 100644
> --- a/lib/lp/registry/vocabularies.py
> +++ b/lib/lp/registry/vocabularies.py
> @@ -2229,5 +2240,75 @@ class OCIProjectVocabulary(StormVocabularyBase):
>      def search(self, query, vocab_filter=None):
>          return getUtility(IOCIProjectSet).searchByName(query)
>  
> +    @property

`OCIRecipeVocabulary` seems to have a similar bug (`_entries` incorrectly being a method rather than a property), though you don't have to fix that in this MP if it isn't breaking anything here.

>      def _entries(self):
> -        return getUtility(IOCIProjectSet).searchByName('')
> +        return getUtility(IOCIProjectSet).searchByName(six.ensure_text(''))

Or just `u''`?

> +
> +    def __contains__(self, obj):
> +        found_obj = IStore(self._table).find(
> +            self._table,
> +            self._table.id == obj.id).one()
> +        return found_obj is not None and found_obj == obj
> +
> +
> +@implementer(IHugeVocabulary)
> +class DistributionPackageVocabulary:
> +    """A simple wrapper to automatically select package vocabulary
> +    (BinaryAndSourcePackageNameVocabulary or OCIProjectVocabulary) depending

Maybe mention `DistributionSourcePackageVocabulary` here too.

> +    on which type of distribution we are dealing with.
> +    """
> +
> +    def __init__(self, context=None):
> +        super(DistributionPackageVocabulary, self).__init__()
> +        if bool(getFeatureFlag('disclosure.dsp_picker.enabled')):
> +            # Replace the default field with a field that uses the better
> +            # vocabulary.
> +            self.packages_vocabulary = DistributionSourcePackageVocabulary(
> +                context)
> +        else:
> +            self.packages_vocabulary = BinaryAndSourcePackageNameVocabulary(
> +                context)
> +
> +        self.oci_projects_vocabulary = OCIProjectVocabulary(context)
> +        self.distribution = None
> +        self.context = context
> +
> +    def setDistribution(self, distribution):
> +        self.distribution = distribution
> +        self.oci_projects_vocabulary.setPillar(distribution)
> +        if isinstance(
> +                self.packages_vocabulary, DistributionSourcePackageVocabulary):
> +            self.packages_vocabulary.setDistribution(distribution)
> +
> +    @property
> +    def is_oci_distribution(self):
> +        distribution = self.distribution
> +        if distribution is None and self.context is not None:
> +            # If distribution was not set yet, try to guess it from context.
> +            distribution = getattr(self.context, 'distribution', None)
> +        oci_traversal_policy = DistributionDefaultTraversalPolicy.OCI_PROJECT
> +        return (
> +            distribution is not None and
> +            distribution.default_traversal_policy == oci_traversal_policy)
> +
> +    def __getattribute__(self, item):

Would it be less confusing to define `__getattr__` instead?  That way you wouldn't need to explicitly exclude local attributes.

> +        local_attrs = {
> +            'is_oci_distribution', 'oci_projects_vocabulary',
> +            'packages_vocabulary', 'distribution', 'setDistribution',
> +            'context'}
> +        if item in local_attrs:
> +            return object.__getattribute__(self, item)
> +        if self.is_oci_distribution:
> +            vocabulary = self.oci_projects_vocabulary
> +        else:
> +            vocabulary = self.packages_vocabulary
> +        return getattr(vocabulary, item)
> +
> +    # Special methods should be declared at the class declaration,
> +    # even if self.__special_method__ will be forwarded to the correct
> +    # vocabulary after, by __getattribute__.
> +    def __iter__(self):
> +        return self.__iter__()
> +
> +    def __contains__(self, obj):
> +        return self.__contains__(obj)


-- 
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/394181
Your team Launchpad code reviewers is subscribed to branch ~pappacena/launchpad:bugtask-oci-project.