Skip to content
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

feat(#6390): update phone widget to allow not checking for dupes #9340

Merged
merged 8 commits into from
Aug 22, 2024

Conversation

jkuester
Copy link
Contributor

@jkuester jkuester commented Aug 16, 2024

Description

Closes #6390

medic/cht-docs#1498

This PR add support for a new way to configure a phone number field in a form. Previously, the phone widget was configured in an xlsxform field by setting type: tel. By default, the phone widget both validates the phone number value and checks for existing contacts that have the same number.

With these new changes, the existing (now deprecated) functionality stays the same, but going forwards, phone number fields should be configured in an xlsxform by setting type: string with appearance: number tel. Fields with this new configuration will always validate the phone number value, but by default they will not check for existing contacts with the same number. To enforce a unique phone number with the new style of field, you should add a new column with instance::cht:unique_tel: true

Testing considerations

We had some existing coverage of the phone widget in the enekto_widgets tests both in e2e and integration/cht-form. However, as we determined when we did the countdown timer feature, it seems more sustainable to break out tests/forms for specific widgets instead of trying to cram everything into the enketo_widgets tests. So, I have created a new phone_widget form and added tests for that, both in e2e and integration/cht-form. I covered as much as possible in the cht-form tests, but part of the phone-widget functionality involves querying the database to check for other contacts with the same phone number. This functionality cannot be properly validated in the cht-form tests, so I added a few cases to the e2e tests.

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

Compose URLs

If Build CI hasn't passed, these may 404:

License

The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.

@jkuester jkuester force-pushed the 6390_optional_dup_check_for_tel branch from dd6b542 to 5ecbdd4 Compare August 16, 2024 16:45
@@ -14,7 +14,7 @@
"overrides": [
{
"files": [
"src/js/bootstrapper/*.js"
"src/js/**/*.js"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it is fine to let any of the js files log to the console. Right?

};

// Remove this when we no longer support the tel xlsxform type
const deprecated = {
Copy link
Contributor Author

@jkuester jkuester Aug 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following a similar pattern we used in the countdown timer widget. I put all the deprecated functionality in this object, so when we remove support for the tel type, it is more clear what parts of the code need to be updated (basically just the places where we use this object).

constructor( element, options, Settings = window.CHTCore.Settings ) {
super(element, options);

_init() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Enketo Widget class has an _init function that is intended to be overridden to include initialization logic. We use that method in our other custom widgets, so just cleaning up this old code to follow the new convention.


// Add a proxy input field, which will send its input, formatted, to the real input field.
// TODO(estellecomment): format the visible field onBlur to user-friendly format.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.get()
.then( function( settings ) {
if ( !phoneNumber.validate( settings, fieldValue ) ) {
throw new Error( 'invalid phone number: "' + fieldValue + '"' );
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the implementation of the other Enketo Type validations, it seems that none of them are deliberately throwing an error. Instead they just return false when the validation fails. The main behavior difference that I could find between returning false and just throwing an error is that an invalidated event is not properly emitted for the input when an error is thrown.

To remain more consistent with existing Enketo behavior, I have updated the validation logic to just return false instead of throwing errors.

@jkuester jkuester marked this pull request as ready for review August 16, 2024 22:20
Copy link
Contributor

@tatilepizs tatilepizs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good Josh, thank you for adding all of the new tests scenarios ⭐

@jkuester jkuester changed the title feat(#6390): update phone widget to support not checking for dupe numbers feat(#6390): update phone widget to allow not checking for dupes Aug 22, 2024
@jkuester jkuester merged commit 6a3d444 into master Aug 22, 2024
47 checks passed
@jkuester jkuester deleted the 6390_optional_dup_check_for_tel branch August 22, 2024 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow for phone number validation but also allow for duplicate phone numbers
3 participants