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

Fix compatibility between Mongoid::Paranoia and Mongoid::Slug #165

Merged
merged 2 commits into from
May 18, 2014

Conversation

johnnyshields
Copy link
Member

Currently, when Mongoid Paranoia deletes a document, it's _slugs field value remains in the DB. This can cause the unique index to throw an error when saving a new record with the same slug.

In order to fix this, we must unset the _slugs field when a paranoid doc is destroyed, and reset the slug when it is restored.

This PR includes the following changes:

  • Add support for callbacks around the restore method, if not natively supported by the Mongoid::Paranoia implementation (simi/mongoid-paranoia supports it, mongoid-paranoia gem and Mongoid 3 do not.)
  • Revert test mongoid-paranoia repo to 'simi/mongoid-paranoia'. Seems currently there are two competing mongoid-paranoia repos (see here: Duplicated unmaintained work ream88/mongoid-paranoia#1)
  • Rename set_slug method to apply_slug. The are new set_slug! and unset_slug! methods which literally call Mongoid set and unset.

Currently, when Mongoid Paranoia deletes a document, it's `_slugs` field value remains in the DB. This can cause the unique index to throw an error when saving a new record with the same slug.

In order to fix this, we must unset the `_slugs` field when a paranoid doc is destroyed, and reset the slug when it is restored.

Includes the following changes:

- Add support for callbacks around the `restore` method, if not natively supported by the Mongoid::Paranoia implementation (simi/mongoid-paranoia supports it, mongoid-paranoia gem and Mongoid 3 do not.)

- Revert test mongoid-paranoia repo to 'simi/mongoid-paranoia'. Seems currently there are two competing mongoid-paranoia repos (see here: ream88/mongoid-paranoia#1)

- Rename `set_slug` method to `apply_slug`. The are new `set_slug` and `unset_slug` methods which literally call Mongoid `set` and `unset`.
@digitalplaywright
Copy link
Collaborator

The change looks reasonable, but it currently fails on Mongoid 4. Have you had a chance to take a look at the cause? It may be related to the change of Mongoid Paranoia version in the Gemfile.

- Add paranoid specs for both permanent and non-permanent slug cases
- Add better handling of edge case of re-saving destroyed paranoid documents
- Rename set_slug, unset_slug etc to set_slug!, etc. (add ! to method name)
- Update CHANGELOG.md and README.md
@johnnyshields
Copy link
Member Author

Issue was not related to Mongoid Paranoia gem; I had forgot to rerun specs for Mongoid 4 on local. Issues were:

  1. An edge case when you resave a paranoid document that has been destroyed (on Mongoid 4, slug was being rebuilt, when it is expected to stay nil)
  2. I had renamed a method and forgot to update specs.

Anyway, seeing edge case (1) has led me to a slightly better implementation which I've committed, and specs are passing.

@digitalplaywright
Copy link
Collaborator

LGTM. Thanks!

I was not aware of @simi's work on Mongoid Paranoia, but after taking a look it seems like simi actively maintains his gem and has significantly more traction. I agree with using simi/mongoid-paranoia for now.

digitalplaywright added a commit that referenced this pull request May 18, 2014
Fix compatibility between Mongoid::Paranoia and Mongoid::Slug
@digitalplaywright digitalplaywright merged commit d5b767c into mongoid:master May 18, 2014
@johnnyshields johnnyshields deleted the paranoia-v2 branch May 18, 2014 16:28
@johnnyshields
Copy link
Member Author

Yeah there's currently some discussion about use of the mongoid-paranoia gem name. Trying to sort it out here: ream88/mongoid-paranoia#1

@snmaynard
Copy link

I'd love to see this released. Just came to the readme to see how to integrate with paranoia and the readme implies that it already is, which was a little misleading. Is there any reason its not already launched? Anything I can help with to get this pushed?

@digitalplaywright
Copy link
Collaborator

We intend to do some changes to the index, and we have already performed some improvements on master so instead of making the user migrate twice we want them to do it max once. So we are essentially waiting for this issue to be resolved: #172

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.

3 participants