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

Slugs should always find their sluggable, even when they are soft deleted by acts_as_paranoid #838

Merged
merged 2 commits into from
Oct 9, 2017

Conversation

padi
Copy link
Contributor

@padi padi commented Oct 4, 2017

Probably fixes #822, as the test case is likely what he has encountered. His report most probably required 2 things to be true:

  1. He used friendly_id ... :dependent => false
  2. friendly_id is used in a Rails 5.0 app

Previously, when a sluggable is soft deleted and a slug persists
(i.e. :dependent => false), slug can't find its corresponding
sluggable.

Additionally, in Rails 5.0, belongs_to_required_by_default has been
introduced to be true by default (unless changed back to false), therefore making Friendly::Slug
invalid with the error message 'Sluggable must exist'.

@padi
Copy link
Contributor Author

padi commented Oct 4, 2017

@padi padi force-pushed the slug-finds-paranoid-sluggable branch from a812a59 to e12cfc6 Compare October 6, 2017 02:30
…eted

... by acts_as_paranoid

Previously, when a sluggable is soft deleted and a slug persists
(i.e.  :dependent => false), slug can't find its corresponding
sluggable, which renders the slug invalid starting in Rails 5.0
with the error message 'Sluggable must exist'.

Based on [the related Rails issue
thread](rails/rails#18233),
either we set the main apps default by setting mattr_accessor
AR::Base#belongs_to_required_by_default back to false, or we
explicitly write our expectations in each call to belongs_to.
We opted for the latter since other gems, depending on the load order,
can still overwrite this global switch, which makes the validation
on FriendlyId::Slug inconsistent.
@padi padi force-pushed the slug-finds-paranoid-sluggable branch from e12cfc6 to d00f1a8 Compare October 6, 2017 02:31
@parndt
Copy link
Collaborator

parndt commented Oct 7, 2017

Hey, thanks for this! I'm uncomfortable adding the development dependency on acts_as_paranoid, but I think that we can get a working test by just reproducing how it works:

  • Set a default scope on the model which filters by deleted_at: nil
  • Update the record to set deleted_at to not nil

What do you think of these changes below?

diff --git a/friendly_id.gemspec b/friendly_id.gemspec
index 52aac43..883e20b 100644
--- a/friendly_id.gemspec
+++ b/friendly_id.gemspec
@@ -26,7 +26,6 @@ Gem::Specification.new do |s|
   s.add_development_dependency 'i18n'
   s.add_development_dependency 'ffaker'
   s.add_development_dependency 'simplecov'
-  s.add_development_dependency 'acts_as_paranoid'

   s.description = <<-EOM
 FriendlyId is the "Swiss Army bulldozer" of slugging and permalink plugins for
diff --git a/lib/friendly_id/slug.rb b/lib/friendly_id/slug.rb
index 8f549f8..9a6694c 100644
--- a/lib/friendly_id/slug.rb
+++ b/lib/friendly_id/slug.rb
@@ -3,7 +3,7 @@ module FriendlyId
   #
   # @see FriendlyId::History
   class Slug < ActiveRecord::Base
-    belongs_to :sluggable, :polymorphic => true, required: true
+    belongs_to :sluggable, :polymorphic => true

     def sluggable
       sluggable_type.constantize.unscoped { super }
diff --git a/test/history_test.rb b/test/history_test.rb
index 6d00915..3c95158 100644
--- a/test/history_test.rb
+++ b/test/history_test.rb
@@ -212,27 +212,25 @@ class DependentDestroyTest < HistoryTest
 end

 if ActiveRecord::VERSION::STRING >= '5.0'
-  class HistoryTestWithActsAsParanoid < HistoryTest
-    require 'acts_as_paranoid'
+  class HistoryTestWithParanoidDeletes < HistoryTest
     class ParanoidRecord < ActiveRecord::Base
-      acts_as_paranoid
       extend FriendlyId
       friendly_id :name, :use => :history, :dependent => false
+
+      default_scope { where(deleted_at: nil) }
     end

     def model_class
       ParanoidRecord
     end

-    test 'slug should have a sluggable even when soft deleted via acts_as_paranoid' do
+    test 'slug should have a sluggable even when soft deleted by a library' do
       transaction do
-        assert FriendlyId::Slug.belongs_to_required_by_default
-
         assert FriendlyId::Slug.find_by_slug('paranoid').nil?
         record = model_class.create(name: 'paranoid')
         assert FriendlyId::Slug.find_by_slug('paranoid').present?

-        record.destroy
+        record.update_attribute(:deleted_at, Time.now)

         orphan_slug = FriendlyId::Slug.find_by_slug('paranoid')
         assert orphan_slug.present?, 'Orphaned slug should exist'

Thanks!

@padi
Copy link
Contributor Author

padi commented Oct 8, 2017

@parndt took your suggestions in the next commit I just pushed. Thanks for the feedback, that is indeed a better way of implementing the soft deletion.

@parndt parndt merged commit 7fddca3 into norman:master Oct 9, 2017
@padi padi deleted the slug-finds-paranoid-sluggable branch October 9, 2017 01:54
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

Successfully merging this pull request may close these issues.

"Sluggable must exist" when using history on a paranoid model
2 participants