Bug #3072

[Workbench] Showing the empty collection in Workbench gives an error page

Added by Abram Connelly almost 6 years ago. Updated almost 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Tim Pierce
Category:
-
Target version:
Start date:
06/20/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.50 h)
Story points:
0.5

Description

Solution:
  1. API server's database should be seeded with the empty collection and a permission link giving anonymous_group permission to read it. (See the anonymous_user method in services/api/lib/current_api_client.rb for creation pattern "create object and/or link as needed".)
  2. Workbench should render links to the empty collection as "Empty collection" instead of the generic "Collection".

Reported as:

I ran a job that failed but had a collection link from the page where individual jobs can be examined. When clicking on the collection link, a "We're sorry, but something went wrong." page appeared.

Offending URL: https://demo.curoverse.com/collections/d41d8cd98f00b204e9800998ecf8427e+0
job page: https://demo.curoverse.com/jobs/qr1hi-8i9sb-ygellbugm4t6viu

Screenshots of the offending page and the page before it are attached.

jobs_page.png (97.6 KB) jobs_page.png Abram Connelly, 06/20/2014 03:34 PM
sorry_something_went_wrong.png (28.4 KB) sorry_something_went_wrong.png Abram Connelly, 06/20/2014 03:34 PM
Screenshot - 07072014 - 01_09_22 PM.png (102 KB) Screenshot - 07072014 - 01_09_22 PM.png Tim Pierce, 07/07/2014 01:09 PM
Screenshot 2014-07-09 at 6.25.17 PM.png (48.3 KB) Screenshot 2014-07-09 at 6.25.17 PM.png Tom Clegg, 07/09/2014 06:25 PM

Subtasks

Task #3237: Review 3072-empty-collection-bugfixResolvedTim Pierce

Associated revisions

Revision a96adab2 (diff)
Added by Tim Pierce almost 6 years ago

3072: add an empty collection in db seeds

Added empty collection (owned by the anonymous group) to test fixtures
and to db/seeds.rb.

API integration tests: in permissions_test.rb, test that the active user
can read objects in the anonymous group, even when there are no explicit
permission links on that group or objects in it.

Workbench integration tests: added a test to confirm that a non-admin
user can get the empty collection.

Cleanup code: User.can? now takes either a uuid or an object for the target.

Refs #3072.

Revision 1be91cdf (diff)
Added by Tim Pierce almost 6 years ago

3072: rendering "Empty Collection" in default_name

Collection.default_name now renders "Empty Collection" for collection
d41d8cd98f00b204e9800998ecf8427e.

Refs #3072.

Revision e74ede46
Added by Tim Pierce almost 6 years ago

Merge branch '3072-empty-collection-bugfix'

closes #3072.

History

#1 Updated by Tom Clegg almost 6 years ago

  • Subject changed from Get a "We're sorry, but something went wrong." message when trying to view a (non-existant?) collection from a failed job to Showing the empty collection in Workbench gives an error page
  • Description updated (diff)

#2 Updated by Tom Clegg almost 6 years ago

  • Story points set to 0.5

#3 Updated by Tom Clegg almost 6 years ago

  • Target version set to 2014-07-16 Sprint

#4 Updated by Tim Pierce almost 6 years ago

  • Assigned To set to Tim Pierce

#5 Updated by Tim Pierce almost 6 years ago

Marking this resolved; workbench on the master branch displays a proper page for /collections/d41d8cd98f00b204e9800998ecf8427e+0 instead of a fiddlesticks page. Can navigate to and from jobs that produced the empty collection without a problem.

#6 Updated by Tom Clegg almost 6 years ago

I still see this bug -- see attached screenshot. (Tim, does it work for you with a non-admin account?)

Adding a test case would be a good idea, either way...

#7 Updated by Tim Pierce almost 6 years ago

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.

#8 Updated by Brett Smith almost 6 years ago

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.

#9 Updated by Tim Pierce almost 6 years ago

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.)

#10 Updated by Tim Pierce almost 6 years ago

  • Target version changed from 2014-07-16 Sprint to 2014-08-06 Sprint

#11 Updated by Radhika Chippada almost 6 years ago

  • Subject changed from Showing the empty collection in Workbench gives an error page to [Workbench] Showing the empty collection in Workbench gives an error page

#12 Updated by Tom Clegg almost 6 years ago

  • Description updated (diff)

#13 Updated by Tim Pierce almost 6 years ago

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.

#14 Updated by Brett Smith almost 6 years ago

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.

#15 Updated by Tim Pierce almost 6 years ago

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.

#16 Updated by Tim Pierce almost 6 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:e74ede4659428af77c50057d69a0d08e4e74a6ef.

Also available in: Atom PDF