feat(#8204): Add feature to support adding phone number when registering patients #8369

PrjShrestha · 2023-07-06T13:21:21Z

Feature (#8204): Support adding phone number when registering patients via SMS

Description

This code aims to complete the following:
Users should be able to add a patient's phone number while registering a patient.
CHT should automatically append the default country code to the phone number before saving it to the database.

Sample Message:

[x] Added unit tests
[x] Tested locally by setting up cht
[x] Fixed eslint issues
[x] Doc update for new field phone_number
[x] [Doc update validation] (medic/cht-docs#1144)

… skeleton

…o include new person with phone number

…tion

…tration

config/default/app_settings.json

shared-libs/transitions/src/transitions/registration.js

…al format

garethbowen

Nice one. A couple of changes requested...

tests/integration/sentinel/transitions/registration.spec.js

…patient creation based upon valid/invalid phone in addPatient trigger

…stead of null

PrjShrestha · 2023-08-08T14:35:30Z

Update:
Moved logic to create patient or not based upon phone validation to addPatient trigger. 2 benefits:
a. add_phone_number trigger is no longer required.
b. The other option would have been to add a new validPhone validation in transition just like we have added uniquePhone. But validating whether a phone is valid or not should be part of the core rather than a choice that should be written in appsettings registration validations.

Also added logic where rather checking for phone_number field to have phone number value it checks for phone_number type filed in form to have phone_number value so people can do like :

Field
Name : NameTheyWant
Type: phone_number

@tatilepizs This (point b) is why the invalid phone validation was still creating a Patient because we weren't hard exiting addPatient when that validation failed, unlike uniquePhone validation which would hard exit patient creation. Thanks for catching that. 👍

@dianabarsan Thanks for you help and please kindly review.

dianabarsan

Really cool stuff. Thanks for implementing all the changes. I have some comments but mostly about readability and maintainability.
Awesome work!

dianabarsan · 2023-08-09T14:23:16Z

api/src/services/report/smsparser.js

+    const formattedAndValidatedPhone = phoneNumberParser.normalize(config.getAll(), raw);
+    if (formattedAndValidatedPhone) {
+      return formattedAndValidatedPhone;
+    } else {


You don't need an else here, because the previous if statement has a return.

dianabarsan · 2023-08-09T14:28:05Z

shared-libs/transitions/src/transitions/registration.js

@@ -461,6 +479,7 @@ const addPatient = (options) => {
  const doc = options.doc;
  const patientShortcode = options.doc.patient_id;
  const patientNameField = getPatientNameField(options.params);
+  const patientPhoneField = getPatientPhoneField(config.getAll(), doc.form);


I don't think this function needs to get all the configs as the parameter.

shared-libs/transitions/src/transitions/registration.js

dianabarsan · 2023-08-09T14:32:52Z

shared-libs/transitions/test/unit/transitions/registration.js

-        contactTypeUtils.isPlace.args[0].should.deep.equal([{}, {_id: 'person', patient_id: '56987', type: 'person' }]);
+        contactTypeUtils.isPlace.args[0].should.deep.equal(
+          [{}, { _id: 'person', patient_id: '56987', type: 'person' }]
+        );


Please add unit tests to cover this new functionality.

Interesting there should be no change here. Think its formatting changes.

Was failing the 120 line eslint length check on this test hence Formatted.

I just picked this line to add the comment. I was referring to adding unit tests in general.

Added tests for registration add_patient flow as well as function to get phone field.

dianabarsan · 2023-08-09T14:34:18Z

shared-libs/validation/test/validations.js

@@ -248,6 +248,61 @@ describe('validations', () => {
    });
  });

+  it('unique phone validation should fail if db query for phone returns doc', () => {
+    clock = sinon.useFakeTimers();


Are fake timers needed here? Same question for the test below.

Removed. Thanks.

dianabarsan · 2023-08-09T14:35:40Z

tests/integration/sentinel/transitions/registration.spec.js

+      })
+      .then(() => utils.getDocs(docIds))
+      .then(updated => {
+        console.log(updated);


Please remove this debug log.

dianabarsan · 2023-08-09T14:38:01Z

tests/integration/sentinel/transitions/registration.spec.js

+        chai.expect(updated[0].fields.phone_number).to.equal(patient_phone);
+        chai.expect(updated[0].patient_id).to.not.be.null;
+      })
+      .then(patients => {


patients will always be undefined here, because the previous block does not return anything. I think the block is missing this call:
return getContactsByReference([newPatientId, 'venus']);

Added get patient call and asserted taht its undefined when phone is invalid.

dianabarsan · 2023-08-09T14:39:23Z

tests/integration/sentinel/transitions/registration.spec.js

+      .then(infos => {
+        infos.forEach(info => {
+          chai.expect(info).to.deep.nested.not.include({ 'transitions.registration.ok': false });
+        });


Please also check that no patient_id field is added to the main doc.

Co-authored-by: Diana Barsan <[email protected]>

dianabarsan

Cool stuff! I spotted some stuff and I'm still missing unit tests updates for the registration transition.

dianabarsan · 2023-08-10T13:04:41Z

shared-libs/transitions/src/transitions/registration.js

+    return;
+  }
+
+  if (formDef && formDef.fields) {


We can further simplify this like:

Suggested change

if (formDef && formDef.fields) {

if (!formDef?.fields) {

return;

}

return Object

.keys(formDef.fields)

.find(key => formDef.fields[key].type === 'phone_number');

Thanks.
P.S. eslintrc for shared-libs was in 2018 which coudln't recognize the optional chaining syntax (?.) so upgraded it to 2020. 3 or 4 folders are already in 2020.

dianabarsan · 2023-08-10T13:06:51Z

shared-libs/transitions/src/transitions/registration.js

@@ -505,6 +525,14 @@ const addPatient = (options) => {
          patient.date_of_birth = doc.birth_date;
        }

+        if (patientPhoneField && doc.fields[patientPhoneField]) {
+          if (!phoneNumberParser.validate(config.getAll(), (doc.fields[patientPhoneField]))) {


To make this whole block more readable, please store doc.fields[patientPhoneField] in a variable.

Suggested change

if (!phoneNumberParser.validate(config.getAll(), (doc.fields[patientPhoneField]))) {

const patientPhone = doc.fields[patientPhoneField];

if (!phoneNumberParser.validate(config.getAll(), patientPhone)) {

....

return;

}

patient.phone = patientPhone;

dianabarsan · 2023-08-10T13:09:02Z

shared-libs/transitions/test/unit/transitions/registration.js

-        contactTypeUtils.isPlace.args[0].should.deep.equal([{}, {_id: 'person', patient_id: '56987', type: 'person' }]);
+        contactTypeUtils.isPlace.args[0].should.deep.equal(
+          [{}, { _id: 'person', patient_id: '56987', type: 'person' }]
+        );


I just picked this line to add the comment. I was referring to adding unit tests in general.

dianabarsan · 2023-08-10T13:09:53Z

tests/integration/sentinel/transitions/registration.spec.js

+
+    return utils
+      .updateSettings(testForm.forms.NP, 'sentinel')
+      .then(utils.saveDocs(docs))


You need to pass a function to the then

Suggested change

.then(utils.saveDocs(docs))

.then()) => utils.saveDocs(docs))

dianabarsan · 2023-08-10T13:10:14Z

tests/integration/sentinel/transitions/registration.spec.js

+
+    return utils
+      .updateSettings(testForm.forms.NP, 'sentinel')
+      .then(utils.saveDocs(docs))


Suggested change

.then(utils.saveDocs(docs))

.then(() => utils.saveDocs(docs))

dianabarsan · 2023-08-10T13:10:42Z

tests/integration/sentinel/transitions/registration.spec.js

+    let newPatientId;
+    return utils
+      .updateSettings(testForm.forms.NP, 'sentinel')
+      .then(utils.saveDocs(docs))


Suggested change

.then(utils.saveDocs(docs))

.then(() => utils.saveDocs(docs))

dianabarsan · 2023-08-10T13:11:35Z

tests/integration/sentinel/transitions/registration.spec.js

@@ -98,6 +98,150 @@ describe('registration', () => {
  after(() => utils.revertDb([], true));
  afterEach(() => utils.revertDb(getIds(contacts), true));

+  it('should add valid phone to patient doc', () => {
+    const patient_phone = '+9779841123123';


Please use camelCase for variables.

Suggested change

const patient_phone = '+9779841123123';

const patientPhone = '+9779841123123';

dianabarsan · 2023-08-10T13:12:30Z

tests/integration/sentinel/transitions/registration.spec.js

+    const patient_phone = '+97796666';
+    const patient_id =uuid();


Please use camelCase for variables.

Suggested change

const patient_phone = '+97796666';

const patient_id =uuid();

const patientPhone = '+97796666';

const patientId = uuid();

dianabarsan · 2023-08-10T13:14:01Z

tests/integration/sentinel/transitions/registration.spec.js

+        return getContactsByReference([newPatientId, 'venus']);
+      })
+      .then(patients => {
+        chai.expect(patients[0]).to.be.undefined;


If I look at the test above, it seems that this should be returning an object with a rows property instead of an array.

Suggested change

chai.expect(patients[0]).to.be.undefined;

chai.expect(patients.rows.length).to.equal(0);

done thanks.

dianabarsan · 2023-08-10T13:14:45Z

tests/integration/sentinel/transitions/registration.spec.js

+      })
+      .then(() => utils.getDocs(docIds))
+      .then(updated => {
+        chai.expect(updated.patient_id).to.be.undefined;


I think that updated will be an array here.

Suggested change

chai.expect(updated.patient_id).to.be.undefined;

chai.expect(updated[0].patient_id).to.be.undefined;

Changed thanks.

…in registration.js

…more tests

PrjShrestha · 2023-08-14T09:42:53Z

Doc update for the phone_number field here. Also closed PR for add_phone_number trigger as that functionality is done in add_patient. Updated description to reflect the same.

dianabarsan

Great stuff @PrjShrestha !

dianabarsan · 2023-08-11T12:22:00Z

shared-libs/transitions/src/transitions/registration.js

+            transitionUtils.addRejectionMessage(doc, options.registrationConfig, 'provided_phone_not_valid');
+            return;
+          }
+          patient.phone = doc.fields[patientPhoneField];


Suggested change

patient.phone = doc.fields[patientPhoneField];

patient.phone = patientPhone;

1yuv · 2023-08-15T03:12:47Z

api/src/services/report/smsparser.js

+  phone_number: (raw) => {
+    const formattedAndValidatedPhone = phoneNumberParser.normalize(config.getAll(), raw);
+    if (formattedAndValidatedPhone) {
+      return formattedAndValidatedPhone;


@PrjShrestha , would you mind adding Nepali digit supporting code for this type as well? Here's the sample PR.

PrjShrestha and others added 15 commits May 16, 2023 10:37

Added sms form in app settings, trigger in registration and unit test…

13e3133

… skeleton

Reverting registration.js tests

0db854d

registration.js add phone number function added

6ab0253

Added phone_number parser using google lib and changed app settings t…

439cb8b

…o include new person with phone number

phone number length validation

1184519

Added support for phone number and concrete unit tests

61de363

Added add_age trigger as well as country code validadtion at registra…

0ffa8c8

…tion

Added sms parser tests and registration.js phone number funciton

4a89954

registration test for valid phone number

3977367

added phone number and age adding code to addpatient funtion in regis…

49086cf

…tration

registration unit tests

86e53ed

added unit test to test that patient isn't created when phone is wrong

ae62cc2

removed debugger and console.log

0185462

eslint issue fixes 1

614a763

eslint Fixes 2

ad94523

PrjShrestha mentioned this pull request Jul 6, 2023

[8204 doc update] updated transitions.md with new trigger - add_phone_number medic/cht-docs#1132

Closed

PrjShrestha added 2 commits July 10, 2023 22:09

confirming to eslint standards to pass build

e2fa163

ident fix

3e607ed

binokaryg reviewed Jul 11, 2023

View reviewed changes

config/default/app_settings.json Outdated Show resolved Hide resolved

binokaryg reviewed Jul 11, 2023

View reviewed changes

shared-libs/transitions/src/transitions/registration.js Outdated Show resolved Hide resolved

binokaryg reviewed Jul 11, 2023

View reviewed changes

shared-libs/transitions/src/transitions/registration.js Outdated Show resolved Hide resolved

PrjShrestha changed the title ~~8204: Add feature to support adding phone number when registring patients~~ 8204: Add feature to support adding phone number when registering patients Jul 12, 2023

PrjShrestha added 3 commits July 14, 2023 10:33

Reverted app settigns added phone duplicate turn of on on appsettings.

2639a19

registration.js false positive and negative fix

5a447b7

added duplicate phone optional logic and changed phone number to norm…

08f7158

…al format

garethbowen requested changes Jul 19, 2023

View reviewed changes

tests/integration/sentinel/transitions/registration.spec.js Outdated Show resolved Hide resolved

tests/integration/sentinel/transitions/registration.spec.js Outdated Show resolved Hide resolved

PrjShrestha added 4 commits July 19, 2023 11:00

"making double quotes to single quote from from-definition"

4cef416

unique phone check in both registration and validation.js

b2e617c

Fixed validation.js function

db5b54c

Doing validation for unique in before transition is run

1abff00

PrjShrestha added 5 commits August 8, 2023 16:33

phone validation in add_patient itself

b5378f4

Implemented suggestion to remove add_phone_number trigger and do the …

a88e85b

…patient creation based upon valid/invalid phone in addPatient trigger

Changed sms parser tests and made sms parser return invalid number in…

807caf4

…stead of null

added null checks on getpatientphonefield function

1bf6514

added null check on formdef

3f35ad1

PrjShrestha requested a review from binokaryg August 8, 2023 14:37

dianabarsan self-requested a review August 8, 2023 15:34

craig-landry mentioned this pull request Aug 8, 2023

Support adding phone number when registering patients via SMS #8204

Closed

Checking if updated config fixes problem, cannot run unit tests locally

85d2065

dianabarsan requested changes Aug 9, 2023

View reviewed changes

PrjShrestha and others added 3 commits August 9, 2023 21:45

Apply suggestions from Diana's code review

470601c

Co-authored-by: Diana Barsan <[email protected]>

code review changes

7f266e2

added null checks to see if it passes tests

5123c8a

PrjShrestha requested a review from dianabarsan August 10, 2023 08:04

dianabarsan requested changes Aug 10, 2023

View reviewed changes

PrjShrestha added 3 commits August 10, 2023 22:20

Added tests to test add_patient trigger changes

f076a63

Code review feedbacks and added unit test for getphonefield function …

325efed

…in registration.js

Upgraded eslint versoin to ecma for optional chaining support, added …

c05ab7a

…more tests

PrjShrestha requested a review from dianabarsan August 11, 2023 12:19

dianabarsan approved these changes Aug 14, 2023

View reviewed changes

1yuv reviewed Aug 15, 2023

View reviewed changes

PrjShrestha added 5 commits August 15, 2023 14:30

Merge branch 'master' into 8204

e06fbe9

Resolve commit

45d74b5

Aded nepali phone number support

c5eacaf

used const value instead or reading from object

49a05b3

Adding comment to trigger build run

9e3116f

garethbowen merged commit bc2304a into medic:master Aug 16, 2023
14 checks passed

binokaryg mentioned this pull request Oct 29, 2024

Check if a phone number is mobile or fixed line #8718

Open

-  if (formDef && formDef.fields) {
+if (!formDef?.fields) {
+  return;
+}
+return Object
+  .keys(formDef.fields)
+  .find(key => formDef.fields[key].type === 'phone_number');

-          if (!phoneNumberParser.validate(config.getAll(), (doc.fields[patientPhoneField]))) {
+const patientPhone = doc.fields[patientPhoneField];
+if (!phoneNumberParser.validate(config.getAll(), patientPhone)) {
+....
+  return;
+}
+patient.phone = patientPhone;

	.then(utils.saveDocs(docs))
	.then()) => utils.saveDocs(docs))

	.then(utils.saveDocs(docs))
	.then(() => utils.saveDocs(docs))

	const patient_phone = '+9779841123123';
	const patientPhone = '+9779841123123';

	chai.expect(patients[0]).to.be.undefined;
	chai.expect(patients.rows.length).to.equal(0);

	chai.expect(updated.patient_id).to.be.undefined;
	chai.expect(updated[0].patient_id).to.be.undefined;

	patient.phone = doc.fields[patientPhoneField];
	patient.phone = patientPhone;

feat(#8204): Add feature to support adding phone number when registering patients #8369

feat(#8204): Add feature to support adding phone number when registering patients #8369

Conversation

PrjShrestha commented Jul 6, 2023 • edited Loading

Description

garethbowen left a comment

Choose a reason for hiding this comment

PrjShrestha commented Aug 8, 2023 • edited Loading

dianabarsan left a 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

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

dianabarsan left a 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

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

PrjShrestha commented Aug 14, 2023 • edited Loading

dianabarsan left a 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

PrjShrestha commented Jul 6, 2023 •

edited

Loading

PrjShrestha commented Aug 8, 2023 •

edited

Loading

PrjShrestha commented Aug 14, 2023 •

edited

Loading