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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Updates checkout tests to cover for out of stock variant #12423

Merged
merged 4 commits into from
May 7, 2024
Merged

Updates checkout tests to cover for out of stock variant #12423

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0f80aca
Updates checkout tests
filipefurtad0 Apr 30, 2024
729c5f0
Adds out of stock check as helper
filipefurtad0 Apr 30, 2024
34bde1c
Reverts changes on checkout steps specs
filipefurtad0 Apr 30, 2024
0583888
Removes legacy checkout spec
filipefurtad0 Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 8 additions & 0 deletions spec/support/checkout_helper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,4 +66,12 @@ def place_order
click_on "Complete order"
expect(page).to have_content "Back To Store"
end

def out_of_stock_check(step)
Copy link
Member

Choose a reason for hiding this comment

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

I find the method name ambiguous. Does it check that something is out of stock or that nothing is out of stock?

When a test helper is mainly asserting expectations, I use that in the name. What about expect_out_of_stock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Naming is indeed a whole science on its own. Thanks for that tip 馃檹

visit checkout_step_path(step)

expect(page).not_to have_selector 'closing', text: "Checkout now"
expect(page).to have_selector 'closing', text: "Your shopping cart"
expect(page).to have_content "An item in your cart has become unavailable"
end
end
12 changes: 12 additions & 0 deletions spec/system/consumer/checkout/details_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,18 @@
visit checkout_path
end

context "when a line item is out of stock" do
before do
variant.on_demand = false
variant.on_hand = 0
variant.save!
Copy link
Member

Choose a reason for hiding this comment

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

Did you try variant.update!(on_demand: false, on_hand: 0)?

end

it "returns me to the cart with an error message" do
out_of_stock_check(:details)
end
end

context "when no selecting a shipping method" do
before do
fill_out_details
Expand Down
13 changes: 12 additions & 1 deletion spec/system/consumer/checkout/payment_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
ship_address_id: nil, state: "cart",
line_items: [create(:line_item, variant:)])
}

let(:fee_tax_rate) { create(:tax_rate, amount: 0.10, zone:, included_in_price: true) }
let(:fee_tax_category) { create(:tax_category, tax_rates: [fee_tax_rate]) }
let(:enterprise_fee) { create(:enterprise_fee, amount: 1.23, tax_category: fee_tax_category) }
Expand Down Expand Up @@ -58,6 +57,18 @@
visit checkout_path
end

context "when a line item is out of stock" do
before do
variant.on_demand = false
variant.on_hand = 0
variant.save!
end

it "returns me to the cart with an error message" do
out_of_stock_check(:payment)
end
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 noticed that you follow this context pattern quite strictly. It's seen as good style by many people. I personally find that it's good for larger tests but find it much easier to write short tests in a simple it block in this style:

it "alerts me about out of stock items" do
  clear_stock_for_variant
  visit step
  expect_out_of_stock_message
end

Some people follow this clear three part structure for tests: setup, action, expectation. Just FYI, you don't have to change anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that makes sense as well. I understand having the setup within the context block is more code lines - I feel it made my reading easier, when set up in this way. But I totally see the redundancy, for more experienced people.

I'll try to keep this in mind for next time. Thanks again 馃檹


context "payment step" do
let(:order) { create(:order_ready_for_payment, distributor:) }

Expand Down
17 changes: 17 additions & 0 deletions spec/system/consumer/checkout/summary_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@
create(:shipping_method, require_ship_address: true,
name: "A Free Shipping with required address")
}
let!(:payment_with_fee) {
create(:payment_method, distributors: [distributor],
name: "Payment with Fee", description: "Payment with fee",
calculator: Calculator::FlatRate.new(preferred_amount: 1.23))
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this missing before ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this is missing in master.

The payment method used for this order currently comes from line 16, when the distributor is created:
let(:distributor) { create(:distributor_enterprise, charges_sales_tax: true) }

We're currently not checking payment fees appear on the summary step - we probably should...
Yes we are - I've checked this but totally forgot, in the meantime, this scenario is covered here (payment_spec), so because of that, we don't need to do that setup in the summary spec:

openfoodnetwork/spec/system/consumer/checkout/payment_spec.rb

Line 74 in 84da904

context "with a transaction fee" do

It's a bit weird to be testing "logged" example in the guest_spec.rb but it makes sense to have all these test next to the shared_example code, so I think that's an acceptable trade of.

Thank you for validating this approach @rioug


before do
add_enterprise_fee enterprise_fee
Expand All @@ -51,6 +56,18 @@
visit checkout_path
end

context "when a line item is out of stock" do
before do
variant.on_demand = false
variant.on_hand = 0
variant.save!
end

it "returns me to the cart with an error message" do
out_of_stock_check(:summary)
end
end
Copy link
Member

Choose a reason for hiding this comment

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

Just noting that we first visit the checkout, then something goes out of stock and then we visit another page. Visits are often expensive in specs. Could the first visit be skipped to save time?


context "summary step" do
let(:order) {
create(:order_ready_for_confirmation, distributor:)
Expand Down