Openfoodnetwork: [Spree Upgrade] Replace search to add products to order as admin

[mkllnk]

When editing an order as admin, our code renders Spree's form to add a product. We override the search and display functionality to show the distributor. Spree 2.0 replaced that search functionality with other API calls. We need to update our modifications accordingly.

Description

Currently, our view app/views/spree/admin/orders/edit.html.haml calls render 'add_product'. That partial is in Spree's code base: backend/app/views/spree/admin/orders/_add_product.html.erb. It uses an autocomplete search which we override in a few places:

• app/views/spree/admin/variants/_autocomplete.js.erb overriding
  backend/app/views/spree/admin/variants/_autocomplete.js.erb
• app/views/spree/admin/shared/_routes.html.erb overrides variants_search to call Spree::Admin::VariantsController.search which we override in
  app/controllers/spree/admin/variants_controller_decorator.rb
• app/views/spree/admin/variants/search.rabl is our view for that action and doesn't exist in Spree any more

Spree 2.0 still has backend/app/views/spree/admin/variants/_autocomplete.js.erb, but modified it. The partial backend/app/views/spree/admin/orders/_add_product.html.erb is modified as well. We could probably just import that partial as well and everything keeps working, because we overrode everything else. We can then simplify the whole feature by moving it into our own namespace.

Expected Behavior

When adding an item to an order as admin, the search displays found variants with their distributor.

Actual Behavior

At the moment, the edit order page is not working.

Steps to Reproduce

1. Log in as admin
2. Go to edit an order
3. enter a search term into the add product form
4. check that the distributor is showing

Context

I just discovered it while working on https://github.com/openfoodfoundation/openfoodnetwork/issues/2014.

Severity

s3 or s4 once the Spree upgrade is done.

[luisramos0]

Looks like it's ready for someone to pick it up. Moved to Dev Ready.

[HugsDaniel]

Another opportunity to make our own views out of Spree's ?

[luisramos0]

yes, I think it is. and looks like another one you will want to do directly in v2.

[luisramos0]

Hey Hugo, how is this going?
I just bumped into this one on the build with a translation error. I have created a new issue for this specific translation error but we will probably fix everything in the same effort. The issue is #2935

As I dived into this, realized how complicated this setup is with ofn code and Spree code tightly coupled between rails views and js code... help!

Two things we should do to javascript template in app/views/admin/variants/_autocomplete.js.erb as part of this issue:

• it should be in assets/javascripts/templates and app/assets/javascripts/admin/utils/directives/variant_autocomplete.js.coffee needs to be adapted
• it has to be converted from erb to haml

[luisramos0]

Also, this is related to what @kristinalim is doing on the subscriptions autocomplete on #2733. On 2733 code review I suggest this autocomplete code cannot depend on subscriptions, but only the other way around: subs autocomplete depend on core autocomplete.

I think a part of this PR has to be done in master, otherwise I believe it will be merge hell.
Maybe @kristinalim can help. Anyway, I think we need to help @HugsDaniel to define an approach to this one.

[HugsDaniel]

There are a lot of changes in variant_autocomplete.js.erb in Spree 2.0, updates are now made in ajax (no update button anymore). Angular syntax is very different from the jquery used in Spree, so it's taking a lot of time to replace stuff.

See here the 2-0-stable version : https://github.com/spree/spree/blob/2-0-stable/backend/app/assets/javascripts/admin/variant_autocomplete.js.erb

They added a bunch of handlers for editing, saving, and so on.

@luisramos0 what part of this should I make in master ?

[luisramos0]

I dont think we need to follow spree. We need our solution for this problem. And I believe that should be done in master so when you move to the upgrade branch it will not fail because it's not dependent on spree. I'd start simply by cleaning up what's in master and make it independent from spree.

[luisramos0]

I believe a good start would be to do the two things I mention above here

[luisramos0]

Also, do you see that in master we are not using the spree version, there's a version of it in ofn, correct?

[HugsDaniel]

The one we use in ofn is a slightly patched "angulared" version of the Spree version. I agree it would be a lot easier to start with master, since it has been modified a lot in 2.0.

[luisramos0]

hey @HugsDaniel can you clarify: is this issue solved when #2955 is merged into 2-0-stable? if not, it should not be in code review.

[HugsDaniel]

Good point, no it's not, it'll have to be addressed for 2-0, #2955 was just the first step. Sorry :v:

[daniellemoorhead]

Hey @HugsDaniel 👋 🙃
It's been 22 days since this was touched last. Are you still working on it or should we move it back to dev ready till you get back onto it?

toomanythingsinthepipe

toottoot

[HugsDaniel]

Hey @daniellemoorhead I'm still on it ! It's actually related to #3109 for which I opened this pr #3194. Maybe I can close this one and add its informations to #3109 , what do you think ?

[daniellemoorhead]

Maybe I can close this one and add its informations to #3109 , what do you think ?

You could close it @HugsDaniel (and sorry about the delayed response!), or you could move this one to be together with #3109 so that they're always in the same part of the delivery pipe?

I added a dependency for this one on #3109 so we can now see they're linked 😄

[luisramos0]

I'll be working on this one this week.