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

Fix: Address lookup validation #98

Merged
merged 8 commits into from
Nov 20, 2024
Merged

Fix: Address lookup validation #98

merged 8 commits into from
Nov 20, 2024

Conversation

Adnan-cds
Copy link
Contributor

@Adnan-cds Adnan-cds commented Oct 18, 2024

What does this change?

The LocalGov address lookup element's validation process is losing existing form error messages set by other parts of a form. Some of its own error messages are also getting lost. This change fixes this.

I have also updated the wording for error messages shown on required fields like Address line 1 and Postcode. Instead of saying "Postcode field is required", it now says "You must enter the postcode." Error message wording courtesy of @DavidBAsch

How to test

  • Create a new Webform.
  • Add a set of radio buttons. Mark it as required.
  • Add a LocalGov address lookup element.
  • Set the visibility condition of the address lookup element dependent on one of the radio buttons.
  • Submit the Webform without selecting any of the radio buttons. At the moment, form submission completes and an error message about the radio button is displayed in the confirmation page rather than on the form page where the form elements are. Once this fix is applied, the form submission fails as expected.

The address lookup validation process was loosing existing form error messages
set by other parts of a form.  Some of its own error messages also got lost.

WIP.
The LocalGov address lookup element should not tamper with error messages raised
by other elements.  It should also remain inactive when none of its subelements
are marked as required.
@Adnan-cds Adnan-cds marked this pull request as ready for review October 22, 2024 18:00
@Adnan-cds Adnan-cds marked this pull request as draft October 24, 2024 16:13
@Adnan-cds
Copy link
Contributor Author

Marking this pull request as "Draft" to add further improvements.

The address selection dropdown was not appearing after a form submission error
involving the address lookup element.

Also:
- removed some unused and commented out Javascript code.
- improved code comments.
Updated error message text in test code to match updated error handling.
Adding custom error messages to several subelements of the Address lookup
element.  This is for better UX.
Selecting an address following lookup and then manually altering the selected
address skipped validation.  Fixed now.
@Adnan-cds Adnan-cds marked this pull request as ready for review November 8, 2024 18:26
@Adnan-cds Adnan-cds requested a review from finnlewis November 8, 2024 18:27
@finnlewis finnlewis self-assigned this Nov 19, 2024
Copy link
Member

@ekes ekes left a comment

Choose a reason for hiding this comment

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

Woo that was niche, even to test.

But does as described, and automated tests look good.

@@ -59,6 +59,9 @@ public static function getCompositeElements(array $element) {
$element_list['address_1']['#prefix'] = '<div class="js-address-entry-container">';
$element_list['postcode']['#suffix'] = '</div>';

// Custom error message for address line 1.
$element_list['address_1']['#required_error'] = t('You must enter an address.');
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking this. But might be worth moving to StringTranslationTrait at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. It caused me some unease as well. But eventually stuck to the t() function for consistency with the rest of the file. I have just raised a ticket to replace the entire lot.

@Adnan-cds
Copy link
Contributor Author

Woo that was niche, even to test.

Yeah, spotted by @DavidBAsch :D The Address lookup element is a lot better because of him.

Thanks for the approval. Merging...

@Adnan-cds Adnan-cds merged commit 3dd9341 into 1.x Nov 20, 2024
8 checks passed
@Adnan-cds Adnan-cds mentioned this pull request Nov 20, 2024
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.

3 participants