Let people customise which shipping methods are available to customers on order cycles #9262

cillian · 2022-06-05T15:18:16Z

What? Why?

This adds a new step 4. Checkout options to editing order cycles. This only applies to order cycles for distributors e.g. sells = 'any'. For 'simple', direct sales, order cycles where :sells = 'own' all shipping methods are to be available by default like they are currently.

This is a bit different from the spec because updating step 3. Outgoing would have be done in Angular and that would have taken a lot longer. I believe the plan is to convert the edit order cycle form to StimulusJs as soon as possible anyway so it was quicker and easier to do this in plain Rails.

Note, the screenshot includes a 'back office only' shipping method which is disabled and cannot be selected. It also includes a shared shipping method which is shared by two enterprises.

The 'Ready for' and 'Customer instructions' fields haven't been moved yet. It might be best to do those in separate PRs are this one is already big enough.

This adds a :shipping_methods_customisable flag to the order cycles table in order to have a record of which order cycles which were created before this feature i.e. order cycles on which it was not possible to customise shipping methods. This excludes active or upcoming order cycles at the time of the migration because shipping methods on these should be customisable so the :shipping_methods_customisable flag will be set to true for these.

What should we test?

See Acceptance criteria & tests in #8974

It would be good to test the migration on a staging instance too.

Release notes

Changelog Category: User facing changes

Documentation updates

The Order Cycles (for hubs) page in the user guide will probably need to be updated.

mkllnk

Wow, great work! ✨

It's a huge chunk of work in one commit though. Would you mind splitting it up? For example, there are some refactors which make it harder to see which logic changed at the same time. And if you have the database migrations in a previous commit then reading the rest of the code makes more sense.

At this stage, I'm still happy to incorporate changes by rewriting the git history.

There are some style violations that need to be address as well.

Your work looks really good and thorough but I didn't read every line. My brain can't keep the whole thing in memory at once.

app/models/order_cycle.rb

app/models/order_cycle_shipping_method.rb

app/models/spree/shipping_method.rb

db/migrate/20220429092052_create_order_cycle_shipping_methods.rb

app/models/order_cycle.rb

lin-d-hop · 2022-06-14T07:49:23Z

So exciting to see this awesome work @cillian 🎉

cillian · 2022-06-17T13:15:54Z

👋 @mkllnk I have changed things so all shipping methods are available if none are specifically selected in the database, it's a lot simpler now. I broke it up into smaller commits too, but it might be good to squash some of them again later.

mkllnk

I reviewed most of this PR but didn't get quite through. I still made quite a few comments below. So maybe let me know what you think about that feedback and then we go from there. It's a big change. 😄

app/models/order_cycle.rb

app/models/spree/shipping_rate.rb

spec/models/spree/shipping_method_spec.rb

app/models/spree/shipping_method.rb

app/models/order_cycle_shipping_method.rb

app/controllers/base_controller.rb

app/helpers/admin/order_cycles_helper.rb

app/helpers/enterprises_helper.rb

app/models/order_cycle.rb

app/models/spree/shipping_method.rb

app/views/admin/order_cycles/checkout_options.html.haml

mkllnk

Great work! I know it's taking some time but I hope that you find it as satisfying as me to produce simpler, more maintainable code. I got a few more comments below.

app/models/order_cycle.rb

app/services/shop/order_cycles_list.rb

mkllnk · 2022-06-28T00:20:01Z

app/views/admin/order_cycles/checkout_options.html.haml

@@ -71,7 +71,9 @@
                  %li
                    = shared_payment_method.name
                    %p
-                      = "&mdash;<em>#{shared_payment_method.distributors.where(id: @order_cycle.distributor_ids).map(&:name).join(", ")}</em>".html_safe
+                      &mdash;
+                      %em


Do you need the space around the dash? Above, I added the > to the %em because I thought that you wanted to avoid the space. But the syntax is much easier if we omit it like here and have the space in there. I haven't checked how it looks and I'm a poor judge of that. Up to you. 😉

I think with the space is probably slightly better.

app/models/order_cycle.rb

app/helpers/admin/order_cycles_helper.rb

app/models/order_cycle_shipping_method.rb

cillian · 2022-06-29T14:19:27Z

@mkllnk 👍 definitely nice to see the size of this PR has been chopped down a good bit since the first draft.

app/services/shop/order_cycles_list.rb

app/services/order_cycle_form.rb

.rubocop_todo.yml

db/migrate/20220429092052_create_order_cycles_shipping_methods.rb

spec/services/order_cycle_form_spec.rb

app/models/order_cycle.rb

app/views/admin/order_cycles/checkout_options.html.haml

app/models/order_cycle.rb

mkllnk

💪

A few more comments.

db/migrate/20220429092052_create_order_cycles_shipping_methods.rb

mkllnk

Great! It looks all good to me. 😄

app/models/order_cycle.rb

cillian · 2022-07-08T13:08:14Z

@mkllnk great, thank you 🎉

jibees

What an awesome work you've done here! Congrats! 💪 🙏

Very well broken into atomic commit, nice comment on each one.

I don't see anything wrong, this looks good to me ;)

drummer83 · 2022-07-11T21:15:04Z

Hi @cillian,
I have started to test this amazing pull request. I know that many user are waiting for this functionality. 💪

This is a big and complex one. So I decided to take separate testing notes in a document here: https://docs.google.com/document/d/1UhiVRpqWcOZ0HvwDdTBVoGxSS0iiwSL1czd_lBkJE_M/edit?usp=sharing

So far everything seems good. I only was confused by the way the shared payment methods are displayed (see no. 4 in the document). I am asking our @openfoodfoundation/train-drivers-product-owners team to comment on this one.

I have to stop for now, there is still a lot more to test. This is just to let you know your contribution is very much appreciated and we are working on testing it! 👍

lin-d-hop · 2022-07-12T09:43:28Z

Thanks for all the dev, review and testing on this.

I'm afraid that when the spec changed half way through this introduced some differences in understanding between the spec and what was developed. This is totally my fault.

I'm going to go through the PR now to better understand where we are at and what's possible and then have a chat with @cillian to work out the best route forward. For now, let's pause.

Sorry to delay this everyone.

The spec was failing from time to time. I hope that this will fix it. ``` Failures: 1) As an administrator I want to create/update complex order cycles with a specific time creating an order cycle with full interface Failure/Error: fill_in 'order_cycle_outgoing_exchange_0_pickup_time', with: 'pickup time' Capybara::ElementNotFound: Unable to find field "order_cycle_outgoing_exchange_0_pickup_time" that is not disabled # ./spec/system/admin/order_cycles/complex_creating_specific_time_spec.rb:138:in `add_distributor_with_fees' # ./spec/system/admin/order_cycles/complex_creating_specific_time_spec.rb:66:in `block (2 levels) in <main>' # ./spec/system/support/cuprite_setup.rb:39:in `block (2 levels) in <main>' # -e:1:in `<main>' ```

…checked This means the code to set the initial value in the view template isn't needed.

…reating_specific_time_spec.rb Not sure if this is correct but it removes the Rubocop violations.

…form" This reverts commit 0dddaa6.

…tions form, any shared shipping methods are to be displayed for each distributor e.g. if they belong to multiple distributors they will be displayed multiple times

…ows instead of same one, the select all checkboxes still need to be added back later This is the new design from openfoodfoundation#9262 (comment)

…ing methods instead of order cycles to shipping methods

…hippingMethods Before if a shipping method was shared between multiple distributors it could only be disabled/enabled on that order cycle for all the distributors which have that shipping method e.g. you couldn't select that shipping method for one distributor but disable it for another.

…ones by default per distributor, not per all distributors

…ecord::Relation sometimes and Array other times Now it will return ActiveRecord::Relation consistently.

…into account Note, this doesn't test checking/unchecking some distributor shipping methods and not others because that is tested in order_cycles/complex_creating_time_specific_spec.rb.

… as :expect_fees_saved

…account that DistributorShippingMethods instead of ShippingMethods are connected to OrderCycles now

… the :schedule_ids was not specified in the parameters Before this schedules were being deleted when the Checkout Options step of editing order cycles was saved because this step wasn't sending the :schedule_ids parameter. It's more awkard for API users if they always need to explicity send the :schedule_ids parameter even if they don't want to add/remove any of the schedules, for example if they just want to update an order cycle name.

…ic_time_spec.rb

cillian · 2022-09-30T12:17:47Z

So, it looks like somehow the migration did not run. Should I delete this entry up 20220429092052 Create order cycles distributor shipping methods and re-stage?

@filipefurtad0 yes I think that should work. Although the old order_cycle_shipping_methods table might need to be deleted manually, there is no migration to remove it because the migration changed.

The other option would be to checkout the commit which contained the old migration and roll the migration back, then checkout the latest commit and re-run the migration again. That would remove the old table so you wouldn't need to delete it manually.

filipefurtad0 · 2022-09-30T12:21:15Z

Yaayy worked 🎉

Resuming testing,
Thanks for your help 🙌

filipefurtad0 · 2022-09-30T13:03:50Z

spec/system/admin/order_cycles/complex_creating_specific_time_spec.rb

-    find("body").send_keys(:escape)
+  def select_opening_and_closing_times
+    select_time("#order_cycle_orders_open_at", order_cycle_opening_time)
+    select_time("#order_cycle_orders_close_at", order_cycle_opening_time)


This is causing this spec to fail:
https://github.com/openfoodfoundation/openfoodnetwork/actions/runs/3158670190/jobs/5140966992

It should be order_cycle_closing_time.

It does not influence the PR, so I'll continue testing 👍

…time_spec.rb

filipefurtad0 · 2022-09-30T13:36:33Z

Ok! Everything looks good now:

Migration


openfoodnetwork=> select * from order_cycles_distributor_shipping_methods;
 order_cycle_id | distributor_shipping_method_id 
----------------+--------------------------------
           3805 |                            853
(1 row)

Which corresponds to:

Behavior and acceptance criteria

After the latest changes it is not possible to de-select all shipping methods, the guard for this works fine ✔️

Screencast.from.30-09-2022.14.30.26.webm

Tested in both legacy and split-checkout: the selected shipping methods appear on the corresponding sections ✔️
The design is resilient in edge cases like de-selecting shipping methods (as admin) during the customer checkout process; Once the customer sees the option to select a shipping method it then "sticks" to the order, checkout proceeds with no issues

I think these were the pending issues!

Thanks so much all the work and patience on this one @cillian
Merging.

cillian force-pushed the order-cycle-shipping-methods branch 4 times, most recently from a56d192 to 89d3e12 Compare June 6, 2022 15:52

cillian marked this pull request as ready for review June 6, 2022 15:53

cillian force-pushed the order-cycle-shipping-methods branch from 89d3e12 to a476e22 Compare June 6, 2022 21:43

mkllnk requested changes Jun 8, 2022

View reviewed changes

cillian force-pushed the order-cycle-shipping-methods branch 3 times, most recently from 7e9e396 to 53b6c15 Compare June 17, 2022 12:11

mkllnk requested changes Jun 20, 2022

View reviewed changes

cillian force-pushed the order-cycle-shipping-methods branch from c0ab024 to 2b1ad3a Compare June 20, 2022 20:27

mkllnk reviewed Jun 20, 2022

View reviewed changes

app/models/order_cycle.rb Outdated Show resolved Hide resolved

app/models/spree/shipping_method.rb Show resolved Hide resolved

app/views/admin/order_cycles/checkout_options.html.haml Outdated Show resolved Hide resolved

cillian force-pushed the order-cycle-shipping-methods branch from f45b73e to 0fdb5bd Compare June 25, 2022 12:07

mkllnk reviewed Jun 28, 2022

View reviewed changes

mkllnk reviewed Jun 30, 2022

View reviewed changes

db/migrate/20220429092052_create_order_cycles_shipping_methods.rb Outdated Show resolved Hide resolved

mkllnk approved these changes Jul 5, 2022

View reviewed changes

app/models/order_cycle.rb Outdated Show resolved Hide resolved

jibees approved these changes Jul 11, 2022

View reviewed changes

drummer83 self-assigned this Jul 11, 2022

drummer83 added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Jul 11, 2022

drummer83 removed their assignment Jul 12, 2022

drummer83 added the feedback-needed label Jul 12, 2022

mkllnk and others added 19 commits September 30, 2022 13:13

Spec "Select all" shipping methods field

3518d1b

Remove unused input name

26d5ea6

Make "Select all" translatable

195f1e1

Check 'Select all' checkboxes on page load if all its checkboxes are …

8e47949

…checked This means the code to set the initial value in the view template isn't needed.

Fix Rubocop Metrics/AbcSize and Layout/LineLength errors in complex_c…

32d16ea

…reating_specific_time_spec.rb Not sure if this is correct but it removes the Rubocop violations.

Revert "Use a more simple layout on the order cycle checkout options …

4b0b4ad

…form" This reverts commit 0dddaa6.

Remove 'shared' shipping method sections from order cycle checkout op…

a73b608

…tions form, any shared shipping methods are to be displayed for each distributor e.g. if they belong to multiple distributors they will be displayed multiple times

Implement new design with shipping and payment methods on different r…

65ee9e1

…ows instead of same one, the select all checkboxes still need to be added back later This is the new design from openfoodfoundation#9262 (comment)

Change join table in migration from order cycles to distributor shipp…

8c483f2

…ing methods instead of order cycles to shipping methods

Bring back select all checkboxes on order cycle checkout options form.

7e40ad3

OrderCycle#distributor_shipping_methods should return all attachable …

5207dbf

…ones by default per distributor, not per all distributors

Fix error from OrderCycle#distributor_shipping methods return ActiveR…

db92e9d

…ecord::Relation sometimes and Array other times Now it will return ActiveRecord::Relation consistently.

Adjust order_cycles/simple_spec.rb to take new Checkout Options step …

23f6901

…into account Note, this doesn't test checking/unchecking some distributor shipping methods and not others because that is tested in order_cycles/complex_creating_time_specific_spec.rb.

Define :oc with let so it is available globally to other methods such…

2827e07

… as :expect_fees_saved

Adjust order_cycles/complex_creating_specific_time_spec to take into …

6542b47

…account that DistributorShippingMethods instead of ShippingMethods are connected to OrderCycles now

Fix Metrics/AbcSize violation in order_cycles/complex_creating_specif…

f9e29f3

…ic_time_spec.rb

filipefurtad0 force-pushed the order-cycle-shipping-methods branch from 478a519 to f9e29f3 Compare September 30, 2022 12:13

filipefurtad0 requested changes Sep 30, 2022

View reviewed changes

Set closing time correctly in order_cycles/complex_creating_specific_…

acd4706

…time_spec.rb

sigmundpetersen removed the feedback-needed label Sep 30, 2022

filipefurtad0 approved these changes Sep 30, 2022

View reviewed changes

filipefurtad0 removed the pr-staged-au staging.openfoodnetwork.org.au label Sep 30, 2022

filipefurtad0 merged commit 4b279d2 into openfoodfoundation:master Sep 30, 2022

drummer83 mentioned this pull request Oct 5, 2022

Include step 4 (checkout options) when creating order cycles openfoodfoundation/ofn-UserGuide#35

Open

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let people customise which shipping methods are available to customers on order cycles #9262

Let people customise which shipping methods are available to customers on order cycles #9262

cillian commented Jun 5, 2022 •

edited

Loading

mkllnk left a comment

lin-d-hop commented Jun 14, 2022

cillian commented Jun 17, 2022

mkllnk left a comment

mkllnk left a comment

mkllnk Jun 28, 2022

cillian Jun 29, 2022

cillian commented Jun 29, 2022

mkllnk left a comment

mkllnk left a comment

cillian commented Jul 8, 2022

jibees left a comment

drummer83 commented Jul 11, 2022

lin-d-hop commented Jul 12, 2022

cillian commented Sep 30, 2022 •

edited

Loading

filipefurtad0 commented Sep 30, 2022

filipefurtad0 Sep 30, 2022

filipefurtad0 commented Sep 30, 2022

Let people customise which shipping methods are available to customers on order cycles #9262

Let people customise which shipping methods are available to customers on order cycles #9262

Conversation

cillian commented Jun 5, 2022 • edited Loading

What? Why?

What should we test?

Release notes

Documentation updates

mkllnk left a comment

Choose a reason for hiding this comment

lin-d-hop commented Jun 14, 2022

cillian commented Jun 17, 2022

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Jun 28, 2022

Choose a reason for hiding this comment

cillian Jun 29, 2022

Choose a reason for hiding this comment

cillian commented Jun 29, 2022

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk left a comment

Choose a reason for hiding this comment

cillian commented Jul 8, 2022

jibees left a comment

Choose a reason for hiding this comment

drummer83 commented Jul 11, 2022

lin-d-hop commented Jul 12, 2022

cillian commented Sep 30, 2022 • edited Loading

filipefurtad0 commented Sep 30, 2022

filipefurtad0 Sep 30, 2022

Choose a reason for hiding this comment

filipefurtad0 commented Sep 30, 2022

cillian commented Jun 5, 2022 •

edited

Loading

cillian commented Sep 30, 2022 •

edited

Loading