Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move order_cycle_serializer#visible_variants_for_outgoing_exchanges to exchange_products_renderer#products_for_outgoing_exchange #4480

Open
Tracked by #4610
luisramos0 opened this issue Nov 22, 2019 · 0 comments

Comments

@luisramos0
Copy link
Contributor

luisramos0 commented Nov 22, 2019

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

exchange_products_renderer is used by the exchange_products_controller to list products that can be added into exchanges, both incoming and outgoing.

Now that these lists of products are done on the server, we can move more of its logic to the server. One of these points is order_cycle_serializer#visible_variants_for_outgoing_exchanges.
This data is renderer as part of the OC and is only used to filter out products from outgoing exchanges on the angular template. All we need to do is to move this to the exchange_products_renderer when we render products_for_outgoing_exchanges.

EDIT: extra work: there's an extra piece of work that needs to be done when we move this code over: the "select all" feature in outgoing exchanges. When the user selects all variants in an outgoing exchange the list of variants comes from the incoming exchanges and is additionally filtered by the visible_variants_for_outgoing_exchanges. If we move the select all process on the outgoing exchanges to the server, this doesnt need to be done.

Tests

In terms of performance this will improve the performance of the general settings and incoming settings as we will remove this computation from the OC serializer. This will also make outgoing settings faster as we will run this computation only for each exchange if the it's products are loaded on the page.

The OC pages should work correctly, particularly the list of products for outgoing exchanges.
We should test for products that can be added to an outgoing exchange by a manager or coordinator but not by the current user. We should test with a user that can see products for some enterprises in the OC but not all.

Quick win

One possible quicker win in this context would be to make these methods not render any master variants (OCs dont take master variants). Currently, for each product we are serializing the master variant which in many cases duplicates the size of the payload.

Stretch goal

We could probably improve/simplify the method
OrderCyclePermissions#visible_variants_for_outgoing_exchanges_to
Some entries have the comment:
"TODO: Remove this when all P-OC are sorted out"
I think this comes from the past where we had ProductDistributions and EnterprisePermissions were not the only way to get products into an OC.
I believe this type of permissions-bypassing logic can be removed now...

Context

Came up from splitting the OC page in 3 steps.

Impact and timeline

This has a big impact/effort ratio, definitely something to be done soon.

@sigmundpetersen sigmundpetersen changed the title OC performance - move order_cycle_serializer#visible_variants_for_outgoing_exchanges to exchange_products_renderer#products_for_outgoing_exchange Move order_cycle_serializer#visible_variants_for_outgoing_exchanges to exchange_products_renderer#products_for_outgoing_exchange May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: All the things 💤
Development

No branches or pull requests

1 participant