-
Notifications
You must be signed in to change notification settings - Fork 7
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
Updated workflows, forms and condition cards #23
base: main
Are you sure you want to change the base?
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.
Nice work @brian-gacheru
I've only looked at the javascript code and left some feedback inline
POSHIT/Example CHT application/malaria-usecase-cht/nools-extras.js
Outdated
Show resolved
Hide resolved
POSHIT/Example CHT application/malaria-usecase-cht/nools-extras.js
Outdated
Show resolved
Hide resolved
// }); | ||
// return result; | ||
// } | ||
function getMostRecentReportForm(reports, form) { |
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.
This is already implemented in cht-nootils.
You can access this method using Utils.getMostRecentReport
The Utils object is globally accessible. The latest cht-core might not have the latest cht-nootils. It helps to check which version of the utility is released in a given cht-core release.
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.
For some reason, the function in cht-nootils does not work as expected in some forms.
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 you have more information to go by? I'm keen to learn how you are using this.
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.
When fetching the most recent reports for a household or household member, using the method in Utils crashes the form giving an error but when we use the function in nools-extras it works. Both seem to be implemented the same way but for some reason using the method in cht-nootils crashes the app.
POSHIT/Example CHT application/malaria-usecase-cht/nools-extras.js
Outdated
Show resolved
Hide resolved
events: [ | ||
{ | ||
id: FORMS.MALARIA_ASSESSMENT_FOR_PREGNANT_MOTHERS, | ||
start: 30, |
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.
This is a very long lead time. Is this by design? Are the referral follow up dates way into the future to warrant this?
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.
This is by design since we want to follow up with a pregnant woman every month.
const { DateTime } = require('luxon'); | ||
const { FORMS } = require('./shared-extras'); | ||
const { getField, isFirstReportNewer } = require('cht-nootils')(); | ||
|
||
const isFormArraySubmittedInWindow = (reports, formsArray, startTime, endTime) => { |
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.
This function exists in cht-nootils. I would recommend reusing it unless you have a very specific need to override some behaviour
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.
@brian-gacheru , you have maintained this. Are you using it to override the function in cht-nootils?
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 this function overrides the one in cht-nootils
. Using the one in cht-nootils
crashes the app
POSHIT/Example CHT application/malaria-usecase-cht/nools-extras.js
Outdated
Show resolved
Hide resolved
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.
This has come a long way @brian-gacheru. Nice work!
I've left some feedback inline
}; | ||
|
||
const isPregnant = (allReports) => { | ||
return allReports.some(report => isActivePregnancy(report)); |
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.
We also have to check that:
- there's no subsequent delivery
- the pregnancy age does not exceed 40-42 weeks
- the pregnancy has not been terminated
A potential example might look like this
const under5AssessmentReport = getMostRecentReportForm(reports, [FORMS.CHILD_ASSESSMENT]); | ||
const under5AssessmentFollowUpreport = getMostRecentReportForm(reports, [FORMS.CHILD_FOLLOW_UP]); | ||
const memberAssessmentReport = getMostRecentReportForm(reports, [FORMS.HOUSEHOLD_MEMBER_ASSESSMENT]); | ||
const u5MalariaConfirmed = !!under5AssessmentFollowUpreport |
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.
it's easier to spot boolean variables if there are named in a manner that communicates that. e.g isXYZ or hasXYZ.
Consider renaming for redability
{ appliesToType: CONTACT_TYPES.AREA_SUPERVISOR_REGION, label: 'contact.phone', value: thisContact.contact && thisContact.contact.phone, width: 4 }, | ||
{ appliesToType: CONTACT_TYPES.AREA_SUPERVISOR_REGION, label: 'contact.parent', value: thisLineage, filter: 'lineage' }, | ||
{ appliesToType: CONTACT_TYPES.AREA_SUPERVISOR_REGION, label: 'Notes', value: thisContact.contact && thisContact.contact.notes, width: 12 }, | ||
{ appliesToType: CONTACT_TYPES.AREA_COMMUNITY_HEALTH_SUPERVISOR, label: 'contact.phone', value: thisContact.phone, width: 4 }, |
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.
Is a name needed for the 'area supervisor region'?
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.
If there was consistency in the way place / person contacts were rendered, this code block could be significantly reduced.
const fields = [];
if (person_type) {
fields.push(...)
fields.push(..)
} else {
fields.push(...)
fields.push(...)
}
return DateTime.fromISO(ammendmentDate).isValid ? ammendmentDate : null; | ||
value: function() { | ||
const assessmentValue = getField(householdAssessmentReport, 'malaria_prone'); | ||
const followUpValue = householdFollowUpReport !== undefined ? getField(householdFollowUpReport, 'malaria_prone') : undefined; |
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.
In some cases, you are using undefined
while in others you are using null
. Let's be consistent
value: function(report) { | ||
return getField(report, 'malaria_prone') ? 'Yes' : 'No'; | ||
value: function() { | ||
const assessmentValue = getField(householdAssessmentReport, 'malaria_prone'); |
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.
assessmentValue does not quite communicate your intention here. It might be more clear if it were renamed to isMalariaProne
or similar. That way, you have an idea of what value it would contain.
followUpValue
is also unclear.
const isMalariaProne = !!householdAssessmentReport && getField(householdAssessmentReport, 'malaria_prone') === 'true'
This way it would be a boolean value (either true or false).
The same pattern would apply for followUpValue
and isContactAvailable
// appliesIf: function (contact, report) { | ||
// return isAlive(contact) && !isPregnancyTaskMuted(contact) && Utils.getField(report, 'group_malaria_assessment_for_pregnant_mothers.treatment_referral_follow_up_date'); | ||
// }, | ||
|
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.
if this isn't going to be used, can we delete?
start: 30, | ||
end: 30, | ||
dueDate: function (event, contact, report) { | ||
return new Date(Utils.getField(report, 'group_malaria_assessment_for_pregnant_mothers.referral_follow_up_date')); |
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.
What would be a safe default if the field doesn't return a value?
}, | ||
|
||
resolvedIf: function (contact, report, event, dueDate) { | ||
if(isPregnancyTaskMuted(contact) && !isAlive(contact)) {return true;} |
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.
This reads as 'return true if the pregnancy is muted and the contact is deceased'.
I think it should be either or. Thoughts?
householdMemberAssessmentResult = await harness.fillForm(FORMS.MEMBER_FOLLOW_UP, ...memberAssessmentTask.followUpMalaria); | ||
expect(householdMemberAssessmentResult.errors).to.be.empty; | ||
//check that target increments | ||
householdMembersWithMalaria = await harness.getTargets({ type: TARGETS.MEMBERS_WITH_MALARIA }); |
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 we should check for the target change after submitting the the assessment
|
||
harness.subject = result.contacts[0]; | ||
//assess new member | ||
householdMemberAssessmentResult = await harness.fillForm(FORMS.MEMBER_ASSESSMENT, ...memberAssessment.showSymptoms); |
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.
This test seems repetitive. We could have used the opportunity to assert that an non-assessment or a an assessmemnt without malaria does not increment
No description provided.