Arvados

• I think the lambda style is superfluous here since the where() doesn't change over time -- i.e., it would work equally well to do this?

  • default_scope where("expires_at IS NULL or expires_at > CURRENT_TIMESTAMP")

• We should get into the habit of using a user or project ID as owner_uuid of a collection (the way collections work in daily life now) instead of using owner_uuid=root and a permission link (the way they used to work in daily life before #3036).

• Do these tests really need permit_unsigned_manifests?
• Should also test whether not-yet-expired and expired collections can be retrieved with get :show, { uuid: blah } (should and shouldn't, respectively)
• Ditto calling "update" on a not-yet-expired collection (should work), on an expired collection (should 404)

I don't see where we do "Changing this value to less than 24 hours should result in an error" but I'm also not sure about the best place to do that. Perhaps in services/api/lib/tasks/config_check.rake? (That gets run during deploy scripts, so config errors can be caught before even trying to start the application.)

Ready for another look at 9d18764

Tom Clegg wrote:

In services/api/app/models/collection.rb

• I think the lambda style is superfluous here since the where() doesn't change over time -- i.e., it would work equally well to do this?
  • [...]

Yes, you're right. (The lambda was left over from doing #{Time.now} until I realized we could just use CURRENT_TIMESTAMP)

In services/api/test/fixtures/collections.yml

• We should get into the habit of using a user or project ID as owner_uuid of a collection (the way collections work in daily life now) instead of using owner_uuid=root and a permission link (the way they used to work in daily life before #3036).

Fixed, and dropped the unnecessary permission links. That's much much nicer.

In services/api/test/functional/arvados/v1/collections_controller_test.rb

• Do these tests really need permit_unsigned_manifests?

No, turns out they don't. That was me trying to be conservative about the test conditions.

• Should also test whether not-yet-expired and expired collections can be retrieved with get :show, { uuid: blah } (should and shouldn't, respectively)
• Ditto calling "update" on a not-yet-expired collection (should work), on an expired collection (should 404)

Added test clauses for these.

I don't see where we do "Changing this value to less than 24 hours should result in an error" but I'm also not sure about the best place to do that. Perhaps in services/api/lib/tasks/config_check.rake? (That gets run during deploy scripts, so config errors can be caught before even trying to start the application.)

Ah, I forgot that completely. Thanks for your help on this: rake config:check now raises an exception if default_trash_lifetime is set to anything less than 86400.

Tim Pierce wrote:

• Should also test whether not-yet-expired and expired collections can be retrieved with get :show, { uuid: blah } (should and shouldn't, respectively)
• Ditto calling "update" on a not-yet-expired collection (should work), on an expired collection (should 404)

Added test clauses for these.

I think these tests need to be written as separate test cases. (Functional tests reuse the controller object if you call multiple actions from one test case, which is not what happens in real life and is very likely to do/test the wrong thing.)

Ah, I forgot that completely. Thanks for your help on this: rake config:check now raises an exception if default_trash_lifetime is set to anything less than 86400.

I think it would be nicer to call Rails.configuration.default_trash_lifetime instead of $application_config['...'] here.

Everything else looks good. Thanks.

New revision at 0a5f8b6

Tom Clegg wrote:

Tim Pierce wrote:

• Should also test whether not-yet-expired and expired collections can be retrieved with get :show, { uuid: blah } (should and shouldn't, respectively)
• Ditto calling "update" on a not-yet-expired collection (should work), on an expired collection (should 404)

Added test clauses for these.

I think these tests need to be written as separate test cases. (Functional tests reuse the controller object if you call multiple actions from one test case, which is not what happens in real life and is very likely to do/test the wrong thing.)

That's useful to know -- I was never sure where the appropriate boundary was for functional tests.

Ah, I forgot that completely. Thanks for your help on this: rake config:check now raises an exception if default_trash_lifetime is set to anything less than 86400.

I think it would be nicer to call Rails.configuration.default_trash_lifetime instead of $application_config['...'] here.

OK.

LGTM, thanks!