Arvados

Deleting a CR should invoke the same hook that runs when priority changes, so (if a container is assigned and the deleted CR had the highest priority of any CR for the assigned container) the container's priority gets updated accordingly.

Would it work just as well to use the existing container and CR called "running" instead of adding new fixtures?

I like the approach of calling update_attributes!(priority: 0) but this seems more like a model concern than a controller concern. Does it work to call self.update_attributes!() in a before_destroy hook on the model instead of the controller? Otherwise, perhaps we just have to call update_priority in an after_destroy (or before_destroy?) hook, and add "...or !persisted?" to the list of conditions in update_priority that trigger an update.

Branch 11682-delete-container-request @ 27b7b66

I like the approach of calling update_attributes!(priority: 0) but this seems more like a model concern than a controller concern. Does it work to call self.update_attributes!() in a before_destroy hook on the model instead of the controller? ...

Good suggestion. Changed to use a before_destroy filter

Would it work just as well to use the existing container and CR called "running" instead of adding new fixtures?

Test works (worked) with the fixture "running" as well. However, if we were to add any other fixtures with priority>0 using the same container_uuid in future, then the test would fail. We could delete all such container_requests before checking priority in the test. However, I think it would be nice to have the test be independent. I can change the test to use "running" if you prefer to.

Thanks. I think this version looks cleaner.

This test looks like it can/should be done as a unit test -- calling "post :destroy" doesn't add any value compared to calling c.destroy directly, does it?

Can you add a test that destroys a container_request that has priority>0 and state=Final? That should be possible, but I'm suspicious it might fail with "can't update priority when state=Final".

@ commit:27027c4

Thanks. I think this version looks cleaner.

It sure does. Thanks.

This test looks like it can/should be done as a unit test -- calling "post :destroy" doesn't add any value compared to calling c.destroy directly, does it?

You are right. Replaced the controller test with a unit test.

Can you add a test that destroys a container_request that has priority>0 and state=Final? That should be possible, but I'm suspicious it might fail with "can't update priority when state=Final".

Thanks for noticing the bug I introduced when moving the code around. Corrected the before_destroy implementation and added a unit test to delete a container_request in Final state.

LGTM, thanks!