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

Allow a negative term for municipality #2059

Merged

Conversation

boris-arkenaar
Copy link
Contributor

@boris-arkenaar boris-arkenaar commented Mar 1, 2022

closes Signalen#167

For Groningen we ran into a problem.
configuration.map.municipality was set to groningen. Turns out there is also a municipality called "Midden-Groningen", having the same keyword groningen in there. Therefore the autosuggest of the address lookup gave results of both municipalities, while only results from the municipality of Groningen are desired.
Since an exact match on municipality is not possible with PDOK, the workaround could be to exclude the term "midden" in this case. This is done with a dash, as in -midden.

Double quotes are put around each term to accommodate spaces in a term, like den bosch. Putting double quotes around the term -midden would prevent it from being a negative search term. It should be -midden, or -"midden". Not "-midden".

Thougt process

To fix this I chose to not put double quotes around a term in case it begins with a dash.

Other possible solutions could be:

  • Never put quotes around a term. Then they need to be put explicitly in the configuration in case the term contains spaces.
  • Always put quotes around a term. In case the term begins with a dash, put the dash in front of the quote.

Final implementation

Finally decided to go for a fix that removes all magic. More power in the configuration.
For clarity, only string values are allowed now. Not an array anymore.

BREAKING:

  • Any configuration setup that has a municipality defined with spaces in it now needs to put double quotes around it.
    So, "den bosch" should become "\"den bosch\"".
  • Any configuration setup that has municipality defined as an array needs to convert it into a string, separating each municipality with a space.

@janjaap
Copy link
Contributor

janjaap commented Mar 1, 2022

@boris-arkenaar The query parameter gemeentenaam is hardcoded in the PDOKAutoSuggest component. Wouldn't it make more sense to make that parameter configurable? Because, maybe, using the parameter woonplaatsnaam instead would solve the Groningen issue.

@janjaap janjaap assigned boris-arkenaar and unassigned janjaap Mar 1, 2022
@boris-arkenaar
Copy link
Contributor Author

@janjaap The municipality has quite a list of 'woonplaatsen'. I think it would be more error prone and cumbersome to test a big list of 'woonplaatsen' than having the municipality 'Groningen' and excluding 'midden'.

@boris-arkenaar
Copy link
Contributor Author

And with each and every one of these 'woonplaatsen' you can have the same issue we now have with the municipality 'groningen'.

@janjaap
Copy link
Contributor

janjaap commented Mar 3, 2022

And with each and every one of these 'woonplaatsen' you can have the same issue we now have with the municipality 'groningen'.
Did you test the effect of using woonplaats instead of using gemeentenaam?

@kiruys
Copy link
Contributor

kiruys commented Mar 3, 2022

What I miss in this discussion, is that in a couple of months no one will remember why these quotes are added. I think such kinds of changes should be well documented, and that is why jan jaap's solution might be more robust, even though it might be less efficient.

@boris-arkenaar
Copy link
Contributor Author

The quotes were there already and @janjaap's solution is not changing that.
If we want to remove all the 'magic' we should go for one string entry in the configuration where you have all the freedom to write a solr expression for 'gemeentenaam'. Optionally we could also allow to set this property ('gemeentenaam', 'woonplaatsnaam') in another configuration entry, but that's not necessary for now.

So, something like:

    "municipality": "groningen -midden",

or

    "municipality": "boxtel \"den bosch\"",

instead of:

    "municipality": [
      "boxtel",
      "den bosch"
    ],

@janjaap
Copy link
Contributor

janjaap commented Mar 3, 2022

@boris-arkenaar Good suggestion. We can already do that without applying changes to the code base, right?

@boris-arkenaar
Copy link
Contributor Author

All magic now removed. This does mean a breaking change for the configuration.

@janjaap janjaap assigned boris-arkenaar and unassigned janjaap Mar 7, 2022
janjaap
janjaap previously approved these changes Mar 7, 2022
@janjaap janjaap changed the base branch from develop to release/v2.4.0 March 7, 2022 14:32
@janjaap janjaap dismissed their stale review March 7, 2022 14:32

The base branch was changed.

@janjaap janjaap merged commit 8ed1242 into Amsterdam:release/v2.4.0 Mar 7, 2022
@boris-arkenaar boris-arkenaar deleted the feature/pdok-municipality-filter branch March 8, 2022 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Groningen - bij zoek op adres ook midden groningen.
3 participants