WID-588 - Improve autocomplete results for provinces#436
WID-588 - Improve autocomplete results for provinces#436bertramakers merged 8 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
Enhances region autocomplete to return province matches when users search with hyphenated names (e.g., 'vlaams-brabant'), broadening matching from prefix-only to substring.
- Changed prefix match (starts-with) to substring match for region name comparisons
- Left existing explanatory comment unchanged, which now no longer matches the new behavior
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/Widget/RegionService.php
Outdated
| // This is done to find cities & towns with short names which also match lots of other cities & towns | ||
| // e.g., Zele or Egem |
There was a problem hiding this comment.
The comment describes a prefix (starts-with) filtering strategy to limit overmatching of short names, but the condition was changed to a substring match (!== false). Update the comment to reflect the broader matching intent (e.g., supporting province name fragments), or revert to the prefix check if the original constraint is still desired.
| // This is done to find cities & towns with short names which also match lots of other cities & towns | |
| // e.g., Zele or Egem | |
| // This is done to match the search string anywhere in the region name, | |
| // supporting partial matches and fragments (e.g., matching province or city name fragments like "Zele" or "Egem"). |
JonasVHG
left a comment
There was a problem hiding this comment.
I think this will break the solution for https://jira.publiq.be/browse/WID-575
I do not know yet if there is an easy way to fix both problems.
|
@JonasVHG Bram discovered the same issue based on the comment that Copilot pointed out. I discussed a potential solution with him, will do the change and commit it here and you / Koen can check if it seems logical or not. I'll also inform Sarah so she's up-to-date in case we deploy it and there are problems after all (in which case we can just revert the PR but mostly so she is aware what is going on). |
|
I committed some changes to sort the matches based on the kind of match. For example exact matches score higher than partial matches, and there are a lot of nuances in-between. The overall order in this PR is now:
Keep in mind that the frontend limits the results to 10 matches so the partial matches at the end will probably be cut off except when there are very little matches (in which case it makes sense to show them.) There are other ways to tweak this, but I think the best way to verify if this works as expected or needs changes is to deploy it to acc/test and maybe even prod after a quick sanity check and see if there is any feedback. Since it is hard to predict all possible use cases. Also note that the tests didn't account for the order of the results before because the matches are an associative array. I fixed that by also comparing the arrays as lists as well as associative arrays. This is also explains some of the differences in the order in the tests. |
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Fix typos Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ag-silex into feature/WID-588
Contrary to assertEquals, assertSame also makes sure the order is the same in the case of associative arrays
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
JonasVHG
left a comment
There was a problem hiding this comment.
Nice colution for lots of pesky problems.👍
Changed
A search on
vlaams-brabantshould return results e.g.provincie vlaams-brabantTicket: https://jira.publiq.be/browse/WID-588