Openfoodnetwork: [Spree Upgrade] Fix ShippingMethods Create page

[luisramos0]

Fix page /admin/shipping_methods/new

Currently broken with an error related to zone id.
It's a heavily defaced page, probably better to use the opportunity to move Spree code to OFN and create a view without deface.
Also, if the approach selected is removing deface, it's probably worth doing it in master and then adapting it to spree 2 in 2-0-stable.
Also, maybe we remove deface not just for new but also for index and edit pages.

This issue is related to #2010, same controller

[HugsDaniel]

May I take this one ?

[luisramos0]

Hi Hugo, yes!
You can see what Matt did here:
https://github.com/openfoodfoundation/openfoodnetwork/pull/2675

You need to pick this
../spree/backend/app/views/spree/admin/shipping_methods
mix with this
./openfoodnetwork/app/overrides/spree/admin/shipping_methods
convert to haml
and put it here
./openfoodnetwork/app/views/admin

My only question is that I'd not put it in app/views/spree/admin as Matt did, I'd put it in app/views/admin

[luisramos0]

You can see better as you go but it's probably better to have one PR per page: new, index and edit.
Also, as I say above, I think it's better to do this in master first.

[HugsDaniel]

I'm not sure rails will use the view if we put it in app/views/admin

[HugsDaniel]

Should I use views from 2-4-stable ? If so with spree step-6a used in master there's no method zones for Spree::ShippingMethods, so with 2-0-4-stable views against master it will end up crashing

[luisramos0]

I'd check if step-6a is very different from 2-0-4 on this view.
If it is, I'd skip building it in master, but otherwise, I'd use step-6a to do this in master.
And then in 2-0-stable we can adapt to the changes like zone.
If you do it in 2-0-stable from 2-0-4-stable you will have to merge all changes that are done in master from now to the final migration...
What's easier?

[luisramos0]

I'm not sure rails will use the view if we put it in app/views/admin

yeah, that's true! we would have to change the route and controller. For this one, maybe easier to keep in spree/. Thanks, I was not seeing this problem.

[HugsDaniel]

@luisramos0 there's a whole bunch of I18n syntax changes and a couple of sections added, it would seem easier to use directly 2-0-4-stable views

[luisramos0]

all right, lets do that!

[luisramos0]

Just leaving a note here to address question from #2728

[luisramos0]

several PRs connected to the same issue make all PRs move workflow together. I'll disconnect everything so that PRs move at their own pace... ok @sigmundpetersen ?

[luisramos0]

PRs are 2752, 2753, 2754, 2755

[HugsDaniel]

The zone_id error comes from the fact that there's no zone_id in spree_shipping_methods, instead there's a join table called spree_shipping_methods_zones. I'm not quite sure how to create a new record of spree_shipping_methods_zones instead... Nested form ? Do it in the controller ?

[sigmundpetersen]

Sure @luisramos0 no problem! Multiple PRs on one issue is a tricky one scrum-wise

[luisramos0]

So, after a slack talk with Hugo, we conclude there are a few things to do here now:

• fix create shipping_method page by removing all zone_id section and testing the new version in spree 2 with zones (one important detail is to verify error cases where zone is not selected)
• add new shipping_method category section from spree 2 - in spree 2 at least one category must be selected for a given shipping_method, we can probably create a default category if we dont have it yet
• cover these screens with auto tests
• verify the question from #2728 mentioned above
• also, from #2695, verify associating shipping_methods to enterprises is working (from reading the code everything looks good), needs to be tested after this page is fixed.

I think that's all. makes sense @HugsDaniel ?

[luisramos0]

I have just realized this issue is a duplicate of #2497. There's more context on this one now that we are fixing it, so I'll close #2497

[luisramos0]

moving this to in dev to represent reality.
@HugsDaniel you finished most things from my comment above but not everything I believe. correct?

[HugsDaniel]

@luisramos0 I'm on the question from #2728 , still haven't found out what's wrong here. I'll keep you posted

[HugsDaniel]

I opened a new issue #2881 for the question in #2728 since it's slightly different from what was intended here. I close this one now.

[luisramos0]

Amazing, OFN v2 is starting to be usable! I gave this shipping methods UI a good try and it just works :-)
well done @HugsDaniel