Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recalculate order after reimbursement creation #2711

Conversation

DanielePalombo
Copy link
Contributor

This PR is going to fix the issue #2606

The issue happens because after reimbursement creation the order is not recalculated.
To fix it we run order#recalculate in after_create callback, in this way order#payment_state is updated to new state.

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

It seems to fix the issue, but I think specs can be improved a bit, thanks!

@@ -19,6 +19,7 @@
let(:reimbursement) { Spree::Reimbursement.new(number: nil) }

before do
allow(reimbursement).to receive(:recalculate_order)
Copy link
Member

Choose a reason for hiding this comment

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

why stubbing this method is needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method was stubbed because the reimbursement in this spec does not receive the order, so the reimbursement creation failed.
Now I'm going to check that order is provided before run recalculate on it.

let(:customer_return) { create(:customer_return, line_items_count: 5) }

context "total is not assigned" do
before { expect(reimbursement).to receive(:calculated_total) { 100 } }
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this expectation into the it block. It makes specs easier to read and we are not DRY anything here since it's just applied to a single spec.


let(:reimbursement) { build(:reimbursement, order: customer_return.order) }

it "should assign total with the calculated_total result " do
Copy link
Member

Choose a reason for hiding this comment

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

should assign -> assigns

before { expect(reimbursement).not_to receive(:calculated_total) }
let(:reimbursement) { build(:reimbursement, total: 10, order: customer_return.order) }

it "should not change the total value" do
Copy link
Member

Choose a reason for hiding this comment

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

should not change -> does not change

expect(customer_return.order).to receive(:recalculate)

reimbursement.save
end
Copy link
Member

Choose a reason for hiding this comment

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

I'd remove the .after_create spec (it's a Rails responsibility to test it) in favor of a more high-level test that ensure that the order is properly recalculated after its creation.

@DanielePalombo DanielePalombo force-pushed the nebulab/recalculate-order-after-reimbursement-creation branch 3 times, most recently from b34b70e to b5d7715 Compare May 11, 2018 14:47
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

I have some concerns doing this in a AR callback

@@ -18,6 +18,8 @@ class IncompleteReimbursementError < StandardError; end
accepts_nested_attributes_for :return_items, allow_destroy: true

before_create :generate_number
before_create :calculate_total
after_create :recalculate_order
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure if a AR hook is the best place to do that calculation (that may include API requests and other funky stuff shops do to determine the order totals). Maybe we should move this into the perform! method.

Calculate total before creation if total is not provided
After reimbursement creation run recalculate on order to recalculate the states of order.
@DanielePalombo DanielePalombo force-pushed the nebulab/recalculate-order-after-reimbursement-creation branch from b5d7715 to f0a734b Compare June 1, 2018 12:12
@DanielePalombo
Copy link
Contributor Author

Hi @tvdeyen, I moved the callback in the controller. What do you think about it now?

@jacobherrington
Copy link
Contributor

Hey @DanielePalombo are you still working on getting this merged?

@DanielePalombo
Copy link
Contributor Author

Yes @jacobherrington, I'm waiting a response on it.

@kennyadsl kennyadsl merged commit e2ef677 into solidusio:master Dec 12, 2018
@aitbw aitbw mentioned this pull request Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants