-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
KV 2 Toolbar delete redesign #11530
KV 2 Toolbar delete redesign #11530
Changes from all commits
0cd6d3f
9b02bd1
a94fefb
5733a64
f613b28
73ef1a1
d66f584
10f5064
bfdc688
86099cc
772b22e
eef8d75
28d6617
7ac7d6a
381c9ca
a37eadc
d470b17
e03c656
3dbe4ba
307eac0
f0cb88d
02f91ea
dd13899
012adea
62c149a
e568983
471d6ec
e9e339b
f605fcd
0c912f8
c365da4
49af0ef
1a9c91b
7f20aca
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,3 @@ | ||
```release-note:improvement | ||
ui: Redesign of KV 2 Delete toolbar. | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,14 +70,27 @@ export default ApplicationAdapter.extend({ | |
|
||
v2DeleteOperation(store, id, deleteType = 'delete') { | ||
let [backend, path, version] = JSON.parse(id); | ||
|
||
// deleteType should be 'delete', 'destroy', 'undelete' | ||
return this.ajax(this._url(backend, path, deleteType), 'POST', { data: { versions: [version] } }).then( | ||
() => { | ||
let model = store.peekRecord('secret-v2-version', id); | ||
return model && model.rollbackAttributes() && model.reload(); | ||
} | ||
); | ||
// deleteType should be 'delete', 'destroy', 'undelete', 'delete-latest-version', 'destroy-version' | ||
if ((!version && deleteType === 'delete') || deleteType === 'delete-latest-version') { | ||
return this.ajax(this._url(backend, path, 'data'), 'DELETE') | ||
.then(() => { | ||
let model = store.peekRecord('secret-v2-version', id); | ||
return model && model.rollbackAttributes() && model.reload(); | ||
}) | ||
.catch(e => { | ||
return e; | ||
}); | ||
} else { | ||
return this.ajax(this._url(backend, path, deleteType), 'POST', { data: { versions: [version] } }) | ||
.then(() => { | ||
let model = store.peekRecord('secret-v2-version', id); | ||
// potential that model.reload() is never called. | ||
return model && model.rollbackAttributes() && model.reload(); | ||
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. I saw these came from the previous iteration of this, but these lines look a little strange to me, I'm not totally sure but is return model && model.rollbackAttributes() && model.reload(); If model is falsey (in this case undefined) just return undefined, else move on to: If the result of model.rollbackAttributes() is falsey (looking at the code rollbackAttributes always returns undefined) then return its result (undefined) else move on to: Return the result of model.reload which is the model wrapped in a promise, but if rollbackAttributes always returns undefined then we never get to this point? As I said I'm not totally sure, and it would be weird that this method always returns undefined wrapped in a promise, maybe its not being used where ever it gets picked up? I just ran this in web console: ({}) && (function(){})() && console.log('hi'); Which never logs 'hi' 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. Investigated and John is potentially right. However, in order to not cause an unknown bug, I added a comment here instead of a complete removal. |
||
}) | ||
.catch(e => { | ||
return e; | ||
}); | ||
} | ||
}, | ||
|
||
handleResponse(status, headers, payload, requestData) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,148 @@ | ||
import Ember from 'ember'; | ||
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. moved all deletion actions into one glimmerized component. |
||
import { inject as service } from '@ember/service'; | ||
import Component from '@glimmer/component'; | ||
import { tracked } from '@glimmer/tracking'; | ||
import { action } from '@ember/object'; | ||
import { alias } from '@ember/object/computed'; | ||
import { maybeQueryRecord } from 'vault/macros/maybe-query-record'; | ||
|
||
const getErrorMessage = errors => { | ||
let errorMessage = errors?.join('. ') || 'Something went wrong. Check the Vault logs for more information.'; | ||
return errorMessage; | ||
}; | ||
export default class SecretDeleteMenu extends Component { | ||
@service store; | ||
@service router; | ||
@service flashMessages; | ||
|
||
@tracked showDeleteModal = false; | ||
|
||
@maybeQueryRecord( | ||
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. TIL that if you have a macro (even a custom one like maybeQueryRecord, you just @'it and then put the name below it. This took me a while to figure out, so I thought it worth pointing out. |
||
'capabilities', | ||
context => { | ||
if (!context.args.model) { | ||
return; | ||
} | ||
let backend = context.args.model.backend; | ||
let id = context.args.model.id; | ||
let path = context.args.isV2 | ||
? `${encodeURIComponent(backend)}/data/${encodeURIComponent(id)}` | ||
: `${encodeURIComponent(backend)}/${encodeURIComponent(id)}`; | ||
return { | ||
id: path, | ||
}; | ||
}, | ||
'isV2', | ||
'model', | ||
'model.id', | ||
'mode' | ||
) | ||
updatePath; | ||
@alias('updatePath.canDelete') canDelete; | ||
@alias('updatePath.canUpdate') canUpdate; | ||
|
||
@maybeQueryRecord( | ||
'capabilities', | ||
context => { | ||
if (!context.args || !context.args.modelForData || !context.args.modelForData.id) return; | ||
let [backend, id] = JSON.parse(context.args.modelForData.id); | ||
return { | ||
id: `${encodeURIComponent(backend)}/delete/${encodeURIComponent(id)}`, | ||
}; | ||
}, | ||
'model.id' | ||
) | ||
deleteVersionPath; | ||
@alias('deleteVersionPath.canUpdate') canDeleteAnyVersion; | ||
|
||
@maybeQueryRecord( | ||
'capabilities', | ||
context => { | ||
if (!context.args || !context.args.modelForData || !context.args.modelForData.id) return; | ||
let [backend, id] = JSON.parse(context.args.modelForData.id); | ||
return { | ||
id: `${encodeURIComponent(backend)}/undelete/${encodeURIComponent(id)}`, | ||
}; | ||
}, | ||
'model.id' | ||
) | ||
undeleteVersionPath; | ||
@alias('undeleteVersionPath.canUpdate') canUndeleteVersion; | ||
|
||
@maybeQueryRecord( | ||
'capabilities', | ||
context => { | ||
if (!context.args || !context.args.modelForData || !context.args.modelForData.id) return; | ||
let [backend, id] = JSON.parse(context.args.modelForData.id); | ||
return { | ||
id: `${encodeURIComponent(backend)}/destroy/${encodeURIComponent(id)}`, | ||
}; | ||
}, | ||
'model.id' | ||
) | ||
destroyVersionPath; | ||
@alias('destroyVersionPath.canUpdate') canDestroyVersion; | ||
|
||
@maybeQueryRecord( | ||
'capabilities', | ||
context => { | ||
if (!context.args.model || !context.args.model.engine || !context.args.model.id) return; | ||
let backend = context.args.model.engine.id; | ||
let id = context.args.model.id; | ||
return { | ||
id: `${encodeURIComponent(backend)}/metadata/${encodeURIComponent(id)}`, | ||
}; | ||
}, | ||
'model', | ||
'model.id', | ||
'mode' | ||
) | ||
v2UpdatePath; | ||
@alias('v2UpdatePath.canDelete') canDestroyAllVersions; | ||
|
||
get isLatestVersion() { | ||
let { model } = this.args; | ||
if (!model) return false; | ||
let latestVersion = model.currentVersion; | ||
let selectedVersion = model.selectedVersion.version; | ||
if (latestVersion !== selectedVersion) { | ||
return false; | ||
} | ||
return true; | ||
} | ||
|
||
@action | ||
handleDelete(deleteType) { | ||
Monkeychip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// deleteType should be 'delete', 'destroy', 'undelete', 'delete-latest-version', 'destroy-all-versions' | ||
if (!deleteType) { | ||
return; | ||
} | ||
if (deleteType === 'destroy-all-versions' || deleteType === 'v1') { | ||
let { id } = this.args.model; | ||
this.args.model.destroyRecord().then(() => { | ||
this.args.navToNearestAncestor.perform(id); | ||
}); | ||
} else { | ||
return this.store | ||
.adapterFor('secret-v2-version') | ||
.v2DeleteOperation(this.store, this.args.modelForData.id, deleteType) | ||
.then(resp => { | ||
if (Ember.testing) { | ||
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. Ah so I'm guessing
catch((e) => {
return e
}) ...in the adapter? 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. correct. We only get a resp if it's an adapter error. |
||
return; | ||
} | ||
if (!resp) { | ||
this.showDeleteModal = false; | ||
this.args.refresh(); | ||
return; | ||
} | ||
if (resp.isAdapterError) { | ||
const errorMessage = getErrorMessage(resp.errors); | ||
this.flashMessages.danger(errorMessage); | ||
} else { | ||
// not likely to ever get to this situation, but adding just in case. | ||
location.reload(); | ||
Monkeychip marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,60 +1,5 @@ | ||
import { maybeQueryRecord } from 'vault/macros/maybe-query-record'; | ||
import Component from '@ember/component'; | ||
import { inject as service } from '@ember/service'; | ||
import { alias, or } from '@ember/object/computed'; | ||
import Component from '@glimmer/component'; | ||
|
||
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. secret-version-menu used to house all the deletion actions, now it only handles version things. |
||
export default Component.extend({ | ||
tagName: '', | ||
store: service(), | ||
version: null, | ||
useDefaultTrigger: false, | ||
onRefresh() {}, | ||
|
||
deleteVersionPath: maybeQueryRecord( | ||
'capabilities', | ||
context => { | ||
let [backend, id] = JSON.parse(context.version.id); | ||
return { | ||
id: `${backend}/delete/${id}`, | ||
}; | ||
}, | ||
'version.id' | ||
), | ||
canDeleteVersion: alias('deleteVersionPath.canUpdate'), | ||
destroyVersionPath: maybeQueryRecord( | ||
'capabilities', | ||
context => { | ||
let [backend, id] = JSON.parse(context.version.id); | ||
return { | ||
id: `${backend}/destroy/${id}`, | ||
}; | ||
}, | ||
'version.id' | ||
), | ||
canDestroyVersion: alias('destroyVersionPath.canUpdate'), | ||
undeleteVersionPath: maybeQueryRecord( | ||
'capabilities', | ||
context => { | ||
let [backend, id] = JSON.parse(context.version.id); | ||
return { | ||
id: `${backend}/undelete/${id}`, | ||
}; | ||
}, | ||
'version.id' | ||
), | ||
canUndeleteVersion: alias('undeleteVersionPath.canUpdate'), | ||
|
||
isFetchingVersionCapabilities: or( | ||
'deleteVersionPath.isPending', | ||
'destroyVersionPath.isPending', | ||
'undeleteVersionPath.isPending' | ||
), | ||
actions: { | ||
deleteVersion(deleteType = 'destroy') { | ||
return this.store | ||
.adapterFor('secret-v2-version') | ||
.v2DeleteOperation(this.store, this.version.id, deleteType) | ||
.then(this.onRefresh); | ||
}, | ||
}, | ||
}); | ||
export default class SecretVersionMenu extends Component { | ||
onRefresh() {} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,7 +66,6 @@ | |
height: auto; | ||
line-height: 1rem; | ||
min-width: $spacing-l; | ||
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. the icons were showing in front of the modal, we determined it was the z-index causing this. |
||
z-index: 100; | ||
border: 0; | ||
box-shadow: none; | ||
color: $grey-light; | ||
|
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.
before we were not handling the delete-latest-version which made it impossible for some people with proper permissions to execute this action. They'd get a permissions denied, because we executing the delete any version action and not the latest version deletion.
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.
We also weren't handling errors, so they would silently fail.