UI - identity details #4502

meirish · 2018-05-03T17:13:10Z

This adds a lot more inline-editing for the various bits of identity (entities, groups, and their associated aliases).

We also added support for disabling entities, and added a number of acceptance tests that we didn't have time to do on the first pass.

This flushed out a few bugs with our front end list caching - which we've added to the internal backlog of tech debt to get rid of / simplify.

joshuaogle · 2018-05-03T18:25:27Z

ui/app/models/identity/entity.js

 const { attr, hasMany } = DS;

 export default IdentityModel.extend({
-  formFields: ['name', 'policies', 'metadata'],
+  formFields: ['name', 'policies', 'metadata', 'disabled'],


What do you think about moving disabled to after name? I've been trying to keep the list builders at the bottom so the other fields don't get lost

Oh yep, we can do that, the spacing just felt a little strange but I totally forgot about list builders haha. I'll update it!

Updated in the latest!

joshuaogle

I took this for a spin. Aside from those very minor suggestions, looks good from the design side 👍

joshuaogle · 2018-05-03T19:59:45Z

ui/app/templates/components/identity/edit-form.hbs

+
+      {{#if (and (eq mode "edit") model.canDelete)}}
+        {{#confirm-action
+          buttonClasses="button is-link is-outlined is-inverted"


Can we just do buttonClasses="button is-ghost" here? I think that's basically what you were going for

(sorry for the late response!) ooo yes, that's much nicer - should we also add has-text-black or is the blue text ok?

Blue text is preferred really. Some places we use black anyway if blue just doesn't look right, but this one needs help making it clear it's an action

Oh! Ha, I'll update the policy edit pages then too. 👌(probably some others that need updating too)

DingoEatingFuzz

Looks good!

Generally thinking here: a lot of the model and popup code is repetitive. It's probably worth giving this some more thought so future popups and modeling is easier/faster to implement.

DingoEatingFuzz · 2018-05-10T18:33:23Z

ui/app/components/confirm-action.js

@@ -16,6 +16,7 @@ export default Ember.Component.extend({
        class={{buttonClasses}}
        type="button"
        disabled={{disabled}}
+        data-test-confirm-action-trigger=true


Was there a particular motivation for assigning true rather than assigning it nothing?

Oh ha, just habit from components 😬

DingoEatingFuzz · 2018-05-10T18:38:49Z

ui/app/components/identity/popup-members.js

+
+  memberId: computed('params', function() {
+    return this.get('params').objectAt(2);
+  }),


In the event that params is not an ember array (most likely null) this will blow up.

DingoEatingFuzz · 2018-05-10T18:48:32Z

ui/app/models/identity/group.js

+    },
+    'id',
+    'identityType'
+  ),


These ten lines can be refactored as a computed property macro, making something like

updatePath: queryCapabilities('identifyType', 'id'),

Thanks for the nudge on this 🙌 - I was able to generalize the pattern we've used across the codebase with a computed macro + the use of a tagged template function to properly interpolate the paths with the named keys. This particular one can even be curried once since the path patterns are the same for all of the identity models ✨. I'll go back in another PR and use it throughout the codebase, but already it's cut down on a lot of code:

feba7a9

DingoEatingFuzz · 2018-05-10T18:52:20Z

ui/app/routes/vault/cluster/access/identity/show.js

+        this.controller.get('model').reload();
+      });
+    }
+  },


Another way to do this, not sure if it's better or not, is to never provide a model to link-to. Instead, only give IDs. Then the route will always go through the model hook. The downside is you have to trust all templates to use link-to as expected.

Yeah we were doing that and refreshing on every tab and I felt like it was overkill, but maybe it's not a terrible thing...

DingoEatingFuzz · 2018-05-10T18:53:08Z

ui/app/styles/core/buttons.scss

+      background-color: darken($orange, 5%);
+      border-color: darken($orange, 5%);
+    }
+  }


$orange isn't in the colors array?

No 😭 - only the colors associated with states - primary, warning, etc.

DingoEatingFuzz · 2018-05-10T18:58:54Z

ui/tests/acceptance/access/identity/_shared-alias-tests.js

+    aliasIndexPage.flashMessage.latestMessage.startsWith('Successfully deleted', `${itemType}: shows flash message`);
+  });
+
+


Extra lines hangin' out.

DingoEatingFuzz · 2018-05-10T19:01:04Z

ui/tests/acceptance/access/identity/_shared-alias-tests.js

+    createItemPage.createItem(itemType, 'external');
+  } else {
+    createItemPage.createItem(itemType);
+  }


This isn't so bad, but I always get nervous putting control flow in test helpers. It makes them brittle and can be really hard to trace values to.

DingoEatingFuzz · 2018-05-10T19:04:22Z

ui/tests/integration/components/identity/item-details-test.js

+    disabled: true,
+    canEdit: true
+  });
+  sinon.spy(model, 'save');


Spies ❤️

DingoEatingFuzz · 2018-05-10T19:04:37Z

ui/tests/integration/components/identity/item-details-test.js

+  sinon.spy(model, 'save');
+  this.set('model', model);
+  this.render(hbs`{{identity/item-details model=model}}`);
+  assert.dom('[data-test-disabled-warning]').exists();


QUnit DOM looks really helpful.

It's been really nice where we don't want / need / already have a full ember-cli-page object.

DingoEatingFuzz · 2018-05-10T19:06:12Z

ui/tests/unit/components/identity/edit-form-test.js

+      `${testCase.identityType} ${testCase.mode}: cancel link is correct`
+    );
+  });
+});


What are your thoughts on putting the test in the forEach, so each test gets its own line in the test runner?

Yep, going to change this - did it in a different PR and it's much nicer IMO (though you can't user arrow functions for the outer one anymore) .

changed in f317154

joshuaogle · 2018-05-21T21:00:38Z

@meirish Can we change those success banners to be the "toaster" type of alerts that float in the lower left? We should use those for alerts that don't require you to do anything (probably all success, sometimes some others)

…n the edit form

… the identity models

meirish requested review from a team May 3, 2018 17:13

joshuaogle reviewed May 3, 2018

View reviewed changes

joshuaogle previously approved these changes May 3, 2018

View reviewed changes

meirish dismissed joshuaogle’s stale review via ed3609b May 8, 2018 03:37

DingoEatingFuzz previously approved these changes May 10, 2018

View reviewed changes

jefferai added this to the 0.10.2 milestone May 22, 2018

meirish added 22 commits May 23, 2018 12:01

add popup to entity / group list

4637e10

add alias popover menu

efba180

add metadata popover

e6362c2

more metadata popups

d247f37

use base popup component and convert identity popups to use it

173d532

add group members popup

2cc75f5

add ability to disable entity and banner when entity is disabled

12d35f4

re-add alias-popup template

73bc868

add accpetance tests for creating entities

855043e

add more entity creation acceptance tests

00ac6ee

add delete to edit-form

bc98ae1

add more identity tests and associated selectors

e3a251c

add onSuccess hook and use UnloadModel route mixins

bf748cc

add ability to toggle entity disabling from the popover

02a0cae

fix store list cache because unloadAll isn't synchronous

b546c7a

fill out tests for identity items and aliases

3da2e1d

add ability to enable entity from the detail page

700bfe7

toArray on the peekAll

c660e07

fix other tests/behavior that relied on a RecordArray

07811f2

adjust layout for disabled entity and label for disabling an entity o…

f290211

…n the edit form

add item-details integration tests

e9314ca

move disable field on the entity form

6b531b3

meirish added 4 commits May 23, 2018 12:01

use ghost buttons for delete in identity and policy edit forms

b050f5f

review feedback

f317154

adding computed macros for lazy capability fetching and using them in…

feba7a9

… the identity models

prettier

d195251

meirish dismissed DingoEatingFuzz’s stale review via d195251 May 24, 2018 01:39

meirish force-pushed the ui-identity-details branch from ed3609b to d195251 Compare May 24, 2018 01:39

Merge branch 'master' into ui-identity-details

8c5aefd

meirish merged commit 3bc90ac into master May 24, 2018

meirish deleted the ui-identity-details branch May 24, 2018 03:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UI - identity details #4502

UI - identity details #4502

meirish commented May 3, 2018

joshuaogle May 3, 2018

meirish May 3, 2018

meirish May 3, 2018

joshuaogle left a comment •

edited

Loading

joshuaogle May 3, 2018

meirish May 8, 2018

joshuaogle May 8, 2018

meirish May 8, 2018 •

edited

Loading

DingoEatingFuzz left a comment

DingoEatingFuzz May 10, 2018

meirish May 23, 2018

DingoEatingFuzz May 10, 2018

DingoEatingFuzz May 10, 2018

meirish May 24, 2018

DingoEatingFuzz May 10, 2018

meirish May 23, 2018

DingoEatingFuzz May 10, 2018

meirish May 23, 2018

DingoEatingFuzz May 10, 2018

DingoEatingFuzz May 10, 2018

DingoEatingFuzz May 10, 2018

DingoEatingFuzz May 10, 2018

meirish May 23, 2018

DingoEatingFuzz May 10, 2018

meirish May 23, 2018

meirish May 24, 2018

joshuaogle commented May 21, 2018

		aliasIndexPage.flashMessage.latestMessage.startsWith('Successfully deleted', `${itemType}: shows flash message`);
		});

UI - identity details #4502

UI - identity details #4502

Conversation

meirish commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshuaogle left a comment • edited Loading

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

meirish May 8, 2018 • edited Loading

Choose a reason for hiding this comment

DingoEatingFuzz 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

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

joshuaogle commented May 21, 2018

joshuaogle left a comment •

edited

Loading

meirish May 8, 2018 •

edited

Loading