openfoodnetwork
openfoodnetwork copied to clipboard
openfoodfoundation
Let people customise which shipping methods are available to customers on order cycles
What? Why?
Closes #8974
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 https://github.com/openfoodfoundation/openfoodnetwork/issues/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.
:wave: @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 :+1: definitely nice to see the size of this PR has been chopped down a good bit since the first draft.
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! 👍
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.
Added 'feedback-needed' until @lin-d-hop comes back to us with her thoughts.
Okay I have updated this to the simpler layout now. Order cycles with multiple distributors will look something like this....
And order cycles with a single distributor...
@cillian I did a bit of work on this but I'm running out of time to finish it. Here are the outstanding issues I found:
- The new organisation of the spec file means that our usual code style metrics apply. We can do a better job at splitting it all up.
- I found a bug with disabled fields. When you click on a disabled field then it's treated like a click on the field above, toggling the checkbox. We should add to the JS spec and solve that.
@filipefurtad0 I did a bit of spec work on this. Maybe you like to have a quick review.
@mkllnk thanks for fixing those bits up. I have added that extra JS spec and reworked the spec to remove the Metrics/AbcSize Rubocop errors now.
Hey @cillian @mkllnk ,
I've split this into the migration and the behavior in the app, so first:
Testing the migration
Some observations:
The new table establishes a correspondence between the order_cycle_id and the shipping_method_id :+1:
On this screen, you cannot not select at least one shipping method :+1:
Selecting all shipping methods -> empty table :+1:
Testing the behavior
For an order cycle with
- more than one distributor (below, left)
- and selecting some shipping methods, other than those provided by the respective distributors (below, middle)
- leads to a scenario similar to #8219 and #6137 :exclamation: (below, right)
I think this might derive from:
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.
I'm not sure we're ready to push this into prod. What do you think @openfoodfoundation/train-drivers-product-owners ?
Great catch @filipefurtad0!
This does need fixing but the logic of the best fix for this needs a little more thinking time. Curly.... :woozy_face:
Okay, here is another round of design for this. Again I'm so sorry for not spotting all of this when the implementation changed from settings within the distribution to settings on the Order Cycle. I think this will be the last round to get the feature through testing....
So this is specifically for the case of multiple distributors on one order cycle:
Acceptance Criteria:
- So we need to split this per Distributor after all :/
- When there is only one shipping method for a distributor the option should be greyed out.
- When there are multiple options but none is select the user should be prevented from saving - save buttons greyed out
- Ideally we will communicate the error to the user if they can't save, however error messaging in the order cycles is not very easy to understand. Since having multiple distributors is an edge case anyway I would say that just preventing saving is sufficient and we can come back to this error messaging when we remove Angular from OCs as we'll be re-implementing this UI then anyway. If anyone has a better suggestion please do share!
@cillian Thanks for your patience in getting this tricky feature through! Let me know if there is anything else I can help with :)
@lin-d-hop no problem, I'll make those adjustments. What should we do if a shipping method is shared between multiple distributors? Should we have a shared section like we had before? i.e.
Or should the shared shipping method be displayed in the row for each distributor it belongs to, so it would be repeated? And if the checkbox is checked for one distributor, the checkbox for the other distributor(s) it belongs is automatically checked/unchecked to match? Maybe that's not the cleanest solution.
Hi @cillian If the pick up method is shared by multiple distributors it should be repeated. So it should be possible for the same distribution method to be available for one distributor and not for another. Does that make sense? Does it fit with the data model you've created for this?
@lin-d-hop that makes sense, I have updated things now. I had to change the data model a bit. Instead of a table which links OrderCycles to Spree::ShippingMethods, there is now a table which links OrderCycles to DistributorPaymentMethods but it was almost a straight swap.
When there are multiple options but none is select the user should be prevented from saving - save buttons greyed out
I left this part a bit differently because previously when no shipping methods were selected it would default to automatically selecting all shipping methods as suggested by Maikel. It's neat because then any currently active order cycles at the time this PR gets deployed will automatically have all shipping methods selected on the order cycle rather than having none selected and appearing as invalid if someone presses save. Let me know if that doesn't work though.
Hi @cillian, Thanks for your continued work on this!
I started testing again and realized that I can't checkout with this PR staged (error 500). This seems independent from Split Checkout being activated and independent from the customer I chose.
There is an entry in Bugsnag which is probably related and I pasted the output of the browser console below. I hope this helps.
If you would like to have a look on our staging server and need support just let me know!
I will move this to In Dev. Thanks!
https://app.bugsnag.com/yaycode/openfoodnetwork-aus/errors/6328b543f90905000a180ad4?event_id=6328b54300973c8e16a30000&i=sk&m=nw
{
"Status": "500Internal Server Error",
"Version": "HTTP/2",
"Transferred": "1.82 KB (1.26 KB size)",
"Referrer Policy": "strict-origin-when-cross-origin",
"Request Priority": "Highest"
}
{
"Response Headers (577 B)": {
"headers": [
{
"name": "cache-control",
"value": "no-store"
},
{
"name": "content-type",
"value": "text/html; charset=utf-8"
},
{
"name": "date",
"value": "Mon, 19 Sep 2022 18:35:13 GMT"
},
{
"name": "expires",
"value": "Fri, 01 Jan 1990 00:00:00 GMT"
},
{
"name": "pragma",
"value": "no-cache"
},
{
"name": "referrer-policy",
"value": "strict-origin-when-cross-origin"
},
{
"name": "server",
"value": "nginx"
},
{
"name": "strict-transport-security",
"value": "max-age=63072000; includeSubDomains"
},
{
"name": "x-content-type-options",
"value": "nosniff"
},
{
"name": "x-content-type-options",
"value": "nosniff"
},
{
"name": "x-download-options",
"value": "noopen"
},
{
"name": "X-Firefox-Spdy",
"value": "h2"
},
{
"name": "x-permitted-cross-domain-policies",
"value": "none"
},
{
"name": "x-request-id",
"value": "6a64df22-42bd-4ff9-b9dd-06d37bfeeede"
},
{
"name": "x-runtime",
"value": "0.504622"
},
{
"name": "x-xss-protection",
"value": "1; mode=block"
},
{
"name": "x-xss-protection",
"value": "1; mode=block"
}
]
}
}
{
"Request Headers (983 B)": {
"headers": [
{
"name": "Accept",
"value": "text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8"
},
{
"name": "Accept-Encoding",
"value": "gzip, deflate, br"
},
{
"name": "Accept-Language",
"value": "en-US,en;q=0.5"
},
{
"name": "Connection",
"value": "keep-alive"
},
{
"name": "Cookie",
"value": "XSRF-TOKEN=_aJe7icU0rYXauKJo8gp9Y19fCuIPaxKgGggx4is8bYa2SGqRH0ln-nvEKFHKDWyx3zjhsVrRj8KRtNwW4MieA; _ofn_session_id=d82ab77af610170705370f38b64184fe; _pk_id.3.cd81=bbe1c4baa95487b2.1663612282.; _pk_ses.3.cd81=1; __stripe_mid=dea8a258-01eb-4a7c-b9c4-3136ba949d5965ed6a; __stripe_sid=90e40833-72ce-4763-9f8c-24a184541462f09dd8; cookies_consent=cookies_consent; _pk_id.3.32b3=6e76bf6af4f58b73.1663612438.; _pk_ses.3.32b3=1"
},
{
"name": "Host",
"value": "staging.openfoodnetwork.org.au"
},
{
"name": "Referer",
"value": "https://staging.openfoodnetwork.org.au/konrad-testhub-staging-au/shop"
},
{
"name": "Sec-Fetch-Dest",
"value": "document"
},
{
"name": "Sec-Fetch-Mode",
"value": "navigate"
},
{
"name": "Sec-Fetch-Site",
"value": "same-origin"
},
{
"name": "Sec-Fetch-User",
"value": "?1"
},
{
"name": "TE",
"value": "trailers"
},
{
"name": "Upgrade-Insecure-Requests",
"value": "1"
},
{
"name": "User-Agent",
"value": "Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:104.0) Gecko/20100101 Firefox/104.0"
}
]
}
}
Thanks for testing this @drummer83 .
@cillian Unfortunately, we've recently noticed that some specs in our build were not running - this is now fixed.
So, in addition to bugsnag, maybe rebasing this PR onto current master will reveal some failing tests and help you move forward.
We prefer a rebase over a merge because it results in a cleaner history and makes sure that we have to resolve merge conflicts only once.
Thanks all. No problem about the rebase, that's done now, I was planning doing a rebase and squashing a lot of this down once we are ready to go.
There were some failures after rebasing but they have been fixed now. I don't have access to that bugsnag to see the backtrace, the tests that were failing were in the admin area rather than at checkout so not sure if that will be fixed or not.
The last failing spec ./spec/system/admin/order_cycles/complex_creating_specific_time_spec.rb:65 is flaky, it seems like a timing issue when checking the Your order cycle has been created flash notice, not sure what to do there yet :thinking:
Hey @cillian ,
I'm getting an error 500 when accessing
https://staging.openfoodnetwork.org.uk/admin/order_cycles/3597/checkout_options
This order cycle has some entries on the newly created table order_cycles_shipping_methods; So I've tried on another order cycle (with no entries), but get the same result.
The error is:
gems/activerecord-6.1.7/lib/active_record/connection_adapters/postgresql_adapter.rb:672:in `exec_params': PG::UndefinedTable: ERROR: relation "order_cycles_distributor_shipping_methods" does not exist
LINE 1: ...e FROM "distributors_shipping_methods" INNER JOIN "order_cyc...
Can you access /admin/order_cycles/<order_cycle_id>/checkout_options in your local system? I wonder why did all specs pass here...
Hi @filipefurtad0
It looks like staging has the old order_cycles_shipping_methods table from a previous round of testing. The table had to be changed to order_cycles_distributor_shipping_methods in order to handle shipping methods which were shared between multiple distributors. I wonder if migrating will fix this, although we would probably still have to delete the old table from staging, what does a rails db:migrate:status show?
Thanks @cillian , I've tried another server and got the same result. The output from rails db:migrate:status:
up 20220105085729 Split customers name
up 20220105085730 Migrate customers data
up 20220112102539 Add mails sent to order cycles
up 20220114110920 Update order cycle mails
up 20220118053107 Convert stripe connect to stripe sca
up 20220221165207 Migrate checkout only to both back office and checkout
up 20220316055458 Create active storage tablesactive storage
up 20220407051248 Remove icon from spree taxons
up 20220410162955 Change visible data type for enterprises
up 20220425102039 Remove bill addresses with null phone
up 20220425230907 Add disabled at to spree users
up 20220429092052 Create order cycles distributor shipping methods
up 20220602013938 Compute checksum for big files
up 20220603140943 Add whatsapp phone to enterprises
up 20220629080906 Add note to orders
up 20220713195433 Add show api key generation view to spree user
up 20220907055044 Delete duplicate customers
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? I mean, in the psql shell running something like:
delete from schema_migrations where version = '20220429092052'
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.
Yaayy worked :tada:
Resuming testing, Thanks for your help :raised_hands:
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 :heavy_check_mark:
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 :heavy_check_mark:
- 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.