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

Prevent Selection of a Location on a Layer with no Infoj #1500

Merged
merged 8 commits into from
Sep 26, 2024
Merged

Prevent Selection of a Location on a Layer with no Infoj #1500

merged 8 commits into from
Sep 26, 2024

Conversation

simon-leech
Copy link
Contributor

@simon-leech simon-leech commented Sep 25, 2024

Prevent Selection of a Location on a Layer with no Infoj

Description

Currently, the location/get method does not check for the presence of an infoj on the locations layer.
This means that its possible to select a location, which then errors as it tries to build a SQL statement in this format
SELECT id, FROM table WHERE id = $1.

This PR addresses this by simply returning if the layer has no infoj, preventing this issue.

Type of Change

Please delete options that are not relevant, and select all options that apply.

  • ✅ Bug fix (non-breaking change which fixes an issue)

Testing Checklist

Please delete options that are not relevant, and select all options that apply.

  • ✅ Existing Tests still pass
  • ✅ Ran locally on my machine

Code Quality Checklist

Please delete options that are not relevant, and select all options that apply.

  • ✅ My code follows the guidelines of XYZ
  • ✅ My code has been commented
  • ✅ Documentation has been updated
  • ✅ New and existing unit tests pass locally with my changes
  • ✅ Main has been merged into this PR

@simon-leech simon-leech marked this pull request as ready for review September 25, 2024 16:04
@simon-leech simon-leech added Bug A genuine bug. There must be some form of error exception to work with. v4.11.0 labels Sep 25, 2024
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.

Review in progress

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.

Added a test for no infoj.

@dbauszus-glx dbauszus-glx self-requested a review September 26, 2024 10:16
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 looks good.

I only updated the wording and removed the location from layer.infoj property and made this optional in jsdocs.

Copy link

Copy link
Contributor

@RobAndrewHurst RobAndrewHurst left a comment

Choose a reason for hiding this comment

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

I am all happy with this.

I added in a test for the getInfoj method so that people can better understand the purpose.

@RobAndrewHurst RobAndrewHurst merged commit 7de8ccf into GEOLYTIX:main Sep 26, 2024
5 checks passed
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