Bug #5010
closed[Workbench] Trash button on collections lists should set expires_at instead of moving to Home
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.
Related issues
Updated by Tim Pierce almost 10 years ago
- Assigned To set to Tim Pierce
- Target version changed from 2015-02-18 sprint to 2015-01-28 Sprint
Updated by Tim Pierce almost 10 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.
Updated by Brett Smith almost 10 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.
Updated by Tim Pierce almost 10 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 itsowner_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
Updated by Brett Smith almost 10 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
Updated by Tim Pierce almost 10 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.
Updated by Brett Smith almost 10 years ago
Tim Pierce wrote:
Thanks for checking the integration tests. I've removed those assertions at ffa928f
This is good to merge, thanks.
Updated by Tim Pierce almost 10 years ago
- Status changed from New to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|commit:e89251c160ae409f8af2f9ecae5ffb210ccd0a8d.