Arvados

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.

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.

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

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

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.

Tim Pierce wrote:

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

This is good to merge, thanks.