-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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: Move legacy ACLs, KVs and Intentions to use form
functionality
#4936
Conversation
} | ||
return prev; | ||
}, model) | ||
); |
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 tried simplifying this a little, by Object.assign
ing instead. Strangely doing that in the token
form broke my tests, see further down, I'll comment in where it broke.
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 I can't theres no change there 😂 , link instead:
consul/ui-v2/app/controllers/dc/acls/tokens/edit.js
Lines 12 to 30 in b153f9b
setProperties: function(model) { | |
// essentially this replaces the data with changesets | |
this._super( | |
Object.keys(model).reduce((prev, key, i) => { | |
switch (key) { | |
case 'item': | |
prev[key] = this.form.setData(prev[key]).getData(); | |
break; | |
case 'policy': | |
prev[key] = this.form | |
.form(key) | |
.setData(prev[key]) | |
.getData(); | |
break; | |
} | |
return prev; | |
}, model) | |
); | |
}, |
So changing that ^ to use Object.assign
broke the tests, so I left things as they are for now.
I might look into it a little deeper, but I want to start feature work again on Monday so I don't want to get in a rabbit hole refactoring too much.
@@ -23,7 +25,7 @@ export default Controller.extend({ | |||
this._super({ | |||
...model, | |||
...{ | |||
item: this.changeset, | |||
item: this.form.setData(model.item).getData(), | |||
SourceName: source, | |||
DestinationName: destination, | |||
}, |
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.
This one does use Object.assign
so I need to take a second look and make them all as similar as possible
// these variables also exist in the template so we know | ||
// the current selection | ||
// basically the difference between | ||
// `item.DestinationName` and just `DestinationName` | ||
set(this, target.name, selected); | ||
break; | ||
} |
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.
Most of this above will eventually end up being done in the form instead
case 'additional': | ||
parent = get(this, 'parent.Key'); | ||
set(this.item, 'Key', `${parent !== '/' ? parent : ''}${target.value}`); | ||
break; |
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.
Most of this above will eventually end up being done in the form instead. The json
one below has nothing to do with the model and is just view state, so will stay here.
…4936) Change legacy acls, kvs and intentions to use `form`s
…4936) Change legacy acls, kvs and intentions to use `form`s
…4936) Change legacy acls, kvs and intentions to use `form`s
…4936) Change legacy acls, kvs and intentions to use `form`s
…4936) Change legacy acls, kvs and intentions to use `form`s
…4936) Change legacy acls, kvs and intentions to use `form`s
This PR moves legacy ACLs, KVs and Intentions to use the newer
form
functionality.This brings everything consistently together and follows exactly how it is done in the rest of the app.
Also see #4789
Further work here will involve moving the 'stranger' functionality in Intentions and KVs out of the Controllers and into the forms themselves, meaning Controllers will revert to just orchestration not functionality. What is done here has simplified it ever so slightly (especially for intentions) but I'd rather the
form
deals with this.