Skip to content

Fix item_cost for split shipments - #6663

Open
senemsoy wants to merge 1 commit into
solidusio:mainfrom
SuperGoodSoft:senem/fix-shipment-item-cost
Open

senemsoy wants to merge 1 commit into
solidusio:mainfrom
SuperGoodSoft:senem/fix-shipment-item-cost

Conversation

@senemsoy

@senemsoy senemsoy commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This fixes #2919. We just cleaned up the work done in nebulab#18 and now this is ready for review.

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

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.

@github-actions github-actions Bot added the changelog:solidus_core Changes to the solidus_core gem label Oct 6, 2026
@codecov

codecov Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.48%. Comparing base (9aa5ebd) to head (2bacc86).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6663   +/-   ##
=======================================
  Coverage   92.47%   92.48%           
=======================================
  Files        1065     1065           
  Lines       21541    21543    +2     
=======================================
+ Hits        19921    19923    +2     
  Misses       1620     1620           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@senemsoy
senemsoy force-pushed the senem/fix-shipment-item-cost branch 2 times, most recently from 1edf548 to 2c6b205 Compare October 20, 2026 18:59
@senemsoy senemsoy changed the title Sum Spree::ShippingManifest::ManifestItem.item_cost Fix item_cost for split shipments Oct 20, 2026
@senemsoy senemsoy changed the title Fix item_cost for split shipments Fix item_cost for split shipments Oct 20, 2026
@senemsoy
senemsoy marked this pull request as ready for review October 20, 2026 20:43
@senemsoy
senemsoy requested a review from a team as a code owner October 20, 2026 20:43

@jarednorman jarednorman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you also update the commit message to properly explain the issue that we're fixing with this commit? There's no mention of the bug, split shipments, or issue 2919. Also (just a nit) the commit should be worded so that it says what it does, not what it is "is going to" do.

Comment thread core/app/models/spree/shipping_manifest.rb Outdated
Comment thread core/spec/models/spree/shipment_spec.rb Outdated
@senemsoy
senemsoy force-pushed the senem/fix-shipment-item-cost branch 2 times, most recently from c43927b to 96fc5b0 Compare October 21, 2026 16:41
@senemsoy
senemsoy requested a review from jarednorman October 21, 2026 16:42
Currently when a line item's units are split across shipments,
`item_cost` returns the full line item total for every shipment that has
some of those units, instead of only the part that is in that shipment.

Now we calculate shipment's `item_cost` from the units it actually
contains.

Fixes solidusio#2919

Co-authored-by: Alistair Norman <alistair@super.gd>
@senemsoy
senemsoy force-pushed the senem/fix-shipment-item-cost branch from 96fc5b0 to 2bacc86 Compare October 23, 2026 19:54
@jarednorman
jarednorman requested a review from a team October 23, 2026 20:18

@mamhoff mamhoff left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we make the struct a class at this point?

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.

Spree::Shipment#item_cost is wrong for split shipments

4 participants