Page MenuHomePhabricator

Rough cut of "Blueprint Authorizations"
ClosedPublic

Authored by epriestley on Oct 9 2015, 9:01 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 12, 6:34 PM
Unknown Object (File)
Tue, Mar 12, 6:32 PM
Unknown Object (File)
Tue, Mar 12, 5:11 PM
Unknown Object (File)
Tue, Mar 12, 3:53 PM
Unknown Object (File)
Tue, Mar 12, 3:02 PM
Unknown Object (File)
Tue, Mar 12, 2:33 PM
Unknown Object (File)
Tue, Mar 12, 2:30 PM
Unknown Object (File)
Sun, Mar 10, 2:22 PM
Subscribers
None

Details

Summary

Ref T9519. This is like 80% of the way there and doesn't fully work yet, but roughly shows the shape of things to come. Here's how it works:

First, there's a new custom field type for blueprints which works like a normal typeahead but has some extra logic. It's implemented this way to make it easy to add to Blueprints in Drydock and Build Plans in Harbormaster. Here, I've added a "Use Blueprints" field to the "WorkingCopy" blueprint, so you can control which hosts the working copies are permitted to allocate on:

Screen Shot 2015-10-09 at 1.51.04 PM.png (582×1 px, 82 KB)

This control has a bit of custom rendering logic. Instead of rendering a normal list of PHIDs, it renders an annotated list with icons:

Screen Shot 2015-10-09 at 1.52.29 PM.png (419×912 px, 71 KB)

These icons show whether the blueprint on the other size of the authorization has approved this object. Once you have a green checkmark, you're good to go.

On the blueprint side, things look like this:

Screen Shot 2015-10-09 at 1.53.38 PM.png (571×1 px, 103 KB)

This table shows all the objects which have asked for access to this blueprint. In this case it's showing that one object is approved to use the blueprint since I already approved it, but by default new requests come in here as "Authorization Requested" and someone has to go approve them.

You approve them from within the authorization detail screen:

Screen Shot 2015-10-09 at 1.54.42 PM.png (571×1 px, 129 KB)

You can use the "Approve" or "Decline" buttons to allow or prevent use of the blueprint.

This doesn't actually do anything yet -- objects don't need to be authorized in order to use blueprints quite yet. That will come in the next diff, I just wanted to get the UI in reasonable shape first.

The authorization also has a second piece of state, which is whether the request from the object is active or inactive. We use this to keep track of the authorization if the blueprint is (maybe temporarily) deleted.

For example, you might have a Build Plan that uses Blueprints A and B. For a couple days, you only want to use A, so you remove B from the "Use Blueprints: ..." field. Later, you can add B back and it will connect to its old authorization again, so you don't need to go re-approve things (and if you're declined, you stay declined instead of being able to request authorization over and over again). This should make working with authorizations a little easier and less labor intensive.

Stuff not in this diff:

  • Actually preventing any allocations (next diff).
  • Probably should have transactions for approve/decline, at least, at some point, so there's a log of who did approvals and when.
  • Maybe should have a more clear/loud error state when no blueprints are approved?
  • Should probably restrict the typeahead to specific blueprint types.
Test Plan
  • Added the field.
  • Typed some stuff into it.
  • Saw the UI update properly.
  • Approved an authorization.
  • Declined an authorization.
  • Saw active authorizations on a blueprint page.
  • Didn't see any inactive authroizations there.
  • Clicked "View All Authorizations", saw all authorizations.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Rough cut of "Blueprint Authorizations".
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: chad, hach-que.
chad edited edge metadata.
chad added inline comments.
src/applications/drydock/controller/DrydockBlueprintViewController.php
238

View All is probably sufficient

This revision is now accepted and ready to land.Oct 10 2015, 12:43 AM

There are several of them on the page ("View All Resources", "View All Logs", in some other controllers "View All Leases", and like 8 of these buttons across all the Drydock controllers) and I think those are better for screenreaders, although that's pretty weak as a justification.

Is your concern mostly just that the button is gigantic? It's definitely pretty huge, and probably 3000 pixels wide in French. The ragged edge looks kind of distracting, too:

Screen Shot 2015-10-09 at 6.45.05 PM.png (363×354 px, 15 KB)

How do you feel about "All Authorizations" instead of "View All"? It's slightly odd to have a non-verb button but the button isn't super button-y and it doesn't feel too terrible to me...

I'll just make 'em "View All", the ragged edge problem is only going to get worse if we drop "View".

epriestley edited edge metadata.
  • Rename all buttons to "View All".

Oh, one component of the alignment issue is that setTable() seems to affect the right margin for the button:

Screen Shot 2015-10-09 at 6.48.57 PM.png (377×1 px, 52 KB)

Note that the third button is shoved a little further to the right than the other two.

The longer versions of the buttons are more clear to me than just "View All", especially when the datasets are empty.

This is primarily an administrative interface, so I don't think it hurts to be explicit here.

Yeah I don't feel super strongly about it. Mostly, if a box is just one thing, buttons like edit, add, view all, search, etc are pretty clear in what they do and much cleaner.

And I obviously must add some value in my "code" reviews. >.<

I'm going to hold this until I cut the release tomorrow at a minimum, maybe I will have a dream about it which reveals the One True Path.

I also miiiiight just put icon tabs on Blueprints since they're starting to get a lot of sections, but I'm generally not too worried about getting any of this quite right for now.

This revision was automatically updated to reflect the committed changes.