Arvados

• Updated the setup popup to also include "groups"
  • Store these, after a successful setup step, in the can_login link as groups property
  • Populate the groups text field in the popup with any stored groups

• Did not do any removal of groups during unsetup process
  • If an admin want to delete groups for a user, just need to execute setup method with the groups text field cleared.

Reviewing 6254-groups-during-setup at cdec4d7

I'm sorry this wasn't spelled out better in the story description, but unfortunately, you're manipulating the wrong kind of can_login permission link. There are two kinds. The first kind has a head_uuid of the user UUID and a tail_uuid of an e-mail address. This is an "OID login" permission. This is related to API server logins somehow, and might be obsolete—I don't really know what it does. (Tom would probably know what it is and whether we still use it, if you're curious.)

The second kind is a virtual machine (VM, a.k.a. shell) login permission. This has a head_uuid of the Arvados VirtualMachine's UUID and a tail_uuid of the user UUID. This is the permission that we need to extend with group information, since the groups are used on virtual machines and not for OID logins.

When you make the change to update those links, you won't need the user_email hidden input anymore, so removing that input from the form would be a nice cleanup.

The user setup method returns all the links that it finds or creates in a list. Instead of re-finding the can_login permission link on the API server, can we save the result of calling the user setup method, find the can_login permission link in the response, and send an update with new properties for that link? That would save a round-trip with the API server and keep Workbench a little more responsive.

You should also be able to use this returned link object to compare the list of requested groups against the list of actual groups. This way, you can avoid propagating the previous list of groups through the HTML form, and doing all the associated string manipulation.

The user setup popup instructs the user to enter a comma-separated list of names in the input title. I believe many users will only see this if they hover a mouse pointer over the input—that's what my Firefox does, at least. I think it would be helpful to include this note as plain text on the form to reduce the risk that users might enter groups in a different format (in particular, they might try just using space-separated names).

Hidden inputs are being added to the user setup popup that have a maxlength attribute set. This attribute is not defined and has no meaning for hidden inputs, so it would be good to remove to avoid potential confusion later.

Then one last small style note, this doesn't need to be addressed before merge: Ruby's string#split method always returns an array of strings. You don't have to worry about the possibility that there are non-string objects, including nil, in the response. This means that the line:

params[:groups].split(',').map(&:strip).compact.select{|i| !i.to_s.empty?}

can be simplified to:

params[:groups].split(',').map(&:strip).select{|i| !i.empty?}

Thanks.

• Brett said:

This has a head_uuid of the Arvados VirtualMachine's UUID and a tail_uuid of the user UUID. This is the permission that we need to extend with group information ...

Corrected this. I should have suspected and checked, because I initially was going to work on this :)

• Brett said:

user setup method returns all the links that it finds or creates in a list. Instead of re-finding the can_login permission link on the API server, can we ...

Unfortunately, setup response is a HashList, not resource list. Hence the links information is coming back as map, not as a link object. So, I needed to get it first to update it. However, this was still a good idea and the updated revision optimized on getting just the uuid specific link from setup response.

• Implemented other updates as suggested.

• Updated Groups text box label. While doing more testing, I noticed that if I do not select the VM, but enter the groups, they will not take effect; no vm link to add these links to. Rather than complicate implementation to disable the text box until a VM is selected, I updated the label to be helpful. Given that it is admin UI, I think this would be sufficient.

Thanks.

d3fc80c is good to merge. One last follow-up:

Radhika Chippada wrote:

• Brett said:

user setup method returns all the links that it finds or creates in a list. Instead of re-finding the can_login permission link on the API server, can we ...

Unfortunately, setup response is a HashList, not resource list. Hence the links information is coming back as map, not as a link object. So, I needed to get it first to update it. However, this was still a good idea and the updated revision optimized on getting just the uuid specific link from setup response.

I think you can say vm_login_link = Link.new(vm_link) to instantiate a proper Link object from the attributes hash. Then you can just modify its properties and save! it as if you got it normally. I'm not 100% sure, but if you want to try it, it might simplify things a little more. But it's not critical; you can merge either way. Thanks.

Brett said:

I think you can say vm_login_link = Link.new(vm_link) to instantiate a proper Link object from the attributes hash ...

This does not work. I had actually already tried with pure find also because I have the link uuid. I could not save the object and get API error. Leaving it as is. Thanks.