M2-6042: Validate Params from Admin to Web App #438
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
Since there's a fallback error for an invalid applet ID, I'll exit earliest for that error. That way the user doesn't have to wait for the other queries to finish unnecessarily
…he take now flow This is a pre-emptive move for an upcoming ticket on the admin side
|
The acceptance criteria for this ticket lists coordinator as one of the roles that should be able to perform this flow. However, I've removed this ability pre-emptively in anticipation of M2-6348 |
This reverts commit ec29433.
sultanofcardio
requested review from
moiskillnadne,
farmerpaul,
mbanting and
ChaconC
There was a problem hiding this comment.
Hey @sultanofcardio, great work on this! Must have been quite a slog having to take into account all of that nuanced validation logic…
I had just a couple minor suggestions, see what you think, happy to hear your thoughts there.
I did run through the peer testing, and I discovered a couple issues. First, here's what I found:
| Source | Subject | Outcome |
|---|---|---|
| owner | Limited account | ❌ Doesn't start activity (no error message) |
| manager | Limited account | ✅ Starts activity |
| coordinator | Limited account | 🤔 You are not authorized to complete this assessment on behalf of this subject. |
| editor | Limited account | 🤔 This subject does not exist |
| reviewer | Limited account | 🤔 This subject does not exist |
| owner | Team member | ❌ Doesn't start activity (no error message) |
| manager | Team member | ✅ Starts activity |
| coordinator | Themself | 🤔 You are not authorized to complete this assessment on behalf of this subject. |
| editor | Themself | 🤔 This subject does not exist |
| reviewer | Themself | 🤔 This subject does not exist |
| respondent | Limited account | 🤔 This subject does not exist |
| respondent | Themself | 🤔 This subject does not exist |
My main concern:
- When I try to use Take Now as the owner, it always takes me to the Activity List, rather than initiate the activity. And there's no error message. I'm wondering if I've got my applet configured wrong, or do you experience the same thing?
Lesser concern:
- I'm not sure what the error message is supposed to be – but I would guess that it's supposed to be the same for all of the error cases above? Mainly, it doesn't make a whole lot of sense to me for the "This subject does not exist" error to appear most of the time.
sultanofcardio
force-pushed
the
feature/M2-6042-validate-take-now-params
branch
from
455e1a8 to
20d4334
Compare
|
Thanks for the thorough review as usual @farmerpaul. I implemented all of your suggestions! About the outcome of your testing, your table should look like the one I included in the PR description. That means something is amiss somewhere.
No, the owner (and manager) should be able to start the activity for any other user. The only cases where the activity list is rendered without an error message is when we are unable to fetch the subject ID for the current user. If you created your workspace and users before multi-informant, your owner user may be missing a subject. Check the console for this error message P.S. I've noticed that I was unable to submit activities in the respondent web app with the owner user since starting work on multi-informant, until I recreated the workspace. If this is indeed the case (I haven't tested robustly) then there may be something wrong with the way pre-MI data is being migrated, and we should create a ticket for it.
The coordinator scenario is correct: They are unable to start take now according to the list of allowed roles. For the other ones, the error message should be the same as for the coordinator scenario. If you have multiple applets, be sure that you're using the subject ID corresponding to the applet that you're testing with (subjects exist in the context of an applet). |
There was a problem hiding this comment.
Thanks for making those updates! Code looks great.
Take now as owner issue
I re-ran tests, following your suggestion of creating a new workspace, with a new applet and activity. This first issue is now resolved – the owner is now considered a subject on the BE, and can now act as a respondent on behalf of another user. Thanks for the suggestion!
- One of us should create a ticket detailing this issue. I would characterize it as a data integrity issue, what do you think? Either the subject records should be created in bulk for existing workspaces, or perhaps done on-demand (i.e. an endpoint request expecting the subject to exist should ensure it's created if found to be missing).
- In the meantime, until that ticket is completed, would it still be prudent to handle this scenario in Take Now flow with a helpful error message?
"This subject does not exist" issue
This one is still happening even with this newly created workspace, applet, participants and team members. I've only ever been using the subjectId for each participant as returned in the response to the respondents query on the applet's Participants screen, so that shouldn't be the issue.
Here's what I see in the JS console when the Take Now page request loads, and I've added a debug statement to output isSubjectError and subjectData:
Looks to me like there's an issue fetching the workspace ID, and resulting in several malformed requests. Let me know if you'd like me to troubleshoot further.
I'm happy to create this ticket, but I would like to investigate further. For now I'll raise it in Slack
I'm not sure this is it. The workspace ID is present in the applet data. The malformed requests you're seeing are a result of the nature of hooks. The hooks have to run even before the proper data is present, so some of them will be malformed at first. Let me do some investigation to see what this could be |
sultanofcardio
force-pushed
the
feature/M2-6042-validate-take-now-params
branch
from
8d1e034 to
c96de7f
Compare
|
@farmerpaul Thanks for catching this. I've made a small patch to account for the permissions of the get subject endpoint. After patching it, I ran these happy path tests:
Can you retest and see if you get the expected outcomes? |
|
Just went through all tests and got the expected results as per your table.
|
There was a problem hiding this comment.
I didn't do as exhaustive testing as @ChaconC (kudos, man!), but I did run through all my previous scenarios, and everything is now tickety-boo! Thanks @sultanofcardio, awesome work.
| Source | Subject | Outcome |
|---|---|---|
| owner | Limited account | ✅ Starts activity |
| manager | Limited account | ✅ Starts activity |
| coordinator | Limited account | ✅ You are not authorized to complete this assessment on behalf of this subject. |
| editor | Limited account | ✅ You are not authorized to complete this assessment on behalf of this subject. |
| reviewer | Limited account | ✅ You are not authorized to complete this assessment on behalf of this subject. |
| owner | Team member | ✅ Starts activity |
| manager | Team member | ✅ Starts activity |
| coordinator | Themself | ✅ You are not authorized to complete this assessment on behalf of this subject. |
| editor | Themself | ✅ You are not authorized to complete this assessment on behalf of this subject. |
| reviewer | Themself | ✅ You are not authorized to complete this assessment on behalf of this subject. |
| respondent | Limited account | ✅ You are not authorized to complete this assessment on behalf of this subject. |
| respondent | Themself | ✅ You are not authorized to complete this assessment on behalf of this subject. |
This is a one-liner to remove `Coordinator` from the list of roles with access to the Take Now menu item in the activity context menu. Other roles remain unaffected. This does not prevent a coordinator from initiating take now using a manually constructed URL. That scenario is handled in the [respondent web app](ChildMindInstitute/mindlogger-web-refactor#438)
📝 Description
🔗 Jira Ticket M2-6042
This PR builds on #431 and introduces validation of the following query parameters:
startActivityOrFlow- An activity or Flow IDrespondentId- The respondent taking the assessmentsubjectId- The subject who the assessment is aboutIf one of these parameter is omitted, then no validation takes place and the app behaves as it does today with one exception: the activity or flow is not started. Starting the activity/flow automatically now requires the provision of all three parameters.
If they are all provided, then the following validation is performed:
startActivityOrFlowidentifies a valid activityrespondentIdmatches the currently authenticated usersubjectIdcorresponds to a real subjectIf all the validation passes, then the activity/flow is started automatically and the context is stored in a multi-informant redux store.
📸 Screenshots
There isn't really anything to show, besides error banners I guess 🤷
🪤 Peer Testing
Note
You'll need to test this change locally, as it depends on backend changes that haven't yet been deployed. Use the feature/multiinformant-metapod branch for your API
Note
This feature depends on the
enableMultiInformantfeature flag. If it it not already enabled, you can manually enable it here like so:features.enableMultiInformant = true;There are quite a few scenarios that need to be tested, so buckle up. For this test, you'll need the following set up in your workspace:
You will also need at least one applet with at least one activity.
Make note of the user ID and subject ID for each of the users above, as well as the applet ID and activity ID then using the following URL template:
https://localhost:5173/protected/applets/{appletId}/?startActivityOrFlow={activityId}&respondentId={sourceUserId}&subjectId={targetSubjectId}Substitute for each of following scenarios, and validate the outcome:
Note
Also, be sure to validate the scenarios mentioned in the description above regarding the query parameter combinations
✏️ Notes
N/A