PC-690 alternative confirmation email for pending LA users

[Glenn-Clarke]

THIS PR BUILDS UPON PC-778

When a user reaches the confirmation page and they've selected a local authority with a Pending status, a "pending" template is sent to the user.

Live LA user:

Pending LA user:

[Glenn-Clarke]

Noticed an issue with the copy text for the email "lcoal". I've fixed this on GovNotify.

[jdgage]

@Glenn-Clarke I expect it would make each of these easier to review if you were to change the base branch to the next one down in the chain, if that makes sense. Can figure out how they're best off being merged later on. Would you mind doing that?

[Glenn-Clarke]

I've chained the merges :)

[jamiehumphries]

It would be good to add a test for your new functionality here, to check that it sends the correct email for a pending LA.

[Glenn-Clarke]

@jamiehumphries I've added the test to this, but to be able to Mock the response from the custodian code lookup, I had to make the lookup function virtual. There must be a better way to do this, surely?

[jamiehumphries]

@Glenn-Clarke, I think the best thing to do is just find a custodian code to use that matches what you need for the tests.

You don't always need to use mocks for everything, if you can set up your data appropriately without them.

e.g.

private string liveCustodianCode;
private string pendingCustodianCode;
...

[SetUp]
public void Setup()
{
    liveCustodianCode = GetExampleCustodianCodeForStatus(LocalAuthorityData.Hug2Status.Live);
    pendingCustodianCode = GetExampleCustodianCodeForStatus(LocalAuthorityData.Hug2Status.Pending);
    ...
}

...

[Test]
public void SomeTestThatNeedsPending()
{
    var testCustodianCode = pendingCustodianCode;
    ...
}

...

private string GetExampleCustodianCodeForStatus(LocalAuthorityData.Hug2Status status)
{
    return LocalAuthorityData.LocalAuthorityDetailsByCustodianCode
        .First(entry => entry.Value.Status == status)
        .Key;
}

[Glenn-Clarke]

Hi this is really useful, thank you for the example,
I think I didn't go for something like this option originally because I thought that with something like Pending, we can't always assume there will be Local Authorities in that stage. So I didn't want to source one from the list, because then the tests would fail if that situation occurred, not because there was an issue with the code.

[jamiehumphries]

Yes, that's definitely a fair concern in general. Your other option in that case would be to add a dummy Pending LA in to the dictionary during the test setup.

I think either is fine in our particular case, where the LA data is all defined in code. The point at which there are no "Pending" LAs is controlled in the code, so the test can't start failing due to a non-code data change.

But yes, if you wanted to defend against there being no "Pending" ones, you could achieve that with a more involved setup that adds dummy data to the dictionary. Up to you if you want to implement that here or not. 🙂

[Glenn-Clarke]

That makes sense, thanks for explaining the thought process :)

[jamiehumphries]

@Glenn-Clarke, I spotted this typo in your image above, but it looks like you have already fixed it on Notify:

Can you confirm that you have indeed fixed it and I'm not just looking in the wrong place?

[Glenn-Clarke]

Hi @jamiehumphries ,
Yes, it's a bit mixed up with all the comments at the top, but I confirmed the change as the first comment after the original PR :)
I ought to have edited the original post at the top alongside the screenshots to be clearer

[jamiehumphries]

When I run these tests locally, I'm seeing three failures:

Do they pass for you?

[jamiehumphries]

NB: I haven't fully reviewed the test code below, since I expect that there will be a reasonable number of changes by the the time you've fixed these tests and implemented my suggestion to remove the need for virtual.

[Glenn-Clarke]

This was my console upon reopening the tab in the IDE today, and I'm sure I ran them prior to committing.

Still, they seem to be failing now, and I'll be making the changes you've given anyway

[jamiehumphries]

@Glenn-Clarke, I think the best thing to do is just find a custodian code to use that matches what you need for the tests.

You don't always need to use mocks for everything, if you can set up your data appropriately without them.

e.g.

private string liveCustodianCode;
private string pendingCustodianCode;
...

[SetUp]
public void Setup()
{
    liveCustodianCode = GetExampleCustodianCodeForStatus(LocalAuthorityData.Hug2Status.Live);
    pendingCustodianCode = GetExampleCustodianCodeForStatus(LocalAuthorityData.Hug2Status.Pending);
    ...
}

...

[Test]
public void SomeTestThatNeedsPending()
{
    var testCustodianCode = pendingCustodianCode;
    ...
}

...

private string GetExampleCustodianCodeForStatus(LocalAuthorityData.Hug2Status status)
{
    return LocalAuthorityData.LocalAuthorityDetailsByCustodianCode
        .First(entry => entry.Value.Status == status)
        .Key;
}

[jamiehumphries]

Hi @jamiehumphries , Yes, it's a bit mixed up with all the comments at the top, but I confirmed the change as the first comment after the original PR :) I ought to have edited the original post at the top alongside the screenshots to be clearer

Ah, so you did! Thanks. 😄

[Glenn-Clarke]

Hi, while sorting the tests I noticed there was one case where Pending wasn't respected,

• User declines to provide an email but gives phone number
• Referral is generated without email
• User enters email on subsequent Confirmation page & their Local Authority is Pending

I've added that case in, and then added the corresponding test for it.

[jamiehumphries]

Hi, while sorting the tests I noticed there was one case where Pending wasn't respected,

• User declines to provide an email but gives phone number
• Referral is generated without email
• User enters email on subsequent Confirmation page & their Local Authority is Pending

I've added that case in, and then added the corresponding test for it.

Well spotted, thanks. 🙂

[jamiehumphries]

Good to merge. 👍