EES-5753 API notification subscribers receive email to highlight new major version has breaking changes #5606

mmoalin · 2025-02-14T13:06:26Z

This PR adds a new email template to be used by API data set notification subscribers. It also uses an existing email template but renames it to be more accurate for its purpose.

When a major data set version is published, this e-mail template will be sent:

When a minor data set version is published, the subscribers will receive a message that uses this template:

If a new published data set's version contains breaking changes (i.e., has a major version increment), then an email which highlights this is sent. Otherwise, if the new published data set contains only minor changes, a different e-mail template will be sent.

This uses a small utility method in the related classes that can be unit tested to decide whether to send the major data set email or the minor data set email by checking the version number received by the function send_email is a new major version (and is not the very first major version published).

This also adds the associated template IDs as secrets in the infrastructure templates json. Please note this requires manual configurations in the infrastructure to add the new template id (major data set updates) and rename the existing other template id (minor data set updates).

...ucation.ExploreEducationStatistics.Notifier.Tests/Functions/ApiSubscriptionFunctionsTests.cs

leeoades · 2025-02-14T16:49:46Z

...ucation.ExploreEducationStatistics.Notifier.Tests/Functions/ApiSubscriptionFunctionsTests.cs

@@ -746,6 +747,43 @@ await functions.RemoveExpiredSubscriptions(
        }
    }

+    public class ApiSubscriptionEmailsHelperTests
+    {
+        [Theory]


It would make more sense I think if the test setup had what the current version is, what the new version is, and then the expected template. Because in the data here, if I go from v.1.9 to v2.1, you're expecting that to be a minor version?

Good question, I don't think it is possible to go from v.1.9 to v2.1 so I believe the test cases are fit for purpose as is unless I've misinterpreted your comment.
I'll try to confirm this from the team but from my testing I have found no way of doing this within the EES system.

It's worth checking. It does seem overly restrictive. For example, you have a test below that says that version 0.0.1 is a minor release, even though that is likely to be the first release.

Thanks Lee!

For example, you have a test below that says that version 0.0.1 is a minor release, even though that is likely to be the first release.

This is the logic as is (and as expected by the ticket): the first release (regardless of if the data set version is a minor or a major release) sends the 'API data set version published' email template ID and because I started to include the word 'minor version' into it i.e., ApiSubscriptionMinorDataSetVersionNotificationId; maybe this is the source of the confusion and is worth a second look from my POV.

I have updated the wording of the shared template ID to make it more accurate to the meaning of the function. Also this is what Cam (the PO) named these templates in the first place so I probably should've just stuck to the script.
I think we should use: ApiSubscriptionDataSetVersionPublishedId & ApiSubscriptionMajorDataSetVersionPublishedId to support this after thinking about your initial impression and feedback Lee. Looking forward to discussing this some more.

I have added coverage of this new logic for the integration test thanks to Lee's help.

...ation.ExploreEducationStatistics.Notifier/Options/EmailTemplateForNotificationSubscribers.cs

src/GovUk.Education.ExploreEducationStatistics.Notifier/Services/ApiSubscriptionService.cs

...ation.ExploreEducationStatistics.Notifier/Options/EmailTemplateForNotificationSubscribers.cs

leeoades · 2025-02-14T17:47:00Z

...ucation.ExploreEducationStatistics.Notifier.Tests/Functions/ApiSubscriptionFunctionsTests.cs

@@ -746,6 +747,43 @@ await functions.RemoveExpiredSubscriptions(
        }
    }

+    public class ApiSubscriptionEmailsHelperTests
+    {
+        [Theory]


It's worth checking. It does seem overly restrictive. For example, you have a test below that says that version 0.0.1 is a minor release, even though that is likely to be the first release.

leeoades · 2025-02-14T17:50:05Z

...ucation.ExploreEducationStatistics.Notifier.Tests/Functions/ApiSubscriptionFunctionsTests.cs

@@ -560,7 +561,7 @@ public async Task Success()
            fixture.NotificationClient
                .Setup(mock => mock.SendEmail(
                    Email,
-                    GetGovUkNotifyOptions().EmailTemplates.ApiSubscriptionNotificationId,
+                    GetGovUkNotifyOptions().EmailTemplates.ApiSubscriptionMinorDataSetVersionNotificationId,


This setup only allows the minor template version to be used. If your intention is that this test doesn't care, then instead consider using A._
Having said that, do you have a test that ensures SendEmail is being called with the expected template anywhere?

Just some background: this change is because I renamed the variable storing a template ID shared by this test and other bits of the codebase. This is a pre-existing test that isn't intended to change unless I have to and so this was one of those cases (as renaming the variable affected this test).

So this test was from before, when the only expectation was that it would send an email with the single template id. There are now two different scenarios, based on the version number. Therefore, this test needs to be expanded on - with two different initial conditions (version major and version minor) and two different expectations (id major and id minor).
To put it another way, you can change the ApiSubscriptionService code to this:

emailService.SendEmail( email: subscription.RowKey, templateId: govUkNotifyOptions.Value.EmailTemplates.ApiSubscriptionMinorDataSetVersionNotificationId, values: personalisation);

and all of the tests pass.

The test case NotifySubscribersTests.Success uses mocks in order to instantiate INotificationClient (which implements SendEmail). The new logic sits outside of what is being mocked which is why I was inclined to use a separate unit test to ensure it's coverage.

There are many instances in the codebase that uses the SendEmail method in tests and I'm put off from making any changes to any tests that are unrelated to this ticket unless I have to which was the case in this instance.
For an expansion of my thoughts please see comments under ApiSubscriptionFunctionsTests.cs

Very grateful for your explanation on this Lee. I've updated the integration tests to test the correct template ID is called based on if the new version contains breaking changes or not.

leeoades · 2025-02-17T17:02:37Z

...ucation.ExploreEducationStatistics.Notifier.Tests/Functions/ApiSubscriptionFunctionsTests.cs

            await CreateApiSubscription(subscription);
-
+            var templates = GetGovUkNotifyOptions().EmailTemplates;


Is this call needed anymore?

leeoades · 2025-02-17T17:05:28Z

.gitignore

@@ -99,3 +99,4 @@ src/GovUk.Education.ExploreEducationStatistics.Admin/appsettings.IdpBootstrapUse

 ## IDE Files (e.g. local Visual Studio configuration)
 *.csproj.user
+/src/GovUk.Education.ExploreEducationStatistics.slnLaunch.user


Might as well ignore all *.user files.

… in new publications of api data sets. Uses a small utility class to determine which template id to use. Adds template ID references to infrastructure (template.json). Note this requires a manual faffy deploy ticket to add the required secrets in each environment.

mmoalin force-pushed the EES-5753-Highlight-breaking-changes-in-API-email-updates branch from dbd33bd to 1ac37c8 Compare February 14, 2025 15:33

mmoalin changed the title ~~EES-5753 Add and use new email template to highlight breaking changes…~~ EES-5753 API notification subscribers receive email to highlight new major version has breaking changes Feb 14, 2025

mmoalin marked this pull request as ready for review February 14, 2025 16:24

leeoades reviewed Feb 14, 2025

View reviewed changes

mmoalin force-pushed the EES-5753-Highlight-breaking-changes-in-API-email-updates branch from f0efa02 to 64485e2 Compare February 14, 2025 17:40

leeoades reviewed Feb 14, 2025

View reviewed changes

mmoalin force-pushed the EES-5753-Highlight-breaking-changes-in-API-email-updates branch 6 times, most recently from faa8f54 to 841b038 Compare February 17, 2025 16:58

leeoades reviewed Feb 17, 2025

View reviewed changes

mmoalin force-pushed the EES-5753-Highlight-breaking-changes-in-API-email-updates branch from b5c09c3 to 95409c5 Compare February 17, 2025 17:10

leeoades approved these changes Feb 17, 2025

View reviewed changes

N-moh added do not merge Don't merge just yet and removed do not merge Don't merge just yet labels Feb 17, 2025

mmoalin merged commit 50118de into dev Feb 18, 2025
10 checks passed

mmoalin deleted the EES-5753-Highlight-breaking-changes-in-API-email-updates branch February 18, 2025 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EES-5753 API notification subscribers receive email to highlight new major version has breaking changes #5606

EES-5753 API notification subscribers receive email to highlight new major version has breaking changes #5606

mmoalin commented Feb 14, 2025 •

edited

Loading

leeoades Feb 14, 2025

mmoalin Feb 14, 2025

leeoades Feb 14, 2025

mmoalin Feb 14, 2025 •

edited

Loading

mmoalin Feb 15, 2025

mmoalin Feb 17, 2025

leeoades Feb 14, 2025

leeoades Feb 14, 2025

mmoalin Feb 14, 2025

leeoades Feb 14, 2025

mmoalin Feb 14, 2025 •

edited

Loading

mmoalin Feb 17, 2025

leeoades Feb 17, 2025

leeoades Feb 17, 2025

		await CreateApiSubscription(subscription);

		var templates = GetGovUkNotifyOptions().EmailTemplates;

EES-5753 API notification subscribers receive email to highlight new major version has breaking changes #5606

EES-5753 API notification subscribers receive email to highlight new major version has breaking changes #5606

Conversation

mmoalin commented Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmoalin Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmoalin Feb 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmoalin commented Feb 14, 2025 •

edited

Loading

mmoalin Feb 14, 2025 •

edited

Loading

mmoalin Feb 14, 2025 •

edited

Loading