Soft-delete ShippingMethod translations

[kennyadsl]

As we are doing with Product translations, we should also soft-delete shipping method 's translations.

At the moment, we are just keeping them around and they will be restored automatically if the shipping is undiscarded. This works but it's not ideal since they should be marked as discarded as well.

We can improve this extension by just adding the deleted_at column to the spree_shipping_methods_transations table, and adding discard methods to that table as it's currently done with Spree::Product (in its decorator).

Ref: 0293d9f

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[zcallanan]

Looking into implementing this and I've encountered an issue with the setup. I've forked, cloned, and run bundle install per contributing. Running bundle exec rake test_app results an InvalidRepository error. It appears that Octokit is not parsing the repository name from the origin URL.

rake aborted!
Octokit::InvalidRepository: "" is invalid as a repository identifier. Use the user/repo (String) format, or the repository ID (Integer), or a hash containing :repo and :user keys.

Octokit Error.txt

[kennyadsl]

@zcallanan 👋 , please take a look at solidusio/solidus_dev_support#163. It's a long-term fix but in the referenced PRs there's a quick solution that could be applied here as well. Let me know if that works!

[zcallanan]

Following the above I replaced:

s.metadata["source_code_uri"] = 's.homepage if s.homepage'

with:

s.metadata["source_code_uri"] = 'https://github.com/zcallanan/solidus_globalize'

Running bundle exec rake test_app now results in the following error:

rake aborted!
Don't know how to build task 'test_app' (See the list of available tasks with `rake --tasks`)

Running rake --tasks results in the following error. Bundle install provided the 3.0.0 alpha version - Gemfile.lock: solidus_core (= 3.0.0.alpha)

rake aborted!
Gem::MissingSpecError: Could not find 'solidus_core' (>= 2.0, < 3) among 476 total gem(s)
Checked in 'GEM_PATH=/Users/astolot/.gem/ruby/2.6.0:/Users/astolot/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0', execute `gem env` for more information
/Users/astolot/code/zcallanan/solidus_globalize/Rakefile:3:in `<top (required)>'

Caused by:
Gem::MissingSpecError: Could not find 'solidus_core' (>= 2.0, < 3) among 476 total gem(s)
Checked in 'GEM_PATH=/Users/astolot/.gem/ruby/2.6.0:/Users/astolot/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0', execute `gem env` for more information
/Users/astolot/code/zcallanan/solidus_globalize/Rakefile:3:in `<top (required)>'

Caused by:
LoadError: cannot load such file -- solidus_dev_support/rake_tasks
/Users/astolot/code/zcallanan/solidus_globalize/Rakefile:3:in `<top (required)>'

[kennyadsl]

That rake task is recently changed, it's now bundle exec rake extension:test_app. If you can share where did you find that command it would be great so we can update the documentation with the new version. Thanks!

[zcallanan]

It's found on line 71 of the CONTRIBUTING.md in this project.

The test application was created with the new command. One thing of note was I received a number of deprecation warnings (two examples follow):

`DEPRECATION WARNING: SolidusSupport::EngineExtensions::Decorators is deprecated! Use SolidusSupport::EngineExtensions instead. (called from class:Engine at /Users/astolot/code/zcallanan/solidus_globalize/lib/solidus_globalize/engine.rb:9)

DEPRECATION WARNING: SolidusSupport.solidus_gem_version is deprecated and will be removed in solidus_support 1.0. Please use Spree.solidus_gem_version instead. (called from module:Admin at /Users/astolot/code/zcallanan/solidus_globalize/app/decorators/controllers/solidus_globalize/spree/admin/resource_controller_decorator.rb:16)`

As an aside, I noticed what looks like a typo on line 8 of the resource_controller_decorator:

retunr if parent_data.blank?

[zcallanan]

I tried running the command bundle exec rspec spec (line 73 of CONTRIBUTING.md) before making any code changes and received a number of deprecation warnings along with a FactoryBot DuplicateDefinition error:

`Provide a CODECOV_TOKEN environment variable to enable Codecov uploads
Omitted Deprecation Errors

An error occurred while loading spec_helper.
Failure/Error: require 'solidus_dev_support/rspec/feature_helper'

FactoryBot::DuplicateDefinitionError:
Factory already registered: address
#./spec/spec_helper.rb:12:in `<top (required)>'

Run options: include {:focus=>true}`

[kennyadsl]

You can safely ignore the deprecation warnings. Thanks for reporting that typo, can you fix it in a separate PR or even just open a new issue to track it.

[kennyadsl]

An error occurred while loading spec_helper.
Failure/Error: require 'solidus_dev_support/rspec/feature_helper'

This should be fixed now that I've released the last SolidusDevSupport version. Can you please delete the Gemfile.lock and the test app and try again? Sorry for the inconvenience, this should be the last thing.

[zcallanan]

I'll open a separate PR for the typo.

Running bundle exec rspec spec now results in an ActionDispatch error (full attached):

`All examples were filtered out; ignoring {:focus=>true}

Randomized with seed 11014

An error occurred in a before(:suite) hook.
Failure/Error: super(route_key, options, recall).map(&:dup)

NoMethodError:
undefined method `map' for #ActionDispatch::Journey::Formatter::RouteWithParams:0x00007faf8a23e510
Did you mean? tap

#/Users/astolot/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/routing-filter-0.6.3/lib/routing_filter/adapters/rails.rb:30:in `block in generate'

#/Users/astolot/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/routing-filter-0.6.3/lib/routing_filter/chain.rb:15:in run'

ActionDispatch error.txt

[kennyadsl]

That happens for some reason running specs against the last released version of Solidus (2.11), see here.

I think you can ignore that failure to resolve this one, or take a look at fix that as well in another PR, up to you!

[zcallanan]

Working through this task, and I have a question below. From the description:

1. Create a migration to add deleted_at to spree_shipping_methods_translations
2. Add discard methods to ./spree/shipping_method_decorator.rb as it is done with Spree::Product decorator

The product translation methods include discard handling for slugs as well as testing of variant callbacks in the method spec. How exact a match should this be to the Product decorator? Should a slug and variant support be added to shipping method translations in order to align with Product translation, or is this task just to add and handle deleted_at at time of soft deletion? My assumption has been the latter, but I wanted to confirm. :)

[kennyadsl]

Since Shipping Methods have no slug and association with variants, you are right, adding them does not make a lot of sense in this context and is not needed. 👍

[zcallanan]

When starting work on this issue I noted that there was already a test for shipping method translation soft deletes, and it's very similar to the product soft delete test.

While investigating how the Discard gem operates, I noticed that only the shipping_method and product are discarded in their tests, and then reloaded. The translations are handled through the after_discard callback and the call of the discard_all method within the decorator.

What I'm curious about is if you look at the return of discard_all, it produces a record set where the translation has a deleted_at timestamp, but if you check the same translation following the shipping method or product discard (in the test), it does not have a deleted_at timestamp. I believe this is because discard_all instantiates each record, and this is reflected in the translation in the set having a different memory address from the translation address in the test.

Shipping Method Decorator, discard_all return:
[#<Spree::ShippingMethod::Translation id: 1, spree_shipping_method_id: 1, locale: "en", created_at: "2021-01-17 16:42:18.879787000 +0000", updated_at: "2021-01-17 16:42:18.883688000 +0000", name: "UPS Ground", deleted_at: "2021-01-17 16:42:18.883502000 +0000">] ---- translation_object: #<Spree::ShippingMethod::Translation:0x00007fcbfe6d9530>

Shipping Method Test output of translation following shipping_method discard:
#<Spree::ShippingMethod::Translation id: 1, spree_shipping_method_id: 1, locale: "en", created_at: "2021-01-17 16:42:18.879787000 +0000", updated_at: "2021-01-17 16:42:18.879787000 +0000", name: "UPS Ground", deleted_at: nil> ---- translation_object: #<Spree::ShippingMethod::Translation:0x00007fcbfe518cc8>

This prevents you from checking whether a shipping method's translation has been discarded following a shipping_method.discard in the test, as shipping_method.translations.first.discarded? returns false. Replacing the use of discard_all with translations.each(&:discard) resolves this by eliminating the instantiation, but may be unnecessary if testing the shipping_method discard is enough here, as is done in the product decorator.

[aldesantis]

@zcallanan have you tried reloading the translations after discarding them?

translations.discard_all
translations.reload

[zcallanan]

That clears up my confusion, thanks! My assumption was that the deleted_at value would be set on discard, before the reload, but that's not the case. Checking the deleted_at value after the reload shows the correctly set deleted_at. Additionally a translation reload isn't necessary, if you look at the translation record after the shipping_method.reload it has the deleted_at set, and the test passes.

Decorator:

after_discard do
    # Delete associated records
    translations.discard_all
end

Test the added deleted_at is correctly set on discard:

it "translation deletion and reload sets deleted_at value" do
    soft_deleting
    expect(shipping_method.translations.first.deleted_at).not_to be_nil
end