-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fix Access to area.geojson. #3157
Conversation
Access to area.geojson in console/account/new and userdetails where broken (in some cases). Fix the OrgsController
@@ -386,7 +375,7 @@ public void getAreaConfig(HttpServletResponse response) throws IOException, JSON | |||
LOG.info("Could not parse value", e); | |||
} | |||
JSONObject areas = new JSONObject(); | |||
areas.put("url", areasUrl); | |||
areas.put("url", "/console/public/area.geojson"); |
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.
Doesn't this look like technical debt ?
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.
We have replaced the discovery of "area" by an HTTP endpoint which will send you a file or a redirect to another HTTP endpoint. The configuration is now part of this "Area HTTP endpoint". Before doing this I looked at the first part of the path and if it can be customized. It seems not, since /console
is hardcoded everywhere in the manager js code (and in some Java class file).
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.
Sorry I should have been more explicit.
"url: /console/public/area.geojson" does not need to be part of the areaConfig.json response, since it is now static.
You could have modified the JS code to directly hit /console/public/area.geojson instead...
27de261
to
74a3ffc
Compare
@@ -121,7 +121,7 @@ class AreaController { | |||
|
|||
this.updateSelection(selected) | |||
this.loading = false | |||
}) | |||
}).catch(ex => console.log('Cannot display area. GeoJSON file is malformed or Area config is wrongly configured in the data', ex)) |
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.
Asking thought of @tonio on this.
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 think a broken GeoJSON file will break at line 97.
What about testing this above, & add a specific test for config with an explicit error message ?
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.
Small comments, otherwise (client side) lgtm. 😎
@@ -97,10 +97,17 @@ class AreaController { | |||
vector.getSource().addFeatures(format.readFeatures(json, conf)) | |||
vector.getSource().forEachFeature(f => { | |||
const group = f.get(config.areas.group) | |||
if (group === undefined) { | |||
throw new Error(`Cannot get AreaGroup ${config.areas.group} in provided geojson. Check datadir config.`) |
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.
throw new Error(`Cannot get AreaGroup ${config.areas.group} in provided geojson. Check datadir config.`) | |
throw new Error(`Cannot get AreaGroup «${config.areas.group}» in provided geojson. Check datadir config.`) |
f.set('_label', f.get(config.areas.value).toString() || '') | ||
const displayName = f.get(config.areas.value) | ||
if (displayName === undefined) { | ||
throw new Error(`Cannot get AreaValue ${config.areas.value} in provided geojson. Check datadir config.`) |
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.
throw new Error(`Cannot get AreaValue ${config.areas.value} in provided geojson. Check datadir config.`) | |
throw new Error(`Cannot get AreaValue «${config.areas.value}» in provided geojson. Check datadir config.`) |
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.
Sorry but « and » are for French. If English I'll use double quote :-)
@@ -83,7 +83,7 @@ class AreaController { | |||
const format = new ol.format.GeoJSON() | |||
|
|||
this.loading = true | |||
this.$injector.get('$http').get(config.areas.url).then( | |||
this.$injector.get('$http').get('/console/public/area.geojson').then( |
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 may create a service so that is easier to track
Ok tests are failing no idea why, will investigate this. |
Access to area.geojson in console/account/new and userdetails where
broken (in some cases). Fix the OrgsController