Skip to content

#10514 - FeatureEditor filter by geometric area#10515

Open
Gaetanbrl wants to merge 15 commits intogeosolutions-it:masterfrom
geo2france:g2f-restricted-area
Open

#10514 - FeatureEditor filter by geometric area#10515
Gaetanbrl wants to merge 15 commits intogeosolutions-it:masterfrom
geo2france:g2f-restricted-area

Conversation

@Gaetanbrl
Copy link
Copy Markdown
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.

https://groups.google.com/g/mapstore-developers/c/6KhmNTJzpDc/m/12J-2t9BAAAJ

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

@Gaetanbrl Gaetanbrl force-pushed the g2f-restricted-area branch from fdacd45 to dbb2fd3 Compare August 21, 2024 12:59
@tdipisa tdipisa requested a review from offtherailz August 23, 2024 12:57
@tdipisa tdipisa linked an issue Aug 23, 2024 that may be closed by this pull request
@tdipisa
Copy link
Copy Markdown
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
Copy Markdown
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.

Comment thread web/client/utils/FeatureGridUtils.js Outdated
Comment thread web/client/epics/featuregrid.js
Comment thread web/client/components/data/featuregrid/toolbars/Toolbar.jsx Outdated
Comment thread web/client/selectors/featuregrid.js Outdated
* Create spatialField filters array.
* Contains filters from viewportFilter, restrictedArea, exsting WFS filter
*/
export const additionnalGridFilters = (state) => {
Copy link
Copy Markdown
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).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it could be practical for the user to see only the allowed rows on the map. What do you think?

Exactly, this is the expected behavior

Using this approach makes the sync with map not working.

Not really sur to understand. additionnalGridFilters methods is not dependent on the screen filter but but also uses it if activate. Maybe my code is wrong with I wanted to do.

Basically you can add to the filter a section filters: that can contain and use cql filter to apply some additional logic.

I will change the code to use filters section as you expect.

Thanks for this proposals @offtherailz .

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this done?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not yet.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will use this filter instead of spatialFilter :

    {
            "format": "cql",
            "version": "1.0.0",
            "body": `${restrictedAreaOperatorSelector(state)}(${attribute}, ${asWKT})`,
            "id": `[${uniqueId("cql_restricted_area_")}]`,
            "restrictedArea": true
    };

Comment thread web/client/plugins/FeatureEditor.jsx Outdated
@Gaetanbrl
Copy link
Copy Markdown
Contributor Author

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

@Gaetanbrl Gaetanbrl force-pushed the g2f-restricted-area branch from dbb2fd3 to 3925cf0 Compare October 29, 2024 09:53
@Gaetanbrl
Copy link
Copy Markdown
Contributor Author

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

@Gaetanbrl
Copy link
Copy Markdown
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.

@Gaetanbrl
Copy link
Copy Markdown
Contributor Author

Gaetanbrl commented Nov 26, 2024

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.

After some discussions, we will make the expected changes (see comments).

Maybe the filter should be applied only in edit mode?

About this point, the natif request is to limit view by an area in edit mode and read mode.

The natif use case is to limit view and edition according to geOrchestra restricted area (this area is applied to the user's organization via geOrchestra console app).

So, we propose to add a configuration to manage 2 options :

  • allow to restrict edition only (to take your request into account)
  • allow to restrict edition and view

I see an error of "max stack exceed" as well as I try to apply a quick filter.

Needs investigation. I've seen this error before with CQL filters and geoServer limits (cross layer filter limit ?).

@Gaetanbrl
Copy link
Copy Markdown
Contributor Author

@offtherailz Thank you so much for previous feedback. But I need another one to continue.

@tdipisa tdipisa removed this from the 2025.01.00 milestone Jan 17, 2025
@tdipisa tdipisa requested a review from MV88 January 20, 2025 14:04
Copy link
Copy Markdown
Contributor

@MV88 MV88 left a comment

Choose a reason for hiding this comment

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

Hi @Gaetanbrl I resumed the review recently and i think you are still working on it right?
if so consider this as a partial review, since changes still have to be submitted yet

all of the following must have unit tests covering changes

  • selectors
  • reducers
  • actions
  • epics

please checkout also inline comments

also if you can go through the various conversation and add a comment to say which can be resolved or not.

Thanks

Comment thread web/client/selectors/featuregrid.js
Comment thread web/client/selectors/featuregrid.js Outdated
Comment thread web/client/selectors/featuregrid.js Outdated
Comment thread web/client/plugins/FeatureEditor.jsx Outdated
Comment thread web/client/reducers/featuregrid.js Outdated
Comment thread web/client/selectors/featuregrid.js Outdated
* Create spatialField filters array.
* Contains filters from viewportFilter, restrictedArea, exsting WFS filter
*/
export const additionnalGridFilters = (state) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this done?

Comment thread web/client/utils/FeatureGridUtils.js Outdated
Comment thread web/client/epics/featuregrid.js
Comment thread web/client/components/data/featuregrid/toolbars/Toolbar.jsx Outdated
Comment thread web/client/plugins/FeatureEditor.jsx Outdated
@Gaetanbrl
Copy link
Copy Markdown
Contributor Author

Gaetanbrl commented Apr 1, 2025

@MV88 thank you for this review and your precious time (which we all miss).

I resumed the review recently and i think you are still working on it right?

Yes, but last comment (here) waiting for some feedback about this comment and this comment.
Especially on my counter proposal (see comment).

So, I didn't move forward, having no additional interaction.

I've been away since your last comment, I'll get back now to work on this contribution to change technicals points but I always need a feedback about functional elements.

@Gaetanbrl Gaetanbrl force-pushed the g2f-restricted-area branch from 3925cf0 to abeeffc Compare April 28, 2025 13:08
@Gaetanbrl
Copy link
Copy Markdown
Contributor Author

@MV88 you answer "Is this done ?" about some remaining returns :

It would be really nice if you could give me some feedback on this

Thank you very much for these clarifications (@offtherailz).

@Gaetanbrl
Copy link
Copy Markdown
Contributor Author

Here is a summary of the covered items.

Changes Following Reviews

  • Fixed the maximum call stack error.
  • Applied restricted area logic only in EDIT mode.
  • Restricted area is never applied for ADMIN users.
  • Restricted area is applied only for USER and ANONYMOUS profiles.
  • Added option to enable synchronization between the attribute table and the map (see this comment).
  • Various technical feedback items addressed (@MV88).

Outstanding Points

  • Unit tests have not been implemented yet.
  • There is an issue when synchronization is enabled and then the user switches to edit mode — the CQL syntax becomes invalid

This last error seems related to srid and CQL syntax. Refer to the google group dev ticket for more details.

@tdipisa
Copy link
Copy Markdown
Member

tdipisa commented Jan 20, 2026

@Gaetanbrl I'm sorry, thank you for this but this PR is quite old now. Do you have plans to re-handle this according to our review above and pending outstanding stuff? Otherwise, I would like to close the PR.

@Gaetanbrl
Copy link
Copy Markdown
Contributor Author

Same as #10524.

I can look into updating the PR, but I’m not certain it will still be sufficient...

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

4 participants