Feat/add org id to create branding (#1781)

* feat(create/update branding): add org id field, default to none

* tests(create/update branding): fix tests

* chore: formatting

* fix broken test

* fix(brand cache): remove unsued broked cache, add new ones, fix tests

app/main/forms.py

@@ -1184,6 +1184,7 @@ class ServiceUpdateEmailBranding(StripWhitespaceForm):
             ("no_branding", "No branding"),
         ],
     )
+    organisation = RadioField("Select an organisation", choices=[])
 
     def validate_name(form, name):
         op = request.form.get("operation")

app/main/views/email_branding.py

@@ -11,7 +11,7 @@
 from flask_login import current_user
 from notifications_utils.template import HTMLEmailTemplate
 
-from app import current_service, email_branding_client
+from app import current_service, email_branding_client, organisations_client
 from app.main import main
 from app.main.forms import (
     BrandingGOCForm,
@@ -48,15 +48,21 @@ def email_branding():
 @main.route("/email-branding/<branding_id>/edit/<logo>", methods=["GET", "POST"])
 @user_is_platform_admin
 def update_email_branding(branding_id, logo=None):
+    all_organisations = organisations_client.get_organisations()
     email_branding = email_branding_client.get_email_branding(branding_id)["email_branding"]
 
     form = ServiceUpdateEmailBranding(
         name=email_branding["name"],
         text=email_branding["text"],
         colour=email_branding["colour"],
         brand_type=email_branding["brand_type"],
+        organisation="-1",
     )
 
+    form.organisation.choices = [(org["id"], org["name"]) for org in all_organisations]
+    # add the option for no org
+    form.organisation.choices.append(("-1", "No organisation"))
+
     logo = logo if logo else email_branding.get("logo") if email_branding else None
 
     if form.validate_on_submit():
@@ -88,6 +94,7 @@ def update_email_branding(branding_id, logo=None):
             text=form.text.data,
             colour=form.colour.data,
             brand_type=form.brand_type.data,
+            organisation_id=form.organisation.data,
         )
 
         if logo:
@@ -110,7 +117,12 @@ def update_email_branding(branding_id, logo=None):
 @main.route("/email-branding/create/<logo>", methods=["GET", "POST"])
 @user_is_platform_admin
 def create_email_branding(logo=None):
-    form = ServiceUpdateEmailBranding(brand_type="custom_logo")
+    all_organisations = organisations_client.get_organisations()
+    form = ServiceUpdateEmailBranding(brand_type="custom_logo", organisation="-1")
+
+    form.organisation.choices = [(org["id"], org["name"]) for org in all_organisations]
+    # add the option for no org
+    form.organisation.choices.append(("-1", "No organisation"))
 
     if form.validate_on_submit():
         if form.file.data:
@@ -134,6 +146,7 @@ def create_email_branding(logo=None):
             text=form.text.data,
             colour=form.colour.data,
             brand_type=form.brand_type.data,
+            organisation_id=None if form.organisation.data == "-1" else form.organisation.data,
         )
 
         if logo:

app/main/views/platform_admin.py

@@ -594,8 +594,8 @@ def clear_cache():
             (
                 "email_branding",
                 [
-                    "email_branding",
                     "email_branding-????????-????-????-????-????????????",
+                    "email_branding-None",
                 ],
             ),
             (

app/notify_client/email_branding_client.py

@@ -17,26 +17,30 @@ def get_all_email_branding(self, sort_key=None, organisation_id=None):
         else:
             return []
 
-    @cache.delete("email_branding")
-    def create_email_branding(self, logo, name, text, colour, brand_type):
+    @cache.delete("email_branding-{organisation_id}")
+    @cache.delete("email_branding-None")
+    def create_email_branding(self, logo, name, text, colour, brand_type, organisation_id):
         data = {
             "logo": logo,
             "name": name,
             "text": text,
             "colour": colour,
             "brand_type": brand_type,
+            "organisation_id": organisation_id,
         }
         return self.post(url="/email-branding", data=data)
 
-    @cache.delete("email_branding")
     @cache.delete("email_branding-{branding_id}")
-    def update_email_branding(self, branding_id, logo, name, text, colour, brand_type):
+    @cache.delete("email_branding-{organisation_id}")
+    @cache.delete("email_branding-None")
+    def update_email_branding(self, branding_id, logo, name, text, colour, brand_type, organisation_id):
         data = {
             "logo": logo,
             "name": name,
             "text": text,
             "colour": colour,
             "brand_type": brand_type,
+            "organisation_id": organisation_id,
         }
         return self.post(url="/email-branding/{}".format(branding_id), data=data)
 

app/templates/views/email-branding/manage-branding.html

@@ -32,7 +32,8 @@
           <div style='margin-top:15px;'>{{textbox(form.name)}}</div>
           <div style='margin-top:15px;'>{{textbox(form.text)}}</div>
           {{ textbox(form.colour, width='w-full md:w-1/4', colour_preview=True) }}
-           {{ radios(form.brand_type) }}
+          {{ radios(form.brand_type) }}
+          {{ radios(form.organisation) }}
           {{ page_footer(
             _('Save'),
             button_name='operation',

tests/app/main/views/test_email_branding.py

@@ -41,7 +41,9 @@ def test_email_branding_page_shows_full_branding_list(platform_admin_client, moc
     ]
 
 
-def test_edit_email_branding_shows_the_correct_branding_info(platform_admin_client, mock_get_email_branding, fake_uuid):
+def test_edit_email_branding_shows_the_correct_branding_info(
+    platform_admin_client, mock_get_email_branding, mock_get_organisations, fake_uuid
+):
     response = platform_admin_client.get(url_for(".update_email_branding", branding_id=fake_uuid))
 
     assert response.status_code == 200
@@ -53,7 +55,9 @@ def test_edit_email_branding_shows_the_correct_branding_info(platform_admin_clie
     assert page.select_one("#colour").attrs.get("value") == "#f00"
 
 
-def test_create_email_branding_does_not_show_any_branding_info(platform_admin_client, mock_no_email_branding):
+def test_create_email_branding_does_not_show_any_branding_info(
+    platform_admin_client, mock_no_email_branding, mock_get_organisations
+):
     response = platform_admin_client.get(url_for(".create_email_branding"))
 
     assert response.status_code == 200
@@ -70,6 +74,7 @@ def test_create_new_email_branding_without_logo(
     mocker,
     fake_uuid,
     mock_create_email_branding,
+    mock_get_organisations,
 ):
     data = {
         "logo": None,
@@ -91,12 +96,13 @@ def test_create_new_email_branding_without_logo(
         text=data["text"],
         colour=data["colour"],
         brand_type=data["brand_type"],
+        organisation_id=None,
     )
     assert mock_persist.call_args_list == []
 
 
 def test_create_email_branding_requires_a_name_when_submitting_logo_details(
-    client_request, mocker, mock_create_email_branding, platform_admin_user
+    client_request, mocker, mock_create_email_branding, platform_admin_user, mock_get_organisations
 ):
     mocker.patch("app.main.views.email_branding.persist_logo")
     mocker.patch("app.main.views.email_branding.delete_email_temp_files_created_by")
@@ -120,7 +126,9 @@ def test_create_email_branding_requires_a_name_when_submitting_logo_details(
     assert mock_create_email_branding.called is False
 
 
-def test_create_email_branding_does_not_require_a_name_when_uploading_a_file(client_request, mocker, platform_admin_user):
+def test_create_email_branding_does_not_require_a_name_when_uploading_a_file(
+    client_request, mocker, platform_admin_user, mock_get_organisations
+):
     mocker.patch("app.main.views.email_branding.upload_email_logo", return_value="temp_filename")
     data = {
         "file": (BytesIO("".encode("utf-8")), "test.png"),
@@ -140,7 +148,9 @@ def test_create_email_branding_does_not_require_a_name_when_uploading_a_file(cli
     assert not page.find(".error-message")
 
 
-def test_create_new_email_branding_when_branding_saved(platform_admin_client, mocker, mock_create_email_branding, fake_uuid):
+def test_create_new_email_branding_when_branding_saved(
+    platform_admin_client, mocker, mock_create_email_branding, fake_uuid, mock_get_organisations
+):
     with platform_admin_client.session_transaction() as session:
         user_id = session["user_id"]
 
@@ -170,6 +180,7 @@ def test_create_new_email_branding_when_branding_saved(platform_admin_client, mo
             "text": data["text"],
             "cdn_url": get_logo_cdn_domain(),
             "brand_type": data["brand_type"],
+            "organisation": "-1",
         },
     )
 
@@ -182,6 +193,7 @@ def test_create_new_email_branding_when_branding_saved(platform_admin_client, mo
         text=data["text"],
         colour=data["colour"],
         brand_type=data["brand_type"],
+        organisation_id=None,
     )
 
 
@@ -193,7 +205,7 @@ def test_create_new_email_branding_when_branding_saved(platform_admin_client, mo
     ],
 )
 def test_deletes_previous_temp_logo_after_uploading_logo(
-    platform_admin_client, mocker, mock_get_all_email_branding, endpoint, has_data, fake_uuid
+    platform_admin_client, mocker, mock_get_all_email_branding, endpoint, has_data, fake_uuid, mock_get_organisations
 ):
     if has_data:
         mocker.patch("app.email_branding_client.get_email_branding", return_value=create_email_branding(fake_uuid))
@@ -232,6 +244,7 @@ def test_update_existing_branding(
     fake_uuid,
     mock_get_email_branding,
     mock_update_email_branding,
+    mock_get_organisations,
 ):
     with platform_admin_client.session_transaction() as session:
         user_id = session["user_id"]
@@ -275,13 +288,15 @@ def test_update_existing_branding(
         text=data["text"],
         colour=data["colour"],
         brand_type=data["brand_type"],
+        organisation_id="-1",
     )
 
 
 def test_temp_logo_is_shown_after_uploading_logo(
     platform_admin_client,
     mocker,
     fake_uuid,
+    mock_get_organisations,
 ):
     with platform_admin_client.session_transaction() as session:
         user_id = session["user_id"]
@@ -295,7 +310,7 @@ def test_temp_logo_is_shown_after_uploading_logo(
 
     response = platform_admin_client.post(
         url_for("main.create_email_branding"),
-        data={"file": (BytesIO("".encode("utf-8")), "test.png")},
+        data={"file": (BytesIO("".encode("utf-8")), "test.png"), "organisation": "-1"},
         content_type="multipart/form-data",
         follow_redirects=True,
     )
@@ -307,7 +322,9 @@ def test_temp_logo_is_shown_after_uploading_logo(
     assert page.select_one("#logo-img > img").attrs["src"].endswith(temp_filename)
 
 
-def test_logo_persisted_when_organisation_saved(platform_admin_client, mock_create_email_branding, mocker, fake_uuid):
+def test_logo_persisted_when_organisation_saved(
+    platform_admin_client, mock_create_email_branding, mocker, fake_uuid, mock_get_organisations
+):
     with platform_admin_client.session_transaction() as session:
         user_id = session["user_id"]
 
@@ -367,12 +384,7 @@ def test_logo_does_not_get_persisted_if_updating_email_branding_client_throws_an
     ],
 )
 def test_colour_regex_validation(
-    platform_admin_client,
-    mocker,
-    fake_uuid,
-    colour_hex,
-    expected_status_code,
-    mock_create_email_branding,
+    platform_admin_client, mocker, fake_uuid, colour_hex, expected_status_code, mock_create_email_branding, mock_get_organisations
 ):
     data = {
         "logo": None,
@@ -388,6 +400,69 @@ def test_colour_regex_validation(
     assert response.status_code == expected_status_code
 
 
+def test_create_new_branding_with_no_org_works(
+    platform_admin_client, mocker, fake_uuid, mock_create_email_branding, mock_get_organisations
+):
+    data = {
+        "logo": None,
+        "colour": "#ff0000",
+        "text": "new text",
+        "name": "new name",
+        "brand_type": "custom_logo",
+    }
+
+    mocker.patch("app.main.views.email_branding.persist_logo")
+    mocker.patch("app.main.views.email_branding.delete_email_temp_files_created_by")
+
+    platform_admin_client.post(
+        url_for(".create_email_branding"),
+        content_type="multipart/form-data",
+        data=data,
+    )
+
+    assert mock_create_email_branding.called
+    assert mock_create_email_branding.call_args == call(
+        logo=data["logo"],
+        name=data["name"],
+        text=data["text"],
+        colour=data["colour"],
+        brand_type=data["brand_type"],
+        organisation_id=None,
+    )
+
+
+def test_create_new_branding_with_org_works(
+    platform_admin_client, mocker, fake_uuid, mock_create_email_branding, mock_get_organisations
+):
+    data = {
+        "logo": None,
+        "colour": "#ff0000",
+        "text": "new text",
+        "name": "new name",
+        "brand_type": "custom_logo",
+        "organisation": "7aa5d4e9-4385-4488-a489-07812ba13383",
+    }
+
+    mocker.patch("app.main.views.email_branding.persist_logo")
+    mocker.patch("app.main.views.email_branding.delete_email_temp_files_created_by")
+
+    platform_admin_client.post(
+        url_for(".create_email_branding"),
+        content_type="multipart/form-data",
+        data=data,
+    )
+
+    assert mock_create_email_branding.called
+    assert mock_create_email_branding.call_args == call(
+        logo=data["logo"],
+        name=data["name"],
+        text=data["text"],
+        colour=data["colour"],
+        brand_type=data["brand_type"],
+        organisation_id=data["organisation"],
+    )
+
+
 class TestBranding:
     def test_edit_branding_settings_displays_stock_options(self, mocker, service_one, platform_admin_client):
         mocker.patch("app.current_service", {"organisation": {"name": "Test org"}})

tests/app/notify_client/test_email_branding_client.py

@@ -76,6 +76,7 @@ def test_create_email_branding(mocker):
         "text": "test name",
         "colour": "red",
         "brand_type": "custom_logo",
+        "organisation_id": "org-id-123",
     }
 
     mock_post = mocker.patch("app.notify_client.email_branding_client.EmailBrandingClient.post")
@@ -86,11 +87,14 @@ def test_create_email_branding(mocker):
         text=org_data["text"],
         colour=org_data["colour"],
         brand_type="custom_logo",
+        organisation_id=org_data["organisation_id"],
     )
 
     mock_post.assert_called_once_with(url="/email-branding", data=org_data)
-
-    mock_redis_delete.assert_called_once_with("email_branding")
+    mock_redis_delete.call_args_list == [
+        call("email_branding-None"),
+        call("email_branding-org-id-123"),
+    ]
 
 
 def test_update_email_branding(mocker, fake_uuid):
@@ -100,6 +104,7 @@ def test_update_email_branding(mocker, fake_uuid):
         "text": "test name",
         "colour": "red",
         "brand_type": "custom_logo",
+        "organisation_id": "org-id-123",
     }
 
     mock_post = mocker.patch("app.notify_client.email_branding_client.EmailBrandingClient.post")
@@ -111,10 +116,12 @@ def test_update_email_branding(mocker, fake_uuid):
         text=org_data["text"],
         colour=org_data["colour"],
         brand_type="custom_logo",
+        organisation_id=org_data["organisation_id"],
     )
 
     mock_post.assert_called_once_with(url="/email-branding/{}".format(fake_uuid), data=org_data)
     assert mock_redis_delete.call_args_list == [
+        call("email_branding-None"),
+        call("email_branding-{}".format(org_data["organisation_id"])),
         call("email_branding-{}".format(fake_uuid)),
-        call("email_branding"),
     ]