-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
feat(ui): prefill parameters for workflow submit form. Fixes #12124 #13766
base: main
Are you sure you want to change the base?
Changes from all commits
fc1c45d
e8ec0aa
46c3503
c254b7f
2340b21
d34ee68
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
import {createBrowserHistory} from 'history'; | ||
import {getWorkflowParametersFromQuery} from './get_workflow_params'; | ||
|
||
describe('get_workflow_params', () => { | ||
it('should return an empty object when there are no query parameters', () => { | ||
const history = createBrowserHistory(); | ||
const result = getWorkflowParametersFromQuery(history); | ||
expect(result).toEqual({}); | ||
}); | ||
|
||
it('should return the parameters provided in the URL', () => { | ||
const history = createBrowserHistory(); | ||
history.location.search = '?parameters[key1]=value1¶meters[key2]=value2'; | ||
const result = getWorkflowParametersFromQuery(history); | ||
expect(result).toEqual({ | ||
key1: 'value1', | ||
key2: 'value2' | ||
}); | ||
}); | ||
|
||
it('should not return any key value pairs which are not in parameters query ', () => { | ||
const history = createBrowserHistory(); | ||
history.location.search = '?retryparameters[key1]=value1&retryparameters[key2]=value2'; | ||
const result = getWorkflowParametersFromQuery(history); | ||
expect(result).toEqual({}); | ||
}); | ||
|
||
it('should only return the parameters provided in the URL', () => { | ||
const history = createBrowserHistory(); | ||
history.location.search = '?parameters[key1]=value1¶meters[key2]=value2&test=123'; | ||
const result = getWorkflowParametersFromQuery(history); | ||
expect(result).toEqual({ | ||
key1: 'value1', | ||
key2: 'value2' | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
import {History} from 'history'; | ||
|
||
function extractKey(inputString: string): string | null { | ||
// Use regular expression to match the key within square brackets | ||
const match = inputString.match(/^parameters\[(.*?)\]$/); | ||
|
||
// If a match is found, return the captured key | ||
if (match) { | ||
return match[1]; | ||
} | ||
|
||
// If no match is found, return null or an empty string | ||
return null; // Or return ''; | ||
} | ||
/** | ||
* Returns the workflow parameters from the query parameters. | ||
*/ | ||
export function getWorkflowParametersFromQuery(history: History): {[key: string]: string} { | ||
const queryParams = new URLSearchParams(history.location.search); | ||
|
||
const parameters: {[key: string]: string} = {}; | ||
for (const [key, value] of queryParams.entries()) { | ||
const q = extractKey(key); | ||
if (q) { | ||
parameters[q] = value; | ||
} | ||
} | ||
|
||
return parameters; | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -136,7 +136,7 @@ export function WorkflowsList({match, location, history}: RouteComponentProps<an | |||
} | ||||
storage.setItem('options', options, {} as WorkflowListRenderOptions); | ||||
|
||||
const params = new URLSearchParams(); | ||||
const params = new URLSearchParams(history.location.search); | ||||
phases?.forEach(phase => params.append('phase', phase)); | ||||
labels?.forEach(label => params.append('label', label)); | ||||
if (pagination.offset) { | ||||
|
@@ -347,10 +347,27 @@ export function WorkflowsList({match, location, history}: RouteComponentProps<an | |||
)} | ||||
</div> | ||||
</div> | ||||
<SlidingPanel isShown={!!getSidePanel()} onClose={() => navigation.goto('.', {sidePanel: null})}> | ||||
<SlidingPanel | ||||
isShown={!!getSidePanel()} | ||||
onClose={() => { | ||||
// Remove any lingering query params | ||||
const qParams: {[key: string]: string | null} = { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is duplicate can be removed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain why this is necessary? The If there hasn't been a path change, I think it's fine to leave these query parameters in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So the workflow this would fix is
In this case, if the parameters are preserved, we would open the prefilled page. Removing the parameters will ensure the developer can start the page fresh without pre filled data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, thanks. IMO, it'd be better to remove this, because if a developer wants to start fresh, they can click on the "Workflows" icon on the left bar (or navigate to another page and back again) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I tried that workflow and the developer would need to go back to the workflows tab and re-trigger a workflow. Most likely the developer who lands on this page through query parameters would try to Submit it. Removing the extra added query parameters in this PR, will make the developer experience much better as they can recover quickly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, that makes sense. I checked and this is the only page using a argo-workflows/ui/src/app/cluster-workflow-templates/cluster-workflow-template-details.tsx Line 145 in e09d2f1
I'm not sure why this page is special in that respect, but it could be a developer experience thing |
||||
sidePanel: null | ||||
}; | ||||
// Remove any lingering query params | ||||
for (const key of queryParams.keys()) { | ||||
qParams[key] = null; | ||||
} | ||||
// Add back the pagination and namespace params. | ||||
qParams.limit = pagination.limit.toString(); | ||||
qParams.offset = pagination.offset || null; | ||||
qParams.namespace = namespace; | ||||
navigation.goto('.', qParams); | ||||
}}> | ||||
{getSidePanel() === 'submit-new-workflow' && ( | ||||
<WorkflowCreator | ||||
namespace={nsUtils.getNamespaceWithDefault(namespace)} | ||||
history={history} | ||||
onCreate={wf => navigation.goto(uiUrl(`workflows/${wf.metadata.namespace}/${wf.metadata.name}`))} | ||||
/> | ||||
)} | ||||
|
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.
I found one side-effect of this change: if you visit http://localhost:8080/workflows?namespace=argo and refresh the page, it'll append
&namespace=argo
each time you refresh. That's happening becausehistoryUrl()
isn't deduplicating query parameters, which it should IMO. Here's a commit that fixes that: MasonM@8b48d5bI haven't extensively tested that, so it's possible there's other side-effects