Disallow changes of canceled order #7287
Conversation
|
|
||
| expect(response.status).to eq(200) | ||
| expect(inventory_units_for(variant).size).to eq(1) | ||
| it "doesn't adjusts stock when adding a variant" do |
| }.to_not change { existing_variant.reload.on_hand } | ||
| end | ||
|
|
||
| it "doesn't adjusts stock when removing a variant" do |
| id: order.shipments.first.to_param | ||
| context "for canceled orders" do | ||
| before do | ||
| expect(order.cancel).to eq true |
There was a problem hiding this comment.
I'm not a big fan of asserting things in the test setup phase. It feels a bit like defensive and unconfident programming and it adds up to the build time. If cancel didn't work we would notice because the test would fail, right?
There was a problem hiding this comment.
I'm not a big fan of asserting in the setup either. But I do want to know if something goes wrong here and I don't think that this assertion slows the tests down much.
If cancel doesn't work then the order might be in an unknown state with unknown behaviour and we don't know if that would pass or fail the test. I wish I could rely on it to work. My experience is though that it can be tricky to get the test setup right and then things don't behave as expected.
The best solution would probably be to add a new factory: create(:cancelled_order). Another time...
| @@ -28,6 +29,10 @@ def set_order_id | |||
| @adjustment.order_id = parent.id | |||
| end | |||
|
|
|||
| def skip_changing_canceled_orders | |||
| redirect_to admin_order_adjustments_path(@order) if @order.canceled? | |||
There was a problem hiding this comment.
It feels like a flash message might be helpful here to give some user feedback?
There was a problem hiding this comment.
I added a commit for this.
Codecov Report
@@ Coverage Diff @@
## master #7287 +/- ##
=======================================
Coverage 89.70% 89.71%
=======================================
Files 649 649
Lines 18865 18873 +8
=======================================
+ Hits 16923 16931 +8
Misses 1942 1942
Continue to review full report at Codecov.
|
|
Hey @mkllnk , Thanks for the awesome work and the detailed steps on how to test this. Going though them, step-wise:
After this, going back to the previous two tabs: ii) Editing or creating new adjustments displays the warning below, and takes no effect - OK
So maybe on point iii) there would be room for improvement. I'll create an issue to prevent deleting of adjustments, if an order was cancelled on a different tab - which is in a way an edge case. But I think this brings plenty of value, so we should merge it 👍 Moving to ready to go. |
This commit is best viewed ignoring whitespaces.
We decided to disallow changing canceled orders in a way that would affect stock or totals.
We have a PR already that removes the UI for this when the order is canceled. Implementing it on controller-side makes sure that it doesn't happen accidentally if the user has multiple tabs open.
What? Why?
Closes #7166.
Complements #7228 and #7265.
Updating cancelled orders messes with stock and payment amounts. So we decided to not allow that. Previous PRs removed the UI but here I'm also putting guards in the controllers so that this doesn't happen accidentally or via the API.
What should we test?
Release notes
Changelog Category: User facing changes
It's not possible to change the contents of cancelled orders any more to avoid inconsistencies.