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

Provided missing gazetteer to Coordinates Entry #1070

Merged
merged 12 commits into from
Jan 25, 2024

Conversation

RobAndrewHurst
Copy link
Contributor

No description provided.

@RobAndrewHurst RobAndrewHurst added the Bug A genuine bug. There must be some form of error exception to work with. label Jan 23, 2024
Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

Thanks @RobAndrewHurst - working as expected for me!
✅ Tested with bogus lat and long and warning appears in the console.
✅ Tested with correct lat and long and i'm zoomed to that location.
❓ When you search a bogus lat and long and the warning appears in the console. However, the user isn't given a message to say this. I wonder if we should have an alert too?
❓ When you search for a correct lat and long that is outside of the locale extent (e.g. searching for a US city but in a UK restricted locale), the zoom to just goes to the further corner of the extent. I think this should also warn and alert the user they can't access that location?

📃 Please could you check that gazetteer searching by lat and long is in the Config Docs too 🙂

Copy link
Contributor

@AlexanderGeere AlexanderGeere left a comment

Choose a reason for hiding this comment

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

Sames test steps as simon, Looks like its zooming in on the real points and warning about ones outside the extent or not real.
The incorrect/bogus co-ordinates get added to the panel.
Screenshot 2024-01-23 145015

@RobAndrewHurst
Copy link
Contributor Author

@dbauszus-glx @simon-leech @AlexanderGeere

How do we feel about logging the warning of "Latitude and longitude values must be within valid ranges" if the user enters a bogus lat lon?

image

@simon-leech
Copy link
Contributor

simon-leech commented Jan 23, 2024

@RobAndrewHurst I like this implementation - as its clear to the user and better than an alert!

➕Please could you add a mapp.dictionaries object for this text so it can be translated in the future into other supported languages?

I think the only other thing I mentioned was this one -
❓ When you search for a correct lat and long that is outside of the locale extent (e.g. searching for a US city but in a UK restricted locale), the zoom to just goes to the further corner of the extent. I think this should also warn in the same way with the text highlighting that the location is not accessible or similar?

@AlexanderGeere
Copy link
Contributor

@RobAndrewHurst I like how that looks, much cleaner than the pop.

@simon-leech
Copy link
Contributor

@RobAndrewHurst I just removed the changes to the fitView.mjs as the changes in the gazetteer.mjs negated the need for them! All looking good!

Please can you test this out using these coordinates 63.032097003338635, -122.06125081191139 on a UK-restricted locale and you will see what I mean about the zoom to location going to the far corner of the restricted extent. I think in this case we should also show the same warning message?

@RobAndrewHurst
Copy link
Contributor Author

@RobAndrewHurst I just removed the changes to the fitView.mjs as the changes in the gazetteer.mjs negated the need for them! All looking good!

Please can you test this out using these coordinates 63.032097003338635, -122.06125081191139 on a UK-restricted locale and you will see what I mean about the zoom to location going to the far corner of the restricted extent. I think in this case we should also show the same warning message?

I am aware of what you mean. I am trying to figure out the best way to solve this

@RobAndrewHurst
Copy link
Contributor Author

Changes Made ✍️

  • Added a check to ensure that the provided target extent is contained within the current extent before performing map operations.

The code introduces a safeguard to verify the validity of the target extent in the optional options. If a target extent is provided, the pull request checks whether it is within the current extent. If not, an alert is triggered, and the function returns false, preventing invalid map operations.

This enhancement aims to improve the reliability of the code by validating geographic boundaries before executing critical map-related operations.

Testing

The changes have been tested locally and are functioning as expected with different locales performing different coordinate searches.

Please review the changes, and let me know if any further changes are needed :)

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

This is brilliant thank you @RobAndrewHurst ! 🎉🎉

Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

Can you check the gazetteer test workspace with this configuration?

            "gazetteer": {
                "layer": "retailpoints",
                "qterm": "store_name",
                "leading_wildcard": true,
                "limit": 5,
                "no_result": "foo",
                "datasets": [
                    {
                        "title": "Postcode",
                        "qterm": "postcode",
                        "no_result": null
                    }
                ]
            },

this currently fails on the xhr check.

@simon-leech
Copy link
Contributor

simon-leech commented Jan 24, 2024

@RobAndrewHurst @dbauszus-glx I put together a fix for this but I think there's probably a more elegant way of doing this. I'm simply calling the search function with the same parameters that are passed if a datasets array is provided.

I think the key bit was the xhr was missing.

Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

This is working for me though I had to make a couple of changes.

  this.Map.getView().fit(extent, {
    padding: [50, 50, 50, 50],
    duration: 1000,
    ...options
  })

The ol.view.fit() method uses an options param as optional argument. This can be provided in custom script. Custom params not understood by the ol API should not be added to this options object. The mapview.fitView returns on error but should not return true or false if the extent isn't matched. This method is called from other script sources than the gazetteer as well.

Dictionary entries should be added when the Gazetteer is created but not on every search, nor every autocomplete keypress.

lng,lat locations should be checked with extent.containsCoordinate()

Please note that the mapview is usually 3857, but could be in a different projection.

  const coord = ol.proj.transform(
    [location.lng, location.lat],
    `EPSG:4326`,
    `EPSG:${gazetteer.mapview.srid}`
  )

  if (!ol.extent.containsCoordinate(gazetteer.mapview.extent, coord)) {
    alert(mapp.dictionary.invalid_lat_lon);
    return;
  }

Copy link

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@RobAndrewHurst
Copy link
Contributor Author

Nice one @dbauszus-glx!

@RobAndrewHurst RobAndrewHurst merged commit 73527a9 into GEOLYTIX:main Jan 25, 2024
5 checks passed
@RobAndrewHurst RobAndrewHurst deleted the gazetteer_bug branch January 25, 2024 06:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A genuine bug. There must be some form of error exception to work with.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants