Don't require description if the category is hidden (#1910)

* fix: dont require description if the category is hidden * test: fix template category in the test data * fix formatting * Fix tests * chore: formatting * test: rename test; add template-categories test suite to CI * fix: default hidden categories to display as "other" on template list and fitlers * fix(test): update test to look for "Other" category * fix: maintain category id of hidden categories * fix: sort filters alphabetically * test: ensure filters are sorted alphabetically * chore: remove default value for FF_TEMPLATE_CATEGORY from test config and rely on .env * chore: formatting * chore: formatting --------- Co-authored-by: Jumana Bahrainwala <[email protected]>
cds-snc · Jul 31, 2024 · b45837b · b45837b
1 parent 1e4461a
commit b45837b
Show file tree

Hide file tree

Showing 7 changed files with 97 additions and 63 deletions.
diff --git a/app/config.py b/app/config.py
@@ -190,7 +190,6 @@ class Development(Config):
     SESSION_PROTECTION = None
     SYSTEM_STATUS_URL = "https://localhost:3000"
     NO_BRANDING_ID = "0af93cf1-2c49-485f-878f-f3e662e651ef"
-    FF_TEMPLATE_CATEGORY = False
 
 
 class Test(Development):

diff --git a/app/main/views/templates.py b/app/main/views/templates.py
@@ -315,14 +315,16 @@ def choose_template(service_id, template_type="all", template_folder_id=None):
 
     template_category_name_col = "name_en" if get_current_locale(current_app) == "en" else "name_fr"
 
-    # Get the full list of template categories, excluding hidden ones
-    template_categories = list(
-        {
-            template.template_category[template_category_name_col]
-            for template in template_list
-            if template.template_category and not template.template_category.get("hidden")
-        }
-    )
+    # Get the full list of template categories, any hidden ones will be called 'Other'
+    template_categories = [
+        template.template_category[template_category_name_col] if not template.template_category.get("hidden") else _("Other")
+        for template in template_list
+        if template.template_category
+    ]
+
+    # Remove duplicates while preserving order
+    template_categories = sorted(set(template_categories))
+
     return render_template(
         "views/templates/choose.html",
         current_template_folder_id=template_folder_id,
@@ -901,17 +903,14 @@ def _get_categories_and_prepare_form(template, template_type):
     form.template_category_id.choices = [(cat["id"], cat[name_col]) for cat in categories if not cat.get("hidden", False)]
 
     # add "other" category to choices, default to the low priority template category
-    form.template_category_id.choices.append((DefaultTemplateCategories.LOW.value, _("Other")))
     # if the template is already in one of the default categories, then dont show the other
-    if template.get("template_category_id", "") in [
-        DefaultTemplateCategories.LOW.value,
-        DefaultTemplateCategories.MEDIUM.value,
-        DefaultTemplateCategories.HIGH.value,
-    ]:
+    if template.get("template_category") and template["template_category"].get("hidden"):
         other_category = None
         form.template_category_other.validators = []
+        form.template_category_id.choices.append((form.template_category_id.data, _("Other")))
     else:
         other_category = {DefaultTemplateCategories.LOW.value: form.template_category_other}
+        form.template_category_id.choices.append((DefaultTemplateCategories.LOW.value, _("Other")))
 
     template_category_hints = {cat["id"]: cat[desc_col] for cat in categories}
 

diff --git a/app/templates/views/templates/_template_list.html b/app/templates/views/templates/_template_list.html
@@ -67,7 +67,7 @@
             {% else %}
               {{ _('Other') }}
             {% endif %}
-        </span>
+          </span>
         {% endif %}
       </div>
     {% endfor %}

diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py
@@ -165,16 +165,24 @@ def test_edit_email_template_cat_other_to_freshdesk(
                     "name": name,
                     "template_content": content,
                     "template_type": "sms",
-                    "template_category_id": DEFAULT_TEMPLATE_CATEGORY_LOW,
+                    "template_category_id": DEFAULT_TEMPLATE_CATEGORY_LOW if app_.config["FF_TEMPLATE_CATEGORY"] else None,
                     "service": SERVICE_ONE_ID,
                     "template_category_other": "hello",
                     "reply_to_text": "[email protected]",
+                    "process_type": None if app_.config["FF_TEMPLATE_CATEGORY"] else DEFAULT_PROCESS_TYPE,
                 },
                 _follow_redirects=True,
             )
 
             mock_update_service_template.assert_called_with(
-                fake_uuid, name, "sms", content, SERVICE_ONE_ID, None, DEFAULT_PROCESS_TYPE, DEFAULT_TEMPLATE_CATEGORY_LOW
+                fake_uuid,
+                name,
+                "sms",
+                content,
+                SERVICE_ONE_ID,
+                None,
+                None if app_.config["FF_TEMPLATE_CATEGORY"] else DEFAULT_PROCESS_TYPE,
+                DEFAULT_TEMPLATE_CATEGORY_LOW if app_.config["FF_TEMPLATE_CATEGORY"] else None,
             )
             assert mock_send_other_category_to_freshdesk.called is True
             mock_send_other_category_to_freshdesk.assert_called_once_with(
@@ -294,7 +302,7 @@ def test_should_show_page_for_choosing_a_template(
     page = client_request.get("main.choose_template", service_id=service_one["id"], **extra_args)
 
     if app_.config["FF_TEMPLATE_CATEGORY"]:
-        expected_nav_links = ["All", "Email template", "Text message template", "All"]
+        expected_nav_links = ["All", "Email template", "Text message template", "All", "Other"]
         links_in_page = page.select('nav[data-testid="filter-content"] a')
     else:
         expected_nav_links = ["All", "Email", "Text message", "Letter"]
@@ -1335,40 +1343,50 @@ def test_should_redirect_to_one_off_if_template_type_is_letter(
     )
 
 
+# parametrize with FF enabled and disabled
+@pytest.mark.parametrize("ff_enabled", [True, False])
 def test_should_redirect_when_saving_a_template(
-    client_request, mock_get_template_categories, mock_get_service_template, mock_update_service_template, fake_uuid, app_
+    client_request,
+    mock_get_template_categories,
+    mock_get_service_template,
+    mock_update_service_template,
+    fake_uuid,
+    app_,
+    ff_enabled,
 ):
-    name = "new name"
-    content = "template <em>content</em> with & entity"
-    page = client_request.post(
-        ".edit_service_template",
-        service_id=SERVICE_ONE_ID,
-        template_id=fake_uuid,
-        _data={
-            "id": fake_uuid,
-            "name": name,
-            "template_content": content,
-            "template_type": "sms",
-            "template_category_id": DEFAULT_TEMPLATE_CATEGORY_LOW,
-            "service": SERVICE_ONE_ID,
-            "process_type": DEFAULT_PROCESS_TYPE,
-        },
-        _follow_redirects=True,
-    )
+    with set_config(app_, "FF_TEMPLATE_CATEGORY", ff_enabled):
+        name = "new name"
+        content = "template <em>content</em> with & entity"
 
-    flash_banner = page.select_one(".banner-default-with-tick").string.strip()
-    assert flash_banner == f"'{name}' template saved"
+        page = client_request.post(
+            ".edit_service_template",
+            service_id=SERVICE_ONE_ID,
+            template_id=fake_uuid,
+            _data={
+                "id": fake_uuid,
+                "name": name,
+                "template_content": content,
+                "template_type": "sms",
+                "template_category_id": DEFAULT_TEMPLATE_CATEGORY_LOW if ff_enabled else None,
+                "service": SERVICE_ONE_ID,
+                "process_type": None if ff_enabled else DEFAULT_PROCESS_TYPE,
+            },
+            _follow_redirects=True,
+        )
 
-    mock_update_service_template.assert_called_with(
-        fake_uuid,
-        name,
-        "sms",
-        content,
-        SERVICE_ONE_ID,
-        None,
-        DEFAULT_PROCESS_TYPE,
-        DEFAULT_TEMPLATE_CATEGORY_LOW if app_.config["FF_TEMPLATE_CATEGORY"] else None,
-    )
+        flash_banner = page.select_one(".banner-default-with-tick").string.strip()
+        assert flash_banner == f"'{name}' template saved"
+        # self, id_, name, type_, content, service_id, subject=None, process_type=None, template_category_id=None
+        mock_update_service_template.assert_called_with(
+            fake_uuid,
+            name,
+            "sms",
+            content,
+            SERVICE_ONE_ID,
+            None,
+            None if ff_enabled else DEFAULT_PROCESS_TYPE,
+            DEFAULT_TEMPLATE_CATEGORY_LOW if ff_enabled else None,
+        )
 
 
 @pytest.mark.parametrize("process_type", [TemplateProcessTypes.NORMAL.value, TemplateProcessTypes.PRIORITY.value])
@@ -1617,16 +1635,16 @@ def test_removing_placeholders_is_not_a_breaking_change(
 
 @pytest.mark.parametrize("template_type", ["sms", "email"])
 def test_should_not_update_if_template_name_too_long(
-    client_request, template_type, fake_uuid, mock_get_service_template, mock_update_service_template_400_name_too_long
+    client_request, template_type, fake_uuid, mock_get_service_template, mock_update_service_template_400_name_too_long, app_
 ):
     template_data = {
         "id": fake_uuid,
         "service": SERVICE_ONE_ID,
         "name": "new name",
         "template_content": "template content!!",
         "template_type": template_type,
-        "template_category_id": DEFAULT_TEMPLATE_CATEGORY_LOW,
-        "process_type": DEFAULT_PROCESS_TYPE,
+        "template_category_id": DEFAULT_TEMPLATE_CATEGORY_LOW if app_.config["FF_TEMPLATE_CATEGORY"] else None,
+        "process_type": TC_PRIORITY_VALUE if app_.config["FF_TEMPLATE_CATEGORY"] else DEFAULT_PROCESS_TYPE,
     }
     if template_type == "email":
         template_data.update({"subject": "subject"})
@@ -1692,10 +1710,7 @@ def test_should_not_create_too_big_template(
 
 
 def test_should_not_update_too_big_template(
-    client_request,
-    mock_get_service_template,
-    mock_update_service_template_400_content_too_big,
-    fake_uuid,
+    client_request, mock_get_service_template, mock_update_service_template_400_content_too_big, fake_uuid, app_
 ):
     page = client_request.post(
         ".edit_service_template",
@@ -1708,7 +1723,7 @@ def test_should_not_update_too_big_template(
             "service": SERVICE_ONE_ID,
             "template_type": "sms",
             "template_category_id": DEFAULT_TEMPLATE_CATEGORY_LOW,
-            "process_type": DEFAULT_PROCESS_TYPE,
+            "process_type": TC_PRIORITY_VALUE if app_.config["FF_TEMPLATE_CATEGORY"] else DEFAULT_PROCESS_TYPE,
         },
         _expected_status=200,
     )

diff --git a/tests/conftest.py b/tests/conftest.py
@@ -785,11 +785,7 @@ def _get_services(params_dict=None):
 def mock_get_service_template(mocker):
     def _get(service_id, template_id, version=None):
         template = template_json(
-            service_id,
-            template_id,
-            "Two week reminder",
-            "sms",
-            "Template <em>content</em> with & entity",
+            service_id, template_id, "Two week reminder", "sms", "Template <em>content</em> with & entity", process_type=None
         )
         if version:
             template.update({"version": version})

diff --git a/tests_cypress/cypress/e2e/admin/ci.cy.js b/tests_cypress/cypress/e2e/admin/ci.cy.js
@@ -5,3 +5,4 @@ import "./sitemap/sitemap.cy";
 import "./branding/all.cy";
 import "./tou_dialog.cy";
 import "./template-filters.cy";
+import "./template-categories.cy";
diff --git a/tests_cypress/cypress/e2e/admin/template-filters.cy.js b/tests_cypress/cypress/e2e/admin/template-filters.cy.js
@@ -86,7 +86,7 @@ describe("Template filters", () => {
 
       context("Filtering by category", () => {
         categories[lang].forEach((type) => {
-          it(`${type}: displays the correct number of rows`, () => {
+          it(`Category "${type}": displays the correct number of rows`, () => {
             cy.visit(url);
 
             // Test type filter works
@@ -129,6 +129,30 @@ describe("Template filters", () => {
               .should("have.length", emailRows);
           });
       });
+
+      it.only("Should list category filters alphabetically", () => {
+        cy.visit(url);
+
+        Page.ToggleFilters();
+
+        Page.Components.CategoryFilter()
+          .find("a")
+          .then(($filters) => {
+            // Extract the text from each filter
+            const filterTexts = $filters
+              .map((index, filter) => Cypress.$(filter).text())
+              .get();
+
+            // Remove the first item, "All"
+            filterTexts.shift();
+
+            // Sort the extracted text alphabetically
+            const sortedFilterTexts = [...filterTexts].sort();
+
+            // Compare the sorted list with the original list to ensure they match
+            expect(filterTexts).to.deep.equal(sortedFilterTexts);
+          });
+      });
     });
   });
 });