[JN-1020] Admin form editing #894

MatthewBemis · 2024-05-16T20:27:03Z

DESCRIPTION (include screenshots, and mobile screenshots for participant UX)

Adds support for admin form editing. As a result, this brings a lot of survey-related infrastructure and types into ui-core.

OUT OF SCOPE: a refined admin UX. This PR is massive and challenging to review as-is, the focus here is just getting the survey infrastructure into core along with all of the related dependencies. Refined UX will come in follow-on PRs.

This needs some extra attention when testing the participant side to ensure that there weren't any regressions. This branch has also undergone several notable rebases during it's life, and accounting for newer changes during a rebase while also moving them ui-core is particularly vulnerable to dropping new functionality. Some areas worth calling out:

Confirm that address validation still works in surveys
Confirm that autosave behaves as expected
Confirm that all of the recently-added proxy survey functionality still works correctly

TO TEST: (simple manual steps for confirming core behavior -- used for pre-release checks)

More specific steps coming soon, but definitely focus on the above areas.

devonbush

Nice work on all the code rearranging! Overall, code looks good. Some minor suggestions. Only commenting for now because, as you said, I want to take a lot of time to test this code out

devonbush · 2024-05-17T01:49:54Z

core/src/main/java/bio/terra/pearl/core/service/survey/SurveyResponseService.java

                    .surveyId(survey.getId())
                    .complete(responseDto.isComplete())
                    .resumeData(responseDto.getResumeData())
                    .build();
+            Optional.ofNullable(operator.getParticipantUser()).ifPresent(pUser -> newResponse.setCreatingParticipantUserId(pUser.getId()));


I think a standard if/else is more appropriate here -- we're not doing any chaining so the functional wrapper makes if harder to pinpoint the if logic--but maybe that's just me. Connor, what's your take? Better yet, add a "setResponsibleUser" method to SurveyResponse that takes a responsibleEntity and sets the ids appropriately. That method might then be reused via interface with Answer

Hmmm... I like the setResponsibleUser on the survey response approach. That mirrors (iirc) the flow on the data audit info. I don't feel super strongly on if/else, but I agree it might make the logic clearer.

devonbush · 2024-05-17T01:55:02Z

ui-admin/src/study/participants/survey/SurveyEditView.tsx

+          Store.addNotification(successNotification('Response saved'))
+        }}
+        onFailure={() => Store.addNotification(failureNotification('Response could not be saved'))}
+        updateProfile={() => {}} //eslint-disable-line @typescript-eslint/no-empty-function


I think you can avoid the typescript warning by having it return null or true, or some other simple value

Suggested change

updateProfile={() => {}} //eslint-disable-line @typescript-eslint/no-empty-function

updateProfile={() => null}

you an also have a commment on the inside

() => { // todo }

devonbush · 2024-05-17T01:59:21Z

ui-admin/src/study/participants/survey/SurveyEditView.tsx


 /** allows editing of a survey response */
-export default function SurveyEditView({ response, survey }: {response: SurveyResponse, survey: Survey}) {
-  console.log(`not implemented ${response.surveyId} ${survey.stableId}`)
+export default function SurveyEditView({ studyEnvContext, response, survey, enrollee, adminUserId, onUpdate }: {


let's rename this to SurveyResponseEditor to avoid confusion with the form editor

devonbush · 2024-05-17T02:00:55Z

ui-admin/src/study/participants/survey/EnrolleeSurveyView.tsx


 /** Show responses for a survey based on url param */
-export default function EnrolleeSurveyView({ enrollee, responseMap, studyEnvContext }:
-  {enrollee: Enrollee, responseMap: ResponseMapT, studyEnvContext: StudyEnvContextT}) {
+export default function EnrolleeSurveyView({ enrollee, responseMap, studyEnvContext, onUpdate }:


SurveyResponseView might be a better name for this component

devonbush · 2024-05-17T02:32:40Z

Initial testing seems solid -- I had tabs open to admin and participant view of the same survey and was taking turns editing both, and it always saved the answers, which were then available to the other side upon refresh. (which is desired behavior -- CMI confirmed earlier they do not want a google-docs-like shared editing experience. They'd rather risk accidental overwrites than have a patient jarred by seeing their data change in front of them

connorlbark

worked great! a really impressive lift, excited to see the final product. also commenting for now, ran into some bugs. i'll be gone all next week, though, so if devon gives it a thumb then consider it worthy 😄

connorlbark · 2024-05-17T13:51:42Z

ui-core/src/index.ts


 export * from './test-utils/asMockedFn'
+export * from './test-utils/router-testing-utils'


I see that we already had some testing stuff in core's index file, but I wonder if we should break this out into something like @ui-core/testing: it seems weird (to me) to have testing utils interspersed with the core app functionality. That is probably out of the scope of this PR, though.

connorlbark · 2024-05-17T19:20:41Z

ui-core/src/components/forms/PagedSurveyView.tsx

+import { Enrollee } from 'src/types/user'
+import { Profile } from 'src/types/address'
+
+const AUTO_SAVE_INTERVAL = 3 * 1000  // auto-save every 3 seconds if there are changes


I'm still wary of relying on autosave for editing participant data, since it seems like a "thing we should not do often", but CMI folks seemed to not mind, so 🤷 . It definitely makes sense in regards to admin-only forms, though.

connorlbark · 2024-05-17T19:31:28Z

ui-core/src/surveyUtils.tsx

+  envName: EnvironmentName,
+  profile?: Profile,
+  proxyProfile?: Profile,


I'm sad that we have to move these to the parameters :( these are so easy to forget, as evidenced by the preenroll breaking. it would be so cool if there was some way to allow useUser/useActiveUser to still function in core.

It feels like more and more things are moving to core as we start to allow the admins to do more of the participant experience in the admin side. As such, I suspect we might start running into more cases like this where we have to awkwardly pass down user information to things in core. it would be cool if these hooks worked in the admin app but returned nothing except for the cases that you are looking at an enrollee's page, then it would return the appropriate user.

potentially, we could rename them to useParticipantUser and useActiveParticipantUser so that it's more clear in the admin side that they are only available when you're viewing a specific participant? dunno just spitballing

100% agree that the parameter list on this is way too burdensome. hooks are an interesting idea here, let me see what I can do to clean it up (but i may defer that work to a followup PR though since this branch is super vulnerable to merge conflicts)

connorlbark · 2024-05-17T19:33:14Z

ui-core/src/types/address.ts

@@ -17,3 +17,17 @@ export type AddressValidationResult = {
  hasInferredComponents?: boolean,
  vacant?: boolean
 }
+
+export type Profile = {


should this be in user.ts?

connorlbark · 2024-05-17T19:33:35Z

ui-core/src/types/user.ts

+    creatingAdminUserId: string
+}
+
+export type Enrollee = {


let's goooooooooo

connorlbark · 2024-05-17T19:41:06Z

ui-core/src/participant/ApiProvider.tsx

+import { SurveyResponse } from 'src/types/forms'
+import { StudyEnvParams } from 'src/types/study'
+import { HubResponse } from 'src/types/user'
+import { AddressValidationResult, MailingAddress } from 'src/types/address'

 export type ImageUrlFunc = (cleanFileName: string, version: number) => string
 export type SubmitMailingListContactFunc = (name: string, email: string) => Promise<object>


I tried the e2e tests (and then tried it myself) and found that, oddly, I couldn't add myself to the mailing list. It wouldn't error in the participant side, but it didn't show up on the admin side. I tried on dev and it worked.

I'm unable to reproduce this locally or with the e2e tests.

Devon, do you see this as well?

connorlbark · 2024-05-17T19:44:33Z

ui-admin/src/study/surveys/FormOptionsModal.tsx

+                onChange={e => updateWorkingForm({
+                  ...workingForm, allowAdminEdit: e.target.checked
+                })}
+              /> Allow admin completion


The word completion feels weird to me, especially when half of the time we really mean admins can edit this participant's responses, but should only edit one small thing not complete it on behalf of them - Imo, this would make more sense to me as a radio group

opt 1 - Participant-only form (e.g., consent form) opt 2 - Admin-editable participant survey (e.g., demographics) opt 3 - Admin-only survey (e.g., medical records)

This also eliminates the weird possibility of a form being editable by no one haha

agreed- this will definitely be touched up in a followup UX PR

connorlbark · 2024-05-17T19:49:31Z

ui-admin/src/study/participants/survey/EnrolleeSurveyView.tsx

+        }
+        }>
+        {isEditing ?
+          <div><FontAwesomeIcon icon={faX}/> Cancel</div> :


To me this feels more like Complete or Finish button since the changes are already saved - but that's probably more of a UX concern.

connorlbark · 2024-05-17T19:59:11Z

ui-participant/src/studies/enroll/PreEnroll.tsx

+    undefined,
+    undefined,


we actually do need to pass these in, if they exist - they are used to prefill proxy/governed user name in the case of adding a new proxy or signing yourself up as a governed user.

devonbush

nice, let's get this merged so we have time to poke at it!

sonarcloud · 2024-05-21T20:22:25Z

Quality Gate passed

Issues
10 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

wluken

Admin survey editing good to merge.

MatthewBemis added 2 commits May 16, 2024 16:22

initial

d010a17

fixes

7c57085

MatthewBemis marked this pull request as ready for review May 16, 2024 20:43

devonbush reviewed May 17, 2024

View reviewed changes

MatthewBemis requested a review from connorlbark May 17, 2024 11:56

connorlbark reviewed May 17, 2024

View reviewed changes

MatthewBemis added 3 commits May 20, 2024 09:52

initial round of feedback

24d67f3

clean up responsible entity

2d60c56

fixes

314e2fd

MatthewBemis requested a review from devonbush May 20, 2024 14:21

devonbush approved these changes May 21, 2024

View reviewed changes

devonbush requested a review from wluken May 21, 2024 19:59

MatthewBemis and others added 3 commits May 21, 2024 16:04

Merge branch 'development' into mb-jn-1020

eefd7b6

thatll teach me for trusting github to rebase a massive pr

debd013

another rebase fix

7829f91

wluken approved these changes May 22, 2024

View reviewed changes

MatthewBemis added this pull request to the merge queue May 22, 2024

Merged via the queue into development with commit dced393 May 22, 2024
12 checks passed

MatthewBemis deleted the mb-jn-1020 branch May 22, 2024 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JN-1020] Admin form editing #894

[JN-1020] Admin form editing #894

MatthewBemis commented May 16, 2024 •

edited

devonbush left a comment •

edited

devonbush May 17, 2024

connorlbark May 17, 2024

devonbush May 17, 2024

connorlbark May 18, 2024

devonbush May 17, 2024

devonbush May 17, 2024

devonbush commented May 17, 2024

connorlbark left a comment •

edited

connorlbark May 17, 2024

connorlbark May 17, 2024

connorlbark May 17, 2024

MatthewBemis May 17, 2024

connorlbark May 17, 2024

connorlbark May 17, 2024

connorlbark May 17, 2024

MatthewBemis May 20, 2024

connorlbark May 17, 2024

MatthewBemis May 17, 2024

connorlbark May 17, 2024

connorlbark May 17, 2024

devonbush left a comment

sonarcloud bot commented May 21, 2024

wluken left a comment

	updateProfile={() => {}} //eslint-disable-line @typescript-eslint/no-empty-function
	updateProfile={() => null}


		export * from './test-utils/asMockedFn'
		export * from './test-utils/router-testing-utils'

[JN-1020] Admin form editing #894

[JN-1020] Admin form editing #894

Conversation

MatthewBemis commented May 16, 2024 • edited

DESCRIPTION (include screenshots, and mobile screenshots for participant UX)

TO TEST: (simple manual steps for confirming core behavior -- used for pre-release checks)

devonbush left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devonbush commented May 17, 2024

connorlbark left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devonbush left a comment

Choose a reason for hiding this comment

sonarcloud bot commented May 21, 2024

Quality Gate passed

wluken left a comment

Choose a reason for hiding this comment

MatthewBemis commented May 16, 2024 •

edited

devonbush left a comment •

edited

connorlbark left a comment •

edited