-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[#172380989] Add html body to DPO's UserDataProcessing email #65
Conversation
Codecov Report
@@ Coverage Diff @@
## master #65 +/- ##
==========================================
+ Coverage 82.29% 82.32% +0.02%
==========================================
Files 22 22
Lines 627 628 +1
Branches 59 59
==========================================
+ Hits 516 517 +1
Misses 111 111
Continue to review full report at Codecov.
|
Affected stories
Generated by 🚫 dangerJS |
const documentHtml = ` | ||
<!doctype html> | ||
<html xmlns="http://www.w3.org/1999/xhtml" xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office"> | ||
<body style="background-color:#ffffff;"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<body style="background-color:#ffffff;"> | |
<body> |
|
||
const documentHtml = ` | ||
<!doctype html> | ||
<html xmlns="http://www.w3.org/1999/xhtml" xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<html xmlns="http://www.w3.org/1999/xhtml" xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office"> | |
<html> | |
<head> | |
<meta name="viewport" content="width=device-width" /> | |
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8" /> | |
<title>${subject}</title> | |
</head> |
); | ||
if (isRight(errorOrMaybeRetrievedProfile)) { | ||
const maybeRetrievedProfile = errorOrMaybeRetrievedProfile.value; | ||
if (isSome(maybeRetrievedProfile)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isNone branch was missing
codice fiscale: ${fiscalCode} | ||
indirizzo email: ${userEmailAddress}.`; | ||
const documentHtml = ` | ||
export const getDpoEmailText = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useful in tests to avoid to type mock text
@@ -91,40 +67,75 @@ export const getSendUserDataProcessingEmailActivityHandler = ( | |||
<title>${subject}</title> | |||
</head> | |||
<body> | |||
<p>${emailText}</p> | |||
<p>${emailText.replace("\n", "<br>\n")}</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle newlines in html
logPrefix = "SendUserDataProcessingEmail" | ||
) => async (context: Context, input: unknown) => { | ||
const failure = failActivity(context, logPrefix); | ||
return fromEither(ActivityInput.decode(input)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when possible avoid assignement to .value
and calls to isLeft
/ isSome
use fp-ts methods to transform an input into the expected output
|
||
if (isLeft(errorOrSentMessageInfo)) { | ||
context.log.error( | ||
`${logPrefix}|Error sending userDataProcessing email|ERROR=${errorOrSentMessageInfo.value.message}` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's not need to repeat userDataProcessing since w have the log prefix
</body> | ||
</html>`; | ||
// Send an email to the DPO containing the information about the IO user's | ||
// choice to download or delete his own private data stored by the platform | ||
const errorOrSentMessageInfo = await sendMail(mailerTransporter, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prefer working with taskeithers than promise
toError | ||
).foldTaskEither<Error, Option<RetrievedProfile>>( | ||
err => TE.left(new Task(async () => err)), | ||
queryErrorOrMaybeProfile => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all this mess is just to convert QueryError
to a simple Error
since we don't need to discriminate + the promise to a taskeither
@@ -63,10 +86,16 @@ const mailerTransporter = isProduction | |||
secure: false | |||
}); | |||
|
|||
const sendMailTask = (mt: Mail) => ( | |||
options: Mail.Options & { html: Mail.Options["html"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
while the proposed changes fix the error, they don't prevent the error (missing html param) to occur again in the future so I've added a required html parameter here
Added a custom html to NodeMailer configuration due to error in sending phase.