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

filipefurtad0 · 2024-04-30T11:24:13Z

We were missing tests to cover the scenario in which a variant on cart gets out of stock.

What? Why?

Contributes to Review and migrate legacy checkout specs #10958

The PR checks that the correct redirect occurs, if a given variant is out of stock and the user proceeds or re-starts checkout.

I chose to follow this reasoning: the PR implements the test as shared examples for guest and logged user, in the guest_spec.rb, the fastest of all checkout specs. This keeps the code close and easier to read/maintain - albeit a bit out of context, as we do log in (within the guest spec 🙈 ) - I hope this is acceptable.

What should we test?

green build

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

Adds out of stock check as helper

filipefurtad0 · 2024-04-30T11:41:05Z

Regarding the first commit - adding the out of stock check - I'm thinking there are two possibilities:

add the check to each checkout step, on the respective files (as it done), or
add the check to each checkout step, all on one file (maybe easier to maintain, and less lines of code?)

There is no huge difference between the spec runtimes, taken from knapsack dashboard:

but perhaps the details_spec.rb is a bit faster... in case reviewers find it better to add it all to one spec. Or even the guest_spec, which would not be very coherent. Not sure. (edit: then again, maybe the guest spec would be the best option.)

As shared examoples, in guest_spec.rb

This can be squashed with the first commit of this PR

All test cases are covered within the respective checkout step specs

rioug

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.
👍

rioug · 2024-05-06T01:09:15Z

spec/system/consumer/checkout/summary_spec.rb

+  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))
+  }


Was this missing before ?

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

mkllnk · 2024-05-07T03:51:38Z

spec/support/checkout_helper.rb

@@ -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)


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?

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

mkllnk · 2024-05-07T03:52:47Z

spec/system/consumer/checkout/details_spec.rb

+        variant.on_demand = false
+        variant.on_hand = 0
+        variant.save!


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

mkllnk · 2024-05-07T03:56:59Z

spec/system/consumer/checkout/summary_spec.rb

      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


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?

mkllnk · 2024-05-07T04:01:51Z

spec/system/consumer/checkout/payment_spec.rb

+    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


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.

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 🙏

mkllnk · 2024-05-07T04:04:00Z

spec/system/consumer/checkout/guest_spec.rb

+        if session == "logged"
+          login_as(user)
+        end


Instead of this conditional, you could just do the login in a context block outside of the shared examples.

Updates checkout tests

0f80aca

Adds out of stock check as helper

filipefurtad0 self-assigned this Apr 30, 2024

filipefurtad0 added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Apr 30, 2024

filipefurtad0 marked this pull request as draft April 30, 2024 11:24

Adds out of stock check as helper

729c5f0

As shared examoples, in guest_spec.rb

filipefurtad0 force-pushed the legacy_checkout_migrate_checkout_spec branch from 0d4bd68 to 729c5f0 Compare April 30, 2024 13:18

filipefurtad0 added 2 commits April 30, 2024 14:22

Reverts changes on checkout steps specs

34bde1c

This can be squashed with the first commit of this PR

Removes legacy checkout spec

0583888

All test cases are covered within the respective checkout step specs

filipefurtad0 mentioned this pull request Apr 30, 2024

Review and migrate legacy checkout specs #10958

Open

8 tasks

filipefurtad0 marked this pull request as ready for review April 30, 2024 13:32

filipefurtad0 changed the title ~~Updates checkout tests~~ Updates checkout tests to cover for out of stock line item Apr 30, 2024

filipefurtad0 changed the title ~~Updates checkout tests to cover for out of stock line item~~ Updates checkout tests to cover for out of stock variant Apr 30, 2024

rioug approved these changes May 6, 2024

View reviewed changes

mkllnk approved these changes May 7, 2024

View reviewed changes

mkllnk merged commit dd4fe86 into openfoodfoundation:master May 7, 2024
50 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

filipefurtad0 commented Apr 30, 2024 •

edited

filipefurtad0 commented Apr 30, 2024 •

edited

rioug left a comment

rioug May 6, 2024

filipefurtad0 May 6, 2024 •

edited

mkllnk May 7, 2024

filipefurtad0 May 7, 2024

mkllnk May 7, 2024

mkllnk May 7, 2024

mkllnk May 7, 2024

filipefurtad0 May 7, 2024

mkllnk May 7, 2024

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

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

Conversation

filipefurtad0 commented Apr 30, 2024 • edited

What? Why?

What should we test?

Release notes

Dependencies

Documentation updates

filipefurtad0 commented Apr 30, 2024 • edited

rioug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filipefurtad0 commented Apr 30, 2024 •

edited

filipefurtad0 commented Apr 30, 2024 •

edited

filipefurtad0 May 6, 2024 •

edited