Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scope_changed? problem with acts_as_paranoid #158

Closed
mtomov opened this issue Mar 9, 2015 · 6 comments
Closed

scope_changed? problem with acts_as_paranoid #158

mtomov opened this issue Mar 9, 2015 · 6 comments

Comments

@mtomov
Copy link

mtomov commented Mar 9, 2015

Hello,

Having both acts_as_list and acts_as_paranoid on a model with
acts_as_list scope: [:taxon_id, :deleted_at]

.. when I call destroy on an element from a list, I get a gap in the list of items instead of having the items below the deleted ones go a position up.

Here is the source of the problem:

>> Spree::Classification.find(4013).destroy
  SQL (1.4ms)  UPDATE "spree_suites_taxons" SET "updated_at" = '2015-03-09 12:48:55.344271', 
            "deleted_at" = '2015-03-09 12:48:55.344271' 
             WHERE "spree_suites_taxons"."id" = 4013

 SQL (0.6ms)  UPDATE "spree_suites_taxons" SET position = (position - 1) 
                       WHERE "spree_suites_taxons"."taxon_id" = 84 
                       AND      "spree_suites_taxons"."deleted_at" = '2015-03-09 12:48:55.344271' 
                       AND (position > 5)

If you pay attention to the last but one line "spree_suites_taxons"."deleted_at" = '2015-03-09 12:48:55.344271', you will notice how acts as lists's decrement_positions_on_lower_items is picking up the deleted_at scope from the previous query.

I think that all is happening in list.rb#check_scope, because swap_changed_attributes is looking for changes for both scopes:

>> c.scope_condition #=> {:taxon_id=>84, :deleted_at=>nil}
>> c.scope_name #=> "[:taxon_id, :deleted_at]"

Any suggestions? Thank you!

@pjungwir
Copy link

Hi @mtomov! I've had issues with combining acts_as_list and soft-delete, and I wrote up my solution for it here:

http://illuminatedcomputing.com/posts/2014/12/acts_as_list-with-soft-delete/

Basically I force deleted_at IS NULL to be part of the scope, which is almost what you're doing but not quite. With my approach, the scope excludes deleted items even after you set deleted_at. Hopefully this helps you!

@mtomov
Copy link
Author

mtomov commented Jul 29, 2015

👍 Good solution. Thank you!

@brendon
Copy link
Owner

brendon commented Jul 29, 2015

Indeed, and setting the scope as a string actually bypasses the check_scope process too if I remember rightly.

@pjungwir
Copy link

That's how the code looks to me @brendon ! One thing I've noticed about acts_as_list is that it seems to get confused when a list item moves from one container object to another (e.g. positions of 1, 2, 2, 3), but maybe that's just because I'm using strings for my scopes.

@brendon
Copy link
Owner

brendon commented Jul 30, 2015

I tend to be quite explicit and not rely on aal when it comes to moving things around. I use ancestry for tree structure and the two don't play too nicely together so I wrote my own moving methods (move to left of, right of and child of) that do use some of aal's internal methods but don't rely on the magic :)

@brendon
Copy link
Owner

brendon commented Apr 17, 2016

Looking at this again, we're expecting the scope attribute values to be the same for all items within a scope. Therefore you can't use a field like :deleted_at because this has a very specific value. You're basically saying you want to order items deleted at exactly the same time.

The alternative as I think you've already alluded to is to manually define the scope method to take into account wether the item is deleted or not (rather than when it was deleted). There's no problem with this; that's why it's possible to override scope_condition and scope_changed?. Here's an example of a complex one of mine:

  def scope_condition
    ['notice_area_id = ? AND ? >= CURDATE()', notice_area_id, end_date.to_s(:db)]
  end

  # A custom acts_as_list scope requires a custom scoped_changed? method
  def scope_changed?
    changed.include?('notice_area_id') ||
    changed.include?('end_date') && (
      changes['end_date'][0] >= Time.zone.now.beginning_of_day &&
      changes['end_date'][1] < Time.zone.now.beginning_of_day ||
      changes['end_date'][1] >= Time.zone.now.beginning_of_day &&
      changes['end_date'][0] < Time.zone.now.beginning_of_day
    )
  end

I'll close this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants