search

openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.1k stars 712 forks source link

Review and migrate legacy checkout specs #10958

Open dacook opened 1 year ago

dacook commented 1 year ago

Follow on from enabling split checkout and,

We've removed the legacy checkout code, but there are still 12 specs that have been disabled as pending and need to be considered for migration to the new split checkout process. (see https://github.com/openfoodfoundation/openfoodnetwork/pull/10913/files/32b8d7201af4effc892f330848984d1f1db1ed05..9e6a7b2f8f51bf7f984339a8b3590757e026d9d6 )

There is also a concurrency spec that was removed, but could be re-instated and updated for the split checkout controller (see https://github.com/openfoodfoundation/openfoodnetwork/pull/10913/files/e4aaecb3d49136e162f3e961a360c47f83673766..73ed3c733a9d60a535cbcc2cdcf02147c0e2d5ce )

dacook commented 1 year ago

We probably should have done this before fully instating the split_checkout toggle!

RachL commented 7 months ago

@filipefurtad0 maybe we should add this one to the roadmap candidate, what do you think?

filipefurtad0 commented 7 months ago

Yes, indeed @RachL. There a a few tests here which need to be updated, and would be great to keep track of them in the roadmap :+1:

sigmundpetersen commented 7 months ago

I suggest we convert this an epic and open the individual specs as issues

sigmundpetersen commented 4 months ago

Hey @filipefurtad0 👋 Added epic label and moved to Dev Ready, same as we treat the other roadmap items

filipefurtad0 commented 4 months ago

Hey @sigmundpetersen :wave: Thank you for that :pray:

filipefurtad0 commented 3 months ago

For reference: split-checkout was fully released on v4.3.4. The legacy-checkout controllers:

   # When the split_checkout feature is disabled for the current user, use the legacy checkout
  constraints FeatureToggleConstraint.new(:split_checkout, negate: true) do
    get '/checkout', to: 'checkout#edit'
    put '/checkout', to: 'checkout#update', as: :update_checkout
    get '/checkout/:state', to: 'checkout#edit', as: :checkout_state
  end

were replaced by the current (split-)checkout controllers:

  get '/checkout', to: 'checkout#edit'

  constraints step: /(details|payment|summary)/ do
    get '/checkout/:step', to: 'checkout#edit', as: :checkout_step
    put '/checkout/:step', to: 'checkout#update', as: :checkout_update
  end

  # Redirects to the new checkout for any other 'step' (ie. /checkout/cart from the legacy checkout)
  get '/checkout/:other', to: redirect('/checkout')
filipefurtad0 commented 1 month ago

Most of this epic is concluded, however, I got stuck on the two last specs.

As mentioned in delivery circle, it's best to park this issue for now. It would only make sense to have system specs for Paypal and Stripe if we could combine VCR and a proxy to redirect real calls (i.e. puffing billy, which I have not managed to implement in the past). This concerns the file:

As for the request spec:

I've tried to address it, but got blocked. I'm not sure how to pass all Stripe parameters within params. So, it's perhaps not a bad idea to address this later on, although a full re-write of the spec would be needed, namely i) considering the new checkout flow and ii) replace the mock requests with real requests using VCR.

I'm unassigning myself for now.