[TTAHUB-3095] Don't remove expired grants from the recipients dropdown when an AR is unlocked #2265
Conversation
nvms
requested review from
thewatermethod,
GarrettEHill,
kryswisnaskas and
AdamAdHocTeam
| @@ -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)); | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Yeah.. I had this same thought. I'll make this change 👍
| @@ -722,6 +722,14 @@ export async function getActivityRecipients(req, res) { | |||
| res.json(activityRecipients); | |||
| } | |||
|
|
|||
| export async function getActivityRecipientsForExistingReport(req, res) { | |||
There was a problem hiding this comment.
Do we need to check permissions here?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I ended up adding AR read permissions to this handler: 440a55a
There was a problem hiding this comment.
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.
|
Everything looks great, just a question and a suggestion |
Description of change
Modifies
possibleRecipientsto accept an optional report id. If the report id is provided, it returns recipients that may be expired, but have been used on this report previously.It also adds a new endpoint
/api/activity-report/activity-recipients/:reportIdHow to test
Use the seeded data. Be Hermione. Give yourself unlock permissions. The report with id 9999 already has a recipient that is Inactive. Unlock report 9999. Edit it, go to the account summary, remove the recipient from the dropdown, expand the dropdown and you should still have the old recipient in the list even though they are expired. Save the report. Be sure the save takes effect.
Issue(s)
Checklists
Every PR
Before merge to main
Production Deploy
After merge/deploy