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

DRY paranoia and discard inclusion into models #3555

Merged
merged 2 commits into from
Mar 27, 2020

Conversation

cedum
Copy link
Contributor

@cedum cedum commented Mar 13, 2020

This PR DRYs the soft delete inclusion logic into a reusable concern.
This also deprecates .with_deleted and .only_deleted Paranoia' scopes, which will be removed in Solidus v3.

This has been extracted from the #3488 PR. After some feedback, I'm extracting the non-breaking bits into separate PRs, so we can merge them without having to wait for Solidus v3.
Once merged, I'll rebase and revisit #3488 to definitely remove Paranoia as a dependency from Solidus v3.

Checklist:

  • I have followed Pull Request guidelines
  • I have added a detailed description into each commit message
  • I have updated Guides and README accordingly to this change (if needed)
  • I have added tests to cover this change (if needed)
  • I have attached screenshots to this PR for visual changes (if needed)

@cedum cedum changed the title Dry soft delete DRY paranoia and discard inclusion into models Mar 13, 2020
@kennyadsl
Copy link
Member

Cool, I think we still have some occurrences of the newly deprecated methods, can you take a look at failing specs? 🙏

@cedum
Copy link
Contributor Author

cedum commented Mar 18, 2020

Ah, I missed that paranoia itself is also using (see here) the newly deprecated scopes (with_deleted and only_deleted) 🤦‍♂
It seems most of the failing cases are related to using really_destroy! in some specs. I'll try to find an alternative solution for these specs without removing the deprecations introduced here.

Extract soft delete gems (paranoia and discard) inclusion from models
into a reusable concern.
Copy link
Contributor

@DanielePalombo DanielePalombo left a comment

Choose a reason for hiding this comment

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

LGTM. 💪

core/spec/models/spree/variant_spec.rb Show resolved Hide resolved
Paranoia is being replaced with Discard. This deprecates with_deleted
and only_deleted Paranoia' scopes.
This also refactors/remove unnecessary #really_destroy! paranoia
method calls which depend on with_deleted scope, hence raises deprecation
warnings.
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

👍 Thanks, Dumi!

@kennyadsl kennyadsl added Code Review Needed changelog:solidus_core Changes to the solidus_core gem labels Mar 24, 2020
@aldesantis
Copy link
Member

@cedum this should now be good to merge. 🙂

@kennyadsl kennyadsl merged commit 68cad80 into solidusio:master Mar 27, 2020
@kennyadsl kennyadsl deleted the dry-soft-delete branch March 27, 2020 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:solidus_core Changes to the solidus_core gem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants