Remove call to private method #update_cancellations from OrderUpdater#recalculate_adjustments
#5633
Conversation
github-actions
bot
added
the
changelog:solidus_core
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5633 +/- ##
==========================================
- Coverage 88.56% 88.55% -0.01%
==========================================
Files 685 685
Lines 16408 16406 -2
==========================================
- Hits 14531 14528 -3
- Misses 1877 1878 +1 ☔ View full report in Codecov by Sentry. |
Unit cancel adjustments are always finalized, and will thus never be recalculated. Attempting to do so by un-finalizing them will result in a ZeroDivisionError. Let's not try.
mamhoff
force-pushed
the
remove-cancellation-recalculate-code-path
branch
from
075f11e to
9d249a3
Compare
mamhoff
changed the title
| @@ -114,7 +114,6 @@ def recalculate_adjustments | |||
| # http://www.boe.ca.gov/formspubs/pub113/ | |||
| update_promotions | |||
| update_taxes | |||
| update_cancellations | |||
There was a problem hiding this comment.
I'm thinking this may be a breaking change for stores that for some reason customized update_cancellations. The deprecation message would never show, since the method is not called anymore. So, I'm wondering if just deprecating would be a safer approach initially, and just remove it later... @mamhoff what's your opinion on this?
There was a problem hiding this comment.
As long as the thing goes away at some point. Sure, I'll leave it in.
There was a problem hiding this comment.
Actually... That makes our test suite extremely loud, to the point that we would not see any meaningful deprecation warnings. This warning is IMO not particularly meaningful, as I cannot imagine people having overridden this code path. If they have, they hopefully have at least one test for it where the deprecation message would show.
There was a problem hiding this comment.
It's a private method, I agree that we should not get to this level of safety for backward compatibility. Still, a mention in the changelog/upgrade guide will be needed.
There was a problem hiding this comment.
@kennyadsl how do we manage this kind of mentions now? In other words: I guess it's not going to show up automatically that we removed a private method, so how will we remember to add the information to the changelog?
Could simply changing the PR title to something like "Remove call to private method OrderUpdater# update_cancellations from #recalculate_adjustments be enough?
There was a problem hiding this comment.
I think that updating the description of the PR is enough, keeping in mind that whoever will release will take a look at the description to build the changelog. We might want to create a label if you think it's needed.
There was a problem hiding this comment.
@kennyadsl I agree the title change should suffice.
@mamhoff do you prefer to update the PR title personally or is it OK if we do it on your behalf?
There was a problem hiding this comment.
Please, do it on my behalf. You know best how you want this change to appear in the change log.
|
Curious: are there no specs for this? I have the feeling we can remove some specs, maybe they are just not failing. Did you already check, @mamhoff? |
There's no mention anywhere in the project of the name of the deprecated method, not in specs, nor is it called from anywhere else. Also, I've tried testing the behavior, and it's hard: It's a no-op, and if you make it some kind of op (by e.g. removing Pretty sure there's no tests for this. |
spaghetticode
changed the title
#update_cancellations from OrderUpdater#recalculate_adjustments
Summary
This removes the final occurrence of Spree::Adjustment#recalculate.
Order Cancellation adjustments are always finalized, and recalculating them is always a no-op. No need to carry the code path forward.
See https://github.com/solidusio/solidus/blob/main/core/app/models/spree/unit_cancel.rb#L27
and https://github.com/solidusio/solidus/blob/main/core/app/models/spree/adjustment.rb#L101
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: