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

[TTAHUB-3095] Don't remove expired grants from the recipients dropdown when an AR is unlocked #2265

Merged
merged 11 commits into from
Jul 15, 2024
Merged

[TTAHUB-3095] Don't remove expired grants from the recipients dropdown when an AR is unlocked #2265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
de51a81
initial pass
nvms Jul 11, 2024
5b4dfbf
different call when new vs. not-new
nvms Jul 11, 2024
aab5350
fix AR tests
nvms Jul 11, 2024
2c10411
fix localStorage tests
nvms Jul 11, 2024
8c2a5e0
don't force programType
nvms Jul 11, 2024
fd1a662
endpoint path change
nvms Jul 12, 2024
0ef613c
update test
nvms Jul 15, 2024
440a55a
put behind read permissions
nvms Jul 15, 2024
c678a4c
update test
nvms Jul 15, 2024
3a79374
get region id from the report
nvms Jul 15, 2024
7cd85f7
dont pass region id
nvms Jul 15, 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
6 changes: 6 additions & 0 deletions frontend/src/fetchers/activityReports.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ export const getRecipients = async (region) => {
return recipients.json();
};

export const getRecipientsForExistingAR = async (reportId) => {
const url = join(activityReportUrl, `${reportId}`, 'activity-recipients');
const recipients = await get(url);
return recipients.json();
};

export const getGoals = async (grantIds) => {
const params = grantIds.map((grantId) => `grantIds=${grantId}`);
const url = join(activityReportUrl, 'goals', `?${params.join('&')}`);
Expand Down
4 changes: 4 additions & 0 deletions frontend/src/pages/ActivityReport/__tests__/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ describe('ActivityReport', () => {

beforeEach(() => {
fetchMock.get('/api/activity-reports/activity-recipients?region=1', recipients);
fetchMock.get('/api/activity-reports/1/activity-recipients', recipients);
fetchMock.get('/api/activity-reports/groups?region=1', [{
id: 110,
name: 'Group 1',
Expand Down Expand Up @@ -155,6 +156,7 @@ describe('ActivityReport', () => {
};

fetchMock.get('/api/activity-reports/activity-recipients?region=1', groupRecipients, { overwriteRoutes: true });
fetchMock.get('/api/activity-reports/1/activity-recipients', groupRecipients, { overwriteRoutes: true });

const data = formData();
fetchMock.get('/api/activity-reports/1', { ...data, activityRecipients: [] });
Expand Down Expand Up @@ -226,6 +228,7 @@ describe('ActivityReport', () => {
};

fetchMock.get('/api/activity-reports/activity-recipients?region=1', groupRecipients, { overwriteRoutes: true });
fetchMock.get('/api/activity-reports/1/activity-recipients', groupRecipients, { overwriteRoutes: true });

const data = formData();
fetchMock.get('/api/activity-reports/1', { ...data, activityRecipients: [] });
Expand Down Expand Up @@ -363,6 +366,7 @@ describe('ActivityReport', () => {

describe('resetToDraft', () => {
it('navigates to the correct page', async () => {
fetchMock.get('/api/activity-reports/3/activity-recipients', recipients);
const data = formData();
// load the report
fetchMock.get('/api/activity-reports/3', {
Expand Down
1 change: 1 addition & 0 deletions frontend/src/pages/ActivityReport/__tests__/localStorage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ describe('Local storage fallbacks', () => {

beforeEach(() => {
fetchMock.get('/api/activity-reports/activity-recipients?region=1', recipients);
fetchMock.get('/api/activity-reports/1/activity-recipients', recipients);
fetchMock.get('/api/activity-reports/groups?region=1', []);
fetchMock.get('/api/users/collaborators?region=1', []);
fetchMock.get('/api/activity-reports/approvers?region=1', []);
Expand Down
13 changes: 11 additions & 2 deletions frontend/src/pages/ActivityReport/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,14 @@ import {
submitReport,
saveReport,
getReport,
getRecipients,
getRecipientsForExistingAR,
createReport,
getCollaborators,
getApprovers,
reviewReport,
resetToDraft,
getGroupsForActivityReport,
getRecipients,
} from '../../fetchers/activityReports';
import useLocalStorage, { setConnectionActiveWithError } from '../../hooks/useLocalStorage';
import NetworkContext, { isOnlineMode } from '../../NetworkContext';
Expand Down Expand Up @@ -277,8 +278,16 @@ function ActivityReport({
};
}

const getRecips = async () => {
if (reportId.current && reportId.current !== 'new') {
return getRecipientsForExistingAR(reportId.current);
}

return getRecipients(report.regionId);
};

const apiCalls = [
getRecipients(report.regionId),
getRecips(),
getCollaborators(report.regionId),
getApprovers(report.regionId),
getGroupsForActivityReport(report.regionId),
Expand Down
18 changes: 18 additions & 0 deletions src/routes/activityReports/handlers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,24 @@ export async function getActivityRecipients(req, res) {
res.json(activityRecipients);
}

export async function getActivityRecipientsForExistingReport(req, res) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to check permissions here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not actually sure. What do you think? What we're potentially exposing (to already-authenticated users) is which grant recipients were previously associated with a given AR. My initial thought is that this isn't sensitive - but maybe I"m wrong

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose without context that info doesn't feel particularly sensitive. The only counterargument is that all the other report-related data is confined to a region. I'm ok with site-access level permissions here, personally. Although, I am not the decider.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I ended up adding AR read permissions to this handler: 440a55a

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks good. One thing to consider is that if you are fetching the report, you no longer need to rely on the region ID being passed as a query parameter, since the report has a regionID.

const { activityReportId } = req.params;

const [report] = await activityReportAndRecipientsById(activityReportId);
const userId = await currentUserId(req, res);
const user = await userById(userId);
const authorization = new ActivityReport(user, report);

if (!authorization.canGet()) {
res.sendStatus(403);
return;
}

const targetRegion = parseInt(report.regionId, DECIMAL_BASE);
const activityRecipients = await possibleRecipients(targetRegion, activityReportId);
res.json(activityRecipients);
}

/**
* Retrieve an activity report
*
Expand Down
3 changes: 3 additions & 0 deletions src/routes/activityReports/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
getReports,
getReportAlerts,
getActivityRecipients,
getActivityRecipientsForExistingReport,
getGoals,
reviewReport,
resetToDraft,
Expand Down Expand Up @@ -40,6 +41,7 @@ router.post('/', transactionWrapper(createReport));
router.get('/approvers', transactionWrapper(getApprovers));
router.get('/groups', transactionWrapper(getGroups));
router.get('/activity-recipients', transactionWrapper(getActivityRecipients));
router.get('/activity-recipients/:reportId', transactionWrapper(getActivityRecipientsForExistingReport));
Copy link
Collaborator

Choose a reason for hiding this comment

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

might be a useless change, but how do we feel about flipping this to be /:reportId/activity-recipients (i.e /api/activity-reports/1/activity-recipients)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah.. I had this same thought. I'll make this change 👍

router.get('/goals', transactionWrapper(getGoals));
router.post('/goals', transactionWrapper(createGoalsForReport));
router.post('/objectives', transactionWrapper(saveOtherEntityObjectivesForReport));
Expand All @@ -60,5 +62,6 @@ router.put('/:activityReportId/review', checkActivityReportIdParam, transactionW
router.put('/:activityReportId/submit', checkActivityReportIdParam, transactionWrapper(submitReport));
router.put('/:activityReportId/unlock', checkActivityReportIdParam, transactionWrapper(unlockReport));
router.put('/:activityReportId/goals/edit', checkActivityReportIdParam, transactionWrapper(setGoalAsActivelyEdited));
router.get('/:activityReportId/activity-recipients', transactionWrapper(getActivityRecipientsForExistingReport));

export default router;
56 changes: 39 additions & 17 deletions src/services/activityReports.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
ActivityReportCollaborator,
ActivityReportFile,
sequelize,
Sequelize,
ActivityRecipient,
File,
Grant,
Expand Down Expand Up @@ -1125,33 +1126,54 @@ export async function setStatus(report, status) {
* @param {number} [regionId] - A region id to query against
* @returns {*} Grants and Other entities
*/
export async function possibleRecipients(regionId) {
const where = { status: 'Active', regionId };

export async function possibleRecipients(regionId, activityReportId = null) {
const grants = await Recipient.findAll({
attributes: ['id', 'name'],
attributes: [
'id',
'name',
],
order: ['name'],
include: [{
where,
model: Grant,
as: 'grants',
attributes: [['id', 'activityRecipientId'], 'name', 'number'],
include: [{
model: Recipient,
as: 'recipient',
},
include: [
{
model: Program,
as: 'programs',
attributes: ['programType'],
model: Grant,
as: 'grants',
attributes: ['number', ['id', 'activityRecipientId'], 'name'],
required: true,
include: [
{
model: Program,
as: 'programs',
attributes: ['programType'],
required: false,
},
{
model: Recipient,
as: 'recipient',
required: true,
},
{
model: ActivityRecipient,
as: 'activityRecipients',
attributes: [],
required: false,
},
],
},
],
where: {
'$grants.regionId$': regionId,
[Op.or]: [
{ '$grants.status$': 'Active' },
{ '$grants->activityRecipients.activityReportId$': activityReportId },
],
}],
},
});

const otherEntities = await OtherEntity.findAll({
raw: true,
attributes: [['id', 'activityRecipientId'], 'name'],
});

return { grants, otherEntities };
}

Expand Down