UI: New ACLs

[johncowen]

This is potentially a temporary PR for ease of early review.

The description of this PR is likely to be edited/added to next week and the base branch is also likely to change.

This PR adds new style ACLs to the UI.

Generally the new code follows the same style as the rest of the app.

There are some new concepts/additions that I've tried to keep purely in the new ACLs areas for the moment, but I plan on moving these across into the rest of the app all being well.

I'll briefly list these additions here (to be expanded upon)

Forms

Forms are view layer objects that are mainly concerned with 'change' events of HTML form elements in a 'data down, events up' way. The allow you to define reusable forms that can be composed into larger forms. In my current case I needed to do this for embedding a policy form into a token form.

For example, creating policies is now part of editing a token, but also policies can be created outside of tokens.

I didn't want to have to repeat the set up of the validations and the 'change' listeners in both places. An alternative to 'forms' would have been writing a component for each form, but then I probably would have needed 2 components for each type of policy form, or lots of conditionals within one. This is also part one of doing something I've been wanting to do which is similar to how the vault UI dynamically generates its form views depending on the data/model.

Lastly, 'forms' integrate well with ember-changeset-validations and follow some conventions there.

dom-buffer

Usecase: I want to buffer a DOM element (or HTMLBars string) from being inserted into the DOM and then flush it out elsewhere, later on.

For modals you generally have to append your modal DOM elsewhere in the DOM. I initially looked at ember-wormhole but due to its use of private API's and the way you use it not being very ember-y, I also looked at ember-elsewhere (which has similar reasons for not using ember-wormhole). I couldn't see if ember-elsewhere could be used without passing you DOM to it as an attribute. So in the end it was easier to write my own simpler thing.

Other things to note:

1. You can extend the dom-buffer component (as I do here) and tell your component to appear somewhere else.
2. Its based on injectable services.
3. It's still a work in progress, but only the straightforward things are left to do, for the moment there is only 1 buffer called 'modal'.

I also looked at ember-maybe-element, I need to look at this further, but at first glance it also uses private APIs currently, and also seems to be a processor rather than anything else (they also mention not wanting to use ember-wormhole)

Dedicated dom service

I decided to move all of my dom utilities into a service. This means that ones that need setting up, only need setting up once, in the service itself, and they are ready to be used and injected where-ever you need them. This feels very much like a jQuery like way to do things, so I'm still slightly uncomfortable doing this, but it means I can inject and unit test things properly without resorting to things like js-dom or min-document. Moving all these to a service means that injecting all the things in one place is less painful than in multiple places.

Lastly here, I'm still deliberating on whether to names of methods here, and I still need to finish up migrating everything to use the service instead of importing.

repo.status()

The way I was catching errors previously never transitioned to the URL you had clicked before displaying the error page. I think this is just the way the ember catchall error catching works? I needed someway to easily display errors whilst making sure the URL was the correct URL, so I'm moving away form the ember catchall error and setting isEnabled and isAuthorized properties for each route depending on the HTTP status instead. repo.status() is a nice way of doing this that works in with my app-view to show error pages whilst maintaining the correct url.

SingleRoute

This is my first attempt at reducing the repetition of create/edit routes. I'm still not massively convinced by this myself, and whilst is good enough for what I wanted to do at first, I think I'd prefer not extending something and using a utility function within model instead. This would keep the traditional model method a lot clearer. Completely in two minds over this still!

Lastly, this entirely PR still needs a good spring clean, there are still a lot of notes in the comments for my own use, and this won't be merged without a full spring clean and possibly a second review.

New localStorage format

We briefly chatted about this offline. I'm now following vaults format for localStorage keys i.e. 'consul:keyname', all values are JSON encoded, even strings. I did mention that I didn't want to save JSON blobs, but I should qualify that now by saying I don't want to save big JSON blobs. Right now I'm saving little JSON blobs where it makes sense (in this case your new type of token - {"AccessorID": "", "SecretID": ""}, feels like it makes sense to just save that as a blob.

Important thing to note here is that there is nothing here to 'upgrade' from the old type of localStorage tokens to the new ones. I'd also rather people relogged in so they get a proper full token with ID and secret.

[johncowen]

Hey @meirish

I've changed the base branch for this now and clean up some of the commented in code that was helping me during dev. Since we spoke yesterday I've also pulled out the storage prefixing into its own module so I can unit test it (and added unit tests).

I think the only thing I might add to this before you are about today is moving siblings and closest into my dom service and move everything within ACLs to use the dom service instead of importing directly (if I get chance, so please don't let that stop you giving a 👍 )

Other than that, I'm just looking for an approval on this now

Cheers!

[meirish]

👍 Might want to remove the WIP from the title before you merge ;)

[meirish]

Oh @johncowen - was also curious if you had any response to the ember-wormhole bit I mentioned here: #4789 (review) (no rush on that though!)

[johncowen]

I was curious about the ember-wormhole removal though - did you run into any problems with it or just didn't like the use of private apis? With the direct dependency on it removed, aren't you still loading it transitively since you're using power-select? What's the plans for the future there (I think power-select now uses that maybe-in-element addon) ?

Ah I've been looking for that comment but couldn't find it, I've been looking in the wrong place (the inline comments)...anyway..

I didn't remove it as such, I just didn't use it.

From what I can see nobody wants to use ember-wormhole, everybody that used to use it is moving away from it, I've even seen the team talking about problems using it. Then of course there was the private API usage.

I looked for others and only found that ember-elsewhere, which I tried but as far as I can see you have to specify your entire template/DOM in an attribute, which wasn't ideal 🤷‍♂️ .

I've not looked/tried that maybe-in-element addon as I wasn't aware of it until you mentioned it (it didn't surface whilst looking for alternatives), but from what I can see it also uses private APIs (that may soon be made public), plus it seems to be a transpiler rather than actual javascript, which immediately made me think a little too strange for now.

In the end I took a quick pass at making my own and it only took a little while, probably shorter than trying out other things, so I went with that.

The modals will be coming out reasonably soon anyway as theres another style of UI coming down the line pretty soon.

With the direct dependency on it removed, aren't you still loading it transitively since you're using power-select?

I 'think' even power-select are using that maybe-in-element one now, but they have a load of code in there to cope with both approaches.. (I was browsing over the power-select and simple-menu source last night)

[DingoEatingFuzz]

@johncowen, I haven't looked at the code yet but excellent write up 👏 👏. Judging by how much you have to say about the high-level decisions after the fact, it makes me think this would have been a good candidate for an RFC before implementation.

Gonna peruse the code now 👀

[DingoEatingFuzz]

Wrote down my thoughts! I only skimmed through the css hbs and tests (you'll have to forgive me, it's a big PR).

Generally looks great though. I'm sure people will appreciate the new ACLs experience (especially with how much thought has gone into backwards compatibility and forward migration).

[DingoEatingFuzz]

IMO, It'd read better if this was imported as DomBuffer. Typically I look right past imports until I see something I don't recognize. In this case, I assumed Component was @ember/component.

[johncowen]

I changed it to DomBufferComponent so it's massively clear what it is. See #5021

I also changed it in other places such as my ember-collection extended TabularCollection to be consistent.

[DingoEatingFuzz]

This will break if the selector doesn't match anything. It generally seems brittle to have a selector like this in a component. I'd feel a little better if it were an id, but it still makes me squeamish.

[johncowen]

Thanks for the suggestion here, I can partly see where you are coming from, but...

I suppose this is similar to the conversation we had around CSS selectors and HTML/template structure.

Usually if I need to change one, it is absolutely no problem to change the other. Indeed in this case other frameworks like React and Vue etc let you, even encourage you, to keep your template in the exact same file as your javascript behavior (that's not something I'm keen on myself).

As far as I'm aware with the ember module unification effort, all of these files will live in the same place, coupling them even more and making them even easier to make changes like this.

I'd feel a little better if it were an id

ids should be unique. With this component it's probably not the case, but if I were to add a component to the same page twice, and the component template used an id in it, it could produce one of those hard to track down bugs. So as a general rule I would almost never put an id in a component.

[DingoEatingFuzz]

It's unfortunate that all this dom-twiddling is happening outside of a template, but I understand it's unavoidable at times.

[DingoEatingFuzz]

I echo what @meirish said here. Overriding setProperties, despite it being a public method, is surprising and is best avoided. I liken it to overriding an Array or String prototype. Sure Array#push is a public method but anyone familiar with JavaScript has a preconceived idea of what it does. The same is true for Ember devs and EmberObject#setProperties.

Take this example:

export Route.extend({
  setupController(controller, model) {
    let here = { i: 'dunno what to put here' };
    controller.setProperties({
      some: 'things',
      go: here,
    });
    return this._super(...arguments);
  },
});

It's now on the developer to look at this code and remember that setProperties means different things depending on what controller this is. It also has deeper implications since the framework code is also allowed to call setProperties.

Something like controller.setProperties(controller.syncFormData({ prop, er, ties })) would be more obvious upon reading.

[johncowen]

I liken it to overriding an Array or String prototype. Sure Array#push is a public method but anyone familiar with JavaScript has a preconceived idea of what it does

I'd definitely not recommend altering any native objects in any way without extending them first. I think we've spoken about this before here hashicorp/nomad#4635 (comment)

I've just looked again at the file, aren't you still overriding Array instance methods here?

https://github.com/hashicorp/nomad/blob/adc05976c2c7f97d46c214518d448b0b13e377d6/ui/app/utils/classes/rolling-array.js

It's not the prototype method, but you couldn't you still do an

array.constructor === Array ? array.push(thing) : somethingElse

...and end up with something you didn't expect?

In my case I'm extending Controller first, so I'm essentially adding functionality to a base Controller to give me a specific PoliciesController which is my own custom class. setProperties is public and not final in anyway (AFAIK) so usually that would mean 'fair game', as long as it does what you expect aswell as the extra functionality you add (which it does by calling super). Additionally what my custom setProperties does is to 'proxy' certain variables you pass it (which I've mentioned here https://github.com/hashicorp/consul/pull/4789/files/88027a940b088d4e3c96b8dbcd9e557c8d524881#r225548050). I also left a comment in to explain this slightly:

consul/ui-v2/app/controllers/dc/acls/policies/edit.js

Line 13 in 88027a9

// essentially this replaces the data with changesets

Changesets are just proxy objects to the underlying data. So it just 'decorates' the data with extra functionality.

Something like controller.setProperties(controller.syncFormData({ prop, er, ties })) would be more obvious upon reading.

One thing I should explain here, is that I went to great lengths to keep the forms (and any mention of them) in the View/ViewController layer, and out of the Routes. They are a view concern, so this is where they should live.

I can appreciate your overall point a little bit and I did think of other options and I've thought of others to try to address this somehow. The answer to the problem here is to find a hook to use in the View layer immediately before the View is rendered. I've considered setProperties to be this hook as the View is re-rendered immediately after that.

Other things I can think of:

1. Use a computed property:

actualItem: computed('item', function(){
  return this.form.setData()
})

This now means that item in my template is no longer the item that I want. It's the unproxied item. To me this is surprising as I'm passing it into the View as item, if I look at the templates it leaves me thinking "what's actualItem, what happened to item from my Route?" Plus I'd have to do this for every property I want to proxy, which would get tedious very quickly. Right now I can just loop over the props in setProperties and proxy the object there.

2. Use computed setter/getters:

item: computed('item', function(){
  {
    set: function(){},
    get: function(){}
  }
})

As far as I know this means having yet another property on the Controller to save the actual value of item, I'd also have to do this for every property (see above).

3. Use a more generically named controller 'setup' function, and remember to call it everytime I call setProperties.

export Route.extend({
  setupController(controller, model) {
   
    controller.setProperties({
      some: 'things',
      go: here,
    });
    if(typeof controller.setup === 'function') {
      controller.setup();
    }
    return this._super(...arguments);
  },
});

controller.setup would mutate the item property into the proxy object. But it's named something generic enough to make sense. I'm actually surprised that I can't find any default setup/reset-like methods for Controller in ember.

Option 3 is my favourite at a pinch, but there is always the danger that you'd forget to call setup somewhere, and therefore produce some hard to spot bugs.

There's even a plugin which attempts to make this less error prone for you:

https://github.com/kellyselden/ember-controller-lifecycle

The only problem with this is, there is still a danger that you might call setProperties somewhere else and your controller.setup() is never called, which would lead to unexpected bugs.

Do you have any further ideas following the above info?

(Apologies if this comment is a bit lengthy, I want to try to make sure follow my thinking here)

[johncowen]

Sorry had another idea for this, thought I'd pop it in here incase.

Option 3 is my favourite at a pinch, but there is always the danger that you'd forget to call setup somewhere, and therefore produce some hard to spot bugs.

Option 4:

If I changed things so that I stopped calling setProperties, but instead called a method on the Controller that then called setProperties. So something like:

// ReopenedController using a similar approach to ember-controller-lifecycle
newMethodNameThatsGenericEnough: function(model) {
  this.setProperties(model);
}

// SomeController
newMethodNameThatsGenericEnough: function(model) {
  const proxiedModel = this.doExtraStuffWithModel(model);
  this._super(proxiedModel);
}

// SomeRoute
setupController: function(controller, model) {
  this._super(...arguments);
  controller.newMethodNameThatsGenericEnough(model);
}

..and now when I have anywhere in my codebase where I was previously calling setProperties and there was the danger of forgetting to then call a setup, I just change it to call newMethodNameThatsGenericEnough instead.

I don't think this method should be called setup though as I also call setProperties when I'm not setting things up, so it wouldn't make sense there. Will think a bit more on a good name. But I think this addresses my problem and satisfies your concerns about overwriting the setProperties method, so win win.

The only tiny thing is, this isn't 'conventional ember' we've added a new undocumented method - but I'm ok with that if you are?

[johncowen]

Related: #5056

[DingoEatingFuzz]

it just calls a method instead of passing a property back down

This is half of the crux of the problem. It makes no longer "data" that is going down. The other half of the crux of the problem is that isn't going down. Since you're pulling this component out of a service, it isn't going down the component tree. It's instead plain message passing, which isn't as strict as DDAU and opens up your views to difficult to follow execution paths.

A more data-down approach to this would be to first deal with the display:none situation in the code editor component rather than making it a concern of this controller. Falling short of that, a grosser but harder to abuse option is to bind an arbitrary property to the code editor that can be observed to call CodeMirror APIs. Example:

// controller.js
if (target.checked) {
  this.set('codeEditorIsVisible', true);
}

{{!-- page-template.hbs --}}
{{code-editor isVisible=codeEditorIsVisible}}

// code-editor.js
toggleVisibility: observer('isVisible', function() {
  if (this.get('isVisible')) {
    CodeMirrorInstance.refresh();
  }
}

[DingoEatingFuzz]

This entire file strikes me as out of scope for ACLs work.

[johncowen]

It was just that the app.css file was getting a bit too full of component file imports 😬 - my app is getting big enough now that I thought it was probably a good time to give them their own index file.

[DingoEatingFuzz]

I don't follow what this input is doing at all. The state has the unhelpful name checked and the action has the unhelpful name change.

[DingoEatingFuzz]

Seeing WIP on the master branch is never reassuring.

[johncowen]

No prob, removed in #5021

[DingoEatingFuzz]

A couple opinions on this, take them or leave them. 1) Saying things like "when I start" in code comes off kind of controlling. Am I supposed to do a git blame to see who "I" is? Should I leave this alone and let "I" deal with it? 2) It seems easy enough to add this line back once there is a staging env rather than having an unused wart for a possible future.

[johncowen]

No prob,I tweaked the comment in #5021 to reword it slightly.

You don't have to, but to explain a little - if you look at the blame for this file:

https://github.com/hashicorp/consul/blame/88027a940b088d4e3c96b8dbcd9e557c8d524881/ui-v2/ember-cli-build.js

You'll see I did it during a 'Comment clean up' commit. I didn't remove this unused code on purpose, but added the comment to explain why I didn't remove it like I did with others. For me it will be useful at somepoint, maybe for others also. It's in 'node-world' so it doesn't end up in shipping code (and is a comment anyway), so I've chosen to leave it in for the moment.

We are currently looking at setting up a more permanent staging environment for the UI, so this might come in useful 🔜

[DingoEatingFuzz]

A major version bump? What changed? Do you PR to this repo? Do the changes get code reviewed?

[johncowen]

A major version bump? What changed?

Yup, major. I changed the name of one of the cookies you can use to debug/test from CONSUL_ENABLE_ACLS to CONSUL_ACLS_ENABLE to bring it in line with all of the other variables I use to debug and test the UI (they all follow CONSUL_<MODEL_NAME>_<SETTING> now). So, unfortunately breaking yes. I'm the only person that I know of using this so I'd say it's not a big deal in the scheme of things, but you never know, plus its habit I suppose. Breaking means major bump.

Do you PR to this repo? Do the changes get code reviewed?

No I don't PR changes, I commit straight onto master and publish to npm to release, if there is a 'bug' I release a patch version, but generally it's reasonably well tested first considering what it is. This repo is only my 'api double' templates. They don't really do anything, it's only a templated version of the consul HTTP API. So its pretty much a tonne of JSON text files.

Also, the module isn't shipping code (in the Consul UI), it's only to make it easier for me to test using massive amounts of data.

I sometimes ask the backend team to review the code if it's for a new upcoming API that hasn't been nailed down yet to make sure I can build out the UI without the backend being built.

If you want to help out with api-double (the actual template renderer, not the specific consul-api-double templates) I'd be more than happy for you to pitch in if that's what you want to do. I've mentioned people helping out a few times but up until now no-one seems interested? Let me know, happy to walk you through.

[johncowen]

I just noticed this whilst I was searching for the 'DDAU alternative' PR, thought it gives some good context api-double vs consul-api-double so posting here for reference:

#4190 (comment)

[johncowen]

Hey @DingoEatingFuzz,

Thanks for the review on this, sorry for taking so long to come back to you on your comments. I've tried to explain a little more so you can see my thinking on some of the points, and I've made a few of changes in #5021 based on your review.

Thanks