launchpad-reviewers team mailing list archive

[Alvaro Crespo Serrano <mp+485727@xxxxxxxxxxxxxxxxxx>]

Alvaro Crespo Serrano has proposed merging ~alvarocs/launchpad:ui-mp-webhook-subscopes into launchpad:master.

Commit message:
testing mp webhooks

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~alvarocs/launchpad/+git/launchpad/+merge/485727
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~alvarocs/launchpad:ui-mp-webhook-subscopes into launchpad:master.

diff --git a/lib/lp/app/widgets/itemswidgets.py b/lib/lp/app/widgets/itemswidgets.py
index 30ea6ef..de80d58 100644
--- a/lib/lp/app/widgets/itemswidgets.py
+++ b/lib/lp/app/widgets/itemswidgets.py
@@ -113,6 +113,7 @@ class LabeledMultiCheckBoxWidget(PlainMultiCheckBoxWidget):
         return self._joinButtonToMessageTemplate % (option_id, elem, text)
 
 
+<<<<<<< lib/lp/app/widgets/itemswidgets.py
 class WebhookCheckboxWidget(PlainMultiCheckBoxWidget):
     """A checkbox widget that indents subscopes event types.
 
@@ -158,6 +159,90 @@ class WebhookCheckboxWidget(PlainMultiCheckBoxWidget):
         )
         option_id = "%s.%s" % (self.name, index)
         return _label % (option_id, label_class, elem, text)
+=======
+class GroupedEventTypesCheckBoxWidget(LabeledMultiCheckBoxWidget):
+    """Checkbox widget that groups subscopes under their parent event types."""
+
+    def getInputValue(self):
+        values = super().getInputValue()
+
+        # Collapse full subscope selection into parent
+        collapsed = self._collapse_full_subscope_selections(values)
+        return collapsed
+
+    def renderValue(self, value):
+        grouped_terms = self._group_terms_by_parent()
+        html = []
+
+        for _parent, items in grouped_terms.items():
+            for index, term in items:
+                is_subscope = "::" in term.token
+                css_style = (
+                    ' style="margin-left: 20px;"' if is_subscope else ""
+                )
+                html.append(
+                    f"<div{css_style}>{self._renderItemForTerm(index, term)}"
+                    "</div>"
+                )
+        return "\n".join(html)
+
+    def _renderItemForTerm(self, index, term):
+        label = term.title
+        token = term.token
+        # If it's a subscope, strip the parent prefix from the title
+        if "::" in token:
+            parent_key = token.split("::")[0]
+            parent_label = self._get_label(parent_key)
+            # If label starts with the parent label, strip it
+            if label.startswith(parent_label):
+                label = label[len(parent_label) :].strip()
+        return self._renderItem(
+            index,
+            label,
+            token,
+            self.name,
+            self.cssClass,
+            checked=(token in self._getFormValue()),
+        )
+
+    def _collapse_full_subscope_selections(self, values):
+        # e.g., if all merge-proposal subscopes selected, replace with parent
+        from lp.services.webhooks.interfaces import WEBHOOK_EVENT_TYPES
+
+        result = set(values)
+
+        # Find all parents
+        parents = {k.split("::")[0] for k in WEBHOOK_EVENT_TYPES if "::" in k}
+
+        for parent in parents:
+            subscopes = {
+                k for k in WEBHOOK_EVENT_TYPES if k.startswith(f"{parent}::")
+            }
+            if subscopes.issubset(result):
+                # Remove subscopes and add parent
+                result -= subscopes
+                result.add(parent)
+
+        return list(result)
+
+    def _group_terms_by_parent(self):
+        groups = {}
+        for index, term in enumerate(self.vocabulary):
+            val = term.token
+            if "::" in val:
+                parent_key = val.split("::")[0]
+                parent_label = self._get_label(parent_key)
+                groups.setdefault(parent_label, []).append((index, term))
+            else:
+                label = self._get_label(val)
+                groups.setdefault(label, []).append((index, term))
+        return groups
+
+    def _get_label(self, event_type):
+        from lp.services.webhooks.interfaces import WEBHOOK_EVENT_TYPES
+
+        return WEBHOOK_EVENT_TYPES.get(event_type, event_type)
+>>>>>>> lib/lp/app/widgets/itemswidgets.py
 
 
 # XXX Brad Bollenbach 2006-08-10 bugs=56062: This is a hack to
diff --git a/lib/lp/services/webhooks/browser.py b/lib/lp/services/webhooks/browser.py
index bf7f30b..4287a5c 100644
--- a/lib/lp/services/webhooks/browser.py
+++ b/lib/lp/services/webhooks/browser.py
@@ -18,7 +18,11 @@ from lp.app.browser.launchpadform import (
     LaunchpadFormView,
     action,
 )
+<<<<<<< lib/lp/services/webhooks/browser.py
 from lp.app.widgets.itemswidgets import WebhookCheckboxWidget
+=======
+from lp.app.widgets.itemswidgets import GroupedEventTypesCheckBoxWidget
+>>>>>>> lib/lp/services/webhooks/browser.py
 from lp.code.interfaces.gitrepository import IGitRepository
 from lp.services.propertycache import cachedproperty
 from lp.services.webapp import (
@@ -116,7 +120,12 @@ class WebhookAddView(LaunchpadFormView):
     page_title = label = "Add webhook"
 
     schema = WebhookEditSchema
+<<<<<<< lib/lp/services/webhooks/browser.py
     custom_widget_event_types = WebhookCheckboxWidget
+=======
+    # custom_widget_event_types = LabeledMultiCheckBoxWidget
+    custom_widget_event_types = GroupedEventTypesCheckBoxWidget
+>>>>>>> lib/lp/services/webhooks/browser.py
     next_url = None
 
     @property
@@ -161,7 +170,12 @@ class WebhookView(LaunchpadEditFormView):
     label = "Manage webhook"
 
     schema = WebhookEditSchema
+<<<<<<< lib/lp/services/webhooks/browser.py
     custom_widget_event_types = WebhookCheckboxWidget
+=======
+    # custom_widget_event_types = LabeledMultiCheckBoxWidget
+    custom_widget_event_types = GroupedEventTypesCheckBoxWidget
+>>>>>>> lib/lp/services/webhooks/browser.py
 
     @property
     def field_names(self):
diff --git a/lib/lp/services/webhooks/configure.zcml b/lib/lp/services/webhooks/configure.zcml
index 3c7ad5c..df6d1db 100644
--- a/lib/lp/services/webhooks/configure.zcml
+++ b/lib/lp/services/webhooks/configure.zcml
@@ -41,6 +41,10 @@
     <class class="lp.services.webhooks.interfaces.ValidWebhookEventTypeVocabulary">
         <allow interface="zope.schema.interfaces.IVocabularyTokenized"/>
     </class>
+<<<<<<< lib/lp/services/webhooks/configure.zcml
+=======
+    
+>>>>>>> lib/lp/services/webhooks/configure.zcml
     <lp:securedutility
         component="lp.services.webhooks.model.WebhookJob"
         provides="lp.services.webhooks.interfaces.IWebhookJobSource">
diff --git a/lib/lp/services/webhooks/interfaces.py b/lib/lp/services/webhooks/interfaces.py
index 1515d2f..8b7cfdd 100644
--- a/lib/lp/services/webhooks/interfaces.py
+++ b/lib/lp/services/webhooks/interfaces.py
@@ -131,14 +131,23 @@ class ValidWebhookEventTypeVocabulary(SimpleVocabulary):
 
 def validate_event_type_parent_subscope(event_types):
     """Validator to handle parent and its subscopes selection."""
+<<<<<<< lib/lp/services/webhooks/interfaces.py
     # Subscopes are identified by the presence of '::' in the event type
     # string.
+=======
+>>>>>>> lib/lp/services/webhooks/interfaces.py
     parents = {et.split("::")[0] for et in event_types if "::" in et}
     for parent in parents:
         if parent in event_types:
             raise LaunchpadValidationError(
+<<<<<<< lib/lp/services/webhooks/interfaces.py
                 f"Please, select either the parent event type ({parent}) "
                 "or its subscopes - not both."
+=======
+                f"Both the parent {parent} and any of its subscopes cannot "
+                "be selected. Please, select either the parent or one or "
+                "more subscopes."
+>>>>>>> lib/lp/services/webhooks/interfaces.py
             )
     return True
 
diff --git a/lib/lp/services/webhooks/tests/test_webservice.py b/lib/lp/services/webhooks/tests/test_webservice.py
index 18c548d..54b9bc0 100644
--- a/lib/lp/services/webhooks/tests/test_webservice.py
+++ b/lib/lp/services/webhooks/tests/test_webservice.py
@@ -312,8 +312,12 @@ class TestWebhook(TestCaseWithFactory):
         )
         self.assertEqual(400, response.status)
         self.assertIn(
+<<<<<<< lib/lp/services/webhooks/tests/test_webservice.py
             b"Please, select either the parent event type (merge-proposal:0.1)"
             b" or its subscopes - not both.",
+=======
+            b"Both the parent merge-proposal:0.1 and any of its subscopes",
+>>>>>>> lib/lp/services/webhooks/tests/test_webservice.py
             response.body,
         )