Skip to content
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

WAITP-1479: Enable sending exports as an attachment to an email #5260

Merged
merged 6 commits into from
Jan 12, 2024

Conversation

alexd-bes
Copy link
Contributor

Issue WAITP-1479: Enable sending exports as an attachment to an email

Changes:

  • Update constructExportEmail to generate an attachment when emailAsAttachment is true
  • Update server-boilerplate Route to handle when response type is download but contents is not set
  • Update export route to trigger a timeout and send attachments via email after 30 seconds

Copy link
Collaborator

@rohan-bes rohan-bes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this @acdunham 🙏

Just a few comments around variable naming a code format, it took me a little bit of time to grasp exactly what was going on so that's why I made the suggestions.

But it's all looking good to pre-approving 👍

};

export const constructExportEmail = async (responseBody, req) => {
const { emailAsAttachment } = req.query;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const { emailAsAttachment } = req.query;
const { emailExportFileMode } = req.query;

Where emailExportFileMode: 'attachment' | 'downloadLink' (and defaulting to downloadLink). Feels a little clearer to me what this configuration does and what the options are

Comment on lines 43 to 51
export const constructExportEmail = async (responseBody, req) => {
const { emailAsAttachment } = req.query;
const attachments = await generateAttachments(responseBody, emailAsAttachment);
const message = constructMessage(responseBody, emailAsAttachment);
return {
subject: 'Your export from Tupaia',
message,
attachments,
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const constructExportEmail = async (responseBody, req) => {
const { emailAsAttachment } = req.query;
const attachments = await generateAttachments(responseBody, emailAsAttachment);
const message = constructMessage(responseBody, emailAsAttachment);
return {
subject: 'Your export from Tupaia',
message,
attachments,
};
export const constructExportEmail = async (responseBody, req) => {
const { emailExportFileMode = 'downloadLink' } = req.query;
const { error, filePath } = responseBody;
const subject = 'Your export from Tupaia';
if (error) {
return {
subject,
message: `Unfortunately, your export failed.
${error}`
}
}
if (!filePath) {
throw new Error('No filePath in export response body');
}
if (emailExportFileMode === 'attachment') {
return {
subject,
message: 'Please find your requested export attached to this email.',
attachments: await generateAttachments(filePath),
}
}
const downloadLink = createDownloadLink(filePath);
return {
subject,
message: `Please click this one-time link to download your requested export: ${downloadLink}
Note that you need to be logged in to the admin panel for it to work, and after clicking it once, you won't be able to download the file again.`
}

Decided to try re-writing this to flow a bit more clearly. What do you think? My preferences for this style were:

  • Fewer instances of passing branching logic between functions (the emailExportFileMode logic now gets handled in one place)
  • Not returning the attachments property when it's not needed feels a bit cleaner

@alexd-bes alexd-bes merged commit 6d81728 into dev Jan 12, 2024
42 checks passed
@alexd-bes alexd-bes deleted the waitp-1479-email-attachments-exports branch January 12, 2024 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants