https://dev.arvados.org/https://dev.arvados.org/favicon.ico?15576888422014-05-07T15:13:53ZArvadosArvados - Idea #2762: Protect database integrity wrt owner_uuidhttps://dev.arvados.org/issues/2762?journal_id=102922014-05-07T15:13:53ZTom Cleggtom@curii.com
<ul><li><strong>Target version</strong> changed from <i>2014-05-07 Storing and Organizing Data</i> to <i>2014-05-28 Pipeline Factory</i></li></ul> Arvados - Idea #2762: Protect database integrity wrt owner_uuidhttps://dev.arvados.org/issues/2762?journal_id=103702014-05-07T16:20:15ZTom Cleggtom@curii.com
<ul><li><strong>Assigned To</strong> set to <i>Tom Clegg</i></li></ul> Arvados - Idea #2762: Protect database integrity wrt owner_uuidhttps://dev.arvados.org/issues/2762?journal_id=104552014-05-11T03:04:06ZTom Cleggtom@curii.com
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li></ul> Arvados - Idea #2762: Protect database integrity wrt owner_uuidhttps://dev.arvados.org/issues/2762?journal_id=105052014-05-14T13:17:55ZBrett Smithbrett.smith@curii.com
<ul></ul><p>Should <code>test "change owner from legit #{o_class} to legit #{new_o_class} owner"</code> say <code>new_o = new_o_class.create</code>?</p>
<p>If possible, I think it might improve readability if <code>destroy_permission_links</code> and its callback declaration lived in the same place. In other words, either define <code>destroy_permission_links</code> in the <code>HasUuid</code> module, or declare its <code>before_destroy</code> callback in <code>ArvadosModel</code>.</p>
<p>Heads-up: Rails 4 split <code>has_many(dependent: :restrict)</code> into <a href="http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#method-i-has_many" class="external">two separate values</a>. Might be good to figure out which one you prefer and be ready to make the switch when the time comes.</p>
<p>There's trailing whitespace in <code>can_be_an_owner.rb</code>. (That header comment that just says "Protect" is also a little enigmatic. :) )</p>
<p>Thanks.</p> Arvados - Idea #2762: Protect database integrity wrt owner_uuidhttps://dev.arvados.org/issues/2762?journal_id=105082014-05-14T14:09:14ZTom Cleggtom@curii.com
<ul></ul><p>Brett Smith wrote:</p>
<blockquote>
<p>Should <code>test "change owner from legit #{o_class} to legit #{new_o_class} owner"</code> say <code>new_o = new_o_class.create</code>?</p>
</blockquote>
<p>Yes, fixed.</p>
<blockquote>
<p>If possible, I think it might improve readability if <code>destroy_permission_links</code> and its callback declaration lived in the same place. In other words, either define <code>destroy_permission_links</code> in the <code>HasUuid</code> module, or declare its <code>before_destroy</code> callback in <code>ArvadosModel</code>.</p>
</blockquote>
<p>Yes, fixed (moved to HasUuid).</p>
<blockquote>
<p>Heads-up: Rails 4 split <code>has_many(dependent: :restrict)</code> into <a href="http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#method-i-has_many" class="external">two separate values</a>. Might be good to figure out which one you prefer and be ready to make the switch when the time comes.</p>
</blockquote>
<p>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 <code>:restrict_with_exception</code>.</p>
<blockquote>
<p>There's trailing whitespace in <code>can_be_an_owner.rb</code>. (That header comment that just says "Protect" is also a little enigmatic. :) )</p>
</blockquote>
<p>Ah yes. Wrote the rest of the sentence.</p>
<p>Now @ <a class="changeset" title="2762: Fix wrong class used in test case." href="https://dev.arvados.org/projects/arvados/repository/arvados/revisions/fb8dd22df54d0c8f87e97f41e1a1677741a47892">fb8dd22</a></p> Arvados - Idea #2762: Protect database integrity wrt owner_uuidhttps://dev.arvados.org/issues/2762?journal_id=105122014-05-14T15:13:53ZBrett Smithbrett.smith@curii.com
<ul></ul><p>Tom Clegg wrote:</p>
<blockquote>
<p>Brett Smith wrote:</p>
<blockquote>
<p>Heads-up: Rails 4 split <code>has_many(dependent: :restrict)</code> into <a href="http://api.rubyonrails.org/classes/ActiveRecord/Associations/ClassMethods.html#method-i-has_many" class="external">two separate values</a>. Might be good to figure out which one you prefer and be ready to make the switch when the time comes.</p>
</blockquote>
<p>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 <code>:restrict_with_exception</code>.</p>
</blockquote>
<p>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.</p> Arvados - Idea #2762: Protect database integrity wrt owner_uuidhttps://dev.arvados.org/issues/2762?journal_id=105252014-05-14T19:20:11ZAnonymous
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Resolved</i></li><li><strong>% Done</strong> changed from <i>80</i> to <i>100</i></li></ul><p>Applied in changeset arvados|commit:ea9592e911a3ccdbbbfeed8812b67e968fe1cc5f.</p>