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

#9739 & #9760: Fix - Dependency management of widgets #9803

Merged
merged 6 commits into from
Jan 15, 2024

Conversation

dsuren1
Copy link
Contributor

@dsuren1 dsuren1 commented Dec 7, 2023

Description

This PR attempts to fix the issue with the dependency management of widgets

Please check if the PR fulfills these requirements

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

  • Bugfix

Issue

What is the current behavior?

What is the new behavior?

  • The dependencies are correctly formulated and enforce the original requirement of dependency of the widget
  • Fixed counter widget doesn't set fallback value when the value is null/undefined
  • Fixed Layer/Map multi-selection doesn't work on Mac

Test data

/dashboard/47595
/dashboard/47592
/dashboard/47714

Screenshot

image

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

@dsuren1 dsuren1 added this to the 2024.01.00 milestone Dec 7, 2023
@dsuren1 dsuren1 requested a review from MV88 December 7, 2023 13:05
@dsuren1 dsuren1 self-assigned this Dec 7, 2023
@dsuren1 dsuren1 linked an issue Dec 7, 2023 that may be closed by this pull request
1 task
@tdipisa tdipisa requested review from offtherailz and removed request for MV88 December 7, 2023 13:31
@dsuren1 dsuren1 linked an issue Dec 7, 2023 that may be closed by this pull request
1 task
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.

The fix works, anyway the code have to be a little reorganized and cleaned up, or it is diffcoult to read and maintain.
Please follow my suggestions @dsuren1 .

I'm asking this for master only I think.

web/client/plugins/Dashboard.jsx Outdated Show resolved Hide resolved
web/client/utils/WidgetsUtils.js Outdated Show resolved Hide resolved
web/client/selectors/widgets.js Outdated Show resolved Hide resolved
web/client/utils/WidgetsUtils.js Outdated Show resolved Hide resolved
web/client/plugins/widgetbuilder/ChartBuilder.jsx Outdated Show resolved Hide resolved
@dsuren1
Copy link
Contributor Author

dsuren1 commented Dec 18, 2023

@offtherailz
In case of new chart widget (not saved yet), when connecting to a transitive depedent widget, the chart view of the current chart widget in wizard will not show updated view (i.e excludes filters from dependent widget) but upon save and back to edit it shows updated chart view and works fine from there on as the chart should have an id which dependenciesToWidget enhancer requires when generating dependenciesMap. Is it behavior okay?

As part of this PR, I have removed just the requirement of the presence of anid alone in the enhancer to allow all widget types to get updated when connecting to a transitive dependent widget (i.e before the widget is saved for the first time)

Example
Try creating a widget (chart/counter) and connect to the map of this dashboard (/dashboard/47592), observe the behavior with id and without id validation in dependenciesToWidget enhancer locally

@dsuren1 dsuren1 requested a review from offtherailz December 18, 2023 16:10
@tdipisa tdipisa assigned offtherailz and unassigned dsuren1 Jan 15, 2024
@offtherailz
Copy link
Member

@offtherailz In case of new chart widget (not saved yet), when connecting to a transitive depedent widget, the chart view of the current chart widget in wizard will not show updated view (i.e excludes filters from dependent widget) but upon save and back to edit it shows updated chart view and works fine from there on as the chart should have an id which dependenciesToWidget enhancer requires when generating dependenciesMap. Is it behavior okay?

As part of this PR, I have removed just the requirement of the presence of anid alone in the enhancer to allow all widget types to get updated when connecting to a transitive dependent widget (i.e before the widget is saved for the first time)

Example Try creating a widget (chart/counter) and connect to the map of this dashboard (/dashboard/47592), observe the behavior with id and without id validation in dependenciesToWidget enhancer locally

@dsuren1 it looks correct the way you did, from the user point of view. The ID of the widget should not be required to be dependent, so also new widget while editing first time can have a consistent preview..

@tdipisa tdipisa assigned dsuren1 and unassigned offtherailz Jan 15, 2024
@offtherailz offtherailz merged commit ac495f8 into geosolutions-it:master Jan 15, 2024
4 checks passed
@tdipisa
Copy link
Member

tdipisa commented Jan 15, 2024

@ElenaGallo please test in DEV and let us know. there are 2 issues connected to this PR and one of them is also for 2023.02.02

@ElenaGallo
Copy link
Contributor

Test passed, @dsuren1 please backport to 2023.02.xx. Thanks

@tdipisa
Copy link
Member

tdipisa commented Jan 15, 2024

@ElenaGallo probably nothing to be backported for this. I'm waiting confirmation from @dsuren1.

@dsuren1
Copy link
Contributor Author

dsuren1 commented Jan 16, 2024

@tdipisa You are right. This fix doesn't need to be backported, a fix without traces has already been merged to stable

@tdipisa
Copy link
Member

tdipisa commented Jan 16, 2024

@dsuren1 thank you for the confirmation.

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