search

brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.04k stars 355 forks source link

check scope_changed? again if it was false in the same transaction #328

Closed shfung closed 3 months ago

shfung commented 5 years ago

If @scope_changed is set to false in a transaction, it will never run the check_scope logic in the same transaction

brendon commented 5 years ago

Hi @shfung, I don't think this is the way to solve this problem. The tests are all failing at the moment by the looks. This also generates heaps of warnings (see the travis output). I think we're checking for the defined status of the variable on purpose as the logic doesn't support it being changed once it's been set. This is from memory. It's been a while since I implemented that.

shfung commented 5 years ago

Sorry the first implementation was rusty because I didn't run against older version of Rails.

From my understanding, it seems that the @scope_changed is memorized and reused because we dont want to run the add_to_list_* methods multiple times in a single transaction. The current approach seems to set @scope_changed to the result of scope_changed? the first time it's computed, and then sequentially it's set to false in the add_to_list_* methods if it's true, and then it stays false until transaction is commit.

However, I think a less strict and more desirable approach is that @scope_changed can only be set to true once during a single transaction. i.e. It can be false the first few times we check scope_changed?, and once it becomes true, it stays true. And the internal_scope_changed method is modified to return false once @scope_changed becomes true, so that the add_to_list_* methods won't execute multiple times in a single transaction.

brendon commented 5 years ago

@shfung, that sounds about right (though I've forgotten the details since I implemented all that). There is probably a better way to do it I agree :)

If you can come up with something that still works (tests pass) and makes logical sense then we'll look to merge it :)

Could you explain this bit?

&& !changes[scope_name.to_s][0].nil?

Isn't that redundant given the first part of the check?

brendon commented 5 years ago

Ping @shfung :)

shfung commented 5 years ago

Sorry, have been really busy and didn't remember this PR. Our orgs has been using the patch for a while now because the tests passed for the Rails version we are using and even the failing tests seem to be only because of the large amount of deprecation messages that killed the log. I'll try to find some time in the future to revisit this PR since it's been a while, and I dont remember the details of it at first glance. Feel free to close this for now if you dont want to have a pending PR hanging around

brendon commented 5 years ago

Thanks for the update @shfung :) I'll leave this open in case you have a chance in the future to get back to it :)

Flixt commented 1 year ago

This is probably "fixed" with #407

brendon commented 1 year ago

@shfung, can you verify this for us? :)

brendon commented 3 months ago

Closing due to inactivity.