-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add Deletion button to archived runs #3285
Conversation
/assign @Bobgy |
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.
/lgtm
Only a few minor issues. You can fix them and merge
frontend/src/pages/Archive.test.tsx
Outdated
@@ -60,17 +67,29 @@ describe('Archive', () => { | |||
expect(updateBannerSpy).toHaveBeenCalledWith({}); | |||
}); | |||
|
|||
it('only enables restore button when at least one run is selected', () => { | |||
it('only enables restore and delete button when at least one run is selected', () => { |
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('only enables restore and delete button when at least one run is selected', () => { | |
it('enables restore and delete button when at least one run is selected', () => { |
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.
done
frontend/src/pages/Archive.test.tsx
Outdated
expect(deleteRunSpy).toHaveBeenCalledWith('id1'); | ||
expect(deleteRunSpy).toHaveBeenCalledWith('id2'); | ||
expect(deleteRunSpy).toHaveBeenCalledWith('id3'); | ||
console.log(tree.state('selectedIds')); |
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.
nit: forget to delete?
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.
done
frontend/src/pages/Archive.test.tsx
Outdated
it('deletes selected ids when Confirm is clicked', async () => { | ||
tree = shallow(<Archive {...generateProps()} />); | ||
tree.setState({ selectedIds: ['id1', 'id2', 'id3'] }); | ||
console.log(tree.state('selectedIds')); |
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.
nit: forget to delete?
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... done
FYI, your mentioned context in #3285 (comment) seems wrong. Could you double check? |
yes. changed to the bug #3283 |
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.
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Bobgy The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/test kubeflow-pipeline-sample-test |
* addd delete button to archived run page * unit test * comments * address comments * remove unintended console log in test
Context
#3283
video https://drive.google.com/file/d/1ruipA6DBIa9QERE9ur5ZGCvUj_Xctr_w/view
This change is