Openfoodnetwork: [Spree Upgrade][Account Invoices] Handle shipping_method_id on finalize_account_invoices

[luisramos0]

We need to review/replace usage of shipping_method_id in app/jobs/finalize_account_invoices.rb

Files that reference shipping_method_id:

• [ ] - app/jobs/finalize_account_invoices.rb
• [ ] - spec/jobs/finalize_account_invoices_spec.rb
• [x] - app/models/spree/app_configuration_decorator.rb
• [x] - app/views/admin/accounts_and_billing_settings/_method_settings.html.haml
• [x] - app/views/admin/accounts_and_billing_settings/edit.html.haml
• [x] - lib/open_food_network/accounts_and_billing_settings_validator.rb
• [x] - spec/features/admin/accounts_and_billing_settings_spec.rb
• [x] - spec/controllers/admin/accounts_and_billing_settings_controller_spec.rb
• [x] - app/controllers/admin/accounts_and_billing_settings_controller.rb

[luisramos0]

I went through all classes relates to account_invoices to verify uses of shipping_method_id.
All looks good because this is just collecting from the admin page the shipping_method_id to use in these invoices and then using that id to create the orders/invoices.
Both accounts_and_billing_settings_controller_spec and accounts_and_billing_settings_spec are green.

The only change that needs to be done here is where this shipping_method_id is used in app/jobs/finalize_account_invoices.rb:43
invoice_order.update_attribute(:shipping_method_id.....

The respective spec (spec/jobs/finalize_account_invoices_spec.rb) is broken probably because of this.

[luisramos0]

The challenge of replacing
invoice_order.update_attribute(:shipping_method_id
is the checkout workflow callback order.create_proposed_shipments that destroys and rebuilds all order.shipments from line_items and stock locations...

I see two options:

• create a product and variant for these invoices
• somehow bypass this workflow step as done here in tests

@mkllnk @sauloperez do you guys know this code? any ideas how we can go through or around the order.create_proposed_shipments flow?

[mkllnk]

No idea. I would need to dig in the code for a while to give you a proper answer.

[luisramos0]

ok, thanks @mkllnk

My preferred approach here would be to create a variant like "OFN Usage" and make these orders totally normal. I think that is easy to do and it will work, but I need confirmation from someone who knows this code.

[sauloperez]

I know nothing about this code, but I do remember I once wondered what was actually about and @oeoeaio knew. Not sure if he was the author.

[luisramos0]

ok, thanks guys.
this is about invoicing user of OFN with their usage.

I am tempted to get rid of the logic "these are invoices and not normal orders" and make these normal orders in the system with product, variant and line items. The "OFN usage" product.

Anyway, I'll leave this for later as it's not blocking any other part of the upgrade.

@oeoeaio if you ever come here, please advise :-)

[mkllnk]

I think we need to ask at least @kirstenalarsen before we can get rid of this logic. We were charging people for using OFN, but we are in the process of reviewing our business model at the moment. The UK is charging as well, I think. I just don't know what reporting they use for that. @Matt-Yorkley?

I think that the OFN should provide the reports to base invoices on. But the actual charging logic is very coupled to the local business model and should probably sit outside OFN.

[luisramos0]

I didnt mean to get rid of this part of OFN! I just meant to get rid of the specific part of the solution which makes these a special order without line items.

[mkllnk]

Okay, if you can change the logic without breaking anything, go ahead. :-)

[luisramos0]

ok, this is ready for dev.

As mentioned above, whoever takes a chance on this issue can:

• option 1 - set shipping_method_id in the order
  -- first get the order to the :delivery state by advancing workflow (to make create_proposed_shipments work you need to set order.ship_address and make sure the shipping_method is available in the zone of that address)
  -- then set the shipping_method_id using OrderShippingMethod concern method
• option 2 - as mentioned above, make these orders normal orders with line_items

I thought option 1 was difficult but now I think option 1 will be so easy it will not be worth considering option 2.

[luisramos0]

Looking at this again, the simple option 1 is made more dificult by this commit
https://github.com/openfoodfoundation/spree/commit/38868bc840eb51850ad650b634aebb4dcc2bc1a9
the order will not move to complete without line items, so we need to add line items to the order. it's one argument to go for option 2.