-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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: Migration - Dashboard Settings Variables (List, Duplicate, Delete) #78917
Merged
Merged
Changes from all commits
Commits
Show all changes
28 commits
Select commit
Hold shift + click to select a range
bdc6235
First attempt: List variables using SceneVariablesSet and also variab…
axelavargas bbf200b
Merge remote-tracking branch 'origin' into axelav/settings-variables
axelavargas 16016d6
Merge remote-tracking branch 'origin' into axelav/settings-variables
axelavargas 129527d
List variables and create Scaffold for Change order, duplicate and de…
axelavargas 3fe0edf
Refactor: use right methods to get variables and implement onDelete
axelavargas cb1ce5a
Implement duplicate variables and clean up code
axelavargas 8d92f02
Modify duplicated function to keep the same format as core grafana
axelavargas 474e46b
Fix betterer and linter issues
axelavargas 640e818
Add confirmation delete modal
axelavargas 87c2297
Fix linter
axelavargas 9e19e38
More linter...
axelavargas 33d3040
Fix React removal
axelavargas 55d1822
Merge remote-tracking branch 'origin' into axelav/settings-variables
axelavargas 8b42494
Attempt to fix scenes types
axelavargas 0be281c
Fix typescript error, pass variableScene to the component
axelavargas 5db6c48
Disable create button and empty variable set
axelavargas aca9c65
Move methods to VariableEditView model
axelavargas f99ea12
Add unit test
axelavargas 3052d19
Merge remote-tracking branch 'origin' into axelav/settings-variables
axelavargas db3e7a9
Fix linter
axelavargas db8ed9d
Use getDashboardSceneFor method in all DashboardSettings
axelavargas dfea723
Remove dashboardRef param from Dashboard Scene Url Sync
axelavargas bcd2a52
Add missing changes
axelavargas dccd570
Apply suggestions from code review
axelavargas 3d835c2
Fix references of PR suggestion
axelavargas 811347a
Add unit test for default variable set
axelavargas 0c4f50d
Remove unnecesary function
axelavargas 7edc34a
Remove unnecesary get _dashboard function
axelavargas File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
148 changes: 148 additions & 0 deletions
148
public/app/features/dashboard-scene/settings/VariablesEditView.test.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,148 @@ | ||
import { SceneVariableSet, CustomVariable, SceneGridItem, SceneGridLayout } from '@grafana/scenes'; | ||
|
||
import { DashboardScene } from '../scene/DashboardScene'; | ||
import { activateFullSceneTree } from '../utils/test-utils'; | ||
|
||
import { VariablesEditView } from './VariablesEditView'; | ||
|
||
describe('VariablesEditView', () => { | ||
describe('Dashboard Variables state', () => { | ||
let dashboard: DashboardScene; | ||
let variableView: VariablesEditView; | ||
|
||
beforeEach(async () => { | ||
const result = await buildTestScene(); | ||
dashboard = result.dashboard; | ||
variableView = result.variableView; | ||
}); | ||
|
||
it('should return the correct urlKey', () => { | ||
expect(variableView.getUrlKey()).toBe('variables'); | ||
}); | ||
|
||
it('should return the dashboard', () => { | ||
expect(variableView.getDashboard()).toBe(dashboard); | ||
}); | ||
|
||
it('should return the list of variables', () => { | ||
const expectedVariables = [ | ||
{ | ||
type: 'custom', | ||
name: 'customVar', | ||
query: 'test, test2', | ||
value: 'test', | ||
}, | ||
{ | ||
type: 'custom', | ||
name: 'customVar2', | ||
query: 'test3, test4', | ||
value: 'test3', | ||
}, | ||
]; | ||
const variables = variableView.getVariables(); | ||
expect(variables).toHaveLength(2); | ||
expect(variables[0].state).toMatchObject(expectedVariables[0]); | ||
expect(variables[1].state).toMatchObject(expectedVariables[1]); | ||
}); | ||
}); | ||
|
||
describe('Dashboard Variables actions', () => { | ||
let variableView: VariablesEditView; | ||
|
||
beforeEach(async () => { | ||
const result = await buildTestScene(); | ||
variableView = result.variableView; | ||
}); | ||
|
||
it('should duplicate a variable', () => { | ||
const variables = variableView.getVariables(); | ||
const variable = variables[0]; | ||
variableView.onDuplicated(variable.state.name); | ||
expect(variableView.getVariables()).toHaveLength(3); | ||
expect(variableView.getVariables()[1].state.name).toBe('copy_of_customVar'); | ||
}); | ||
|
||
it('should handle name when duplicating a variable twice', () => { | ||
const variableIdentifier = 'customVar'; | ||
variableView.onDuplicated(variableIdentifier); | ||
variableView.onDuplicated(variableIdentifier); | ||
expect(variableView.getVariables()).toHaveLength(4); | ||
expect(variableView.getVariables()[1].state.name).toBe('copy_of_customVar_1'); | ||
expect(variableView.getVariables()[2].state.name).toBe('copy_of_customVar'); | ||
}); | ||
|
||
it('should delete a variable', () => { | ||
const variableIdentifier = 'customVar'; | ||
variableView.onDelete(variableIdentifier); | ||
expect(variableView.getVariables()).toHaveLength(1); | ||
expect(variableView.getVariables()[0].state.name).toBe('customVar2'); | ||
}); | ||
|
||
it('should change order of variables', () => { | ||
const fromIndex = 0; // customVar is first | ||
const toIndex = 1; | ||
variableView.onOrderChanged(fromIndex, toIndex); | ||
expect(variableView.getVariables()[0].state.name).toBe('customVar2'); | ||
expect(variableView.getVariables()[1].state.name).toBe('customVar'); | ||
}); | ||
|
||
it('should keep the same order of variables with invalid indexes', () => { | ||
const fromIndex = 0; | ||
const toIndex = 2; | ||
|
||
const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); | ||
|
||
variableView.onOrderChanged(fromIndex, toIndex); | ||
expect(errorSpy).toHaveBeenCalledTimes(1); | ||
expect(variableView.getVariables()[0].state.name).toBe('customVar'); | ||
expect(variableView.getVariables()[1].state.name).toBe('customVar2'); | ||
|
||
errorSpy.mockRestore(); | ||
}); | ||
}); | ||
}); | ||
|
||
async function buildTestScene() { | ||
const variableView = new VariablesEditView({}); | ||
const dashboard = new DashboardScene({ | ||
title: 'Dashboard with variables', | ||
uid: 'dash-variables', | ||
meta: { | ||
canEdit: true, | ||
}, | ||
$variables: new SceneVariableSet({ | ||
variables: [ | ||
new CustomVariable({ | ||
name: 'customVar', | ||
query: 'test, test2', | ||
}), | ||
new CustomVariable({ | ||
name: 'customVar2', | ||
query: 'test3, test4', | ||
}), | ||
], | ||
}), | ||
body: new SceneGridLayout({ | ||
children: [ | ||
new SceneGridItem({ | ||
key: 'griditem-1', | ||
x: 0, | ||
y: 0, | ||
width: 10, | ||
height: 12, | ||
body: undefined, | ||
}), | ||
], | ||
}), | ||
editview: variableView, | ||
}); | ||
|
||
activateFullSceneTree(dashboard); | ||
|
||
await new Promise((r) => setTimeout(r, 1)); | ||
|
||
dashboard.onEnterEditMode(); | ||
variableView.activate(); | ||
|
||
return { dashboard, variableView }; | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
If it is mandatory to get always a valid value in
variables
, probably it is a good idea to add a test for it. WDYT?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.
Good idea, I have added the test :)
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.
yeah, +1 to ivan's. Overall what we see is it's better to attach even empty objects rather than conditonally adding theme. It simplifies downstream code a lot.