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

PAF-129/ PAF-128 - Fix bug to add country from list and validate country textbox. #96

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

TemitopeAyokuHO
Copy link
Contributor

@TemitopeAyokuHO TemitopeAyokuHO commented Dec 15, 2023

What?

PAF-128 Country' fields allow any text
PAF-129- All 'Country' fields allow any text without the use of drop down list

Why?

remove any url from the input text and accept validation entry.
this validation with control the textbox for both country and phone numbers

How?

  • added validation attribute in the field/index.js file
  • added validation message in the validation.js file

Testing?

Manual testing via UI

Screenshots (optional)

Anything Else? (optional)

Copy link
Contributor

@jamiecarterHO jamiecarterHO left a comment

Choose a reason for hiding this comment

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

Screenshot 2023-12-15 at 13 47 58 If we're removing the ability to type in an entry to these fields we should also remove or update any bits of hint text that suggest 'Start to type...' or similar.

Otherwise the update to code looks fine - stops users from entering a non-country-list entry here.

I would actually question if this is a bug in the first place. I wonder from a UCD perspective whether any text entry should be allowed in case the countryList becomes out of date or is incomplete. It may be more of an advisory list. As far as I know every other HOF form field with a country select has the typeahead and accepts non-list entry.

If business stakeholders have said these fields must absolutely have a value from the countryList then no problem. But might be worth confirming with the ticket reporter.

@TemitopeAyokuHO TemitopeAyokuHO force-pushed the PAF-129-Bug-fix branch 2 times, most recently from 2786914 to 711a511 Compare December 27, 2023 22:43
@TemitopeAyokuHO TemitopeAyokuHO changed the title Fix bug for Dropdown box PAF-129- Fix bug for Dropdown box Dec 27, 2023
@TemitopeAyokuHO TemitopeAyokuHO changed the title PAF-129- Fix bug for Dropdown box PAF-129- Add validation on textbox for country Dec 28, 2023
@TemitopeAyokuHO
Copy link
Contributor Author

The business came back with responses
"I have had a think about the autocomplete component. I think the best solution to the problem you highlighted would be :
to include validation on the field when it is not empty, for example letting the user know that they have entered an option that is not available
the data should also clear if the user empties the field, so their previous response would not appear on the CYA page"

I have added a validation on each textbox for both country and phone number. a new ticket was created for this PAF-128 but i have made the modification on this branch

@jamiecarterHO
Copy link
Contributor

jamiecarterHO commented Jan 4, 2024

Now that the phone number fields are required on the page "Do you want to tell us about another location where the crime is taking place?" if I choose 'no' I still need to go back and fill in a phone number on the 'yes' bit before I can continue. I think the phone number field might need a dependent: {field: 'crime-location',value: 'yes'} (or whatever choice is conditional for other phone fields) in /fields/index.js like the other fields in the address do.

@jamiecarterHO
Copy link
Contributor

The business came back with responses "I have had a think about the autocomplete component. I think the best solution to the problem you highlighted would be : to include validation on the field when it is not empty, for example letting the user know that they have entered an option that is not available the data should also clear if the user empties the field, so their previous response would not appear on the CYA page"

The business comment above seems to me to suggest that empty field should be an option, but if they want to put something it should be a country from the list. Now that fields are required they must have a value, and once a value from the list has been added it can't be removed again except with another one from the list.

@TemitopeAyokuHO
Copy link
Contributor Author

Now that the phone number fields are required on the page "Do you want to tell us about another location where the crime is taking place?" if I choose 'no' I still need to go back and fill in a phone number on the 'yes' bit before I can continue. I think the phone number field might need a dependent: {field: 'crime-location',value: 'yes'} (or whatever choice is conditional for other phone fields) in /fields/index.js like the other fields in the address do.

I have just realised this issue after replicating it, i will be fixing it

@TemitopeAyokuHO
Copy link
Contributor Author

The business came back with responses "I have had a think about the autocomplete component. I think the best solution to the problem you highlighted would be : to include validation on the field when it is not empty, for example letting the user know that they have entered an option that is not available the data should also clear if the user empties the field, so their previous response would not appear on the CYA page"

The business comment above seems to me to suggest that empty field should be an option, but if they want to put something it should be a country from the list. Now that fields are required they must have a value, and once a value from the list has been added it can't be removed again except with another one from the list.

Reason for doing this was if the user enters a country which is not from the list, in the summary page the country will not render. to fix this bug a validation control is necessary or dropdown menu. I hope this answers that?

@TemitopeAyokuHO TemitopeAyokuHO changed the title PAF-129- Add validation on textbox for country PAF-129- Fix bug to add country from list and validate country textbox. Jan 12, 2024
@jamiecarterHO
Copy link
Contributor

Reason for doing this was if the user enters a country which is not from the list, in the summary page the country will not render. to fix this bug a validation control is necessary or dropdown menu. I hope this answers that?

Yes, that makes sense to me, thanks.

[PAF- 129](https://collaboration.homeoffice.gov.uk/jira/browse/PAF-129) All 'Country' fields allow any text without the use of drop down list

[PAF-128](https://collaboration.homeoffice.gov.uk/jira/browse/PAF-128 URL's are being accepted in all free text fields

- added validation rules to all countries fields in field/index.js

- added dependent property to avoid error message when yes  is selected on another location

Tested manually
@TemitopeAyokuHO TemitopeAyokuHO changed the title PAF-129- Fix bug to add country from list and validate country textbox. PAF-129/ PAF-128 - Fix bug to add country from list and validate country textbox. Jan 18, 2024
@TemitopeAyokuHO TemitopeAyokuHO merged commit 9676e08 into master Jan 30, 2024
2 of 3 checks passed
@TemitopeAyokuHO TemitopeAyokuHO deleted the PAF-129-Bug-fix branch January 30, 2024 16:04
JHoldergov pushed a commit that referenced this pull request Feb 1, 2024
…try textbox. (#96)

* PAF-128 & PAF-129 Added validation for phone and country textbox

[PAF- 129](https://collaboration.homeoffice.gov.uk/jira/browse/PAF-129) All 'Country' fields allow any text without the use of drop down list

[PAF-128](https://collaboration.homeoffice.gov.uk/jira/browse/PAF-128 URL's are being accepted in all free text fields

- added validation rules to all countries fields in field/index.js

- added dependent property to avoid error message when yes  is selected on another location

Tested manually

* add behaviour to unset values
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.

2 participants