Reorganize Stock::SimpleCoordinator for improved debugging

[BenMorganIO]

Summary

This is all about making it easier for a developer to debug issues when they get the dreaded

     Failure/Error: order.next!

     StateMachines::InvalidTransition:
       Cannot transition state via :next from :address (Reason(s): We are unable to calculate shipping rates for the selected items.)

The first step we wanted to take was to make the instance variables accessible to the developer. A developer's journey to this issue is likely going to be discovering Spree::Order#create_proposed_shipments is what's failing. Then discovering that it builds an instance of this class. After that, they're going to want to start looking into the data that is set. Since they're all instance variables, they would likely want to be able to access some of the data.

However, what the developer will soon realize is that it's the shipping rates that are holding back the shipments from being validated. So the real issue likely lies in an empty array being returned from:

shipment.shipping_rates = Spree::Config.stock.estimator_class.new.shipping_rates(package)

If this is the issue, we'll want them to be able to easily recreate the packages the same way the class builds the packages. We've reorchestrated how the packages are built into a build_packages method and stored it there. I've also taken the time to simplify what build_shipments does so they can utilize that code as well.

One other issue I noticed was that there was a global Spree::StockLocation.all. In a normal store, there will likely not be many, but in a marketplace, there could be thousands - so let's give them a method to easily make that code more intelligent if needed.

I hope this work also makes it easier for the developer to extend this class.

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.

[codecov]

Codecov Report

Merging #5249 (9f11959) into main (568f4ed) will increase coverage by 0.00%.
Report is 22 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 9f11959 differs from pull request most recent head 6355850. Consider uploading reports for the commit 6355850 to get more accurate results

@@           Coverage Diff           @@
##             main    #5249   +/-   ##
=======================================
  Coverage   88.71%   88.71%           
=======================================
  Files         563      563           
  Lines       13890    13896    +6     
=======================================
+ Hits        12322    12328    +6     
  Misses       1568     1568           

Files Changed	Coverage Δ
core/app/models/spree/stock/simple_coordinator.rb	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[waiting-for-dev]

Thanks for taking the time to improve this and submitting your collaboration ❤️ I don't see any breaking change and I'm sure that will help other developers!

[kennyadsl]

👍 Nice, and now those variables are easier to deprecate if we need to change or eliminate them one day.

[kennyadsl]

@BenMorganIO I saw that the coverage decreased a bit with this PR (see failing check). I think it's related to all the new public methods added that are never tested. Maybe it can be fixed "easily" by testing the #initialize method and checking that those attributes contain the expected value. Would you mind giving this approach a try?

[jarednorman]

I'm not in favour of adding to the API of our objects for debugging purposes, but I've spent enough time debugging this class that I'm willing to go along regardless. 😄

That said, is there a way to make it explicit that these methods are not part of the official public API of this class? I would rather we not end up going through a deprecation cycle for a bunch of methods that are public only for debugging purposes when this class inevitably changes.

[BenMorganIO]

@jarednorman we could prefix them with an underscore? That seems to tell devs not to trust that piece of the API from remaining consistent.

[jarednorman]

Rather than use underscores or something, we can just make it explicit.

Suggested change

	attr_reader :order, :inventory_units, :splitters, :stock_locations,
	:filtered_stock_locations, :inventory_units_by_variant, :desired,
	:availability, :allocator, :packages
	attr_reader :order
	# This class is notoriously opaque. To aid in debugging, we've provided
	# the following methods. These are not considered part of the public API
	# of this class and may be removed without deprecation in a future
	# version of Solidus. Do not use them in production code.
	attr_reader :inventory_units, :splitters, :stock_locations,
	:filtered_stock_locations, :inventory_units_by_variant, :desired,
	:availability, :allocator, :packages

[waiting-for-dev]

@api private should also do the trick 🙂

[jarednorman]

🙏🏻 Perfect!

[kennyadsl]

@BenMorganIO did you see this comment about using @api private?

[waiting-for-dev]

@BenMorganIO, what do you think about just using @api private? This one is very close to being merged 🙂

[waiting-for-dev]

nvm, I did it myself 🙂

@jarednorman, are you ok with merging it now?