Arvados

At 2769713

• Reviving tags might be a good idea in general but please resist further creep in this story/branch. Clicking a tag on the "show project" page takes me to a list of other collections, but they're surrounded by a bunch of indigestible stuff like head/tail/link_class, a "create link" button, more UUIDs. I'd rather un-hyperlink the tags until we have a decent target to link to (e.g., a post-#3149 search dialog).
• On collections#index, remove the "storage" column in thead to match tbody.
• The tag lookup code at the end of ProjectsController#load_contents_objects seems to duplicate existing functionality provided by preload_links_for_objects. I think you just need to call preload_links_for_objects(@objects) in the controller, and links_for_object(object).select{|link|link.name=='tag'} in the view.
• I can't quite figure out what the change in apps/workbench/app/views/application/_show_attributes.html.erb is for. It seems to have the effect of restricting what can be achieved with @attribute_sortkey in ArvadosBase, but I haven't yet thought of a reason why.
• In Collection#portable_data_hash, it might be safer to return self[:uuid] rather than self.uuid if self[:portable_data_hash] is nil (and similar in #uuid). If both are nil, returning nil seems more appropriate than going OOM or hitting an arbitrary recursion depth limit.
• Collection#attribute_editable? looks like it returns true for 'manifest_text' which is technically wrong if still talking to a pre-#3036 API server. I don't know of anywhere in Workbench that actually acts differently either way though, so it's probably not worth bothering with.

• CollectionReader __init__ expects md5, needs to accept regular UUID as well

(Tests aren't passing, but I'm pretty sure you already know that.)

Tom Clegg wrote:

At 2769713

Workbench

• Reviving tags might be a good idea in general but please resist further creep in this story/branch. Clicking a tag on the "show project" page takes me to a list of other collections, but they're surrounded by a bunch of indigestible stuff like head/tail/link_class, a "create link" button, more UUIDs. I'd rather un-hyperlink the tags until we have a decent target to link to (e.g., a post-#3149 search dialog).

Agreed that the links index page is not very nice. However I'm going to leave the code it because it is still at least somewhat useful and it would be more effort to rip it out at this point.

• On collections#index, remove the "storage" column in thead to match tbody.

Fixed.

• The tag lookup code at the end of ProjectsController#load_contents_objects seems to duplicate existing functionality provided by preload_links_for_objects. I think you just need to call preload_links_for_objects(@objects) in the controller, and links_for_object(object).select{|link|link.name=='tag'} in the view.

Fixed.

• I can't quite figure out what the change in apps/workbench/app/views/application/_show_attributes.html.erb is for. It seems to have the effect of restricting what can be achieved with @attribute_sortkey in ArvadosBase, but I haven't yet thought of a reason why.

Late-night attempt at improving the look of the details pages, and not aware of @attribute_sortkey. Reverted.

• In Collection#portable_data_hash, it might be safer to return self[:uuid] rather than self.uuid if self[:portable_data_hash] is nil (and similar in #uuid). If both are nil, returning nil seems more appropriate than going OOM or hitting an arbitrary recursion depth limit.

Fixed.

• Collection#attribute_editable? looks like it returns true for 'manifest_text' which is technically wrong if still talking to a pre-#3036 API server. I don't know of anywhere in Workbench that actually acts differently either way though, so it's probably not worth bothering with.

Left it alone.

Python

• CollectionReader __init__ expects md5, needs to accept regular UUID as well

Fixed.

(Tests aren't passing, but I'm pretty sure you already know that.)

Tests pass now.

Peter Amstutz wrote:

Agreed that the links index page is not very nice. However I'm going to leave the code it because it is still at least somewhat useful and it would be more effort to rip it out at this point.

• -          <%= link_to tag_link.name, controller: "links", filters: [["link_class", "=", "tag"], ["name", "=", tag_link.name]].to_json %>
  +          <%= tag_link.name %>
  

• On collections#index, remove the "storage" column in thead to match tbody.

Fixed.

Great. Should make the remaining column widths add up to 100% though. (Add the spare 12% to the last (tags) column?)

Rest of the fixes look good.

Just a couple of new things:

In CollectionsController#choose we concatenate @collections + @objects. If I'm following correctly, old API will put nothing in @collections while new API will put nothing in @objects so this is just a way to "do whatever's appropriate for the current API version" without actually deciding which version it is. Is that right? (Either way I think a quick comment would be a good investment. "Only one of @objects and @collections will be non-empty, depending on API version, see #3036. FIXME: remove 'name links' stuff here after #3036 is deployed everywhere."?)

(Mentioned in #3036 but seems to be fixed differently in 3504 branch) In sdk/python/arvados/commands/put.py we set owner_uuid during collections().create() and then update it to the same value in collections().update(). Should probably remove owner_uuid from the update call, unless of course it's needed for some reason I'm not thinking of.

Thanks.

Tom Clegg wrote:

The changes to the links view etc. are harmless, I'm just asking for the following change, to avoid luring the poor users into a construction zone:

• [...]

Fixed

Great. Should make the remaining column widths add up to 100% though. (Add the spare 12% to the last (tags) column?)

Fixed

In CollectionsController#choose we concatenate @collections + @objects. If I'm following correctly, old API will put nothing in @collections while new API will put nothing in @objects so this is just a way to "do whatever's appropriate for the current API version" without actually deciding which version it is. Is that right? (Either way I think a quick comment would be a good investment. "Only one of @objects and @collections will be non-empty, depending on API version, see #3036. FIXME: remove 'name links' stuff here after #3036 is deployed everywhere."?)

Added comment.

(Mentioned in #3036 but seems to be fixed differently in 3504 branch) In sdk/python/arvados/commands/put.py we set owner_uuid during collections().create() and then update it to the same value in collections().update(). Should probably remove owner_uuid from the update call, unless of course it's needed for some reason I'm not thinking of.

Fixed. Juggling changes between branches is tricky.

At 73bbd41...

• The "add tag" button doesn't actually work on the projects#show→collections tab. I think it successfully adds a tag with head_uuid=uuid_of_name_link, which succeeds, but that tag doesn't get loaded when I hit Refresh.
• The "add tag" button appears in a project where I have no write permission.

Other fixes look good

(If it's not already removed) I think this is obsolete now in projects.css.scss

.removable-tag:hover {
    cursor: pointer;
}
\ No newline at end of file

da8b740

1. Removed tag rendering code from project rows.
2. Merged a few small fixes from 3036
3. Tests pass

All that looks good, and I haven't found any further problems. Thanks!