-
Notifications
You must be signed in to change notification settings - Fork 12k
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
PublicDashboard: Add RTK Query with loading and error state. Add MSW dependency for testing. #55518
Conversation
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34308 |
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34426 |
...c/app/features/dashboard/components/ShareModal/SharePublicDashboard/PubDashConfiguration.tsx
Outdated
Show resolved
Hide resolved
...c/app/features/dashboard/components/ShareModal/SharePublicDashboard/SharePublicDashboard.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! The refactor is great and I love the new state functionality 🤩
I have tested it and it works as expected.
Left some nit comments.
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34462 |
Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/34495 |
publicDashboardEnabled: data.isEnabled, | ||
}); | ||
}, | ||
invalidatesTags: ['Config'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this means calling the saveConfig
endpoint invalidates the cached results for the getConfig
endpoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
exactly, because getConfig has 'Config' tag
<Checkbox | ||
label="Your entire dashboard will be public" | ||
value={acknowledgements.public} | ||
data-testid={selectors.WillBePublicCheckbox} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍 I like how were keeping all our selectors separate
let mockDashboard: DashboardModel; | ||
let mockPanel: PanelModel; | ||
|
||
beforeAll(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was everything in here needed to be able to render ShareModal
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's right
import { ShareModal } from '../ShareModal'; | ||
|
||
const server = setupServer( | ||
rest.get('/api/dashboards/uid/:uId/public-config', (req, res, ctx) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this mocked api specifically for RTK? Or would it still work if we're not using RTK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would still work without using RTK Query. MSW is a library which intercepts the request in the network layer, so it's agnostic about how you make that request.
However, it's recommended to use this library for RTK Query and I couldn't find another way of mocking the requests
}), | ||
}); | ||
|
||
export const { useGetConfigQuery, useSaveConfigMutation } = publicDashboardApi; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we simplify the names to useGetConfig
and useSaveConfig
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this is something that is added automatically by RTK Query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! I really like how you wrote your tests. I definitely learned a few things from reading through them. Tested it out and had no issues. Left a few comments, mostly just trying to get clarification for myself. Awesome work 🥳
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes: https://github.com/grafana/grafana-enterprise/issues/4457
Special notes for your reviewer:
Fetch Loading state:
Save Loading state: