-
Notifications
You must be signed in to change notification settings - Fork 4.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
Remove decodeURIComponent method for KVv2 secret path on list view #28698
Conversation
CI Results: |
Build Results: |
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.
Thanks for addressing this and adding test coverage! Just comments about the subtext, as it's not quite accurate right now. Looks good otherwise
assert | ||
.dom(PAGE.breadcrumbAtIdx(3)) | ||
.hasText('foo%2fbar', 'foo%2fbar is the second directory and shows up as a separate breadcrumb'); | ||
assert.dom(PAGE.breadcrumbAtValue('world')).exists('the current breadcrumb is value world'); |
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.
How do we know from this assertion that it's the current breadcrumb? It seems like this might be better as
assert.dom(PAGE.breadcrumbAtValue('world')).exists('the current breadcrumb is value world'); | |
assert.dom(PAGE.breadcrumbAtIdx(4)).hasText('world', 'the current breadcrumb is value world'); |
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.
That was my original path, but it can't find the Idx4 because it's not a link but the active page view. I could do an additional check for class hds-breadcrumb__current
.
or I could create a new selector that does not include the a
. Below is the current selector:
[data-test-breadcrumbs] li:nth-child(${idx + 1}) a
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.
Gotcha, that makes sense. Using classes isn't the greatest because they can change, but I'm in favor of that, since it's clearer that's what we're testing. Then we don't have to add another selector.
That, or if we could update the existing breadcrumb
selector - which doesn't look like it's used much or at all to take a value
arg
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.
Removed the breadcrumb selector and it borked things even though I too couldn't find a usage.
Ended up creating a breadcrumbCurrentAtIdx which I feels like is valuable. It tells us we're confirming location and that it's a current status.
ui/tests/acceptance/secrets/backend/kv/kv-v2-workflow-navigation-test.js
Outdated
Show resolved
Hide resolved
ui/tests/acceptance/secrets/backend/kv/kv-v2-workflow-navigation-test.js
Outdated
Show resolved
Hide resolved
…28698) * remove encoding for KVv2 * test coverage * changelog * validations * Revert "validations" This reverts commit d6fd291. * update subtext for secret path * Update list.js * Update secret-edit.js * test coverage for data-octets * Update list-directory.js * fix modelForm test * amend subText * test selector things
* Remove decodeURIComponent method for KVv2 secret path on list view (#28698) * remove encoding for KVv2 * test coverage * changelog * validations * Revert "validations" This reverts commit d6fd291. * update subtext for secret path * Update list.js * Update secret-edit.js * test coverage for data-octets * Update list-directory.js * fix modelForm test * amend subText * test selector things * fix / update to old test selectors
Description
Fixes Issue #25905.
Previously, the KVv2 secretPath was run through a
decodeURIComponent
method while rendering the list view. This method found percent encoded data-octets and decoded them. In practice this means that something like%2f
would decode to/
. This is problematic for a user not intending to make a directory during path name creation but include the%2f
as part of the path name.This fix only applies to KVv2 because KVv1, Cubbyhole, etc pull the path name from the url where the URL is already encoded so decoding something like
hello%252fmeep
would equalhello%2fmeep
. Note: there is another bug I where these engines cannot render the create page on initial transitions for paths that contain data-octets. I've ticketed this bug and will address in a separate PR.Video of before fix and after fix
Screen.Recording.2024-10-15.at.9.07.25.AM.mov