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

Remove solidus deprecations #120

Merged

Conversation

kennyadsl
Copy link
Member

This PR removes all the deprecation warning currently present when running specs against solidus master. There's a commit for each warning, so it will be easier to read and review.

Copy link
Member

@spaghetticode spaghetticode left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you @kennyadsl 👍

I have one question about having the I18n namespace in the views, then I noticed that commit 0293d9f includes a suggestion for a future possible improvement, what about creating an issue, so we don't lose track of it?

@@ -1,10 +1,10 @@
<!-- insert_bottom '[data-hook="localization_settings_body"]' -->
<div class="form-group">
<label for="supported_locales_">
<%= Spree.t(:'i18n.supported_locales') %>
<%= I18n.t(:'spree.i18n.supported_locales') %>
Copy link
Member

Choose a reason for hiding this comment

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

What about using ActionView::Helpers::TranslationHelper#t by dropping the I18n namespace in the ERB view code?

@kennyadsl
Copy link
Member Author

Issue opened: #121, thanks for your suggestion. Also, I think you are right about changing I18n.t to just t in views. I will get that done soon.

This option has been deprecated and will be removed in
future versions of Solidus.
In favor of a standard link_to helper.
It changed some time and changes have not been reflected here
causing visual issues.
Which is now deprecated. It has been replaced by setting the
preference directly on the configuration. Sometimes stubbing
is not the right thing to do, especially in specs that are
testing preferences modification.
This removes the deprecation warning we were having calling
Spree::Product#destroy in specs.

Once it has been converted in Spree::Product#discard, the callbacks
are no longer executed by default on translations and we needed to
call discard_all explicitely.

This commit also adds discard support to product translations
This is the only model (with Spree::Product) that has
paranoia/discard enabled. This commit does not change the
behavior but adds some specs to describe what's the expected one:
when a shipping method is soft-deleted, translations are kept.

A future improvement could be adding the deleted_at column to
that translation table (as it's done with produts) and adding
discard to that model in order to mark as discarded the
translations associated to a discared shipping method.
It can lead to false positive.
@kennyadsl kennyadsl force-pushed the kennyadsl/remove-deprecations branch from 87799e4 to fb89fa2 Compare January 28, 2020 10:19
@kennyadsl kennyadsl merged commit f8f4e30 into solidusio-contrib:master Jan 28, 2020
@kennyadsl kennyadsl deleted the kennyadsl/remove-deprecations branch January 28, 2020 15:17
@aldesantis aldesantis removed their request for review January 29, 2020 09:09
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.

2 participants