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

#10514 - FeatureEditor filter by geometric area #10515

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Gaetanbrl
Copy link
Contributor

@Gaetanbrl Gaetanbrl commented Aug 21, 2024

Description

This improvement will change the featureEditor plugin to allow to limit attributes table features loading by a geometric area (WKT or GeoJSON) for non ADMIN users.

This improvement don't use GeoFence and use a simple geometry.

Please check if the PR fulfills these requirements

What kind of change does this PR introduce? (check one with "x", remove the others)

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

Issue

What is the current behavior?

User can filter attribute table by custom filter or viewport.

#10514

What is the new behavior?

If configured, attribute table keep natives filters and use an additional geometric filter that can be create from URL (e.g geOrchestra restricted area) or from RAW (GEOJSON or WKT).

Breaking change

Does this PR introduce a breaking change? (check one with "x", remove the other)

  • Yes, and I documented them in migration notes
  • No

Other useful information

@tdipisa
Copy link
Member

tdipisa commented Aug 23, 2024

Hi @Gaetanbrl thank you for contributing. We will review asap.

@tdipisa tdipisa added this to the 2025.01.00 milestone Aug 30, 2024
Copy link
Member

@offtherailz offtherailz left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. In this review I reported all the things that I found that may look unconsistent or problematic.

  • see my comments inline
  • The original proposal and issue was regarding to restrict editing, but in this implementation, also the view mode encounter this filter. Also this makes the things a little unclear. Maybe the filter should be applied only in edit mode?
  • I found a bug that I didn't investigated, so I write about it here, instead of inline. With this tool active, I see an error of "max stack exceed" as well as I try to apply a quick filter.
    https://github.com/user-attachments/assets/0193f17b-bfc2-41a2-a152-184e9482c445
    I didn't debug so I don't know the exact reason. Anyway using spatialField can problematic, because too related to the advanced filter form . I'd suggest to use "filters" instead, as reported in another inline comment.

@@ -392,3 +394,38 @@ export const supportsFeatureEditing = (layer) => includes(supportedEditLayerType
* @returns {boolean} flag
*/
export const areLayerFeaturesEditable = (layer) => !layer?.disableFeaturesEditing && supportsFeatureEditing(layer);

export const isWKT = (wktString) => {
Copy link
Member

Choose a reason for hiding this comment

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

These utils can not be mapping library specific. So feature grid can work with multiple mapping libraries.
For this reason mapstore provides utilities for WKT parsing.
Please use that utilities.

export const requestRestrictedArea = (action$, store) =>
action$.ofType(OPEN_FEATURE_GRID, LOGIN_SUCCESS)
.filter(() => {
return !isAdminUserSelector(store.getState())
Copy link
Member

Choose a reason for hiding this comment

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

This makes the restriction not be applied for anonymous users. Only for logged, not-admin.
It is a little unconsistent as a restriction.

image

I think we should at least apply the same restrictedArea to the anonymous users?

Maybe a configuration inside the restrictedAreas that defines who has the restriction can be more generic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, we have think avout authenticated users (to works with geOrchestra restricted org area) first and forget anonymous users.

To apply this restriction, we needs to confirm if restricted area is apply in edit mode only or not as explain in global review comment.

Comment on lines +39 to +53
isRestrictedByArea: ({ restrictedArea }) => {
return (<Button
id="fg-isRestrictedByArea-button"
keyProp="fg-restrictedarea-button"
className="square-button-md"
bsStyle="warning"
tooltipId="featuregrid.toolbar.restrictedByArea"
disabled
style={isEmpty(restrictedArea) ? {
width: 0,
padding: 0,
borderWidth: 0
} : {}}>
<Glyphicon glyph="1-point-dashed" />
</Button>);
Copy link
Member

Choose a reason for hiding this comment

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

Here the tooltipId but the tooltip is not present in translation file.
Moreover the property do not exist for normal bootstrap button. In order to use it you should use TButton (created for feature grid) or Button with tooltip enhancer.
I noticed that bsStyle property anyway is overridden by the TButton.
So in order to have a tooltip here you should:

  • Apply the following changes.
Suggested change
isRestrictedByArea: ({ restrictedArea }) => {
return (<Button
id="fg-isRestrictedByArea-button"
keyProp="fg-restrictedarea-button"
className="square-button-md"
bsStyle="warning"
tooltipId="featuregrid.toolbar.restrictedByArea"
disabled
style={isEmpty(restrictedArea) ? {
width: 0,
padding: 0,
borderWidth: 0
} : {}}>
<Glyphicon glyph="1-point-dashed" />
</Button>);
isRestrictedByArea: ({ restrictedArea }) => {
return (<TButton
id="fg-isRestrictedByArea-button"
keyProp="fg-restrictedarea-button"
className="square-button-md"
bsStyle="warning"
visible={!isEmpty(restrictedArea)}
tooltipId="featuregrid.toolbar.restrictedByArea"
glyph="1-point-dashed"
/>);
},
  • insert a localized string for each translation file (at least mandatory ones, as usual)
  • Fix TButton like this (putting bsStyle after the forced one).
+++ b/web/client/components/data/featuregrid/toolbars/TButton.jsx
@@ -18,7 +18,7 @@ const hideStyle = {
 const normalStyle = {};
 const getStyle = (visible) => visible ? normalStyle : hideStyle;
 export const SimpleTButton = forwardRef(({ disabled, id, visible, onClick, glyph, active, className = "square-button-md", ...props }, ref) => {
-    return (<Button ref={ref} {...props} bsStyle={active ? "success" : "primary"} disabled={disabled} id={`fg-${id}`}
+    return (<Button ref={ref} bsStyle={active ? "success" : "primary"} {...props} disabled={disabled} id={`fg-${id}`}
         style={getStyle(visible)}
         className={className}
         onClick={() => !disabled && onClick()}>

Schermata del 2024-10-21 14-42-54

Note: I removed "disabled" flag for a better aspect.

* Create spatialField filters array.
* Contains filters from viewportFilter, restrictedArea, exsting WFS filter
*/
export const additionnalGridFilters = (state) => {
Copy link
Member

Choose a reason for hiding this comment

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

Using this approach makes the sync with map not working. While it still makes sense with viewport filter (because the map already shows the features in map), here this makes things a little unconsistent, and it could be practical for the user to see only the allowed rows on the map. What do you think?

A more functional approach is to add / remove the filter to the query object, as well as the other filters, like quick filter, do.
image

The filters section recently added to the filter may help you to manage it in a easier way.

Basically you can add to the filter a section filters: that can contain and use cql filter to apply some additional logic.
You can identify these filters by id, adding your own property, to manage them (e.g. adding/removing).

Comment on lines +82 to +84
* @prop {string} cfg.restrictedArea.url Geometry definition as WKT or GeoJSON loaded from URL or path.
* @prop {string} cfg.restrictedArea.raw Geometry definition as WKT or GeoJSON.
* @prop {string} cfg.restrictedArea.operator Spatial operation to performed between features and the given geometry.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @prop {string} cfg.restrictedArea.url Geometry definition as WKT or GeoJSON loaded from URL or path.
* @prop {string} cfg.restrictedArea.raw Geometry definition as WKT or GeoJSON.
* @prop {string} cfg.restrictedArea.operator Spatial operation to performed between features and the given geometry.
* @prop {object} cfg.restrictedArea object containing settings for restricted area. If present, it restricts the editing area to the given geometry. It requires at least `url` or `raw` to be defined.
* @prop {string} cfg.restrictedArea.raw Geometry definition as WKT or GeoJSON. This attribute allows to define the geometry directly in the configuration.
* @prop {string} cfg.restrictedArea.url Geometry definition as WKT or GeoJSON loaded from URL or path. If present, this wins over the raw geometry configuration. By default, the filter will use `CONTAINS` if not defined.

@Gaetanbrl
Copy link
Contributor Author

@offtherailz Thank you for this detailed review.
I'll think about functional proposals with and try to plan technical changes.

@Gaetanbrl
Copy link
Contributor Author

The source branch is now rebase / align with master (8e2d443)

@Gaetanbrl
Copy link
Contributor Author

Anyway using spatialField can problematic, because too related to the advanced filter form . I'd suggest to use "filters" instead, as reported in another inline comment.

After read the filters doc it's not easy to identify what filter I need to use and how to use it correctly.

This doc say about spatialField (considered as problematic) :

For backward compatibility, the filters "mapstore" of version "1.0.0" can contain also several other fields that will be deprecated in the future in favor of a mapstore-query-panel format, that is the format used by the query panel, and currently mostly supported in MapStore. So a filter like this is still valid:

I guess this is a state changes to apply with a specific filter object.

But what should I replace the spatialFilter filter with ? logic or mapstore-query-panel or cql ?

This doc is a good practice overview but a How to use it part could increase developer experience.

I can enhance this doc if i correctly understand how to use it with a use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FeatureEditor - Restrict editing by geometric area
3 participants