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.

Sign up for GitHub

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

Fix for grant assign email with missing description #2725

Merged
merged 8 commits into from
Mar 12, 2024
Merged

Fix for grant assign email with missing description #2725

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
1a6a3b2
Add test for grant assigned email without grant description
jeffsmohan Mar 6, 2024
e21cb79
Fix for missing description when generating grant detail for email
jeffsmohan Mar 6, 2024
fbf92cf
Make assign grant email resilient to unexpected send errors
jeffsmohan Mar 6, 2024
ddae6bd
Properly await async functions in try/catch blocks
jeffsmohan Mar 7, 2024
9e70696
Merge branch 'main' into jmo-grant-assignment-email-errors
jeffsmohan Mar 8, 2024
ec2b933
Merge branch 'main' into jmo-grant-assignment-email-errors
jeffsmohan Mar 11, 2024
786676a
Merge branch 'main' into jmo-grant-assignment-email-errors
jeffsmohan Mar 11, 2024
f8d887e
Merge branch 'main' into jmo-grant-assignment-email-errors
jeffsmohan Mar 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/server/__tests__/db/seeds/fixtures.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ const grants = {
search_terms: '[in title/desc]+',
reviewer_name: 'none',
opportunity_category: 'Discretionary',
description: '',
description: null,
eligibility_codes: '',
opportunity_status: 'posted',
raw_body_json: {
Expand Down
10 changes: 10 additions & 0 deletions packages/server/__tests__/email/email.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,16 @@ describe('Email sender', () => {
expect(sendFake.secondCall.args[0].emailHTML.includes('<table')).to.equal(true);
expect(sendFake.secondCall.args[0].subject).to.equal('Grant Assigned to State Board of Accountancy');
});
it('is resilient to missing grant description', async () => {
const sendFake = sinon.fake.returns('foo');
sinon.replace(email, 'sendGrantAssignedNotficationForAgency', sendFake);
const grant = fixtures.grants.noDescOrEligibilityCodes;

await email.sendGrantAssignedEmail({ grantId: grant.grant_id, agencyIds: [0], userId: 1 });

expect(sendFake.called).to.equal(true);
expect(sendFake.firstCall.args[1].includes('... View on')).to.equal(true);
});
});
context('async report email', () => {
it('sendAsyncReportEmail delivers an email with the signedURL for audit report', async () => {
Expand Down
16 changes: 12 additions & 4 deletions packages/server/src/lib/email.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const asyncBatch = require('async-batch').default;
const fileSystem = require('fs');
const path = require('path');
const mustache = require('mustache');
const { log } = require('./logging');
const emailService = require('./email/service-email');
const db = require('../db');
const { notificationType } = require('./email/constants');
Expand Down Expand Up @@ -155,7 +156,7 @@ function sendWelcomeEmail(email, httpOrigin) {
function getGrantDetail(grant, emailNotificationType) {
const grantDetailTemplate = fileSystem.readFileSync(path.join(__dirname, '../static/email_templates/_grant_detail.html'));

const description = grant.description.substring(0, 380).replace(/(<([^>]+)>)/ig, '');
const description = grant.description?.substring(0, 380).replace(/(<([^>]+)>)/ig, '');
const grantsUrl = new URL(process.env.WEBSITE_DOMAIN);
if (emailNotificationType === notificationType.grantDigest) {
grantsUrl.pathname = 'grants';
Expand Down Expand Up @@ -240,9 +241,16 @@ async function sendGrantAssignedEmail({ grantId, agencyIds, userId }) {
2b. For each user part of the agency
i. Send email
*/
const grantDetail = await buildGrantDetail(grantId, notificationType.grantAssignment);
const agencies = await db.getAgenciesByIds(agencyIds);
agencies.forEach((agency) => module.exports.sendGrantAssignedNotficationForAgency(agency, grantDetail, userId));
try {
const grantDetail = await buildGrantDetail(grantId, notificationType.grantAssignment);
const agencies = await db.getAgenciesByIds(agencyIds);
agencies.forEach((agency) => module.exports.sendGrantAssignedNotficationForAgency(agency, grantDetail, userId));
} catch (err) {
log.error({
err, grantId, agencyIds, userId,
}, 'Failed to send grant assigned email');
throw err;
}
}

async function buildDigestBody({ name, openDate, matchedGrants }) {
Expand Down
7 changes: 6 additions & 1 deletion packages/server/src/routes/grants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,7 +348,12 @@ router.put('/:grantId/assign/agencies', requireUser, async (req, res) => {
}

await db.assignGrantsToAgencies({ grantId, agencyIds, userId: user.id });
email.sendGrantAssignedEmail({ grantId, agencyIds, userId: user.id });
try {
email.sendGrantAssignedEmail({ grantId, agencyIds, userId: user.id });
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be await email.sendGrantAssignedEmail to ensure that it waits for the function to complete to catch the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, good catch! And that probably also explains why the second test I was trying to write wasn't working... let me see if I can get a test for the 500 response as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, I think the test is unfortunately not worth the effort (unless you have a quick read on how to get it working). I was trying to add something like this to the PUT /api/grants/:grantId/assign/agencies tests in __tests__/api/grants.test.js:

            it('gracefully returns 500 response for unexpected email errors', async () => {
                const emailSpy = sandbox.spy(email, 'sendGrantAssignedNotficationForAgency');
                sandbox.stub(email, 'buildGrantDetail').throws();
                const response = await fetchApi(`/grants/${assignEndpoint}`, agencies.own, {
                    ...fetchOptions.admin,
                    method: 'put',
                    body: JSON.stringify({ agencyIds: [agencies.own] }),
                });
                expect(response.statusText).to.equal('Internal Server Error');
                expect(emailSpy.notCalled).to.equal(true);
            });

I believe the issue is that our existing tools for stubbing and throwing don't have access to stub out the method that's getting called, since it's really more of an integration test than a unit test here. (Otherwise, we could use rewire or sinon to mock out the function the code is calling.) Anyway, I'm going to move on, but if you have the answer here, I'd love to hear it!

} catch {
res.sendStatus(500);
return;
}

res.json({});
});
Expand Down
Loading