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

Dashboards: Fix 'Make Editable' button not working in Dashboard Settings #60306

Merged
merged 2 commits into from Dec 14, 2022

Conversation

joshhunt
Copy link
Contributor

What is this feature?

PR #52682 introduced a regression where the "Make Editable" button in Dashboard Settings for uneditable dashboards doesn't do anything.

This fixes that.

Why do we need this feature?

So people can edit their dashboards.

Who is this feature for?

Admins and editors of Grafana

Which issue(s) does this PR fix?:

Fixes #60305

@joshhunt joshhunt added this to the 9.3.3 milestone Dec 14, 2022
@joshhunt joshhunt requested review from torkelo, a team, dprokop and kaydelaney and removed request for a team December 14, 2022 11:59
dashboard.meta.canMakeEditable = false;
dashboard.meta.canEdit = true;
dashboard.meta.canSave = true;
// TODO add some kind of reload
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having a TODO here was so handy to know where to fix it! 😉

Comment on lines +37 to +45
const [updateId, setUpdateId] = useState(0);
useEffect(() => {
dashboard.events.subscribe(DashboardMetaChangedEvent, () => setUpdateId((v) => v + 1));
}, [dashboard]);

// updateId in deps so we can revaluate when dashboard is mutated
// eslint-disable-next-line react-hooks/exhaustive-deps
const pages = useMemo(() => getSettingsPages(dashboard), [dashboard, updateId]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not super in love with this change, but I felt like it was the most direct and easiest to understand of the possible solutions.

dashboard.meta does get a new identity when it changes, so we could stick that in the useMemo props, but it would still fail exhaustive props, unless we pass that in seperately into getSettingsPages?

Definitely open to feedback on this!

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

was missing function call in the onClick callback, but otherwise looks good

@joshhunt
Copy link
Contributor Author

ironic 😭
image

@joshhunt joshhunt modified the milestones: 9.3.3, 9.3.2 Dec 14, 2022
@joshhunt joshhunt merged commit 2586964 into main Dec 14, 2022
@joshhunt joshhunt deleted the joshhunt/fix-make-editable branch December 14, 2022 14:24
grafanabot pushed a commit that referenced this pull request Dec 14, 2022
…ngs (#60306)

* Dashboards: Fix 'Make Editable' button not working in Dashboard Settings

* comment

(cherry picked from commit 2586964)
joshhunt added a commit that referenced this pull request Dec 14, 2022
…ard Settings (#60330)

Dashboards: Fix 'Make Editable' button not working in Dashboard Settings (#60306)

* Dashboards: Fix 'Make Editable' button not working in Dashboard Settings

* comment

(cherry picked from commit 2586964)

Co-authored-by: Josh Hunt <joshhunt@users.noreply.github.com>
@Hipska
Copy link
Contributor

Hipska commented Dec 16, 2022

Actually, it worked. As in you could go back and the dashboard was editable. There was only no visible user feedback indicating clicking the button actually worked..

@joshhunt
Copy link
Contributor Author

Yup. The state was being updated, but because we mutate the dashboard model, we need to force a re-render (or navigate away and back again) to see the changes.

GuYounes pushed a commit to paul-wurth/BIXpert that referenced this pull request Feb 8, 2023
…ard Settings (grafana#60330)

Dashboards: Fix 'Make Editable' button not working in Dashboard Settings (grafana#60306)

* Dashboards: Fix 'Make Editable' button not working in Dashboard Settings

* comment

(cherry picked from commit 2586964)

Co-authored-by: Josh Hunt <joshhunt@users.noreply.github.com>
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.

Dashboards: 'Make Editable' in uneditable Dashboards Settings doesn't do anything
4 participants