Arvados

Should test "change owner from legit #{o_class} to legit #{new_o_class} owner" say new_o = new_o_class.create?

If possible, I think it might improve readability if destroy_permission_links and its callback declaration lived in the same place. In other words, either define destroy_permission_links in the HasUuid module, or declare its before_destroy callback in ArvadosModel.

Heads-up: Rails 4 split has_many(dependent: :restrict) into two separate values. Might be good to figure out which one you prefer and be ready to make the switch when the time comes.

There's trailing whitespace in can_be_an_owner.rb. (That header comment that just says "Protect" is also a little enigmatic. :) )

Thanks.

Brett Smith wrote:

Should test "change owner from legit #{o_class} to legit #{new_o_class} owner" say new_o = new_o_class.create?

Yes, fixed.

If possible, I think it might improve readability if destroy_permission_links and its callback declaration lived in the same place. In other words, either define destroy_permission_links in the HasUuid module, or declare its before_destroy callback in ArvadosModel.

Yes, fixed (moved to HasUuid).

Heads-up: Rails 4 split has_many(dependent: :restrict) into two separate values. Might be good to figure out which one you prefer and be ready to make the switch when the time comes.

Saw that. Unfortunately it seems there's no (documented) option that works in both rails3 and rails4 so we just have to notice that tests are failing in rails4, and change to :restrict_with_exception.

There's trailing whitespace in can_be_an_owner.rb. (That header comment that just says "Protect" is also a little enigmatic. :) )

Ah yes. Wrote the rest of the sentence.

Now @ fb8dd22

Tom Clegg wrote:

Brett Smith wrote:

Heads-up: Rails 4 split has_many(dependent: :restrict) into two separate values. Might be good to figure out which one you prefer and be ready to make the switch when the time comes.

Saw that. Unfortunately it seems there's no (documented) option that works in both rails3 and rails4 so we just have to notice that tests are failing in rails4, and change to :restrict_with_exception.

That sounds good to me. I thought it would be good to have a migration plan in mind, but I'm fine with something this rare being manual. Everything looks good to me; please go ahead and merge this. Thanks.