launchpad-reviewers team mailing list archive

[Guruprasad <mp+432249@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/services/features/browser/configure.zcml b/lib/lp/services/features/browser/configure.zcml
> index d858fca..48e7d12 100644
> --- a/lib/lp/services/features/browser/configure.zcml
> +++ b/lib/lp/services/features/browser/configure.zcml
> @@ -9,36 +9,36 @@
>      xmlns:xmlrpc="http://namespaces.zope.org/xmlrpc";
>      i18n_domain="launchpad">
>  
> -    <!-- View or edit all feature rules.
> +  <browser:url
> +      for="lp.services.features.interfaces.IFeatureRules"
> +      path_expression="string:+feature-rules"
> +      parent_utility="lp.services.webapp.interfaces.ILaunchpadRoot"/>
>  
> -     Readonly access is guarded by launchpad.Edit on ILaunchpadRoot, which
> -     limits it to ~admins + ~registry, which are all trusted users.  Write
> -     access is for admins only.
> -    -->
> -    <browser:page
> -        for="lp.services.webapp.interfaces.ILaunchpadRoot"
> -        class="lp.services.features.browser.edit.FeatureControlView"
> -        name="+feature-rules"
> -        permission="launchpad.Edit"
> -        template="../templates/feature-rules.pt"/>
> +  <browser:defaultView
> +      for="lp.services.features.interfaces.IFeatureRules"
> +      name="+index"/>
>  
> -    <!-- View documentary info about the available feature flags.
> +  <!-- View or edit all feature rules. -->
> +  <browser:page
> +      for="lp.services.features.interfaces.IFeatureRules"
> +      class="lp.services.features.browser.edit.FeatureControlView"
> +      name="+index"
> +      permission="launchpad.View"

Shouldn't this be `launchpad.Edit` since the view allows viewing or editing the feature rules?

Edit: nvm, found the relevant check for `launchpad.Edit` below.

> +      template="../templates/feature-rules.pt"/>
>  
> -     Access is guarded by launchpad.Edit on ILaunchpadRoot, just like
> -     +feature-rules.
> -    -->
> -    <browser:page
> -        for="lp.services.webapp.interfaces.ILaunchpadRoot"
> -        class="lp.services.features.browser.info.FeatureInfoView"
> -        name="+feature-info"
> -        permission="launchpad.Edit"
> -        template="../templates/feature-info.pt"/>
> +  <!-- View documentary info about the available feature flags. -->

`documentary info` feels a bit redundant to me. Would it be better to use something like `View documentation about the available feature flags`?

> +  <browser:page
> +      for="lp.services.features.interfaces.IFeatureRules"
> +      class="lp.services.features.browser.info.FeatureInfoView"
> +      name="info"
> +      permission="launchpad.View"
> +      template="../templates/feature-info.pt"/>
>  
> -    <browser:page
> -        for="lp.services.webapp.interfaces.ILaunchpadRoot"
> -        class="lp.services.features.browser.changelog.FeatureChangeLogView"
> -        name="+feature-changelog"
> -        permission="launchpad.Edit"
> -        template="../templates/feature-changelog.pt"/>
> +  <browser:page
> +      for="lp.services.features.interfaces.IFeatureRules"
> +      class="lp.services.features.browser.changelog.FeatureChangeLogView"
> +      name="changelog"
> +      permission="launchpad.View"
> +      template="../templates/feature-changelog.pt"/>
>  
>  </configure>
> diff --git a/lib/lp/services/features/browser/tests/test_feature_editor.py b/lib/lp/services/features/browser/tests/test_feature_editor.py
> index a6fb812..475bdcf 100644
> --- a/lib/lp/services/features/browser/tests/test_feature_editor.py
> +++ b/lib/lp/services/features/browser/tests/test_feature_editor.py
> @@ -42,22 +63,17 @@ class TestFeatureControlPage(BrowserTestCase):
>                  team.addMember(self.user, reviewer=team.teamowner)
>          return self.getUserBrowser(url=None, user=self.user)
>  
> -    def getUserBrowserAsAdmin(self):
> -        """Make a new TestBrowser logged in as an admin user."""
> -        admin_team = getUtility(ILaunchpadCelebrities).admin
> -        return self.getUserBrowserAsTeamMember([admin_team])
> -
>      def getFeatureRulesViewURL(self):
> -        root = getUtility(ILaunchpadRoot)
> -        return canonical_url(root, view_name="+feature-rules")
> +        root = getUtility(IFeatureRules)

Now that we are not using `ILaunchpadRoot` directly, would it be better to rename this variable to something like `feature_rules`? Applies to all similar statements below.

> +        return canonical_url(root, view_name="+index")

Since `+index` is the default view for `IFeatureRules`, can't it be omitted?

>  
>      def getFeatureRulesEditURL(self):
> -        root = getUtility(ILaunchpadRoot)
> -        return canonical_url(root, view_name="+feature-rules")
> +        root = getUtility(IFeatureRules)
> +        return canonical_url(root, view_name="+index")
>  
>      def test_feature_page_default_value(self):
>          """No rules in the sampledata gives no content in the page"""
> -        browser = self.getUserBrowserAsAdmin()
> +        browser = self.admin_browser
>          browser.open(self.getFeatureRulesViewURL())
>          textarea = browser.getControl(name="field.feature_rules")
>          # and by default, since there are no rules in the sample data, it's
> diff --git a/lib/lp/services/features/security.py b/lib/lp/services/features/security.py
> new file mode 100644
> index 0000000..8f813e7
> --- /dev/null
> +++ b/lib/lp/services/features/security.py
> @@ -0,0 +1,33 @@
> +#  Copyright 2022 Canonical Ltd.  This software is licensed under the
> +#  GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +from lp.app.security import AuthorizationBase
> +from lp.services.features.interfaces import IFeatureRules
> +
> +
> +class ViewFeatureRules(AuthorizationBase):
> +    """
> +    A member of ~admin, ~registry or ~launchpad can view the feature rules.
> +    """
> +
> +    permission = "launchpad.View"
> +    usedfor = IFeatureRules
> +
> +    def checkAuthenticated(self, user):
> +        return (
> +            user.in_admin
> +            or user.in_registry_experts
> +            or user.in_launchpad_developers
> +        )
> +
> +
> +class EditFeatureRules(AuthorizationBase):
> +    """
> +    A member of ~admin or ~launchpad can view the feature rules.

`A member of ~admin or ~launchpad can edit the feature rules.`

> +    """
> +
> +    permission = "launchpad.Edit"
> +    usedfor = IFeatureRules
> +
> +    def checkAuthenticated(self, user):
> +        return user.in_admin or user.in_launchpad_developers


-- 
https://code.launchpad.net/~andrey-fedoseev/launchpad/+git/launchpad/+merge/432249
Your team Launchpad code reviewers is requested to review the proposed merge of ~andrey-fedoseev/launchpad:edit-feature-rules into launchpad:master.