From be553c4657508f2f25dfaa605859c22c5ccb6f9c Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Thu, 18 Feb 2021 10:09:15 -0800 Subject: [PATCH 01/15] Allow to filter grants/grantees based on region --- frontend/src/__tests__/permissions.js | 44 ++++++++++++++++++++- frontend/src/fetchers/activityReports.js | 5 ++- frontend/src/pages/ActivityReport/index.js | 3 +- frontend/src/permissions.js | 31 +++++++++++++++ src/routes/activityReports/handlers.js | 6 ++- src/routes/activityReports/handlers.test.js | 15 ++++++- src/services/activityReports.js | 21 +++++++++- src/services/activityReports.test.js | 34 ++++++++++++++-- 8 files changed, 146 insertions(+), 13 deletions(-) diff --git a/frontend/src/__tests__/permissions.js b/frontend/src/__tests__/permissions.js index 33cbf5f35b..80eba22eaf 100644 --- a/frontend/src/__tests__/permissions.js +++ b/frontend/src/__tests__/permissions.js @@ -1,4 +1,5 @@ -import isAdmin from '../permissions'; +import isAdmin, { allRegionsUserHasPermissionTo } from '../permissions'; +import { SCOPE_IDS } from '../Constants'; describe('permissions', () => { describe('isAdmin', () => { @@ -6,7 +7,7 @@ describe('permissions', () => { const user = { permissions: [ { - scopeId: 2, + scopeId: SCOPE_IDS.ADMIN, }, ], }; @@ -20,4 +21,43 @@ describe('permissions', () => { expect(isAdmin(user)).toBeFalsy(); }); }); + + describe('allRegionsUserHasPermissionTo', () => { + it('returns an array with all the correct regions', () => { + const user = { + permissions: [ + { + scopeId: SCOPE_IDS.ADMIN, + regionId: 14, + }, + { + scopeId: SCOPE_IDS.SITE_ACCESS, + regionId: 14, + }, + { + scopeId: SCOPE_IDS.SITE_ACCESS, + regionId: 1, + }, + { + scopeId: SCOPE_IDS.APPROVE_ACTIVITY_REPORTS, + regionId: 1, + }, + { + scopeId: SCOPE_IDS.READ_WRITE_ACTIVITY_REPORTS, + regionId: 3, + }, + { + scopeId: SCOPE_IDS.READ_ACTIVITY_REPORTS, + regionId: 4, + }, + { + scopeId: SCOPE_IDS.APPROVE_ACTIVITY_REPORTS, + regionId: 4, + }, + ], + }; + const regions = allRegionsUserHasPermissionTo(user); + expect(regions).toEqual(expect.arrayContaining([14, 3, 4])); + }); + }); }); diff --git a/frontend/src/fetchers/activityReports.js b/frontend/src/fetchers/activityReports.js index 414c9883dc..fd6aafe1bf 100644 --- a/frontend/src/fetchers/activityReports.js +++ b/frontend/src/fetchers/activityReports.js @@ -41,8 +41,9 @@ export const getReportAlerts = async () => { return reports.json(); }; -export const getRecipients = async () => { - const recipients = await get(join(activityReportUrl, 'activity-recipients')); +export const getRecipients = async (regions) => { + const params = regions.map((region) => `regions=${region}`); + const recipients = await get(join(activityReportUrl, 'activity-recipients', `?${params.join('&')}`)); return recipients.json(); }; diff --git a/frontend/src/pages/ActivityReport/index.js b/frontend/src/pages/ActivityReport/index.js index 0c26d8a6e8..77d10c5fa2 100644 --- a/frontend/src/pages/ActivityReport/index.js +++ b/frontend/src/pages/ActivityReport/index.js @@ -10,6 +10,7 @@ import ReactRouterPropTypes from 'react-router-prop-types'; import { useHistory, Redirect } from 'react-router-dom'; import { Alert, Grid } from '@trussworks/react-uswds'; import moment from 'moment'; +import { allRegionsUserHasPermissionTo } from '../../permissions'; import pages from './Pages'; import Navigator from '../../components/Navigator'; @@ -94,7 +95,7 @@ function ActivityReport({ match, user, location }) { updateLoading(true); const apiCalls = [ - getRecipients(), + getRecipients(allRegionsUserHasPermissionTo(user)), getCollaborators(region), getApprovers(region), ]; diff --git a/frontend/src/permissions.js b/frontend/src/permissions.js index 049d9383cb..e916b63b46 100644 --- a/frontend/src/permissions.js +++ b/frontend/src/permissions.js @@ -13,4 +13,35 @@ const isAdmin = (user) => { ) !== undefined; }; +/** + * Return all regions that user has a minimum of read access to. + * All permissions that qualify this criteria are: + * Admin + * Read Activity Reports + * Read Write Activity Reports + * @param {*} - user object + * @returns {array} - An array of integers, where each integer signifies a region. + */ + +export const allRegionsUserHasPermissionTo = (user) => { + const permissions = _.get(user, 'permissions'); + + if (!permissions) return []; + + const minPermissions = [ + SCOPE_IDS.ADMIN, + SCOPE_IDS.READ_ACTIVITY_REPORTS, + SCOPE_IDS.READ_WRITE_ACTIVITY_REPORTS, + ]; + + const regions = []; + permissions.forEach((perm) => { + if (minPermissions.includes(perm.scopeId)) { + regions.push(perm.regionId); + } + }); + + return _.uniq(regions); +}; + export default isAdmin; diff --git a/src/routes/activityReports/handlers.js b/src/routes/activityReports/handlers.js index 6b14fe14ba..ca95c1c484 100644 --- a/src/routes/activityReports/handlers.js +++ b/src/routes/activityReports/handlers.js @@ -120,7 +120,11 @@ export async function submitReport(req, res) { } export async function getActivityRecipients(req, res) { - const activityRecipients = await possibleRecipients(); + const { regions } = req.query; + const targetRegions = regions + ? regions.map((region) => parseInt(region, DECIMAL_BASE)) + : undefined; + const activityRecipients = await possibleRecipients(targetRegions); res.json(activityRecipients); } diff --git a/src/routes/activityReports/handlers.test.js b/src/routes/activityReports/handlers.test.js index 2e78260f2a..86d21a376e 100644 --- a/src/routes/activityReports/handlers.test.js +++ b/src/routes/activityReports/handlers.test.js @@ -234,10 +234,21 @@ describe('Activity Report handlers', () => { }); describe('getActivityRecipients', () => { - it('returns recipients', async () => { + it('returns recipients when regions query param is passed', async () => { const response = [{ test: 'test' }]; possibleRecipients.mockResolvedValue(response); - await getActivityRecipients(mockRequest, mockResponse); + + const mockRequestWithRegion = { ...mockRequest, query: { regions: ['1', '14'] } }; + await getActivityRecipients(mockRequestWithRegion, mockResponse); + expect(mockResponse.json).toHaveBeenCalledWith(response); + }); + + it('returns recipients when regions query param is not passed', async () => { + const response = [{ test: 'test' }]; + possibleRecipients.mockResolvedValue(response); + + const mockRequestWithNoRegion = { ...mockRequest, query: {} }; + await getActivityRecipients(mockRequestWithNoRegion, mockResponse); expect(mockResponse.json).toHaveBeenCalledWith(response); }); }); diff --git a/src/services/activityReports.js b/src/services/activityReports.js index 5dbbca0c13..dfcfb19405 100644 --- a/src/services/activityReports.js +++ b/src/services/activityReports.js @@ -362,10 +362,29 @@ export async function createOrUpdate(newActivityReport, report) { return activityReportById(savedReport.id); } -export async function possibleRecipients() { +/* + * Queries the db for relevant recipients depending on the region ids. + * If no region ids are passed, then default to returning all available recipients. + * Note: This only affects grants and grantees. Non Grantees remain unaffected by region ids. + * + * @param {Array} [regionIds] - List of region ids to query against + * @returns {*} Grants and Non grantees + */ + +export async function possibleRecipients(regionIds) { + let where; + if (regionIds) { + where = { + regionId: { + [Op.in]: regionIds, + }, + }; + } + const grants = await Grantee.findAll({ attributes: ['id', 'name'], include: [{ + where, model: Grant, as: 'grants', attributes: [['id', 'activityRecipientId'], 'name', 'number'], diff --git a/src/services/activityReports.test.js b/src/services/activityReports.test.js index 547dd3b2fb..03d7ef6ef1 100644 --- a/src/services/activityReports.test.js +++ b/src/services/activityReports.test.js @@ -1,8 +1,8 @@ import db, { - ActivityReport, ActivityRecipient, User, Grantee, NonGrantee, Grant, + ActivityReport, ActivityRecipient, User, Grantee, NonGrantee, Grant, Region, } from '../models'; import { - createOrUpdate, activityReportById, + createOrUpdate, activityReportById, possibleRecipients, } from './activityReports'; import { REPORT_STATUSES } from '../constants'; @@ -27,8 +27,11 @@ describe('Activity Reports DB service', () => { beforeAll(async () => { await User.create(mockUser); - grantee = await Grantee.create({ id: 100, name: 'grantee' }); - await Grant.create({ id: 100, number: 1, granteeId: grantee.id }); + await Region.create({ name: 'office 17', id: 17 }); + grantee = await Grantee.create({ id: 100, name: 'grantee', regionId: 17 }); + await Grant.create({ + id: 100, number: 1, granteeId: grantee.id, regionId: 17, + }); await NonGrantee.create({ id: 100, name: 'nonGrantee' }); }); @@ -39,6 +42,7 @@ describe('Activity Reports DB service', () => { await NonGrantee.destroy({ where: { id: 100 } }); await Grant.destroy({ where: { id: 100 } }); await Grantee.destroy({ where: { id: 100 } }); + await Region.destroy({ where: { id: 17 } }); db.sequelize.close(); }); @@ -85,4 +89,26 @@ describe('Activity Reports DB service', () => { expect(foundReport.resourcesUsed).toBe('test'); }); }); + + describe('possibleRecipients', () => { + it('retrieves correct recipients in region', async () => { + const regions = [17]; + const recipients = await possibleRecipients(regions); + + expect(recipients.grants.length).toBe(1); + }); + + it('retrieves no recipients in empty region ', async () => { + const regions = [100]; + const recipients = await possibleRecipients(regions); + + expect(recipients.grants.length).toBe(0); + }); + + it('retrieves all recipients when not specifying region ', async () => { + const recipients = await possibleRecipients(); + // 8 From db being seeded + 1 that we create for this test suite = 9 + expect(recipients.grants.length).toBe(9); + }); + }); }); From 41772869bbc32b2edad34c701335bd2dd121076e Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Fri, 19 Feb 2021 13:25:40 -0800 Subject: [PATCH 02/15] Refactor to query for an exact region instead of multiple regions --- frontend/src/fetchers/activityReports.js | 5 ++--- frontend/src/pages/ActivityReport/index.js | 3 +-- src/routes/activityReports/handlers.js | 8 +++----- src/routes/activityReports/handlers.test.js | 8 ++++---- src/seeders/20201210172017-grantees.js | 12 ++++++++++++ src/seeders/20201211172017-grants.js | 18 ++++++++++++++++++ src/services/activityReports.js | 19 ++++++------------- 7 files changed, 46 insertions(+), 27 deletions(-) diff --git a/frontend/src/fetchers/activityReports.js b/frontend/src/fetchers/activityReports.js index fd6aafe1bf..34f3034c10 100644 --- a/frontend/src/fetchers/activityReports.js +++ b/frontend/src/fetchers/activityReports.js @@ -41,9 +41,8 @@ export const getReportAlerts = async () => { return reports.json(); }; -export const getRecipients = async (regions) => { - const params = regions.map((region) => `regions=${region}`); - const recipients = await get(join(activityReportUrl, 'activity-recipients', `?${params.join('&')}`)); +export const getRecipients = async (region) => { + const recipients = await get(join(activityReportUrl, 'activity-recipients', `?region=${region}`)); return recipients.json(); }; diff --git a/frontend/src/pages/ActivityReport/index.js b/frontend/src/pages/ActivityReport/index.js index 77d10c5fa2..2a50c84d0b 100644 --- a/frontend/src/pages/ActivityReport/index.js +++ b/frontend/src/pages/ActivityReport/index.js @@ -10,7 +10,6 @@ import ReactRouterPropTypes from 'react-router-prop-types'; import { useHistory, Redirect } from 'react-router-dom'; import { Alert, Grid } from '@trussworks/react-uswds'; import moment from 'moment'; -import { allRegionsUserHasPermissionTo } from '../../permissions'; import pages from './Pages'; import Navigator from '../../components/Navigator'; @@ -95,7 +94,7 @@ function ActivityReport({ match, user, location }) { updateLoading(true); const apiCalls = [ - getRecipients(allRegionsUserHasPermissionTo(user)), + getRecipients(region), getCollaborators(region), getApprovers(region), ]; diff --git a/src/routes/activityReports/handlers.js b/src/routes/activityReports/handlers.js index ca95c1c484..efcecb8d34 100644 --- a/src/routes/activityReports/handlers.js +++ b/src/routes/activityReports/handlers.js @@ -120,11 +120,9 @@ export async function submitReport(req, res) { } export async function getActivityRecipients(req, res) { - const { regions } = req.query; - const targetRegions = regions - ? regions.map((region) => parseInt(region, DECIMAL_BASE)) - : undefined; - const activityRecipients = await possibleRecipients(targetRegions); + const { region } = req.query; + const targetRegion = region? parseInt(region, DECIMAL_BASE) : undefined; + const activityRecipients = await possibleRecipients(targetRegion); res.json(activityRecipients); } diff --git a/src/routes/activityReports/handlers.test.js b/src/routes/activityReports/handlers.test.js index 86d21a376e..6972a10712 100644 --- a/src/routes/activityReports/handlers.test.js +++ b/src/routes/activityReports/handlers.test.js @@ -234,12 +234,12 @@ describe('Activity Report handlers', () => { }); describe('getActivityRecipients', () => { - it('returns recipients when regions query param is passed', async () => { + it('returns recipients when region query param is passed', async () => { const response = [{ test: 'test' }]; possibleRecipients.mockResolvedValue(response); - const mockRequestWithRegion = { ...mockRequest, query: { regions: ['1', '14'] } }; - await getActivityRecipients(mockRequestWithRegion, mockResponse); + const mockRequestWithRegion = { ...mockRequest, query: { region: 14 } }; + const resp = await getActivityRecipients(mockRequestWithRegion, mockResponse); expect(mockResponse.json).toHaveBeenCalledWith(response); }); @@ -248,7 +248,7 @@ describe('Activity Report handlers', () => { possibleRecipients.mockResolvedValue(response); const mockRequestWithNoRegion = { ...mockRequest, query: {} }; - await getActivityRecipients(mockRequestWithNoRegion, mockResponse); + const resp = await getActivityRecipients(mockRequestWithNoRegion, mockResponse); expect(mockResponse.json).toHaveBeenCalledWith(response); }); }); diff --git a/src/seeders/20201210172017-grantees.js b/src/seeders/20201210172017-grantees.js index 725c3e9545..a798477b98 100644 --- a/src/seeders/20201210172017-grantees.js +++ b/src/seeders/20201210172017-grantees.js @@ -31,6 +31,18 @@ const grantees = [ id: 8, name: 'Agency 4, Inc.', }, + { + id: 9, + name: 'Agency 1.a in region 1, Inc.', + }, + { + id: 10, + name: 'Agency 1.b in region 1, Inc.', + }, + { + id: 11, + name: 'Agency 2 in region 1, Inc.', + }, ]; module.exports = { diff --git a/src/seeders/20201211172017-grants.js b/src/seeders/20201211172017-grants.js index 208d855436..35fc702c6a 100644 --- a/src/seeders/20201211172017-grants.js +++ b/src/seeders/20201211172017-grants.js @@ -53,6 +53,24 @@ const grants = [ regionId: 9, granteeId: 8, }, + { + id: 10, + number: '01HP044444', + regionId: 1, + granteeId: 9, + }, + { + id: 11, + number: '01HP022222', + regionId: 1, + granteeId: 10, + }, + { + id: 12, + number: '09HP01111', + regionId: 1, + granteeId: 11, + }, ]; module.exports = { diff --git a/src/services/activityReports.js b/src/services/activityReports.js index dfcfb19405..8eabf5bb36 100644 --- a/src/services/activityReports.js +++ b/src/services/activityReports.js @@ -363,23 +363,16 @@ export async function createOrUpdate(newActivityReport, report) { } /* - * Queries the db for relevant recipients depending on the region ids. - * If no region ids are passed, then default to returning all available recipients. - * Note: This only affects grants and grantees. Non Grantees remain unaffected by region ids. + * Queries the db for relevant recipients depending on the region id. + * If no region id is passed, then default to returning all available recipients. + * Note: This only affects grants and grantees. Non Grantees remain unaffected by the region id. * - * @param {Array} [regionIds] - List of region ids to query against + * @param {number} [regionId] - A region id to query against * @returns {*} Grants and Non grantees */ -export async function possibleRecipients(regionIds) { - let where; - if (regionIds) { - where = { - regionId: { - [Op.in]: regionIds, - }, - }; - } +export async function possibleRecipients(regionId) { + const where = regionId? { regionId } : undefined; const grants = await Grantee.findAll({ attributes: ['id', 'name'], From 20d19157d6835d1deaac29bfd694d22cfe51b11d Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Fri, 19 Feb 2021 14:23:41 -0800 Subject: [PATCH 03/15] eslint fixes --- src/routes/activityReports/handlers.js | 2 +- src/routes/activityReports/handlers.test.js | 4 ++-- src/services/activityReports.js | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/routes/activityReports/handlers.js b/src/routes/activityReports/handlers.js index efcecb8d34..ff31473e08 100644 --- a/src/routes/activityReports/handlers.js +++ b/src/routes/activityReports/handlers.js @@ -121,7 +121,7 @@ export async function submitReport(req, res) { export async function getActivityRecipients(req, res) { const { region } = req.query; - const targetRegion = region? parseInt(region, DECIMAL_BASE) : undefined; + const targetRegion = region ? parseInt(region, DECIMAL_BASE) : undefined; const activityRecipients = await possibleRecipients(targetRegion); res.json(activityRecipients); } diff --git a/src/routes/activityReports/handlers.test.js b/src/routes/activityReports/handlers.test.js index 6972a10712..8eedf74e32 100644 --- a/src/routes/activityReports/handlers.test.js +++ b/src/routes/activityReports/handlers.test.js @@ -239,7 +239,7 @@ describe('Activity Report handlers', () => { possibleRecipients.mockResolvedValue(response); const mockRequestWithRegion = { ...mockRequest, query: { region: 14 } }; - const resp = await getActivityRecipients(mockRequestWithRegion, mockResponse); + await getActivityRecipients(mockRequestWithRegion, mockResponse); expect(mockResponse.json).toHaveBeenCalledWith(response); }); @@ -248,7 +248,7 @@ describe('Activity Report handlers', () => { possibleRecipients.mockResolvedValue(response); const mockRequestWithNoRegion = { ...mockRequest, query: {} }; - const resp = await getActivityRecipients(mockRequestWithNoRegion, mockResponse); + await getActivityRecipients(mockRequestWithNoRegion, mockResponse); expect(mockResponse.json).toHaveBeenCalledWith(response); }); }); diff --git a/src/services/activityReports.js b/src/services/activityReports.js index 8eabf5bb36..aa9c1af938 100644 --- a/src/services/activityReports.js +++ b/src/services/activityReports.js @@ -372,7 +372,7 @@ export async function createOrUpdate(newActivityReport, report) { */ export async function possibleRecipients(regionId) { - const where = regionId? { regionId } : undefined; + const where = regionId ? { regionId } : undefined; const grants = await Grantee.findAll({ attributes: ['id', 'name'], From f87716639937f6c3f5d9d668b30c01a63f29a90d Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Mon, 22 Feb 2021 08:52:06 -0800 Subject: [PATCH 04/15] Add way of finding out region to use when no region is passed --- frontend/src/__tests__/permissions.js | 2 +- frontend/src/pages/ActivityReport/index.js | 11 +++++++---- frontend/src/permissions.js | 16 ++++++++++++++++ src/services/activityReports.test.js | 4 ++-- 4 files changed, 26 insertions(+), 7 deletions(-) diff --git a/frontend/src/__tests__/permissions.js b/frontend/src/__tests__/permissions.js index 96f1262ae9..93ff520c21 100644 --- a/frontend/src/__tests__/permissions.js +++ b/frontend/src/__tests__/permissions.js @@ -1,5 +1,5 @@ import { SCOPE_IDS } from '../Constants'; -import isAdmin, { hasReadWrite } from '../permissions'; +import isAdmin, { hasReadWrite, allRegionsUserHasPermissionTo } from '../permissions'; describe('permissions', () => { describe('isAdmin', () => { diff --git a/frontend/src/pages/ActivityReport/index.js b/frontend/src/pages/ActivityReport/index.js index 2a50c84d0b..88b074b615 100644 --- a/frontend/src/pages/ActivityReport/index.js +++ b/frontend/src/pages/ActivityReport/index.js @@ -17,6 +17,7 @@ import Navigator from '../../components/Navigator'; import './index.css'; import { NOT_STARTED } from '../../components/Navigator/constants'; import { REPORT_STATUSES } from '../../Constants'; +import { getRegionWithReadWrite } from '../../permissions'; import { submitReport, saveReport, @@ -80,6 +81,8 @@ function ActivityReport({ match, user, location }) { const [initialLastUpdated, updateInitialLastUpdated] = useState(); const reportId = useRef(); + const regionIdToUse = region || getRegionWithReadWrite(user); + const showLastUpdatedTime = (location.state && location.state.showLastUpdatedTime) || false; useEffect(() => { @@ -94,9 +97,9 @@ function ActivityReport({ match, user, location }) { updateLoading(true); const apiCalls = [ - getRecipients(region), - getCollaborators(region), - getApprovers(region), + getRecipients(regionIdToUse), + getCollaborators(regionIdToUse), + getApprovers(regionIdToUse), ]; if (activityReportId !== 'new') { @@ -173,7 +176,7 @@ function ActivityReport({ match, user, location }) { if (canWrite) { if (reportId.current === 'new') { if (activityRecipientType && activityRecipients && activityRecipients.length > 0) { - const savedReport = await createReport({ ...data, regionId: region }, {}); + const savedReport = await createReport({ ...data, regionId: regionIdToUse }, {}); reportId.current = savedReport.id; updatedReport = false; } diff --git a/frontend/src/permissions.js b/frontend/src/permissions.js index 49d0677e6b..64de97b8fd 100644 --- a/frontend/src/permissions.js +++ b/frontend/src/permissions.js @@ -44,6 +44,22 @@ export const allRegionsUserHasPermissionTo = (user) => { return _.uniq(regions); }; +/** + * Search the user's permissions for any region they have read/write permissions to. + * Return *first* region that matches this criteria. Otherwise return -1. + * @param {*} user - user object + * @returns {number} - region id if the user has read/write access for a region, -1 otherwise + */ + +export const getRegionWithReadWrite = (user) => { + const { permissions } = user; + if (!permissions) return -1; + + const perm = permissions.find((p) => p.scopeId === SCOPE_IDS.READ_WRITE_ACTIVITY_REPORTS); + return perm? permission.regionId : -1; +} + +/** * Search the user's permissions for a read/write permisions for a region * @param {*} user - user object * @returns {boolean} - True if the user has re/write access for a region, false otherwise diff --git a/src/services/activityReports.test.js b/src/services/activityReports.test.js index 03d7ef6ef1..5aa316b981 100644 --- a/src/services/activityReports.test.js +++ b/src/services/activityReports.test.js @@ -107,8 +107,8 @@ describe('Activity Reports DB service', () => { it('retrieves all recipients when not specifying region ', async () => { const recipients = await possibleRecipients(); - // 8 From db being seeded + 1 that we create for this test suite = 9 - expect(recipients.grants.length).toBe(9); + // 11 From db being seeded + 1 that we create for this test suite = 12 + expect(recipients.grants.length).toBe(12); }); }); }); From 18e53de664a956a31afaf105c368f30b19b8a46c Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Mon, 22 Feb 2021 08:54:40 -0800 Subject: [PATCH 05/15] Fix variable name typo --- frontend/src/permissions.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/permissions.js b/frontend/src/permissions.js index 64de97b8fd..110163055a 100644 --- a/frontend/src/permissions.js +++ b/frontend/src/permissions.js @@ -56,8 +56,8 @@ export const getRegionWithReadWrite = (user) => { if (!permissions) return -1; const perm = permissions.find((p) => p.scopeId === SCOPE_IDS.READ_WRITE_ACTIVITY_REPORTS); - return perm? permission.regionId : -1; -} + return perm ? perm.regionId : -1; +}; /** * Search the user's permissions for a read/write permisions for a region From 8b950e8eb98b79ebf8d3a9a75854146cd6462d0f Mon Sep 17 00:00:00 2001 From: Josh Salisbury Date: Wed, 24 Feb 2021 11:28:19 -0600 Subject: [PATCH 06/15] Duration supports half hours --- frontend/src/pages/ActivityReport/Pages/activitySummary.js | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/src/pages/ActivityReport/Pages/activitySummary.js b/frontend/src/pages/ActivityReport/Pages/activitySummary.js index f75f1b4bb3..22d70720f0 100644 --- a/frontend/src/pages/ActivityReport/Pages/activitySummary.js +++ b/frontend/src/pages/ActivityReport/Pages/activitySummary.js @@ -257,6 +257,7 @@ const ActivitySummary = ({ name="duration" type="number" min={0} + step={0.5} inputRef={ register({ required: 'Please enter the duration of the activity', From 673bcbc44ab17f1fdcc8c4051a69305714f0020c Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Thu, 25 Feb 2021 13:43:50 -0800 Subject: [PATCH 07/15] Updat test --- frontend/src/permissions.js | 2 +- src/services/activityReports.test.js | 18 +++++++++++------- 2 files changed, 12 insertions(+), 8 deletions(-) diff --git a/frontend/src/permissions.js b/frontend/src/permissions.js index 110163055a..807bbab90c 100644 --- a/frontend/src/permissions.js +++ b/frontend/src/permissions.js @@ -22,7 +22,7 @@ const isAdmin = (user) => { * @param {*} - user object * @returns {array} - An array of integers, where each integer signifies a region. */ - +// FIXME: Descide if we will keep this or remove export const allRegionsUserHasPermissionTo = (user) => { const permissions = _.get(user, 'permissions'); diff --git a/src/services/activityReports.test.js b/src/services/activityReports.test.js index c60a917eb1..e04a972247 100644 --- a/src/services/activityReports.test.js +++ b/src/services/activityReports.test.js @@ -1,5 +1,5 @@ import db, { - ActivityReport, ActivityRecipient, User, Grantee, NonGrantee, Grant, NextStep, + ActivityReport, ActivityRecipient, User, Grantee, NonGrantee, Grant, NextStep, Region, } from '../models'; import { createOrUpdate, activityReportById, possibleRecipients, @@ -29,8 +29,11 @@ describe('Activity Reports DB service', () => { beforeAll(async () => { await User.create(mockUser); - grantee = await Grantee.create({ id: RECIPIENT_ID, name: 'grantee' }); - await Grant.create({ id: RECIPIENT_ID, number: 1, granteeId: grantee.id }); + grantee = await Grantee.create({ id: RECIPIENT_ID, name: 'grantee', regionId: 17 }); + await Region.create({ name: 'office 17', id: 17 }); + await Grant.create({ + id: RECIPIENT_ID, number: 1, granteeId: grantee.id, regionId: 17, + }); await NonGrantee.create({ id: RECIPIENT_ID, name: 'nonGrantee' }); }); @@ -42,6 +45,7 @@ describe('Activity Reports DB service', () => { await Grant.destroy({ where: { id: RECIPIENT_ID } }); await Grantee.destroy({ where: { id: RECIPIENT_ID } }); await NextStep.destroy({ where: {} }); + await Region.destroy({ where: { id: 17 } }); db.sequelize.close(); }); @@ -234,15 +238,15 @@ describe('Activity Reports DB service', () => { describe('possibleRecipients', () => { it('retrieves correct recipients in region', async () => { - const regions = [17]; - const recipients = await possibleRecipients(regions); + const region = 17; + const recipients = await possibleRecipients(region); expect(recipients.grants.length).toBe(1); }); it('retrieves no recipients in empty region ', async () => { - const regions = [100]; - const recipients = await possibleRecipients(regions); + const region = 100; + const recipients = await possibleRecipients(region); expect(recipients.grants.length).toBe(0); }); From a476d3f0a2450a8f950404dbd82b289d50a99c9d Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Thu, 25 Feb 2021 13:56:27 -0800 Subject: [PATCH 08/15] Update Mock in test --- frontend/src/pages/ActivityReport/__tests__/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/pages/ActivityReport/__tests__/index.js b/frontend/src/pages/ActivityReport/__tests__/index.js index 654f233988..ae6d568eb3 100644 --- a/frontend/src/pages/ActivityReport/__tests__/index.js +++ b/frontend/src/pages/ActivityReport/__tests__/index.js @@ -62,7 +62,7 @@ describe('ActivityReport', () => { afterEach(() => fetchMock.restore()); beforeEach(() => { - fetchMock.get('/api/activity-reports/activity-recipients', recipients); + fetchMock.get('/api/activity-reports/activity-recipients?region=1', recipients); fetchMock.get('/api/users/collaborators?region=1', []); fetchMock.get('/api/activity-reports/approvers?region=1', []); }); From f18d01ed2a82b140af82dc8800d224daa0bca031 Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Thu, 25 Feb 2021 14:18:25 -0800 Subject: [PATCH 09/15] Add missing tests --- frontend/src/__tests__/permissions.js | 51 ++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/frontend/src/__tests__/permissions.js b/frontend/src/__tests__/permissions.js index 93ff520c21..526faad366 100644 --- a/frontend/src/__tests__/permissions.js +++ b/frontend/src/__tests__/permissions.js @@ -1,5 +1,5 @@ import { SCOPE_IDS } from '../Constants'; -import isAdmin, { hasReadWrite, allRegionsUserHasPermissionTo } from '../permissions'; +import isAdmin, { hasReadWrite, allRegionsUserHasPermissionTo, getRegionWithReadWrite } from '../permissions'; describe('permissions', () => { describe('isAdmin', () => { @@ -86,4 +86,53 @@ describe('permissions', () => { expect(hasReadWrite(user)).toBeFalsy(); }); }); + + describe('getRegionWithReadWrite', () => { + it('returns region where user has permission', () => { + const user = { + permissions: [ + { + regionId: 4, + scopeId: SCOPE_IDS.READ_ACTIVITY_REPORTS, + }, + { + regionId: 1, + scopeId: SCOPE_IDS.ADMIN, + }, + { + regionId: 2, + scopeId: SCOPE_IDS.READ_WRITE_ACTIVITY_REPORTS, + }, + ], + }; + + const region = getRegionWithReadWrite(user); + expect(region).toBe(2); + }); + + it('returns no region', () => { + const user = { + permissions: [ + { + regionId: 4, + scopeId: SCOPE_IDS.READ_ACTIVITY_REPORTS, + }, + { + regionId: 1, + scopeId: SCOPE_IDS.ADMIN, + }, + ], + }; + + const region = getRegionWithReadWrite(user); + expect(region).toBe(-1); + }); + + it('returns region because user object has no permissions', () => { + const user = {}; + + const region = getRegionWithReadWrite(user); + expect(region).toBe(-1); + }); + }); }); From 46d47e1a6b9dd142fea6486979e9036d30f68322 Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Fri, 26 Feb 2021 11:40:50 -0800 Subject: [PATCH 10/15] Add review from feedback --- frontend/src/__tests__/permissions.js | 6 +++ .../pages/ActivityReport/__tests__/index.js | 11 ++++ frontend/src/pages/ActivityReport/index.js | 50 ++++++++++++------- 3 files changed, 48 insertions(+), 19 deletions(-) diff --git a/frontend/src/__tests__/permissions.js b/frontend/src/__tests__/permissions.js index 526faad366..af256fceb9 100644 --- a/frontend/src/__tests__/permissions.js +++ b/frontend/src/__tests__/permissions.js @@ -59,6 +59,12 @@ describe('permissions', () => { const regions = allRegionsUserHasPermissionTo(user); expect(regions).toEqual(expect.arrayContaining([14, 3, 4])); }); + + it('returns empty array when user has no permissions', () => { + const user = {} + const regions = allRegionsUserHasPermissionTo(user); + expect(regions).toEqual([]); + }) }); describe('hasReadWrite', () => { diff --git a/frontend/src/pages/ActivityReport/__tests__/index.js b/frontend/src/pages/ActivityReport/__tests__/index.js index ae6d568eb3..cedc253fd4 100644 --- a/frontend/src/pages/ActivityReport/__tests__/index.js +++ b/frontend/src/pages/ActivityReport/__tests__/index.js @@ -14,6 +14,7 @@ import { withText } from '../../../testHelpers'; import ActivityReport from '../index'; const formData = () => ({ + regionId: 1, deliveryMethod: 'in-person', ttaType: ['training'], duration: '1', @@ -74,9 +75,18 @@ describe('ActivityReport', () => { expect(alert).toHaveTextContent('Unable to load activity report'); }); + it('handles when region is invalid', async () => { + fetchMock.get('/api/activity-reports/-1', () => { throw new Error('unable to download report'); }); + + renderActivityReport('-1'); + const alert = await screen.findByTestId('alert'); + expect(alert).toHaveTextContent('Unable to load activity report'); + }); + describe('last saved time', () => { it('is shown if history.state.showLastUpdatedTime is true', async () => { const data = formData(); + fetchMock.get('/api/activity-reports/1', data); renderActivityReport('1', 'activity-summary', true); await screen.findByRole('group', { name: 'Who was the activity for?' }, { timeout: 4000 }); @@ -85,6 +95,7 @@ describe('ActivityReport', () => { it('is not shown if history.state.showLastUpdatedTime is null', async () => { const data = formData(); + fetchMock.get('/api/activity-reports/1', data); renderActivityReport('1', 'activity-summary'); await screen.findByRole('group', { name: 'Who was the activity for?' }); diff --git a/frontend/src/pages/ActivityReport/index.js b/frontend/src/pages/ActivityReport/index.js index 7908d640be..fe440b4501 100644 --- a/frontend/src/pages/ActivityReport/index.js +++ b/frontend/src/pages/ActivityReport/index.js @@ -16,7 +16,7 @@ import Navigator from '../../components/Navigator'; import './index.css'; import { NOT_STARTED } from '../../components/Navigator/constants'; -import { REPORT_STATUSES } from '../../Constants'; +import { REPORT_STATUSES, DECIMAL_BASE } from '../../Constants'; import { getRegionWithReadWrite } from '../../permissions'; import { submitReport, @@ -56,12 +56,12 @@ const defaultValues = { status: REPORT_STATUSES.DRAFT, }; -// FIXME: default region until we have a way of changing on the frontend -const region = 1; + const pagesByPos = _.keyBy(pages.filter((p) => !p.review), (page) => page.position); const defaultPageState = _.mapValues(pagesByPos, () => NOT_STARTED); -function ActivityReport({ match, user, location }) { +// FIXME: default region until we have a way of changing on the frontend +function ActivityReport({ match, user, location, region=1 }) { const { params: { currentPage, activityReportId } } = match; const history = useHistory(); const [error, updateError] = useState(); @@ -73,8 +73,6 @@ function ActivityReport({ match, user, location }) { const [initialLastUpdated, updateInitialLastUpdated] = useState(); const reportId = useRef(); - const regionIdToUse = region || getRegionWithReadWrite(user); - const showLastUpdatedTime = (location.state && location.state.showLastUpdatedTime) || false; useEffect(() => { @@ -84,26 +82,29 @@ function ActivityReport({ match, user, location }) { }, [activityReportId, history]); useEffect(() => { + let report; + const fetch = async () => { try { updateLoading(true); - - const apiCalls = [ - getRecipients(regionIdToUse), - getCollaborators(regionIdToUse), - getApprovers(regionIdToUse), - ]; - if (activityReportId !== 'new') { - apiCalls.push(getReport(activityReportId)); + report = await getReport(activityReportId); } else { - apiCalls.push( - Promise.resolve({ ...defaultValues, pageState: defaultPageState, userId: user.id }), - ); + report = { + ...defaultValues, + pageState: defaultPageState, + userId: user.id, + regionId: region || getRegionWithReadWrite(user), + }; } - const [recipients, collaborators, approvers, report] = await Promise.all(apiCalls); + const apiCalls = [ + getRecipients(report.regionId), + getCollaborators(report.regionId), + getApprovers(report.regionId), + ]; + const [recipients, collaborators, approvers] = await Promise.all(apiCalls); reportId.current = activityReportId; const isCollaborator = report.collaborators @@ -123,6 +124,11 @@ function ActivityReport({ match, user, location }) { updateError(); } catch (e) { updateError('Unable to load activity report'); + // If the error was caused by an invalid region, we need a way to communicate that to the + // component so we can redirect the user. We can do this by updating the form data + if (report && parseInt(report.regionId, DECIMAL_BASE) === -1) { + updateFormData({ regionId: report.regionId }); + } } finally { updateLoading(false); } @@ -138,6 +144,12 @@ function ActivityReport({ match, user, location }) { ); } + // If no region was able to be found, we will re-reoute user to the main page + // FIXME: when re-routing user show a message explaining what happened + if (formData && parseInt(formData.regionId, DECIMAL_BASE) === -1) { + return ; + } + if (error) { return ( @@ -168,7 +180,7 @@ function ActivityReport({ match, user, location }) { if (canWrite) { if (reportId.current === 'new') { if (activityRecipientType && activityRecipients && activityRecipients.length > 0) { - const savedReport = await createReport({ ...data, regionId: regionIdToUse }, {}); + const savedReport = await createReport({ ...data, regionId: formData.regionId }, {}); reportId.current = savedReport.id; const current = pages.find((p) => p.path === currentPage); updatePage(current.position); From 6e189dd1c7db384e374220a308e43dfc006d47d0 Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Fri, 26 Feb 2021 11:48:48 -0800 Subject: [PATCH 11/15] Lint fix --- frontend/src/__tests__/permissions.js | 4 ++-- frontend/src/pages/ActivityReport/index.js | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/frontend/src/__tests__/permissions.js b/frontend/src/__tests__/permissions.js index af256fceb9..8f14a6ab73 100644 --- a/frontend/src/__tests__/permissions.js +++ b/frontend/src/__tests__/permissions.js @@ -61,10 +61,10 @@ describe('permissions', () => { }); it('returns empty array when user has no permissions', () => { - const user = {} + const user = {}; const regions = allRegionsUserHasPermissionTo(user); expect(regions).toEqual([]); - }) + }); }); describe('hasReadWrite', () => { diff --git a/frontend/src/pages/ActivityReport/index.js b/frontend/src/pages/ActivityReport/index.js index fe440b4501..35dc379000 100644 --- a/frontend/src/pages/ActivityReport/index.js +++ b/frontend/src/pages/ActivityReport/index.js @@ -56,12 +56,13 @@ const defaultValues = { status: REPORT_STATUSES.DRAFT, }; - const pagesByPos = _.keyBy(pages.filter((p) => !p.review), (page) => page.position); const defaultPageState = _.mapValues(pagesByPos, () => NOT_STARTED); // FIXME: default region until we have a way of changing on the frontend -function ActivityReport({ match, user, location, region=1 }) { +function ActivityReport({ + match, user, location, region = 1, +}) { const { params: { currentPage, activityReportId } } = match; const history = useHistory(); const [error, updateError] = useState(); @@ -241,6 +242,7 @@ function ActivityReport({ match, user, location, region=1 }) { ActivityReport.propTypes = { match: ReactRouterPropTypes.match.isRequired, location: ReactRouterPropTypes.location.isRequired, + region: PropTypes.number, user: PropTypes.shape({ id: PropTypes.number, name: PropTypes.string, @@ -248,4 +250,8 @@ ActivityReport.propTypes = { }).isRequired, }; +ActivityReport.defaultProps = { + region: 1, +}; + export default ActivityReport; From 3fde7c5174a3a66aea7f4958d16a4b2579ad28ef Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Fri, 26 Feb 2021 12:45:10 -0800 Subject: [PATCH 12/15] Updates from review --- .../src/pages/ActivityReport/__tests__/index.js | 5 +++++ frontend/src/pages/ActivityReport/index.js | 13 ++++--------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/frontend/src/pages/ActivityReport/__tests__/index.js b/frontend/src/pages/ActivityReport/__tests__/index.js index cedc253fd4..e0dc542be5 100644 --- a/frontend/src/pages/ActivityReport/__tests__/index.js +++ b/frontend/src/pages/ActivityReport/__tests__/index.js @@ -12,6 +12,11 @@ import userEvent from '@testing-library/user-event'; import { withText } from '../../../testHelpers'; import ActivityReport from '../index'; +// import { getRegionWithReadWrite } from '../../../permissions'; + +jest.mock('../../../permissions', () => ({ + getRegionWithReadWrite: jest.fn(() => 1), +})); const formData = () => ({ regionId: 1, diff --git a/frontend/src/pages/ActivityReport/index.js b/frontend/src/pages/ActivityReport/index.js index 35dc379000..59218cd532 100644 --- a/frontend/src/pages/ActivityReport/index.js +++ b/frontend/src/pages/ActivityReport/index.js @@ -59,9 +59,8 @@ const defaultValues = { const pagesByPos = _.keyBy(pages.filter((p) => !p.review), (page) => page.position); const defaultPageState = _.mapValues(pagesByPos, () => NOT_STARTED); -// FIXME: default region until we have a way of changing on the frontend function ActivityReport({ - match, user, location, region = 1, + match, user, location, region, }) { const { params: { currentPage, activityReportId } } = match; const history = useHistory(); @@ -83,9 +82,9 @@ function ActivityReport({ }, [activityReportId, history]); useEffect(() => { - let report; - const fetch = async () => { + let report; + try { updateLoading(true); if (activityReportId !== 'new') { @@ -242,7 +241,7 @@ function ActivityReport({ ActivityReport.propTypes = { match: ReactRouterPropTypes.match.isRequired, location: ReactRouterPropTypes.location.isRequired, - region: PropTypes.number, + region: PropTypes.number.isRequired, user: PropTypes.shape({ id: PropTypes.number, name: PropTypes.string, @@ -250,8 +249,4 @@ ActivityReport.propTypes = { }).isRequired, }; -ActivityReport.defaultProps = { - region: 1, -}; - export default ActivityReport; From 2d7201e59a15923fc1d3e53f8d9bd195ed2df786 Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Fri, 26 Feb 2021 12:56:36 -0800 Subject: [PATCH 13/15] Remove unused file --- frontend/src/hooks.js | 13 ------------- 1 file changed, 13 deletions(-) delete mode 100644 frontend/src/hooks.js diff --git a/frontend/src/hooks.js b/frontend/src/hooks.js deleted file mode 100644 index 3a5764bcd2..0000000000 --- a/frontend/src/hooks.js +++ /dev/null @@ -1,13 +0,0 @@ -// Disable prefer default export, this file will probably accumulate additional -// custom hooks at some point -/* eslint-disable import/prefer-default-export */ -// See https://reactjs.org/docs/hooks-faq.html#how-to-get-the-previous-props-or-state -import { useEffect, useRef } from 'react'; - -export function usePrevious(value) { - const ref = useRef(); - useEffect(() => { - ref.current = value; - }); - return ref.current; -} From 7282fd55ff3dacea204560aa4e9adeae94843f5e Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Mon, 1 Mar 2021 06:31:13 -0800 Subject: [PATCH 14/15] Make Region non-required --- frontend/src/pages/ActivityReport/index.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/frontend/src/pages/ActivityReport/index.js b/frontend/src/pages/ActivityReport/index.js index 59218cd532..8a190c8c88 100644 --- a/frontend/src/pages/ActivityReport/index.js +++ b/frontend/src/pages/ActivityReport/index.js @@ -241,7 +241,7 @@ function ActivityReport({ ActivityReport.propTypes = { match: ReactRouterPropTypes.match.isRequired, location: ReactRouterPropTypes.location.isRequired, - region: PropTypes.number.isRequired, + region: PropTypes.number, user: PropTypes.shape({ id: PropTypes.number, name: PropTypes.string, @@ -249,4 +249,8 @@ ActivityReport.propTypes = { }).isRequired, }; +ActivityReport.defaultProps = { + region: undefined, +}; + export default ActivityReport; From f0533413363aace05321356d497582b92d6cb147 Mon Sep 17 00:00:00 2001 From: Daniel Riquiac Gopar Date: Mon, 1 Mar 2021 06:48:45 -0800 Subject: [PATCH 15/15] Switch to deep compare useEffect --- frontend/src/pages/ActivityReport/index.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/frontend/src/pages/ActivityReport/index.js b/frontend/src/pages/ActivityReport/index.js index 8a190c8c88..ef2ff94911 100644 --- a/frontend/src/pages/ActivityReport/index.js +++ b/frontend/src/pages/ActivityReport/index.js @@ -9,6 +9,7 @@ import { Helmet } from 'react-helmet'; import ReactRouterPropTypes from 'react-router-prop-types'; import { useHistory, Redirect } from 'react-router-dom'; import { Alert, Grid } from '@trussworks/react-uswds'; +import useDeepCompareEffect from 'use-deep-compare-effect'; import moment from 'moment'; import pages from './Pages'; @@ -81,7 +82,7 @@ function ActivityReport({ history.replace(); }, [activityReportId, history]); - useEffect(() => { + useDeepCompareEffect(() => { const fetch = async () => { let report; @@ -134,7 +135,7 @@ function ActivityReport({ } }; fetch(); - }, [activityReportId, user.id, showLastUpdatedTime]); + }, [activityReportId, user, showLastUpdatedTime, region]); if (loading) { return (