Arvados

Ooh, I didn't try with a non-admin account. Good call. And yes, a test case would obviously help uncover this. I'll get right on it.

Reviewing a96adab

Was there a discussion about using a different implementation that's not reflected in this ticket? The ticket description here calls for a special case in Collections#show, rather than seeding the database with the appropriate record. This has some noticeable functional differences—for example, the ticket implementation would leave the empty collection out of Collections#index, while the one in the branch means it will always be there. I'm not sure which is better myself, but I do want to double-check that we're all on the same page about the impact of this change.

I also don't see anything in the branch addressing point #2, "Workbench should render links to the empty collection as 'Empty collection' instead of the generic 'Collection'." (Do you maybe have a local commit that didn't get pushed, or something?)

Assuming we go forward with this implementation: the migration is unlikely to fix the bug on existing installs. They all probably have records for the empty collection already, and the migration will leave those alone, without ensuring their owner_uuid is the anonymous group.

Thanks.

Brett Smith wrote:

Reviewing a96adab

Was there a discussion about using a different implementation that's not reflected in this ticket? The ticket description here calls for a special case in Collections#show, rather than seeding the database with the appropriate record. This has some noticeable functional differences—for example, the ticket implementation would leave the empty collection out of Collections#index, while the one in the branch means it will always be there. I'm not sure which is better myself, but I do want to double-check that we're all on the same page about the impact of this change.

Yes, there was a discussion about it, but unfortunately offline. Yesterday Tom and I discussed it in person. I am generally opposed to special cases when possible, and for something as crucial as permissions it makes me particularly nervous. Tom observed that the newly-implemented anonymous group would be a reasonable way to ensure that all users have read permission on the empty collection, so that's the path I took. I think we're all on the same page here.

I also don't see anything in the branch addressing point #2, "Workbench should render links to the empty collection as 'Empty collection' instead of the generic 'Collection'." (Do you maybe have a local commit that didn't get pushed, or something?)

No, I, um, overlooked it. Will add that appropriately.

Assuming we go forward with this implementation: the migration is unlikely to fix the bug on existing installs. They all probably have records for the empty collection already, and the migration will leave those alone, without ensuring their owner_uuid is the anonymous group.

Excellent point -- I'll update the migration to take that into account. (I am not sure that there's a good way to reliably migrate backwards; deleting the empty collection isn't necessarily the right way. Perhaps for rollback we'll just leave the collection as-is.)

New changes at 2e66bcd:

• empty_collection is owned by the system user, like all other collections. When it is created, a permission link is also created giving the anonymous group permission to read it.
• Migrating up will not modify any existing empty collection, but will add an appropriate permission link if one does not already exist.
• Migrating down does nothing (leaves the empty collection and permission link in place).
• Workbench renders links to the empty collection as Empty Collection.

Tim Pierce wrote:

• Workbench renders links to the empty collection as Empty Collection.

Does it work to implement this by overriding Collection#default_name? If it does, I feel like that would be a nicer apporach—mostly because link_to_if_arvados_object is already a doozy.

That's my only comment, so I'm happy to see this merged if you make this change or if it's not doable. Thanks.

Brett Smith wrote:

Tim Pierce wrote:

• Workbench renders links to the empty collection as Empty Collection.

Does it work to implement this by overriding Collection#default_name? If it does, I feel like that would be a nicer apporach—mostly because link_to_if_arvados_object is already a doozy.

It seems to. I was reluctant to muck with default_name because the semantics are not entirely clear to me, and I didn't want to create new hidden bugs by changing it to some custom value. But I guess if Job.default_name can be something like "$script job finished at $date" then the empty collection can muck with this too.