Ensure db:seed runs before spree_sample:load #4907

lsizani · 2023-02-06T15:28:54Z

Summary

When running bundle exec rake spree_sample:load against an empty database, an exception is raised in db/samples/tax_rates.rb on account of missing a Spree::Zone with the name "North America.

In this PR we:

Add db:seed as a dependent task of spree_sample:load
Check that Spree::Zone "North America" is present in db/samples/tax_rates.rb before running the rest of the seed file. This makes it consistent with db/samples/shipping_methods.rb

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

The following are not always needed:

📖 ~~I have updated the README to account for my changes.~~
📑 I have documented new code with YARD.
🛣️ I have opened a PR to update the guides.
✅ I have added automated tests to cover my changes.
📸 I have attached screenshots to demo visual changes.

waiting-for-dev

Thanks a lot for your collaboration, @lsizani 🙌 🙌 It's a great and long-due improvement 🙂

I left some suggestions. We can probably take the occasion to leverage the new flexibility also in the installer by removing the && @load_seed_data check here:

solidus/core/lib/generators/solidus/install/install_generator.rb

Line 67 in 26a867f

@load_sample_data = options[:sample] && @run_migrations && @load_seed_data

core/app/models/spree/order_merger.rb

sample/lib/spree/sample.rb

sample/solidus_sample.gemspec

sample/spec/lib/load_sample_spec.rb

kennyadsl

Thanks! I left a comment. Also, I'm curious about what happens if db:seed and sample:load are run independently, one after the other. Is db:seed executed twice in that case?

sample/db/samples/tax_rates.rb

lsizani · 2023-02-07T11:21:42Z

Thanks! I left a comment. Also, I'm curious about what happens if db:seed and sample:load are run independently, one after the other. Is db:seed executed twice in that case?

It will run db:seed twice in that case. I checked on this and I can confirm that the current seeds can be run multiple times without raising errors or duplicating data.

kennyadsl · 2023-02-08T08:48:35Z

@lsizani do you know if we have any spec that verify we can run seeds multiple times without breaking or creating duplicate records?

lsizani · 2023-02-08T09:42:53Z

@lsizani do you know if we have any spec that verify we can run seeds multiple times without breaking or creating duplicate records?

We do not... But I'm sure I can write one

core/spec/generators/solidus/install/install_generator_spec.rb

adammathys

🎉 Great change! LGTM!

waiting-for-dev

Thanks, it's perfect! ❤️ Last ask, could you please rebase from master so that CI runs? 🙏

kennyadsl

I just noticed that with this change in the solidus installer, we are executing seeds twice: once during the seeds step and another during the samples step.

I think this is not ideal, no matter if it's safe to run seeds multiple times. Maybe we can merge populate_seed_data and load_sample_data to call a single rake command in case we want both?

kennyadsl · 2023-06-13T12:18:44Z

Hey @lsizani! I lost track of this PR, I didn't notice you updated it following my suggestions. I will re-review it now, in the meantime there are some conflict with the main branch to address, if you have time for a rebase. Thanks again!

kennyadsl

Left some questions, thanks again Lunga!

kennyadsl · 2023-06-13T12:21:43Z

sample/spec/lib/load_sample_spec.rb

+describe "Load seeds multiple times" do
+  it "doesn't duplicate records" do
+    4.times do
+      pid = fork { Spree::Core::Engine.load_seed }


Will this local variable be overridden at each cycle? Might this interfere with the process killing in the ensure block (it only always kills the last process but previous ones might still be executing)?

kennyadsl · 2023-06-13T12:23:06Z

sample/db/samples/shipping_methods.rb

@@ -3,8 +3,11 @@
 begin
 north_america = Spree::Zone.find_by!(name: "North America")
 rescue ActiveRecord::RecordNotFound
-  puts "Couldn't find 'North America' zone. Did you run `rake db:seed` first?"
-  puts "That task will set up the countries, states and zones required for Spree."
+  puts <<~TEXT


Love this DX, thanks!

kennyadsl · 2023-06-13T12:23:59Z

sample/spec/lib/load_sample_spec.rb

-    expect {
-      Spree::Core::Engine.load_seed
+    expect do
+      pid = fork { Spree::Core::Engine.load_seed }


Why do we need a separate process here instead of just running load_seed and load_sample one after the other?

lsizani requested a review from a team as a code owner February 6, 2023 15:28

github-actions bot added changelog:solidus_core Changes to the solidus_core gem changelog:solidus_sample Changes to the solidus_sample gem labels Feb 6, 2023

waiting-for-dev reviewed Feb 6, 2023

View reviewed changes

core/app/models/spree/order_merger.rb Show resolved Hide resolved

sample/lib/spree/sample.rb Show resolved Hide resolved

sample/solidus_sample.gemspec Show resolved Hide resolved

sample/spec/lib/load_sample_spec.rb Outdated Show resolved Hide resolved

lsizani force-pushed the make-sample-load-depend-on-seed branch from cecf524 to 858e37f Compare February 7, 2023 07:29

github-actions bot removed the changelog:solidus_core Changes to the solidus_core gem label Feb 7, 2023

kennyadsl reviewed Feb 7, 2023

View reviewed changes

sample/db/samples/tax_rates.rb Outdated Show resolved Hide resolved

lsizani force-pushed the make-sample-load-depend-on-seed branch from 858e37f to f20a8b1 Compare February 8, 2023 06:54

github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Feb 8, 2023

lsizani requested review from waiting-for-dev and kennyadsl and removed request for waiting-for-dev and kennyadsl February 8, 2023 07:01

waiting-for-dev reviewed Feb 8, 2023

View reviewed changes

core/spec/generators/solidus/install/install_generator_spec.rb Outdated Show resolved Hide resolved

lsizani force-pushed the make-sample-load-depend-on-seed branch 3 times, most recently from bb272d4 to e191991 Compare February 9, 2023 17:06

github-actions bot added the changelog:solidus Changes to the solidus meta-gem label Feb 9, 2023

adammathys approved these changes Feb 9, 2023

View reviewed changes

waiting-for-dev approved these changes Feb 10, 2023

View reviewed changes

lsizani force-pushed the make-sample-load-depend-on-seed branch 2 times, most recently from 397224f to 320235f Compare February 10, 2023 10:50

kennyadsl reviewed Feb 10, 2023

View reviewed changes

lsizani force-pushed the make-sample-load-depend-on-seed branch 2 times, most recently from 6a4fdff to 64a4c0f Compare February 16, 2023 08:07

Ensure db:seed runs before spree_sample:load

b28bf2b

Fix typos and add https on solidus.io

a14fcd0

lsizani force-pushed the make-sample-load-depend-on-seed branch from 64a4c0f to a14fcd0 Compare February 16, 2023 16:36

chrean assigned kennyadsl Jun 13, 2023

kennyadsl reviewed Jun 13, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure db:seed runs before spree_sample:load #4907

Ensure db:seed runs before spree_sample:load #4907

lsizani commented Feb 6, 2023 •

edited

Loading

waiting-for-dev left a comment •

edited

Loading

kennyadsl left a comment

lsizani commented Feb 7, 2023

kennyadsl commented Feb 8, 2023

lsizani commented Feb 8, 2023

adammathys left a comment

waiting-for-dev left a comment

kennyadsl left a comment

kennyadsl commented Jun 13, 2023

kennyadsl left a comment

kennyadsl Jun 13, 2023

kennyadsl Jun 13, 2023

kennyadsl Jun 13, 2023

Ensure db:seed runs before spree_sample:load #4907

Are you sure you want to change the base?

Ensure db:seed runs before spree_sample:load #4907

Conversation

lsizani commented Feb 6, 2023 • edited Loading

Summary

Checklist

waiting-for-dev left a comment • edited Loading

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

lsizani commented Feb 7, 2023

kennyadsl commented Feb 8, 2023

lsizani commented Feb 8, 2023

adammathys left a comment

Choose a reason for hiding this comment

waiting-for-dev left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl commented Jun 13, 2023

kennyadsl left a comment

Choose a reason for hiding this comment

kennyadsl Jun 13, 2023

Choose a reason for hiding this comment

kennyadsl Jun 13, 2023

Choose a reason for hiding this comment

kennyadsl Jun 13, 2023

Choose a reason for hiding this comment

lsizani commented Feb 6, 2023 •

edited

Loading

waiting-for-dev left a comment •

edited

Loading