Closed PrjShrestha closed 1 year ago
Update: April 26th: Have been able to successfully set up the CHT development environment. May 6th: Able to run and debug unit tests. Small overview from Mokhtar Currently on hold due to Nepal MOH SMS and Nepal Echis task priorities.
I have also gone over the code flow and figured out these changes might be necessary. Discussed with @binokaryg and @m5r about this.
Changes in registration.js: Seems registration.js handles the orchestration and is the entry point for SMS parsing so we need to
Changes in SMSparser.js Seems the correct parser is chosen at runtime based and it splits the given SMS and based on the index figures out which index represents what. So
3.. Add a phone_number data type
Parsing and Validation:
Unit tests and DevQA
@PrjShrestha Just chiming in to make sure we include @medic/quality-assurance as early as possible in the process and can provide feedback along the way
Thanks @m5r . @PrjShrestha Ideally, when we add a new feature or improvement, we'd like to add some automated tests to cover the added part. Sometimes it might be a simple as fixing an existing spec to reflect the new change. We have existing e2e tests that cover sms, including rapidpro . Maybe that could be your starting point. It could be confusing atm because we have tests written in protractor and others in webdriverIO, but that's going to change soon. Please feel let the QA know if you need any assistance on this.
@ngaruko Sure thanks I have added multiple unit tests
Thanks for the heads up. :)
@PrjShrestha We might need to add one or 2 specs to existing sms tests. If you could please link me to your PR/branch, I would have a look and suggest what test to add.
@ngaruko Please find the branch here.
This can be done simply by adding a phone_number field with uniquePhone validation:
"property": "phone_number",
"translation_key": "",
"rule": "uniquePhone('phone_number')",
"message": [
{
"content": "The registration format for {{patient_name}} is incorrect, please ensure that phone number is not already registered",
"locale": "en"
}
]
This internally checks our "medic-client/_view/contacts_by_phone" view to find pre-registered contacts and errors or continues based upon that.
Hi @PrjShrestha,
Thank you for these changes, and for all the information added in this ticket and in the PR, it is super helpful to understand clearly what the feature is about β¨
I don't have a lot of experience with how these SMS config forms work, so I just have a couple of questions that I am sure you can help me with:
I will suggest adding an individual test to verify the "happy path". It should:
app_settings.json
file.This is an example that you can use, it will be pretty similar, you only will need to change the config, the form message and the asserts. Please let me know if you need any help and I will be happy to help you.
Good luck β¨
@tatilepizs Thanks for the testing!
Given this is a blocker for 4.3.0 release would you be comfortable with us adding the e2e test later? I accept that this isn't the best practice.
Yes, that will be fine. It is not a very complicated test, and we can definitely open a separate PR to create it
Great! Issue raised here: #8431
Thank you @garethbowen !
Hi @tatilepizs
- I am not 100% sure about where the creation of the patient is configured if it is in the config file or in the core code. I used the config that you used for your tests, please let me know if there is something that I need to add there, but when I sent the SMS with an incorrect phone number or an incorrect age the report is showing the error but the patient was still created. I was expecting to see only the report with the error, not the patient actually created. Can you please clarify?
[Uploading 8204 registration form and .form configβ¦]()
- The age is not saved in the patient information card with the config form that I am using. Can you please help me create one that will save both? the phone number and the age.
- I have removed the age being added to the patient doc because it wasn't a part of this story. I was doing that as a part of testing on my side and gradually forgot to remove it. My bad :)
This is an example that you can use, it will be pretty similar, you only will need to change the config, the form message and the asserts. Please let me know if you need any help and I will be happy to help you.
- I will surely look into this on Monday but would be grateful if we can skipt it if I cannot complete it by Monday EOD. e2e tests on Windows are an absolute headache. While writing registration trigger tests, most of the times the logs didn't generate, transitions fail for no reason etc π and I talked with various people and just couldn't verify why.
Good luck β¨ Thanks π
Thank you for very descriptive feedback and unit test advice :D
@PrjShrestha,
I have the "It works on my machine" syndrome :D
π€£ Probably is something that I am missing. This is the complete app_settings.json file that I am using, it is the one from the standard config with the "NP" form added. I don't have a branch, I just changed that file locally. So, using your branch and the attached file it's my complete local env π
I have removed the age being added to the patient doc because it wasn't a part of this story.
Thanks! π
... would be grateful if we can skip it ...
Sure, Gareth created ticket #8431 to work in the e2e test part of the ticket.
@tatilepizs The env setup sounds right.
Added validations in registrations part of the app_settings for the NP form in the above app settings. Due to missing validations in registrations, patient might still be created. Internally A failure in validation (uniquePhone validation for phone_number field) stops the patient from being created. I had stopped committing app settings midway through working on my ticket as it is not good to do so hence maybe those weren't in the app settings you used as template.
Can you please try with these app settings below which has the added parts. I have modified the form "NP" and the registration entry for the form as well. app_settings.zip
Please kindly try with the latest commit and above app settings. Hopefully it works π
P.S. It seems the config I tried to upload didn't upload in the last comment as json file upload wasn't supported. My bad :)
Seems we have almost 12 hour time diff. If necessary we can catch up from 7:30am to 10:30am or 7:30 pm to 9:30pm your time (Hi from the other side of the world π)
@PrjShrestha,
Thank you for checking! I tried with the latest commit and using the app_settings file that you attached and the person is still created when the phone number is wrong π΅βπ« I don't really understand what I am doing wrong.
I will be online during my night, let me know when you are available to talk.
Thank you!
@PrjShrestha,
Attached is a video with all the processes that I am doing.
Here are the steps that you can see in the video:
/standard/app_settings.json
file to verify that I am using the correct one (the one that you shared with me).cht --local
Carol
, phone number: +50689363636
App Management
section, SMS
option, Test Message
tab.NP 23 2323 Armando
and in the From phone number field the phone of the new person that I created when I created the district (+50689363636
).Let me know if there is anything else I can do to help π
It looks like with the new changes the feature is working fine π
Using this app_settings.json file I sent three different messages to try to create three different person:
NP 8569 Benedict
- It should not create the new personNP +50688191919 Colin
- It should create successfully the new personNP +50688191919 Daphne
- It should not create the new personI just have one last question @PrjShrestha, do you know why in the report that failed due to the incorrect phone number it is showing two times the status and the phone number for the Automated reply
section? Only in that report, it is shown two times.
@tatilepizs Thank you for the testing snips and description. I am unsure why there are 2 pending messages as you highlighted. I could only see the message being logged once in code and only one message in the outgoing tab as well.
Sadly, I couldn't figure out the root cause today. Will take a look again tomorrow.
@tatilepizs The issue of double messages turned out to be a behavior that was present before. So basically what happens is if while adding a patient (using add_patient trigger) we have an error (phone invalid in this case) and end the add_patient midway , the code still checks form config for messages and _appends any message that has "reportaccepted" or empty _"eventtype" messages. Hence we see 2 messages there. First one is the invalid phone message which is expected and second is the empty message which was there due to the config I sent having empty event_type in one of the messages for the form. (Similarly if we add a message which has report_accepted event type, it would show even if there was an invalid_phone error due to the above issue)
Below is my attempt to capture the code, UI and causes in 1 picture.
Tried resolutions: _Throw error from registration addpatient trigger: I tried hard exiting by throwing an error but it turns out even if we throw an error it will come up as Error in Transition in the errors section of the report in UI which is not ideal as well.
_bool_expr check for errors =0 before adding empty or reportaccepted type messages One way to fix this error was to check errors = 0 in the "bool_expr" for the message of event type "report_accepted" so that only where there are no errors the report accepted message is fired.
_Delete patient_Id and check it in bool_expr before adding empty or reportaccepted type messages: Diana suggested to delete the patient Id for any patient that cannot be registered due to errors and checking for empty patient_id in the bool_expr. (i.e. doc.patient_id !== undefined ) check before sending the report_accepted message.
Hence as for all the possible fixes we need to change code and unit tests and that this is a prexisting issue we plan to fix this in another ticket.
cc: @dianabarsan @binokaryg @yrimal
@tatilepizs Hi Tatiana. Here is an updated config which you can use to retest if necessary. From the previous config
@PrjShrestha,
First I really wanted to thank you for the level of detail about the progress that you added to the ticket, especially in this comment, it is great to see and understand what was happing in the feedback process, I really appreciate that, so thank you again! π€©
I re-tested it and it worked perfectly, thank you for all the improvements. And since the PR was already approved I think that it is ready to go! π’ π’
Settings file that I used to test: app_settings.json
Merged. Thank you @PrjShrestha ! This is a great feature.
@tatilepizs @garethbowen Thank you both for your reviews and feedbacks :)
Is your feature request related to a problem? Please describe. @binokaryg was trying to register a patient with phone number using SMS form that utilizes the add_patient trigger but it wasn't possible.
Describe the solution you'd like
Update: July 3, 2023
Describe alternatives you've considered Using RapidPro to process the messages you can use a flow to update the phone number of the contact.
Additional context Add any other context or screenshots about the feature request here.
Update: PR for this is https://github.com/medic/cht-core/pull/8369