-
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
Conversation
} | ||
); | ||
// deleteType should be 'delete', 'destroy', 'undelete', 'delete-latest-version', 'destroy-version' | ||
if ((!version && deleteType === 'delete') || deleteType === 'delete-latest-version') { |
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.
@@ -0,0 +1,147 @@ | |||
import Ember from 'ember'; |
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.
moved all deletion actions into one glimmerized component.
|
||
@tracked showDeleteModal = false; | ||
|
||
@maybeQueryRecord( |
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.
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.
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 comment
The 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.
@@ -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 comment
The 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.
@@ -33,75 +33,15 @@ | |||
</ToolbarFilters> | |||
{{/unless}} | |||
<ToolbarActions> | |||
{{#if (and (eq @mode "show") this.isV2 (not @model.failedServerRead))}} |
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.
the ordering of toolbar-actions changed. Now deletion comes first and version dropdown is scooted to the far right.
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.
🛴 😄
P.S. Boooooo github not putting comments inline in the Convo tab spoils my emoji fun 😄
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.
Really great work so far, and good questions. I'll pull down the branch to play around with it too. 👏
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.
Hey @Monkeychip !
I took a quick look at this and noticed something that looked a little strange, but not massively consequential. Also, I checked it out and tried a different approach to closing the modal which 'seemed' to work, but I might have missed something due to lack of context. I tried to detail it in the comments.
Happy to hook up separately to chat through this if it helps though, feel free to give me a ping when you are around if that helps.
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(); |
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 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 model.reload()
ever called?
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 comment
The 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.
} | ||
let backend = context.args.model.backend; | ||
let id = context.args.model.id; | ||
let path = context.args.isV2 ? `${backend}/data/${id}` : `${backend}/${id}`; |
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'm guessing this eventually ends up in an API call? Is this one of those places that might need its parameters URL encoding? Is id
or backend
every likely to have URL unsafe characters in it? Maybe it gets slash split and url encoded elsewhere tho?
(there are a few more of these further down also)
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 point. Let me check this out.
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.
Note to self: it errors before it ever gets here if the user has entered an unsafe id or backend. Something to revisit.
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 think it's worth urlEncoding here even though it currently fails upstream, so that in the future if/when the upstream thing is fixed we don't have unsafe strings here
@@ -33,75 +33,15 @@ | |||
</ToolbarFilters> | |||
{{/unless}} | |||
<ToolbarActions> | |||
{{#if (and (eq @mode "show") this.isV2 (not @model.failedServerRead))}} |
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.
🛴 😄
P.S. Boooooo github not putting comments inline in the Convo tab spoils my emoji fun 😄
.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 comment
The reason will be displayed to describe this comment to others. Learn more.
Ah so I'm guessing resp
is where the undefined
comes in from the comment I made above for the adapter bits?
resp
is only ever set when its come in via the:
catch((e) => {
return e
})
...in the adapter?
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.
correct. We only get a resp if it's an adapter error.
<button | ||
type="button" | ||
class="button has-text-danger" | ||
{{on "click" (fn this.handleDelete this.deleteType)}} |
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.
Potential Modal Closing Solution Part 1:
Add a third 'success' function here:
{{on "click" (fn this.handleDelete this.deleteType (action (mut this.showDeleteModal) false))}}
return; | ||
} | ||
if (!resp) { | ||
location.reload(); |
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.
Potential Modal Closing Solution Part 3:
Remove the location.reload() and use success()
instead:
if(!resp) {
success()
}
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.
@johncowen this is very helpful! Thank you. I'll start working through some of these solutions. Closing the modal was the whole reason I was forcing a reload of the page, but in this case I may not have to. Thanks again!
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.
Awesome 🙌
if (!context.args || !context.args.modelForData || !context.args.modelForData.id) return; | ||
let [backend, id] = JSON.parse(context.args.modelForData.id); | ||
return { | ||
id: `${backend}/delete/${id}`, |
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.
here too probably (re: encoding)
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 on this!
This PR redesigns the Delete functionality of the KV2 secret engine. Before we didn't cover all the situations for the various deletes based on permissions. Under this new design, we make sure to cover all the various combinations of deletes based on permissions as well as make it a little more intuitive on how to reach the delete actions and what they each do.
Different Delete options:
1. Delete latest version only.
In this scenario we only show the delete dropdown option when the user is on the current version, it disappears if the user is on an older version.
2. Delete any version
In this scenario, we show the delete dropdown option for all versions.
3. Destroy version
Whenever a user has the destroy permissions we show a modal instead of the dropdown. The modal will then either show all three options or a mix of them based on permissions but it will always show a destroy option. In the example, the user also can delete any version so that option is also shown.
4. Permanently remove all versions
This policy allows you to permanently remove all the versions, essentially a destroy on all versions.
This is the modal with all permissions.
5. Undelete only shows if you have the following permissions
When the delete/destroy/undelete action occurs the page is refreshed. Below is a gif. I couldn't find a better solution for refreshing the model and closing the modal, but I'm very open to suggestions. However, as it currently works the functionality is clean.
Icons were also added to the version dropdown menu as well as the version page (where we removed deleted actions).