Skip to content

Conversation

iamdharmesh
Copy link
Collaborator

Description of the Change

This PR includes validation improvements to address the following issues:

Closes #104
Closes #101
Closes #107

How to test the Change

#104:
Follow the steps in the issue description and verify that the issue no longer exists and has been fixed.

#101:
Follow the steps in the issue description and verify that the issue no longer exists and has been fixed.

#107:
Ensure that required field validation works as expected.

Changelog Entry

Fixed - Phone number and required field validations.

Credits

Props @MaxwellGarceau @jeffpaul @dkotter @iamdharmesh

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@github-actions github-actions bot added this to the 1.9.0 milestone May 19, 2025
@iamdharmesh iamdharmesh marked this pull request as ready for review May 19, 2025 14:21
@iamdharmesh iamdharmesh requested a review from dkotter May 19, 2025 14:21
@github-actions github-actions bot added the needs:code-review This requires code review. label May 19, 2025
dkotter
dkotter previously approved these changes May 19, 2025
@dkotter dkotter requested a review from qasumitbagthariya May 19, 2025 16:50
@vikrampm1 vikrampm1 modified the milestones: 1.9.0, 2.0.0 Jun 3, 2025
@qasumitbagthariya
Copy link
Collaborator

QA Update: ✅


I have verified this PR in the enhancement/341 branch, which has been fixed and is functioning as intended.

I tested the following on this branch:

#101 - Resolved ✅

Tested by updating the phone number in the test user's Mailchimp account to US format and clicking the "Update List" button in the Mailchimp admin. Then submitted a form with a phone number containing a letter. The validation worked as expected, and the form was not submitted with an invalid phone number. ✅

Screen.Recording.2025-06-11.at.3.01.57.PM.mov

#104 - Resolved ✅

Submitted a form with one digit missing in the phone number input. Validation worked as expected and the form was not submitted. ✅

Screen.Recording.2025-06-11.at.3.18.25.PM.mov

#107 - Resolved ✅

Did not change the placement of the error message, but fixed the required field check and added email validation as mentioned #107 (comment)

image

Testing Environment

  • WordPress: 6.8
  • Theme: Storefront 4.6.1
  • PHP: 8.0.30
  • Web Server: Nginx 1.20.2
  • Browser: Chrome
  • OS: macOS 15.2
  • Branch: fix/104

Steps to Test- As mentioned in the PR description.
Test Results - It is working as expected.
Functional Demo / Screencast -
Special Notes - Ready for code review (Woo)
Testing Document status:
Cases related to this Issue/PR are added to the Critical Flow Wiki pages:

  • Yes
  • Not Required/Applicable for this PR

@jeffpaul
Copy link
Collaborator

I recognize this isn't changed in this PR, but a question worth asking at this point anyway... the phone number input fields appears to force the US-centric 3 separate fields whereas testing on the Mailchimp platform side with a sample embed form there and the data in our test audience list appears to accept international formats (which would not render nicely if forced within the US-centric 3 fields). It sounds like from @dkotter quick scan in the code that there is some sort of toggle and that there's a setting on the Mailchimp platform side, but if I had to guess that setting may not be exposed in the API and almost certainly not in a way where we could edit that based on how someone might change that via plugin or block settings?

All that to say, @iamdharmesh may be worth some testing of various input types for a phone number with different settings on the Mailchimp side to see which is the most flexible approach (E.g. can we just have a single input field and allow Mailchimp to save/render it as they desire)?

@iamdharmesh
Copy link
Collaborator Author

the phone number input fields appears to force the US-centric 3 separate fields whereas testing on the Mailchimp platform side with a sample embed form there and the data in our test audience list appears to accept international formats (which would not render nicely if forced within the US-centric 3 fields).

Yes, in the audience list it is shown in a generic/international format. However, in the embedded form, it's displayed as a single field, but structured using 3 separate inputs inside it.
image

It sounds like from @dkotter quick scan in the code that there is some sort of toggle and that there's a setting on the Mailchimp platform side, but if I had to guess that setting may not be exposed in the API and almost certainly not in a way where we could edit that based on how someone might change that via plugin or block settings?

Yes, we can update the phone number format from the field settings, and based on the documentation, API support is available to update the phone format. However, during testing via an API call, it doesn’t seem to be working at the moment.

I tried setting the phone format to US and sending data in international format via the API, and it was accepted without error. So, we’re good to update the phone number input field as per our requirements, regardless of the format set on the Mailchimp side. It seems the current 3 separate fields were added to match the hosted form, and since it's a country-specific format, it gives us more control over validation. So, validation is added for the US format, whereas for international phone numbers, we don’t have validation at the plugin level, since users might enter phone numbers in multiple formats (with dashes, with or without country codes, etc.).

Since we’re not being forced by the Mailchimp API to stick with the three-field format, and we can update the phone number fields as per our requirements, please let me know how you’d like to proceed.

Thanks!

@jeffpaul
Copy link
Collaborator

Since we’re not being forced by the Mailchimp API to stick with the three-field format, and we can update the phone number fields as per our requirements, please let me know how you’d like to proceed.

I guess let's just do a super basic validation there, something that would work for US and international numbers which probably means what like no letters and perhaps limiting special characters to space, -, +, (, and )?

@iamdharmesh
Copy link
Collaborator Author

I guess let's just do a super basic validation there, something that would work for US and international numbers which probably means what like no letters and perhaps limiting special characters to space, -, +, (, and )?

Ok. So, Same single input for international and US phone numbers and update validation as per your suggestion. right?

@jeffpaul
Copy link
Collaborator

jeffpaul commented Aug 4, 2025

Ok. So, Same single input for international and US phone numbers and update validation as per your suggestion. right?

Yes.

@qasumitbagthariya
Copy link
Collaborator

Regression / Smoke Test Report ✅

Status- Working expected with Plugin Archive/Zip file same as fix specific branch.

Testing Environment

  • WordPress: 6.8.2
  • Theme: Twenty Twenty-Five 1.2
  • WooCommerce - 10.0.4
  • PHP: 8.0.30
  • Web Server: Nginx 1.20.2
  • Browser: Chrome
  • OS: macOS 15.2
  • Branch: smoke-testing

Next Step- Ready to Merge 🚀

@github-actions github-actions bot added the needs:refresh This requires a refreshed PR to resolve. label Aug 7, 2025
@vikrampm1 vikrampm1 mentioned this pull request Aug 7, 2025
30 tasks
@github-actions github-actions bot removed the needs:refresh This requires a refreshed PR to resolve. label Aug 7, 2025
@dkotter dkotter merged commit 202bfea into develop Aug 7, 2025
7 of 9 checks passed
@dkotter dkotter deleted the fix/104 branch August 7, 2025 15:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment