UI: refactor KMIP role model #28418

hashishaw · 2024-09-17T18:18:22Z

Description

This PR refactors the kmip/role model in preparation for our Ember Data upgrade. During testing, the computed values in the model were causing weirdness with the new path-help approach.

Our latest best practices include having the models as thin as possible, and calculating things like fieldGroups in the components where they are used instead of on the model itself. However, to keep the scope of this PR small I opted to keep some of the model getters for field groups, specifically for the role details page. In the future we may choose to remove these in favor of component getters.

Before / After

Ent tests pass

hashishaw · 2024-09-17T18:21:22Z

ui/app/models/kmip/role.js

-export default Model.extend(COMPUTEDS, {
-  backend: attr({ readOnly: true }),
-  scope: attr({ readOnly: true }),
-  name: attr({ readOnly: true }),


name was set on the model, but role is the attribute that comes back from OpenAPI. Since there's no way to filter out attributes from OpenAPI, I opted to keep role instead of name

github-actions · 2024-09-17T18:40:06Z

CI Results:
All Go tests succeeded! ✅

github-actions · 2024-09-17T19:53:28Z

Build Results:
All builds succeeded! ✅

kiannaquach · 2024-09-17T22:21:27Z

ui/app/models/kmip/role.js

-  }),
+@withExpandedAttributes()
+export default class KmipRoleModel extends Model {
+  @attr({ readOnly: true }) backend;


do we need to specify the role attribute here?

I believe that withExpandedAttributes will populate the model with the properties returned from OpenAPI and it sounds like role is coming from there.

hellobontempo

Nice work! Just a couple of questions, nothing blocking! ⭐

hellobontempo · 2024-09-20T17:12:03Z

ui/lib/kmip/addon/components/kmip/role-form.hbs

+  <MessageError @model={{@model}} data-test-edit-form-error />
+  <div class="box is-sideless is-fullwidth is-marginless">
+    <NamespaceReminder @mode="save" />
+    {{#if (eq @mode "create")}}


Instead of passing @mode do we want to leverage @model.isNew, since that's been the newer pattern we've used?

hellobontempo · 2024-09-20T17:17:20Z

ui/app/utils/kmip-role-fields.js

Could you explain your reasoning behind making this util file? Just wondering about when to use this pattern in the future. To me it feels like a one-off while we bridge the old model pattern and transition to the new "schema" models, and that typically this sort of thing would live in the form component. But I could be missing something and am curious about your perspective! 💭

Good question! I have it in a util because it was used in the form as well as the adapter for ensuring that the correct fields get sent to the API. As we move toward thinner models it's worth considering what we want the pattern to be, and this file could definitely become obsolete (or part of a different file structure) in the future

Gotcha! Did we ever get around to writing the schema model readme file? I wonder if it's worth considering a model/helpers file or something. But would have to think about this more... It also seems like since Ember is moving away from adapters/serializers all together (in the looong term plan) so, I think you're right that the need for this may become obsolete all together 🤔 Just spitballin

I didn't -- probably worth doing a follow-up PR with the doc and a folder pattern for things like this. We are probably a long way from eliminating our adapters and serializers so it's definitely worth establishing a pattern

hellobontempo · 2024-09-20T17:18:46Z

ui/app/models/kmip/role.js


-  operationFormFields: computed('operationFieldsWithoutSpecial', function () {
+  get operationFormFields() {


It looks like this has been renamed and moved to operationFormGroups below in the form component, do we still need this getter here?

Yes, this is used in the KMIP OperationDetailsDisplay component for role details page.

Awesome! I had done a search and missed that, thanks for following up

hellobontempo · 2024-09-20T17:22:09Z

ui/lib/kmip/addon/templates/role/edit.hbs

  @model={{this.model}}
+  @mode="edit"


Same question as above about passing @mode

hellobontempo · 2024-09-20T17:22:30Z

ui/lib/kmip/addon/templates/scope/roles/create.hbs

  @model={{this.model}}
  @mode="create"
  @onSave={{transition-to "vault.cluster.secrets.backend.kmip.scope.roles" this.scope}}
-  @cancelLinkParams={{array "scope.roles" this.scope}}
+  @onCancel={{transition-to "vault.cluster.secrets.backend.kmip.scope.roles" this.scope}}


hellobontempo · 2024-09-20T17:22:50Z

ui/lib/kmip/index.js

@@ -17,6 +17,10 @@ module.exports = EngineAddon.extend({
    enabled: true,
  },

+  babel: {
+    plugins: [require.resolve('ember-concurrency/async-arrow-task-transform')],


What is this?!

😂 required to use the async-arrow notation -- the other engines already had it but when I refactored the form in KMIP I got an error that it needed this transform

Aha! Thank you for explaining 😂

hashishaw added 5 commits September 14, 2024 13:54

update kmip/role model and adapter

88783b5

New KMIP role form component

aa5356e

cleanup on kmip role adapter/model

7b1e92f

fix role details view

b38f059

update tests to check for kmip role form and details validity

fd7ce4a

hashishaw added ui pr/no-changelog labels Sep 17, 2024

hashishaw added this to the 1.19.0-rc milestone Sep 17, 2024

github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Sep 17, 2024

hashishaw commented Sep 17, 2024

View reviewed changes

hashishaw and others added 4 commits September 17, 2024 13:59

cleanup

18acc98

Add kmip-role-fields test

2db6d0e

add headers, remove old component

6416b29

Merge branch 'main' into ui/update-kmip-model-prep-ember-data-upgrade

bd8d73d

hashishaw marked this pull request as ready for review September 17, 2024 19:50

hashishaw requested a review from a team as a code owner September 17, 2024 19:50

vercel bot deployed to Preview September 17, 2024 19:57 View deployment

Merge branch 'main' into ui/update-kmip-model-prep-ember-data-upgrade

97b6889

kiannaquach reviewed Sep 17, 2024

View reviewed changes

Merge branch 'main' into ui/update-kmip-model-prep-ember-data-upgrade

9ad1fc9

vercel bot deployed to Preview September 19, 2024 18:30 View deployment

hashishaw mentioned this pull request Sep 19, 2024

UI: Refactor path-help service #28444

Merged

2 tasks

hellobontempo approved these changes Sep 20, 2024

View reviewed changes

Address PR comments

ecd43d7

hashishaw enabled auto-merge (squash) September 20, 2024 18:36

hashishaw merged commit 520f141 into main Sep 20, 2024
31 checks passed

hashishaw deleted the ui/update-kmip-model-prep-ember-data-upgrade branch September 20, 2024 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI: refactor KMIP role model #28418

UI: refactor KMIP role model #28418

hashishaw commented Sep 17, 2024 •

edited

Loading

hashishaw Sep 17, 2024

github-actions bot commented Sep 17, 2024 •

edited

Loading

github-actions bot commented Sep 17, 2024

kiannaquach Sep 17, 2024

zofskeez Sep 18, 2024

hellobontempo left a comment

hellobontempo Sep 20, 2024

hellobontempo Sep 20, 2024

hashishaw Sep 20, 2024

hellobontempo Sep 20, 2024

hashishaw Sep 20, 2024

hellobontempo Sep 20, 2024

hashishaw Sep 20, 2024

hellobontempo Sep 20, 2024

hellobontempo Sep 20, 2024

hellobontempo Sep 20, 2024

hellobontempo Sep 20, 2024

hashishaw Sep 20, 2024

hellobontempo Sep 20, 2024


		operationFormFields: computed('operationFieldsWithoutSpecial', function () {
		get operationFormFields() {

UI: refactor KMIP role model #28418

UI: refactor KMIP role model #28418

Conversation

hashishaw commented Sep 17, 2024 • edited Loading

Description

Choose a reason for hiding this comment

github-actions bot commented Sep 17, 2024 • edited Loading

github-actions bot commented Sep 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hellobontempo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashishaw commented Sep 17, 2024 •

edited

Loading

github-actions bot commented Sep 17, 2024 •

edited

Loading