launchpad-reviewers team mailing list archive

[Vaishnavi Asawale <mp+491284@xxxxxxxxxxxxxxxxxx>]

Vaishnavi Asawale has proposed merging ~vaishnavi-asawale/launchpad:fetch-service-dropdown-charmcraft into launchpad:master.

Commit message:
Add choice of permissive or strict mode to the fetch service policy on charm Edit recipe
    
Enable a dropdown for Charms to select between 'permissive' or 'strict' mode for the fetch
service if 'use_fetch_service' is selected. The default mode is 'strict'.


Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~vaishnavi-asawale/launchpad/+git/launchpad/+merge/491284
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~vaishnavi-asawale/launchpad:fetch-service-dropdown-charmcraft into launchpad:master.

diff --git a/lib/lp/charms/browser/charmrecipe.py b/lib/lp/charms/browser/charmrecipe.py
index 5183ce8..7273b16 100644
--- a/lib/lp/charms/browser/charmrecipe.py
+++ b/lib/lp/charms/browser/charmrecipe.py
@@ -34,6 +34,7 @@ from lp.app.browser.launchpadform import (
 from lp.app.browser.lazrjs import InlinePersonEditPickerWidget
 from lp.app.browser.tales import format_link
 from lp.app.widgets.snapbuildchannels import SnapBuildChannelsWidget
+from lp.buildmaster.builderproxy import FetchServicePolicy
 from lp.charms.interfaces.charmhubclient import BadRequestPackageUploadResponse
 from lp.charms.interfaces.charmrecipe import (
     CHARM_RECIPE_WEBHOOKS_FEATURE_FLAG,
@@ -282,6 +283,8 @@ class ICharmRecipeEditSchema(Interface):
             "auto_build",
             "auto_build_channels",
             "store_upload",
+            "use_fetch_service",
+            "fetch_service_policy",
         ],
     )
 
@@ -292,6 +295,13 @@ class ICharmRecipeEditSchema(Interface):
     store_name = copy_field(ICharmRecipe["store_name"], required=True)
     store_channels = copy_field(ICharmRecipe["store_channels"], required=True)
 
+    use_fetch_service = copy_field(
+        ICharmRecipe["use_fetch_service"], required=True
+    )
+    fetch_service_policy = copy_field(
+        ICharmRecipe["fetch_service_policy"], required=True
+    )
+
 
 def log_oops(error, request):
     """Log an oops report without raising an error."""
@@ -529,6 +539,8 @@ class CharmRecipeEditView(BaseCharmRecipeEditView):
         "store_upload",
         "store_name",
         "store_channels",
+        "use_fetch_service",
+        "fetch_service_policy",
     ]
     custom_widget_git_ref = GitRefWidget
     custom_widget_auto_build_channels = CustomWidgetFactory(
@@ -540,6 +552,17 @@ class CharmRecipeEditView(BaseCharmRecipeEditView):
     )
     custom_widget_store_channels = StoreChannelsWidget
 
+    def validate_widgets(self, data, names=None):
+        if (
+            self.widgets.get("use_fetch_service") is not None
+            and self.widgets.get("fetch_service_policy") is not None
+        ):
+            super().validate_widgets(data, ["use_fetch_service"])
+            self.widgets["fetch_service_policy"].context.required = data.get(
+                "use_fetch_service"
+            )
+        super().validate_widgets(data, names=names)
+
     def validate(self, data):
         super().validate(data)
         owner = data.get("owner", None)
@@ -560,6 +583,11 @@ class CharmRecipeEditView(BaseCharmRecipeEditView):
             except NoSuchCharmRecipe:
                 pass
 
+    def updateContextFromData(self, data, context=None, notify_modified=True):
+        if not data.get("use_fetch_service"):
+            data["fetch_service_policy"] = FetchServicePolicy.STRICT
+        super().updateContextFromData(data, context, notify_modified)
+
 
 class CharmRecipeAuthorizeView(LaunchpadEditFormView):
     """View for authorizing charm recipe uploads to Charmhub."""
diff --git a/lib/lp/charms/browser/tests/test_charmrecipe.py b/lib/lp/charms/browser/tests/test_charmrecipe.py
index 08313ab..b135c7b 100644
--- a/lib/lp/charms/browser/tests/test_charmrecipe.py
+++ b/lib/lp/charms/browser/tests/test_charmrecipe.py
@@ -33,6 +33,7 @@ from zope.testbrowser.browser import LinkNotFoundError
 
 from lp.app.enums import InformationType
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.buildmaster.builderproxy import FetchServicePolicy
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.interfaces.processor import IProcessorSet
 from lp.charms.browser.charmrecipe import (
@@ -1022,6 +1023,54 @@ class TestCharmRecipeEditView(BaseTestCharmRecipeView):
             extract_text(find_tags_by_class(browser.contents, "message")[1]),
         )
 
+    def test_edit_fetch_service_policy(self):
+        login_person(self.person)
+        recipe = self.factory.makeCharmRecipe(
+            registrant=self.person,
+            owner=self.person,
+            git_ref=self.factory.makeGitRefs()[0],
+        )
+        browser = self.getViewBrowser(recipe, user=self.person)
+        browser.getLink("Edit charm recipe").click()
+        with person_logged_in(self.person):
+            self.assertFalse(recipe.use_fetch_service)
+            self.assertEqual(
+                recipe.fetch_service_policy, FetchServicePolicy.STRICT
+            )
+        browser.getControl("Use fetch service").selected = True
+        with person_logged_in(self.person):
+            self.assertEqual(
+                recipe.fetch_service_policy, FetchServicePolicy.STRICT
+            )
+        browser.getControl("Fetch service policy").value = ["PERMISSIVE"]
+        browser.getControl("Update charm recipe").click()
+
+        browser = self.getViewBrowser(recipe, user=self.person)
+        browser.getLink("Edit charm recipe").click()
+        with person_logged_in(self.person):
+            self.assertTrue(recipe.use_fetch_service)
+            self.assertEqual(
+                recipe.fetch_service_policy, FetchServicePolicy.PERMISSIVE
+            )
+
+    def test_fetch_service_policy_when_opted_in(self):
+        # If the use_fetch_service parameter is set to 'True', the
+        # fetch_service_policy defaults to 'strict' mode unless specified
+        login_person(self.person)
+        recipe = self.factory.makeCharmRecipe(
+            registrant=self.person,
+            owner=self.person,
+            git_ref=self.factory.makeGitRefs()[0],
+            use_fetch_service=True,
+        )
+        browser = self.getViewBrowser(recipe, user=self.person)
+        browser.getLink("Edit charm recipe").click()
+        with person_logged_in(self.person):
+            self.assertTrue(recipe.use_fetch_service)
+            self.assertEqual(
+                recipe.fetch_service_policy, FetchServicePolicy.STRICT
+            )
+
 
 class TestCharmRecipeAuthorizeView(BaseTestCharmRecipeView):
     def setUp(self):
diff --git a/lib/lp/charms/interfaces/charmrecipe.py b/lib/lp/charms/interfaces/charmrecipe.py
index 0399864..863914a 100644
--- a/lib/lp/charms/interfaces/charmrecipe.py
+++ b/lib/lp/charms/interfaces/charmrecipe.py
@@ -808,7 +808,11 @@ class ICharmRecipeEditableAttributes(Interface):
             default=FetchServicePolicy.STRICT,
             description=_(
                 "Which policy to use when using the fetch service. Ignored if "
-                "`use_fetch_service` flag is False."
+                "`use_fetch_service` flag is False. The “strict” mode only "
+                "allows certain resources and formats, and errors out in any "
+                "case the restrictions are violated. The “permissive” mode "
+                "works similarly, but only logs a warning when encountering "
+                "any violations."
             ),
         )
     )
diff --git a/lib/lp/charms/templates/charmrecipe-edit.pt b/lib/lp/charms/templates/charmrecipe-edit.pt
index 8e4398d..1afb86f 100644
--- a/lib/lp/charms/templates/charmrecipe-edit.pt
+++ b/lib/lp/charms/templates/charmrecipe-edit.pt
@@ -83,9 +83,42 @@
             </table>
           </td>
         </tr>
+
+        <tal:widget define="widget nocall:view/widgets/use_fetch_service">
+          <metal:block use-macro="context/@@launchpad_form/widget_row" />
+        </tal:widget>
+
+        <div id="fetch-service-policy-container">
+          <tal:widget define="widget nocall:view/widgets/fetch_service_policy">
+            <metal:block use-macro="context/@@launchpad_form/widget_row" />
+          </tal:widget>
+        </div>
       </table>
     </metal:formbody>
   </div>
+
+  <!-- Script to enable/disable the fetch_service_policy dropdown depending on whether the use_fetch_service flag is set -->
+  <script type="text/javascript">
+    LPJS.use('node', function(Y) {
+      Y.on('domready', function () {
+        var fs_checkbox = Y.one("input[name='field.use_fetch_service']");
+        var fs_policy_select = Y.one("select[name='field.fetch_service_policy']");
+
+        if (!fs_checkbox || !fs_policy_select) return;
+
+        function toggleFetchPolicy() {
+          if (fs_checkbox.get('checked')) {
+            fs_policy_select.set('disabled', false);
+          } else {
+            fs_policy_select.set('disabled', true);
+          }
+        }
+
+        toggleFetchPolicy(); // On page load
+        fs_checkbox.on('change', toggleFetchPolicy);
+      });
+    });
+  </script>
 </div>
 
 </body>