search

openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.11k stars 718 forks source link

OC edit performance: Angular #4144

Closed Matt-Yorkley closed 4 years ago

Matt-Yorkley commented 5 years ago

What we should change and why (this is tech debt)

I think the Angular for edit Order Cycles will need to be reworked if we want 10000 products in an OC. I found this code comment, which kind of sums it up:

https://github.com/openfoodfoundation/openfoodnetwork/blob/86aeb6a3c796ae09e15b5a576b4e8424e076aa37/app/assets/javascripts/admin/order_cycles/services/order_cycle.js.coffee#L79-L84

These services will probably both need looking at:

app/assets/javascripts/admin/order_cycles/services/order_cycle.js.coffee app/assets/javascripts/admin/order_cycles/services/enterprise.js.coffee

Context

Working on OC performance.

Impact and timeline

sigmundpetersen commented 5 years ago

Should this be added to the Performance backlog and #4129 ? Or do we treat it as tech debt for later? @lin-d-hop @sauloperez

sauloperez commented 5 years ago

Might be worth having it in the Perf backlog as a reference when we start with OCs, at least. I leave the final decision up to you @lin-d-hop to avoid messing with the list.

luisramos0 commented 5 years ago

this is probably a spike in itself. the angular code for the OC edit page is insane...

I dont think we will have satisfactory results in #4129 if we don't do this one...

I am trying to come up with a list of alternatives: A - I think Matt suggested loading only part of the variants in each exchange, that would work but I think it would be very difficult to make the existing code work with that because when you change an incoming exchange, the outgoing exchange needs to be updated... B - we can try to rewrite part of the services above but it's hard, insane code... C - we can send this to the UX department... a bigger variation on option A, if we want this page to work for large catalogs and on the way make it more user friendly, we better have a easier way for users to pick and choose from their catalogs into the Order Cycle... C.1 - one option is to have a separate page where users can add variants to exchanges C.2 - another option is to have something like a search popup (like in the admin order edit page) where the user can pick variants, but then on the OC edit page we only show the ones already added...

I wouldnt waste time on B, imo this angular code that manages the variants in the exchanges on the browser is not something we want to keep for very long...

thoughts?

Matt-Yorkley commented 5 years ago

Agreed on the assessment. It doesn't seem clear what is the best way to proceed. There are no easy options here...

Matt-Yorkley commented 5 years ago

So option B we try to work with what's there, or option C we remake it from scratch, right? I think I agree option B would be a nightmare.

With option C maybe we can go back to basics and see what exactly the page needs to do, and maybe split it into 2 pages. In a scenario with 10,000 products in an OC we can't really keep the existing Angular code, or the existing UX.

Matt-Yorkley commented 5 years ago

So the hub manager has to 1) select producers to be in an order cycle, and then from the available products for each producer; 2) select which ones will be included in the OC and which will not. If we imagine an OC with 4 producers and each has 2,500 products then this process of selecting is not simple (it involves 10,000 choices / many clicks). I think a separate page (or probably a modal) for each exchange would be good, and the products shown in the modal would need to be paginated and searchable / filterable. It also makes sense to have an option to immediately add all the producer's products to the OC, probably before even looking at the available products. This removes the need to select them all (potentially 10,000 products could be added with a few clicks), or means that a few products could then be de-selected for each exchange. And we don't query everything from the DB or load everything into Angular...

Matt-Yorkley commented 5 years ago

I'd opt for something like the above, creating simple, readable Angular that makes queries to some nice endpoints, and :fire: a lot of the existing code.

luisramos0 commented 5 years ago

yes! I agree Matt.

I cant really work with the existing angular code. I have just being knocked out on a single blow (one line): https://github.com/openfoodfoundation/openfoodnetwork/blob/master/app/assets/javascripts/templates/admin/panels/exchange_distributed_products.html.haml#L14

there's so much to this line of code (stuff coming from all sides: enterprises, exchanges, permissions). Even just trying to optimize filter:productSuppliedToOrderCycle...

Matt-Yorkley commented 5 years ago

Yeah, I looked at it before and had a similar experience.

Matt-Yorkley commented 5 years ago

I'll add this to the epic #4129, it at least needs to be discussed as part of it.