-
Notifications
You must be signed in to change notification settings - Fork 7
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
CLDC-3787 Autocomplete address search #2924
base: main
Are you sure you want to change the base?
Conversation
Created review app at https://review.submit-social-housing-data.communities.gov.uk/2924 |
@@ -18,7 +18,7 @@ def index | |||
end | |||
elsif query.match?(/[a-zA-Z]/) | |||
# Query contains letters, assume it's an address | |||
service = AddressClient.new(query) | |||
service = AddressClient.new(query, { minmatch: 0.2 }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The minmatch I mean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah forgot to talk about this. It will reduce the threshold for returning acceptable results, so OS Places via AddressClient will return results for search queries with fewer characters and less certainty, meaning as a user types they get a slightly better experience with results starting to popup sooner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of also increasing the number of results returned from the default, that is now possible via this options
parameter should we want to do that. For example passing: { minmatch: 0.2, maxresults: 15 }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What the minmatch
change now means in practice is that where previously the ability to search for address and uprn at the same time was theoretically possible in practice there weren't any values of say less than 5 numeric characters where both the Address and UPRN api would return anything. In reality you'd end up for an small numeric input e.g. "134" just getting a single UPRN result back because the Address one had a high threshold and would not return anything yet. This way we now return the UPRN result for "134" first and also 10 other address results. I had thought about returning only 9 other address results if 1 is returned from UPRN to keep the overall results to 10 but did not make the change and it isn't that important, we can make that change though using maxresults
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there at all any sorting? Do we know that the best matches would always be in that top 10, or is there a chance that there will be a lot of not very close results purely because they get returned as first on the list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t perform any sorting, but OS Places does return a somewhat sorted list. For example, if you type in a full postcode, you’ll see all the addresses for that road in order. Since we’re passing the maxResults variable of 10 to OS Places, they return the top 10 best results on their end. If we were receiving 100 results and selecting the top 10 ourselves, there might be a chance of getting less relevant results. However, because OS Places handles the sorting, we can assume they are returning the top 10 best matches based on our request.
The results they send back could be better sorted and more accurate in matching. When there is sufficient information, such as a postcode and address line, we can rely on getting a good list. With less data, the results can be a bit random, but they are still fairly relevant to the query entered.
@@ -50,7 +50,7 @@ def validate_property_postcode(record) | |||
end | |||
|
|||
def validate_la_in_england(record) | |||
return unless record.form.start_year_2025_or_later? | |||
return unless record.form.start_year_2025_or_later? || (record.startdate_changed? && record.startdate_was.year == 2025 && record.startdate.year == 2024) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we now also want to validate it for 2024?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No? I think we're not adding the LA validation in 2024 because it's mid year? This change just checks the address is in England in all of 2025 plus the one scenario when someone tries to change a 2024 log to a 2025 log, they'd be prevented from making that change. They'd see the same error message we use in 2025 and an all related answers page prompting them to clear one of the start date or address. Think this work, unless we want to also add the validation on LA to all logs in 2024 going forward from when this is released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm misreading this, but isn't this checking that it changed from 2025 (startdate_was) to 2024 (startdate)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, this change is incorrect. The additional condition was not needed as the validation is already correctly triggering when switching between years. I've found the issue is to do with uprn_selection not being cleared and am working on a fix. I've removed the changes to these validations.
This PR creates a new way to add addresses to logs.
Before
Currently we ask users if they know the property's UPRN, if they do they can enter it and continue. We'll derive the address and local authority from the UPRN.
Alternatively a user can attempt to find the address by search, inputing an address line and postcode. The results who show on the next page and a user is asked to pick from the 10 available results. We'll derive the UPRN and local authority from their selection.
Should a user not find the address they're looking for they can enter it manually. We'd store the address but not derive a UPRN. If the postcode is valid then we'll still be able to derive a local authority but it's unlikely if the address was not found in the previous steps.

After
In this PR we replace the URPN question and Find Address feature with a search box, returning results as the user types. We also show only one version of the address question at a time (search/manual).
This search box uses the same OS Places API to populate the results as we use in the old question but allows the user to make changes to their query dynamically. However, should they be unable to find their address they can switch between this search question and the manual entry question.
After: Users with JavaScript disabled
Should a user have JavaScript disabled the search box feature will not be available. We'd then add a message that the user should manually enter the address.

Possible future improvements: