-
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
UI: Refactor path-help service #28444
UI: Refactor path-help service #28444
Conversation
CI Results: |
dc92a29
to
cbbe403
Compare
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.
I wanted to get my comments in so you had plenty of time to review. Nothing blocking. I'll move on to smoke testing.
urlForItem(id, isList, dynamicApiPath) { | ||
const itemType = sanitizePath(this.getPath); | ||
let url; | ||
id = encodePath(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.
For my own understanding, because we're encoding the id here, i would expect it could be something like /test?
. Is that correct? ID to me indicates a string that shouldn't need encoding, but then again we manual change ids to be concatenations of strange things.
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 believe this can be anything that an auth item (such as userpass username) can be. In that case, things like spaces or special characters need to be encoded, which is why we do this here.
get mutableId() { | ||
return this._id || this.id; | ||
} | ||
set mutableId(value) { |
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.
nice!
*/ | ||
_registerModel(owner, NewKlass, modelType, isNew = false) { | ||
const store = owner.lookup('service:store'); | ||
// bust cache in ember's registry |
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.
very helpful comments, thank you.
* @param {Attribute[]} attrs array of attributes {name, type, options} | ||
* @returns new ModelClass extended from passed one, with the passed attributes added | ||
*/ | ||
_upgradeModelSchema(Klass, attrs, newFields) { |
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 like how you worked around the off-limits Class
name, and used Klass
😎
@@ -8,11 +8,12 @@ import { module, test } from 'qunit'; | |||
import { setupApplicationTest } from 'ember-qunit'; | |||
import { v4 as uuidv4 } from 'uuid'; | |||
|
|||
import authPage from 'vault/tests/pages/auth'; | |||
import { login, loginNs } from 'vault/tests/helpers/auth/auth-helpers'; |
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.
🤩
Description
Prep for ember-data upgrade -- refactors the path-help service so that it no longer uses deprecated Ember Data method
ember-data:deprecate-model-reopen
. Our goal with this work is to keep the functionality the same (ability to hydrate new and existing models with OpenAPI data) with more modern patterns. There should be no user-facing changes.