Visibility limits - The resolution option is not retained as Limits type #10391 #10598

rowheat02 · 2024-10-04T13:44:30Z

Description

fixes #10391
Persist Visibility limit type after saving the map.

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/geosolutions-it/MapStore2/blob/master/CONTRIBUTING.md
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

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

#10391

What is the current behavior?

#10391
Visibility limit type reset back to Scale even selecting Resolution

What is the new behavior?

Visibility Limit type persist even after saving map

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 · 2024-10-07T07:40:22Z

@rowheat02 Thank you for raising this. Can we close #10391 in favor of this?

rowheat02 · 2024-10-07T07:46:13Z

@tdipisa Yes, old PR #10391 can be closed as this PR is solving #10355

offtherailz

Missing unit test.
Does it make sense to save it in the map too? Actually this solution retains it only in local map, but when save or export, this value is not maintained. @tdipisa should we add also this?

offtherailz · 2024-10-07T15:46:11Z

web/client/utils/LayersUtils.js

@@ -707,7 +707,8 @@ export const saveLayer = (layer) => {
        legendOptions: layer.legendOptions,
        tileSize: layer.tileSize,
        version: layer.version,
-        expanded: layer.expanded || false
+        expanded: layer.expanded || false,
+        ...(layer.visibilityLimitType ? { ...layer.visibilityLimitType, ...layer.visibilityLimitType } : {})


Why an object to spread?
I think it is ok to make it undefined.

offtherailz · 2024-10-08T14:28:07Z

web/client/utils/LayersUtils.js

@@ -707,7 +707,8 @@ export const saveLayer = (layer) => {
        legendOptions: layer.legendOptions,
        tileSize: layer.tileSize,
        version: layer.version,
-        expanded: layer.expanded || false
+        expanded: layer.expanded || false,
+        ...(layer.visibilityLimitType ? { visibilityLimitType: layer.visibilityLimitType } : undefined)


Here I meant, why not ?

Suggested change

...(layer.visibilityLimitType ? { visibilityLimitType: layer.visibilityLimitType } : undefined)

visibilityLimitType: layer.visibilityLimitType

Tests were failing because the functionality checks the data that is effectively saved to be the one you want.
You can keep this implementation, but add a test that if visibilityLimitType is passed, then is persisted, otherwise it is not (it is a temporary variable, so it makes sense to have it.

if we don't need to persist visibilityLimitType then this is unnecessary. So removing it. @offtherailz

offtherailz · 2024-10-08T14:42:22Z

@rowheat02 replying to your doubt:

I did not write new tests since there were already tests which were handling defaultLimitType . https://github.com/geosolutions-it/MapStore2/blob/master/web/client/components/TOC/fragments/settings/__tests__/VisibilityLimitsForm-test.jsx#L51, I only fixed in implementation by passing defaultLimitType while using Visibilitylimitform.

You can test on onChange , that has been added.

Here we had a misunderstanding, I clarified my question. Visibility limits - The resolution option is not retained as Limits type #10391 #10598 (comment)
I clarified with Tobia that is not necessary to persist this setting.

rowheat02 · 2024-10-09T09:35:16Z

@offtherailz

removed part to persist visibilityLimitType after saving map
unit test to check onChange added

offtherailz · 2024-10-11T15:57:04Z

@ElenaGallo, could you please test this on DEV ? Thank you

ElenaGallo · 2024-10-14T10:22:55Z

@rowheat02 hte Limits type set to Resolution is still not saved correctly. See the video attached:

resolution.mp4

rowheat02 · 2024-10-14T10:32:13Z

@rowheat02 replying to your doubt:

I did not write new tests since there were already tests which were handling defaultLimitType . https://github.com/geosolutions-it/MapStore2/blob/master/web/client/components/TOC/fragments/settings/__tests__/VisibilityLimitsForm-test.jsx#L51, I only fixed in implementation by passing defaultLimitType while using Visibilitylimitform.

You can test on onChange , that has been added.

Here we had a misunderstanding, I clarified my question. Visibility limits - The resolution option is not retained as Limits type #10391 #10598 (comment)

I clarified with Tobia that is not necessary to persist this setting.

@ElenaGallo this setting(Resolution) remains the same only before saving the map. It's not saved on backend while saving the map, we avoided that. Agreed by @tdipisa, @offtherailz

ElenaGallo · 2024-10-14T10:42:40Z

Thanks @rowheat02, please backport to 2024.02.xx.

* Update Changelog for version 2024.02.00 (geosolutions-it#10602) Co-authored-by: github-actions <[email protected]> * Bump commons-io:commons-io from 2.7 to 2.14.0 (geosolutions-it#10593) * Removed react-confirm-button unused dependency (geosolutions-it#10495) * Update release_steps.md (geosolutions-it#10568) * Remove istambul loader (geosolutions-it#10491) * Attempt to remove instambul loader * removed also package dependency * Apply suggestions from code review clean * Remove jsonpath (geosolutions-it#10494) jsonpath is not needed. It is a dependency of geosolutions-it/patcher actually. Having this dependency here is only confusing for dependency analyisis * Fix geosolutions-it#10595 add missing 'FORMAT' parameter to WMTS GetFeatureInfo requests (geosolutions-it#10596) * add missing 'FORMAT' parameter to WMTS GetFeatureInfo requests this is required by the WMTS spec to be the same format as would be used for a GetTile request, and this allows GFI requests to succeed on https://data.geopf.fr/wmts. * fix mapinfo wmts utils test * Fix geosolutions-it#10505 Allow to specify use of proxy or cors at layer level (geosolutions-it#10526) * fix: remove ui element for force proxy and Allow not secure layers * fix: ajax logic changed, autoDetectCORS is set to true by default * new central CORS util file created and used in ajax * checking CORS before adding in common layer file * null check on getProxyUrl * updated individual layer considring to use proxy if needed * avoid proxy cache to update if response is not okey * enable user to add http url, show warning instead of error, warning text updated * test cases updated * fix: resolve conflicts with url check * fixed the failed test * review cesium layers * include add method in model layer * improve http check for openlayers wms layer * fix tests --------- Co-authored-by: allyoucanmap <[email protected]> * Update openId.md (geosolutions-it#10610) * Bump spring-security version to 5.7.12 (fixes geosolutions-it#10611) (geosolutions-it#10612) somehow something in the build already drags this version, and we end up with two conflicting versions of spring-security in the war which results in at least failure to authenticate with basic auth. * Visibility limits - The resolution option is not retained as Limits type geosolutions-it#10391 (geosolutions-it#10598) * Add lib check release step (geosolutions-it#10614) * geosolutions-it#4675 Remove unused code (geosolutions-it#10442) * geosolutions-it#4675 Remove unused code * remove additional code from review * fix tests folder * remove additional unused files * geosolutions-it#10545: Option to disable identify popup in case of no results (geosolutions-it#10557) * geosolutions-it#10545: Option to disable identify popup in case of no results Description: - handle adding an option called 'hidePopupIfNoResults' to hide the identify popup - add unit tests - add jsdoc * geosolutions-it#10545: Option to disable identify popup in case of no results Description: - revert changes in popupSupport files for ol and leaflet + related tests files - handle hide popup for map viewer with css for openlayers and leaflet approach - add unit tests - edit map-popup.less file to handle hide the popup * geosolutions-it#10545: revert unnecessary changes * Fix geosolutions-it#10615 removed eval from marker utils (geosolutions-it#10616) * geosolutions-it#10545: remove marker in case no results + hover identify mode active and hideEmptyPopupOption with true (geosolutions-it#10619) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Lorenzo Natali <[email protected]> Co-authored-by: Landry Breuil <[email protected]> Co-authored-by: RowHeat <[email protected]> Co-authored-by: allyoucanmap <[email protected]> Co-authored-by: mahmoud adel <[email protected]> Co-authored-by: Matteo V. <[email protected]>

…ype #10391 (#10598)

…ype geosolutions-it#10391 (geosolutions-it#10598)

…ype #10391 (#10598) (#10647)

fix: persist visibility limit type

14eef05

rowheat02 changed the title ~~fix: persist visibility limit type~~ Visibility limits - The resolution option is not retained as Limits type #10391 Oct 4, 2024

fix: changed visibilityLimitType assignment while saving map

635709b

rowheat02 self-assigned this Oct 7, 2024

rowheat02 added Good first issue BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch bug Internal labels Oct 7, 2024

rowheat02 added this to the 2024.02.01 milestone Oct 7, 2024

rowheat02 linked an issue Oct 7, 2024 that may be closed by this pull request

Visibility limits - The revolution option is not retained as Limits type #10355

Closed

1 task

rowheat02 marked this pull request as ready for review October 7, 2024 05:54

rowheat02 requested a review from offtherailz October 7, 2024 05:54

tdipisa mentioned this pull request Oct 7, 2024

Visibility limits - The resolution option is not retained as Limits type #10391

Closed

4 tasks

offtherailz reviewed Oct 7, 2024

View reviewed changes

fix: updated spread operator

243b80a

offtherailz reviewed Oct 8, 2024

View reviewed changes

rowheat02 added 2 commits October 9, 2024 15:15

fix: remove code to persist visibilityLimitType after saving map

e792b4e

fix: test added

6759407

rowheat02 requested a review from offtherailz October 9, 2024 09:35

offtherailz approved these changes Oct 11, 2024

View reviewed changes

offtherailz merged commit 1e3365b into geosolutions-it:master Oct 11, 2024
6 checks passed

offtherailz pushed a commit that referenced this pull request Oct 18, 2024

Visibility limits - The resolution option is not retained as Limits t…

b62d599

…ype #10391 (#10598)

rowheat02 added a commit to rowheat02/MapStore2 that referenced this pull request Nov 5, 2024

Visibility limits - The resolution option is not retained as Limits t…

6f772e8

…ype geosolutions-it#10391 (geosolutions-it#10598)

rowheat02 mentioned this pull request Nov 5, 2024

[Backport 2024.02.xx]- #10355: Visibility limits - The resolution option is not retained as Limits type #10391 #10647

Merged

tdipisa pushed a commit that referenced this pull request Nov 5, 2024

Visibility limits - The resolution option is not retained as Limits t…

f91c4b3

…ype #10391 (#10598) (#10647)

tdipisa removed the BackportNeeded Commits provided for an issue need to be backported to the milestone's stable branch label Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visibility limits - The resolution option is not retained as Limits type #10391 #10598

Visibility limits - The resolution option is not retained as Limits type #10391 #10598

rowheat02 commented Oct 4, 2024 •

edited

Loading

tdipisa commented Oct 7, 2024

rowheat02 commented Oct 7, 2024

offtherailz left a comment

offtherailz Oct 7, 2024

offtherailz Oct 8, 2024

offtherailz Oct 8, 2024

rowheat02 Oct 9, 2024 •

edited

Loading

offtherailz commented Oct 8, 2024 •

edited

Loading

rowheat02 commented Oct 9, 2024

offtherailz commented Oct 11, 2024

ElenaGallo commented Oct 14, 2024

rowheat02 commented Oct 14, 2024

ElenaGallo commented Oct 14, 2024

	...(layer.visibilityLimitType ? { visibilityLimitType: layer.visibilityLimitType } : undefined)
	visibilityLimitType: layer.visibilityLimitType

Visibility limits - The resolution option is not retained as Limits type #10391 #10598

Visibility limits - The resolution option is not retained as Limits type #10391 #10598

Conversation

rowheat02 commented Oct 4, 2024 • edited Loading

Description

Breaking change

Other useful information

tdipisa commented Oct 7, 2024

rowheat02 commented Oct 7, 2024

offtherailz left a comment

Choose a reason for hiding this comment

offtherailz Oct 7, 2024

Choose a reason for hiding this comment

offtherailz Oct 8, 2024

Choose a reason for hiding this comment

offtherailz Oct 8, 2024

Choose a reason for hiding this comment

rowheat02 Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

offtherailz commented Oct 8, 2024 • edited Loading

rowheat02 commented Oct 9, 2024

offtherailz commented Oct 11, 2024

ElenaGallo commented Oct 14, 2024

rowheat02 commented Oct 14, 2024

ElenaGallo commented Oct 14, 2024

rowheat02 commented Oct 4, 2024 •

edited

Loading

rowheat02 Oct 9, 2024 •

edited

Loading

offtherailz commented Oct 8, 2024 •

edited

Loading