-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix: changes to form save validations for duplicate facilities #93
base: development
Are you sure you want to change the base?
fix: changes to form save validations for duplicate facilities #93
Conversation
…y in different surveys
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.
Thanks @MatiasArriola , looks good.
currentQuestion.id === AMR_SURVEYS_MORTALITY_TEA_SURVEY_ID_DF || | ||
currentQuestion.id === AMR_SURVEYS_MORTALITY_TEA_SURVEY_ID_COH || | ||
(parentPrevalenceSurveyIdList.includes(currentQuestion.id) || | ||
// TODO: check if patientIdList can be used here (not all IDs overlap) |
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.
@MatiasArriola maybe we can check with Miquel if all the ids in patientIdList should be disabled. Maybe we have missed out a few and they should ideally overlap?
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.
@9sneha-n what do you mean by disabled? That the field in a detail is not editable?
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.
Yes, I think It could be a good idea to handle this refactor in a separate task/PR.
It is related to the list of metadata IDs that disable a question (e.g. survey id, patient id). Recently parentPrevalenceSurveyIdList
and patientIdList
were introduced as a code improvement to group these Ids. Although unrelated to this PR, I noticed an opportunity to reuse these lists, but it wasn't straightforward for patientIdList
. We would need to review the IDs first.
As Sneha mentioned, reviewing this list might reveal some inconsistent behavior.
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.
Noted. We can add this backlog.
📌 References
📝 Implementation
SaveFormDataUseCase
:validate
methodPPSHospitalForm
, to avoid Hospital duplicates in the same SurveygetParentSurveyId
method toQuestionnaire
.SURVEY_ID_FACILITY_LEVEL_DATAELEMENT_ID
I saw the opportunity to refactor and reuse the newparentPrevalenceSurveyIdList
.patientIdList
, but as Ids were different, I kept the same behavior but just left a TODO comment.📹 Screenshots/Screen capture
2025-02-19.13-29-07.mp4