[MCR-2538] States are notified when CMS uploads questions about a submission #2068
Conversation
pearl-truss
changed the title
…e-review into mcr-2538-email-notifications-to-states
| @@ -186,6 +195,27 @@ function newSESEmailer(config: EmailConfiguration): Emailer { | |||
| return await this.sendEmail(emailData) | |||
| } | |||
| }, | |||
| sendQuestionsStateEmail: async function ( | |||
There was a problem hiding this comment.
as you are working on these four emails, I wonder if we could refactor the SESEmailer and the localEmailer code to be a little less repetitive
|
|
||
| if (template instanceof Error) { | ||
| console.error(template) | ||
| return |
There was a problem hiding this comment.
I would throw this error if it's an error, that will fail the test. I don't think returning will fail the test but maybe it will b/c there haven't been any assertions? In general, if there's some kind of error in a test I like to throw it to fail.
Lots of our test helpers throw (see createAndSubmitTestHealthPlanPackage for instance)
This is a big exception to our general rule of "return Errors, don't throw" but it's basically the API of jest so it makes sense to me
| @@ -77,6 +83,47 @@ export function createQuestionResolver( | |||
| throw new UserInputError(errMessage) | |||
| } | |||
|
|
|||
| const conversionResult = convertContractWithRatesToFormData( | |||
There was a problem hiding this comment.
Can we move away from converting the contract to the old HealthPlanPackage and HealthPlanFormData types? The rates refactor was the first step in deprecating those types. We're still converting to them in order to return them from our handlers, but when we convert the front end to be able to support rates across submissions the plan is to try and root those types out all together.
I think the only real change this would require would be for the sendQuestionsStateEmail function to take in a ContractWithRatesType instead of a HPFD type, but it should have all the data you need to compose the email in it, then.
…ub.com/Enterprise-CMCS/managed-care-review into mcr-2538-email-notifications-to-states
|
@macrael thanks for the review! This is ready for another pass when you get a chance |
There was a problem hiding this comment.
This looks great, so glad to see the HPP stuff gone. I think more refactoring can come in further PRs or in this PR up to you. Also I'm happy to meet and try and work on the refactor together, I think I finally see something in how we could pull the sendEmail function out of the emailer thing
| ], | ||
| } | ||
| const sub: ContractRevisionWithRatesType = { | ||
| ...mockContractRev({ formData: formDataWithDuplicateStateContacts }), |
There was a problem hiding this comment.
what's up with expanding mockContractRev here instead of just using it by itself?
| ) | ||
|
|
||
| if (template instanceof Error) { | ||
| console.error(template) |
There was a problem hiding this comment.
there's a bunch more of these instances where you're returning an error instead of throwing them, can you fix those too?
| @@ -86,6 +102,30 @@ export function createQuestionResolver( | |||
| throw new Error(errMessage) | |||
| } | |||
|
|
|||
| const dateAsked = new Date() | |||
| const sendQuestionsStateEmailResult = | |||
There was a problem hiding this comment.
yay, I love how much less prep there is here now.
| } | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| function newSESEmailer(config: EmailConfiguration): Emailer { |
There was a problem hiding this comment.
I'm very glad you maintained these entry points. I think we can simplify things a bit more. Looking at it I think the main weirdness is that sendEmail is not like the other functions. And I forgot that we also duplicate this structure in our testing helper too. I think we could break out the sendTestEmail, sendSESEmail, sendLocalEmail functions into something that you pass into a single "emailer". I can sketch it out or we can meet and talk it through.
There was a problem hiding this comment.
Since I'm out this afternoon I'll take a pass at making these updates and push them up today. And maybe we can meet tomorrow if there are more changes to work through?
There was a problem hiding this comment.
I pushed up some changes that refactor the emailer that I believe address what you raised here. I do need to fix failing test next, the sendEmail function for the testEmailer isn't returning errors as expected
Summary
This PR creates a way for state users to be notified via email when someone from CMS sends a question for a submission they are a contact for
To Do
createQuestionresolverRelated issues
https://qmacbis.atlassian.net/browse/MCR-2538
Screenshots
Test cases covered
QA guidance