Arvados

12125-workbench-project-trash @ 71da9f42b396e6ae8d7ef83b1855d5bb407c2a17

Adds "Trashed projects" tab to trash page.

Also adds check to 404 page if an item is in the trash, and offers to untrash it.

Adds tests.

https://ci.curoverse.com/job/developer-run-tests/474/

• On the 404 page, do you think that adding the date when the trashed item is going to be deleted is something that could be useful to the user?
• When accessing a collection that’s inside a trashed project, the 404 doesn’t notify the user this fact, and visiting the Trash page doesn’t show the collection, even searching it by uuid. Maybe the easier path to allow the user find those kind of lost collections is to inform that it’s parent project is trashed?
• The above also happens with projects inside a trashed project, it doesn’t appear on Trash and the 404 page just says that’s not found.
• If the previous points are valid, maybe it would be convenient to write tests about them.

Lucas Di Pentima wrote:

• On the 404 page, do you think that adding the date when the trashed item is going to be deleted is something that could be useful to the user?

Sure, can't hurt.

• When accessing a collection that’s inside a trashed project, the 404 doesn’t notify the user this fact, and visiting the Trash page doesn’t show the collection, even searching it by uuid. Maybe the easier path to allow the user find those kind of lost collections is to inform that it’s parent project is trashed?

Yes, the 404 page could follow the ownership links to find the trashed project and offer to untrash it.

• The above also happens with projects inside a trashed project, it doesn’t appear on Trash and the 404 page just says that’s not found.

We have an include_trashed flag which would allow us to search for items regardless of whether they are trashed or not. However, this creates a couple of challenges with the current API:

• We can't efficiently determine if an item is inside a trashed project
• We can't efficiently determine which trashed project contains an item

Doing this for a single items (on the 404 page) isn't so bad, but doing it across sets of items is much more expensive.

I think the ideal API would be to add a "inside_trashed_project_uuid" field that is returned on all objects when include_trashed=true, however that will require reopening #12032 first.

• If the previous points are valid, maybe it would be convenient to write tests about them.

They are valid, I'm just not sure whether to address them or not.

Strategy

• When listing/searching trashed collections
  • use include_trashed
  • collect owner uuids and query that without include_trashed
  • trashed collections will either
    • have trashed_at set (trashed directly)
    • have a owner uuid that isn't in the list (trashed indirectly)
  • may need to iterate a few times so we have enough items to show

12125-workbench-project-trash @ 6e68c94524cfb19fc0b51591eab7e4a55e485164

When visiting the trash page, initially only is_trashed=true items are shown. However, user can search for trashed collections and subprojects that are contained within a trashed project. There is a note on the page informing the user about this behavior.

The rational for this behavior is:

1. Limits the visual clutter of the initial listing to only projects and collections that were explicitly trashed.
2. It requires two queries to fetch items within a trashed project, one to get all items, a second to get non-trashed items (which are subtracted from the "all" list to get just the trashed items). This is relatively expensive so limiting it to searches ensures it is only done for queries where the result set is expected to be small.

Rows returned within a trashed project as a result of a search behave slightly differently from rows where is_trashed=true. To avoid expensive lookups during page rendering, the trash_at and delete_at times are not displayed. In addition, the selection check box is not rendered, because the item can't be untrashed directly, but requires untrashing the parent.

The 404 page now informs the user when an item is part of a trashed project, and offers to untrash the parent project. If a project is under multiple levels of trash (for example, a trashed collection in a trashed project) the user will click through multiple untrash pages until the item is fully untrashed.

The recycle button to untrash a single item on the trash page now directs to the 404 page for consistency in the flow for untrashing a single item whether it is within a trashed project or trashed directly.

The new behavior looks good to me. I ran workbench tests locally, got some failures:

  5) Error:
JobsTest#test_view_log_via_keep-web_redirect:
Capybara::ExpectationNotMet: expected to find text "log message 1" in "workbench:test Dashboard Projects Home A Project Trash (No description provided) Copy to project... Move job... Status Log Details Provenance Advanced Summary Download the full log Log Select all Select none Sort by time Sort by node Sort by task Show all tasks Only successful tasks Only failed tasks Show crunch diagnostics Show task dispatch Show task diagnostics Show compute usage Timestamp Node Slot Log type Task Message" 
    test/integration/jobs_test.rb:80:in `block in <class:JobsTest>'
    test/test_helper.rb:303:in `run'

  6) Error:
AnonymousAccessTest#test_view_file:
Capybara::ExpectationNotMet: expected to find text "7med30yd5b23ekjqyfswrpg15bf0glmwnkoto2imzdw0jxsyw1ghs8oecdhn5k295quvdo87ekmrrxmql49ff06phl9oob07jk1" in "workbench:test Browse public projects Log in Public Projects Unrestricted public data (none) (none) Collection UUID: Content address: Content size: 1 file totalling 99 bytes No source information available. Welcome to Arvados You are accessing public data. Files Tags Provenance graph Used by Advanced 99 bytes Hello world.txt" 
    test/integration/anonymous_access_test.rb:130:in `block in <class:AnonymousAccessTest>'
    test/test_helper.rb:303:in `run'

  7) Failure:
CollectionsTest#test_can_download_an_entire_collection_with_a_reader_token [/home/lucas/arvados_local/apps/workbench/test/integration/collections_test.rb:91]:
download page did provide strictly file links.
Expected: ["foo"]
  Actual: []

Passes on jenkins:

https://ci.curoverse.com/job/developer-run-tests/488/

Reviewed branch 12125-fuse-project-trash - 4d2dfa766

LGTM. Just a tiny observation:

• File services/fuse/tests/test_mount.py - Line 768: I think the use of sorted() is unnecessary, correct?

Lucas Di Pentima wrote:

Reviewed branch 12125-fuse-project-trash - 4d2dfa766

LGTM. Just a tiny observation:

• File services/fuse/tests/test_mount.py - Line 768: I think the use of sorted() is unnecessary, correct?

Yes, that's a copy and paste from another bit of code that expected more than one element. I choose to merge it as-is, but thanks for pointing it out.