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

Conditionally turn off :dependent => :destory on FriendlyId::Slugs #724

Merged
merged 1 commit into from
Jan 13, 2016

Conversation

vkmita
Copy link
Contributor

@vkmita vkmita commented Jan 12, 2016

Hey guys!

This adds the ability not to delete historic slugs when the sluggable class is destroyed.

When using acts_as_paranoid on sluggable models, we aren't actually deleting the models when calling destroy. If we delete the slugs, we’re losing the history. We're also setting up a condition where a unique constraint ends up getting violated.

This is a revival of #268

@vkmita vkmita changed the title added the ability not to delete historic slugs when the sluggable class is destroyed Conditionally turn off :dependent => :destory on FriendlyId::Slugs Jan 12, 2016
@@ -190,6 +190,7 @@ module Base
# @yieldparam config The model class's {FriendlyId::Configuration friendly_id_config}.
def friendly_id(base = nil, options = {}, &block)
yield friendly_id_config if block_given?
friendly_id_config.options = options
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for me, it's better to follow existing concept of explicitly specified configuration options and not to add whole hash. By the way, this config is available only for history module. So, we should clarify this fact in method's documentation above.

@vkmita
Copy link
Contributor Author

vkmita commented Jan 12, 2016

Thanks @kimrgrey does this work?

@kimrgrey
Copy link
Contributor

@vkmita, cool! Looks much better to me 👍 And Travis CI said that everything is ok. I think it's good idea to add some tests for another policies, isn't it? We should at least add test to check that :destroy is used by default.

@parndt, what do you think about this PR? As for me it's useful and should be merged.

…ss is destroyed

When using acts_as_paranoid on a sluggable models, we aren't actually
deleting the models when calling destroy. If we delete the slugs,
we’re losing the history. We're alsosetting up a condition where a
 unique contstraint may end up being violated
@vkmita
Copy link
Contributor Author

vkmita commented Jan 12, 2016

I added a test that checks that :destroy is default. What other policies would you want tested?

@kimrgrey
Copy link
Contributor

@vkmita, it's enough for me. Let's wait a 👍 from another one contributor to merge it 😉

parndt added a commit that referenced this pull request Jan 13, 2016
Conditionally turn off :dependent => :destory on FriendlyId::Slugs
@parndt parndt merged commit 95a834b into norman:master Jan 13, 2016
@parndt
Copy link
Collaborator

parndt commented Jan 13, 2016

Thanks 👍 looks like @norman wanted this for a previous version of FriendlyId and this is a good implementation.

kimrgrey added a commit that referenced this pull request Jan 13, 2016
@benortiz
Copy link

Hello, I'm trying to use this in one of my models. I was originally using version 5.1, but updated to 5.2 beta just in case.

When I add :dependent => false as it is done in the test to my model, I get an error that says that dependent is not defined (NoMethodError - undefined methoddependent=' for #<#Class:0x007f83b9667da0:0x007f83b9667d50>:`.

my model has the line:

friendly_id :slug_candidates, use: [:slugged, :history], :dependent => false

There isn't a lot of documentation that I could find on this, so this is the best I could come up with. I've tried various rearrangements of this to no avail. Perhaps I need to do something in the friendly_id initializer?

If there's any further information I can provide, let me know.

Thanks in advance for your help.

@benortiz
Copy link

Ah, looks like it may have been my misunderstanding. I checked the beta and found out it wasn't included in that. I switched to master and everything worked as described. Thanks so much for an awesome gem!

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

Successfully merging this pull request may close these issues.

4 participants