Bug #5010

[Workbench] Trash button on collections lists should set expires_at instead of moving to Home

Added by Tom Clegg over 5 years ago. Updated over 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Tim Pierce
Category:
Workbench
Target version:
Start date:
01/23/2015
Due date:
% Done:

100%

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

Description

When a user clicks the trash icon next to a collection in the Projects display, Workbench should ask for confirmation and then set the expires_at date on the collection to the current time, so the API server will no longer return that collection to queries.


Subtasks

Task #5044: Review 5010-trash-button-for-collectionsResolvedTim Pierce


Related issues

Related to Arvados - Story #3150: [API] Add "trash" behavior for all object types, instead of literal rdbms "delete"Rejected07/04/2014

Related to Arvados - Feature #5032: [Workbench] User can trash and un-trash itemsResolved01/20/2015

Associated revisions

Revision e89251c1
Added by Tim Pierce over 5 years ago

Merge branch '5010-trash-button-for-collections'

Fixes #5010.

History

#1 Updated by Tim Pierce over 5 years ago

  • Assigned To set to Tim Pierce
  • Target version changed from 2015-02-18 sprint to 2015-01-28 Sprint

#2 Updated by Tim Pierce over 5 years ago

  • Category set to Workbench

#3 Updated by Tim Pierce over 5 years ago

  • Description updated (diff)

#4 Updated by Tim Pierce over 5 years ago

Ready for review at 89fbb8bf

No unit test has been added to exercise this: I believe that the test "project admin can remove items from the project" already covers it.

#5 Updated by Brett Smith over 5 years ago

Tim Pierce wrote:

No unit test has been added to exercise this: I believe that the test "project admin can remove items from the project" already covers it.

The request in that test is definitely the one we care about for this story, and the assertions there all remain sensible too. But this story specifies that the underlying Collection should be manipulated a specific way at the API level, and the test doesn't check that at all. Ideally, we would assert that, after the request, the underlying Collection has an expires_at attribute in the past, while its owner_uuid hasn't changed.

It would also be nice to have a similar test on an object in a project that doesn't expire, to make sure the old behavior continues to work as expected.

You could do this with mocks, or with the test helper method find_fixture (to get the corresponding object from the API server after Workbench is done manipulating it).

Thanks.

#6 Updated by Tim Pierce over 5 years ago

  • Description updated (diff)

#7 Updated by Tim Pierce over 5 years ago

Brett Smith wrote:

Tim Pierce wrote:

No unit test has been added to exercise this: I believe that the test "project admin can remove items from the project" already covers it.

The request in that test is definitely the one we care about for this story, and the assertions there all remain sensible too. But this story specifies that the underlying Collection should be manipulated a specific way at the API level, and the test doesn't check that at all. Ideally, we would assert that, after the request, the underlying Collection has an expires_at attribute in the past, while its owner_uuid hasn't changed.

Thanks for reminding me that I didn't finish cleaning up from the story drift here. I've updated the story for this ticket to describe what we want to do right now, and am updating #3150 to make sure all of the pieces are there too.

It would also be nice to have a similar test on an object in a project that doesn't expire, to make sure the old behavior continues to work as expected.

You could do this with mocks, or with the test helper method find_fixture (to get the corresponding object from the API server after Workbench is done manipulating it).

Changed the existing test to confirm that the deleted collection cannot be found at all in the API server. (Also confirmed that this test fails in the old workbench interface, which just moved the collection to the home project.)

Also added a test to confirm that objects which don't include an expires_at field, like Specimen, continue to exhibit the old behavior (and confirmed that this test passes in both old and new workbench).

There isn't yet a way to get the API server to report on the existence of trashed collections. (Again, my fault for not updating this story.) Until we implement that -- which is going to require some interesting rejiggering of how we apply this filter in the first place -- there is literally no way for a client to tell whether an object has been deleted from the API server or has simply been filtered out based on its expires_at field.

Tests added at 6f3e7bf

#8 Updated by Brett Smith over 5 years ago

Tim Pierce wrote:

Tests added at 6f3e7bf

These are great, thanks for following up on that. I like everything in the branch, but it looks like a couple of integration tests need to be updated to expect the new behavior. Unfortunately, I got these failures:

  1) Failure:
ProjectsTest#test_selection_Remove_->_nil_for_project [/home/brett/repos/arvados/apps/workbench/test/integration/projects_test.rb:245]:
Collection not found in home project after remove

  2) Failure:
ProjectsTest#test_selection_Remove_->_true_for_project [/home/brett/repos/arvados/apps/workbench/test/integration/projects_test.rb:247]:
Collection with update name is not found in home project after remove

#9 Updated by Tim Pierce over 5 years ago

Thanks for checking the integration tests. I've removed those assertions at ffa928f

This code now passes all tests in test/integration/projects_test.rb. I'm running a full suite of workbench tests just to make sure I haven't incidentally broken anything else.

#10 Updated by Brett Smith over 5 years ago

Tim Pierce wrote:

Thanks for checking the integration tests. I've removed those assertions at ffa928f

This is good to merge, thanks.

#11 Updated by Tim Pierce over 5 years ago

  • Status changed from New to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:e89251c160ae409f8af2f9ecae5ffb210ccd0a8d.

Also available in: Atom PDF