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

Dashboard: New panel in a dashboard is not deleted after "Discard"-ing changes in Panel Edit #66476

Merged
merged 7 commits into from
Apr 25, 2023

Conversation

polibb
Copy link
Contributor

@polibb polibb commented Apr 13, 2023

What is this feature?

Remove newly created panel from dashboard when it is discarded during editing it before it is ever saved to the dashboard.

Problem?

How do we distinguish between a panel that was just added to the dashboard and a panel that was always there?
In the first case when we hit "Discard" during editing we want it deleted from the dashboard model, but in the second case we want to only clean up the last changes on it.
We are always adding the panel to the dashboard immediately when creating it, before going to editing it: https://github.com/grafana/grafana/blob/main/public/app/features/dashboard/utils/dashboard.ts#L17

I have added a suggestion about where and which method we might want to use to remove the panel when we hit "Discard", but don't know how to make the distinction between new and existing panel.

Which issue(s) does this PR fix?:

Fixes #65588

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@@ -131,6 +132,10 @@ export class PanelEditorUnconnected extends PureComponent<Props> {
};

onDiscard = () => {
// if this is a newly created panel we want to remove it from the dashboard
// todo how do we check it's a new panel?
Copy link
Member

Choose a reason for hiding this comment

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

not sure you can right now, would have to add some new state to it, I know we have had a "isNew" state before but seems to be gone now.

Just make sure we don't save this new state prop / get's removed when you leave panel edit (without discarding)

@dprokop dprokop self-requested a review April 18, 2023 13:16
@polibb polibb marked this pull request as ready for review April 19, 2023 13:30
@polibb polibb requested a review from a team as a code owner April 19, 2023 13:30
@polibb polibb requested review from kaydelaney and removed request for a team April 19, 2023 13:30
} else {
dashboard && removePanel(dashboard, sourcePanel, true);
}
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is one way of handling the new panel: adding a prop to the model and looking for it when leaving the editing page (because Apply and going back would not remove the property from the model).

Another way would be to let the Panel Edit page know whether its panel is new (with an "isNew" URL param e.g.) and let it call removePanel() on discard. That way we wouldn't have to worry about Apply and "going back manually" because only the Add buttons would trigger the URL with isNew param.

@polibb polibb added add to changelog backport v9.5.x Bot will automatically open backport PR labels Apr 20, 2023
@polibb polibb added this to the 9.5.x milestone Apr 20, 2023
@grafanabot
Copy link
Contributor

Hello @polibb!
Backport pull requests need to be either:

  • Pull requests which address bugs,
  • Urgent fixes which need product approval, in order to get merged,
  • Docs changes.

Please, if the current pull request addresses a bug fix, label it with the type/bug label.
If it already has the product approval, please add the product-approved label. For docs changes, please add the type/docs label.
If the pull request modifies CI behaviour, please add the type/ci label.
If none of the above applies, please consider removing the backport label and target the next major/minor release.
Thanks!

Copy link
Member

@axelavargas axelavargas left a comment

Choose a reason for hiding this comment

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

Hey @polibb, fantastic job tackling this challenging issue! 🙌🏾 You've made great progress. I quickly tested it and noticed a small problem.

When editing a panel, if a user saves to the DB and then hits "discard," the changes are still discarded even though they're saved in the database.

DiscardAlsoRemoveChangesWhenTheyAreSavedInBackend.mp4

@polibb
Copy link
Contributor Author

polibb commented Apr 25, 2023

When editing a panel, if a user saves to the DB and then hits "discard," the changes are still discarded even though they're saved in the database.

@axelavargas nice catch! I tried it in play and am seeing this bug easily. However, this PR only solves bug related to a new panel discarding when the panel itself is not discarded as an entity.

The discarded changes on an existing panel is in a different area and will need another approach (regardless of the isNew prop we introduce here). Probably something with the store or updating the configRev is not updating properly after saved changes (here (?)).
I will open a separate bug for that in order to re-prioritise it later.

@polibb polibb merged commit fe23c76 into main Apr 25, 2023
23 checks passed
@polibb polibb deleted the polibb/discard-panel-remove branch April 25, 2023 14:19
grafanabot pushed a commit that referenced this pull request Apr 25, 2023
…g changes in Panel Edit (#66476)

* add isNew notPersistedProperty to PanelModel

* if panel is newly created and user "Discard"s it, the panel is removed entirely

* add Todo's for when we remove the emptyDashboardPage FF

* add isNew to new panel after file dropping on dashboard page

* handle the "Apply" case

* CSV file dropping is not relevant to a new panel bc it doesnt open edit page

(cherry picked from commit fe23c76)
@zerok zerok modified the milestones: 9.5.x, 10.0.0 May 3, 2023
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023
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.

The panel still added into the dashboard after discarding changes while editing
5 participants