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

Adds a way to prevent the default_scope when desired #239

Open
wants to merge 2 commits into
base: rails4
Choose a base branch
from

Conversation

hannesfostie
Copy link

Sometimes setting the default_scope is not desired. This PR will allow you to add an option to acts_as_paranoid to never set the default_scope in a model.

@hannesfostie
Copy link
Author

This branch is kind of a proof of concept with a very minimal test. I'm going to use this version in our project to see what happens and if everything behaves the way I expect it to.

It's also a WIP as I figure out what's required to make this behave properly. See my last WIP commit. Do you prefer not to have this as a PR already? Should I make an issue instead while I work on it?

To give some background:

We currently use a lib we wrote ourselves but decided that reinventing the wheel is probably a bit over the top. I started by ripping out our implementation, added paranoia, and started fixing tests. One of our associations (a has_many through with inverse_of) was giving us problems, in the sense that deleted_blog_post_comment.blog was nil, whereas deleted_blog_post_comment.blog_post.blog was not.

I could have changed all these but then realized we actually don't have a default_scope right now, so instead of carefully adding with_deleted where needed I decided to give this a go. We generally don't remove deleted objects from our queries, so it made no sense to have that default scope in there in the first place.

I hope that makes sense. Happy to listen to suggestions while I see if this works for us!

@radar
Copy link
Collaborator

radar commented Jun 5, 2015

I have considered making the default_scope OFF by default, so you have to be explicit about finding only deleted records. In my opinion, that would be better.

But then associations break. I spent hours looking at Active Record code (ew) to figure out how to get around this and I couldn't figure it out. So I gave up.

I will only merge this PR if others are interested in this feature. I don't want to go adding features that nobody else is interested in. This is mainly because you're adding this feature for yourself, and then if I merged it I would be responsible for maintaining more code; not something that I want to do.

@hannesfostie
Copy link
Author

@radar I understand, and I appreciate you taking the time to have a look.

How can we figure out if more people could use this?

Do you have some old code where you turned the default scope off by default? I'd be happy to give it a go as well. If not, do you remember vaguely what you did and what broke? I don't immediately understand what you describe as the problem.

@jhawthorn
Copy link
Collaborator

I'm quite interested in this, it would be nice to have the means to selectively disable parts of paranoia's functionality. Could be very helpful when migrating away from it to tracking soft deletions manually.

I'll try to take a look later this week to see what would need to be done.

@radar
Copy link
Collaborator

radar commented Jun 11, 2015

Here is the code that I was helping with to disabling paranoia by default: https://github.com/oshiho3/paranoia/tree/remove_default_scope

It caused issues around the associations and I couldn't think of a good way to fix it. I believe I got stuck on this test:

def test_default_scope_for_has_many_through_relationships

You're welcome to adapt that code for your needs. Good luck!

@hannesfostie
Copy link
Author

@radar after reading the test, here's what I found:

The breaking line is #303:

assert_equal 0, employee.employers.paranoia.count

I believe this test fails because the classes have an association that doesn't really make sense. I changed the associations as such:

class Employer < ActiveRecord::Base
  acts_as_paranoid
  has_many :jobs
  has_many :current_jobs, -> { where :deleted_at => nil }, :class_name => 'Job'
  has_many :employees, :through => :current_jobs
end

class Employee < ActiveRecord::Base
  acts_as_paranoid
  has_many :jobs
  has_many :current_jobs, -> { where :deleted_at => nil }, :class_name => 'Job'
  has_many :employers, :through => :current_jobs
end

The tests weren't all fixed right away of course, because they reflect the current/old way of thinking about these associations.

That said, I wonder if we aren't shooting ourselves in the foot by using this real world example (jobs, employers, employees). The way I "fixed" the associations makes complete sense because when a job is no longer around, you don't have an employer. You just have past a employer.

I need to think about this again in another context to see if we can make this work. Happy to hear your thoughts as well!

@hannesfostie
Copy link
Author

Thought about it some more this morning with the Patient <-> Physician through Appointment example from the RoR guides, as well as a (Lego)Castle <-> Block through Component example and no matter how I try to think about it, I still seem to come to the same conclusion:

You need to either use #merge in your query to only select the non-deleted objects of your through table, or you have to add a second association where this scope is active. It's something you just need to be aware of, but you should know your domain logic anyway so I'm not sure if that is a problem.

@hannesfostie
Copy link
Author

@radar I'd love your input on my previous comment. How do you feel about it?

@aganov
Copy link

aganov commented Dec 17, 2015

+1

1 similar comment
@tagrudev
Copy link

+1

@rbr
Copy link
Contributor

rbr commented Feb 21, 2016

Is this equal to what #301 added?

@hannesfostie
Copy link
Author

It looks like it!

Sadly I am no longer using paranoia in any of my projects, and don't have the bandwidth to properly follow up with this and get things up to date. I think this PR can be closed in favor of the other one.

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

Successfully merging this pull request may close these issues.

None yet

7 participants