-
Notifications
You must be signed in to change notification settings - Fork 5
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.
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
[JN-1591] Show responses associated with participant documents #1455
[JN-1591] Show responses associated with participant documents #1455
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like the UX a lot. Just would like some tweaks so that I don't have to go down a schema rabbit hole reminding myself "wait, how are files and responses tied together again?" next time :)
@@ -28,4 +31,7 @@ public class ParticipantFile extends BaseEntity { | |||
private UUID enrolleeId; | |||
|
|||
private String notes; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the need to send this association to the frontend, but I worry this obfuscates the underlying data model of response -> answer -> file too much.
How about we make this a List<Answer>
instead? then those answers will still have the SurveyResponse id you need, and you'll also get other juicy tidbits too and a reminder aof how it's actually attached
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, while you're at it, please add a comment on the List<ParticipantFile>
on SurveyResponse talking about how that relation is actually stored on the answers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that will make the fetch logic simpler too -- instead of a nested for match and a new dao method in SurveyResponse, you'll just need to create a findWithAnswers
method in ParticipantFileDao
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
return <div className={'mt-2 fst-italic text-muted'}>not associated with any tasks</div> | ||
} | ||
|
||
return ( | ||
<div className={'mt-2 text-muted'}> | ||
<span>shared in response to:</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we use task
in other places participant facing? It feels like admin language, but I could be wrong. IMO I would just return <></>
and specify in the description up above that study staff can see these files even if they aren't associated with a survey.
Also, this needs to be i18n'd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point re: the language, I'll make that more participant friendly.
Regarding i18n- I'm planning to do the i18n at the very end after the UX for everything related to file upload is 100% solid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the UX to remove the task-related language in #1478
Co-authored-by: Devon <[email protected]>
Co-authored-by: Devon <[email protected]>
48e6db5
to
96f3292
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice!
DESCRIPTION (include screenshots, and mobile screenshots for participant UX)\
TO TEST: (simple manual steps for confirming core behavior -- used for pre-release checks)