Arvados

10538-trash-delete @ 6cec281e8653731602ab871cc41ddd21ca8182ab https://ci.curoverse.com/job/developer-run-tests/120/

• services/api/db/migrate/20161213172944_full_text_search_indexes.rb
  • I think it's not convenient to modify old migration files because those servers that already applied the migration won’t get the update.
• services/api/app/models/collection.rb
  • Line 241: should @validation_timestamp be used to get the expiration timestamp?
  • Line 450: Isn’t ‘|| false’ unnecessary?
  • Line 456: I find the repeated conditionals a little difficult to read, do you think this would be clearer?
    

    def default_trash_interval
      if trash_at_changed? && !delete_at_changed?
        self.delete_at = trash_at.nil? ? nil : trash_at + Rails.configuration.default_trash_lifetime.seconds
      end
    end
    

  • Line 465, 466: If trash_at is nil and delete_at is not nil, then the error message is confusing, because (at least for me) it only mentions the case for when delete_at is nil. Maybe change it to “delete_at should be set when trash_at is set, and should be nil otherwise”?
• sdk/go/arvados/collection.go
  • Line 24: Should this be IsTrashed/is_trashed instead of IsTrash/is_trash?
• I think it would be clearer if in the tests instead of referring to the fixture collections by uuid, we do it using their fixture label. If the issue is that the default scope avoid this, maybe some comment besides the UUID describing what type of collection is being referred to can be helpful to improve readability.
  • services/api/test/functional/arvados/v1/collections_controller_test.rb Lines 978, 979, 936, 946, 967
  • services/api/test/unit/collection_test.rb Lines 473, 492

Lucas Di Pentima wrote:

• services/api/db/migrate/20161213172944_full_text_search_indexes.rb
  • I think it's not convenient to modify old migration files because those servers that already applied the migration won’t get the update.

I'm assuming you mean 924b783e07028b17cd5403205361e4715f7a212f although it's not on this branch ... this fixed a bug where an infrequently-updated system running a lot of migrations at once would use an outdated schema in full_text_tsvector. I suppose the best fix would be to add a new migration that checks whether the indexes are correct, and re-runs the 20161213 migration if not?

• services/api/app/models/collection.rb
  • Line 241: should @validation_timestamp be used to get the expiration timestamp?

I considered doing this to avoid the extra query, but validation isn't a prerequisite to calling signed_manifest_text -- e.g., collections#show would call signed_manifest_text without a validation.

• Line 450: Isn’t ‘|| false’ unnecessary?

Strangely, no: trash_at might be nil, and "nil && false" is nil (which makes some sense since you don't need to evaluate "x" in "nil && x")... so '|| false' is necessary to ensure self.is_trashed is either true or false.

• Line 456: I find the repeated conditionals a little difficult to read, do you think this would be clearer?
  [...]

Yes, updated.

• Line 465, 466: If trash_at is nil and delete_at is not nil, then the error message is confusing, because (at least for me) it only mentions the case for when delete_at is nil. Maybe change it to “delete_at should be set when trash_at is set, and should be nil otherwise”?

Yes, updated (but with "must" instead of "should").

• sdk/go/arvados/collection.go
  • Line 24: Should this be IsTrashed/is_trashed instead of IsTrash/is_trash?

Yes, oops.

• I think it would be clearer if in the tests instead of referring to the fixture collections by uuid, we do it using their fixture label. If the issue is that the default scope avoid this, maybe some comment besides the UUID describing what type of collection is being referred to can be helpful to improve readability.
  • services/api/test/functional/arvados/v1/collections_controller_test.rb Lines 978, 979, 936, 946, 967
  • services/api/test/unit/collection_test.rb Lines 473, 492

Indeed, updated. Added comments about default scope in the fixture file.

Thanks for the updates on fixture recalling, it's a lot clearer now and will help us as a reminder for future test writings. I searched if there was some way to tell the test suite to get unscoped fixtures but couldn't find any reference about that.
Please check services/api/test/unit/collection_test.rb:473, I suppose that commented line is not needed anymore.
With that, lgtm.