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

v14 - v16 splitting shopinvader module ? #1372

Open
sebastienbeau opened this issue Jun 19, 2023 · 12 comments
Open

v14 - v16 splitting shopinvader module ? #1372

sebastienbeau opened this issue Jun 19, 2023 · 12 comments
Assignees
Labels

Comments

@sebastienbeau
Copy link
Contributor

sebastienbeau commented Jun 19, 2023

In V16 a lot of change have been started (new api based on fastapi, huge refactor on search engine....).
A lot of change are breaking change, but as customer can change their odoo version without migrating the shop at the same time. The old API must be installable on V16.

In order to make it possible here is the plan :

Regarding the API

  • old api will still be based on base_rest for V14 and V16 (this avoid to change too much code on existing project and spent useless money to adapt their custom)
  • the new api is based on fastapi for V14 and V16

Regarding the product sync with search engine
On V16 the concept of binding is removed (no more shopinvader.product/shopinvader.variant)
On V14 we must split the shopinvader module in several module:

  • shopinvader
  • shopinvader_product
  • shopinvader_partner

The migration script will automatically install the module (shopinvader_product and shopinvader_partner) when upgrading shopinvader module

Dependency of shopinvader addons that need the "shopinvader.product/variant/partner" will depend on the related modules

By doing this we do two thing:

  • we can use the new backported version of search-engine on V14 (maybe we can have an "official" 14.0-backport branch
  • we simplify the migration to V16 as the module will be more "V16 compatible"

@hparfr @lmignon @simahawk @sbidoul it's ok for you ?

@sbidoul
Copy link
Member

sbidoul commented Jun 19, 2023

I agree. We should preserve the REST APIs contracts as much as we can.

I understand some are impossible to preserve when moving to a SPA frontend model because they depend on the locomotive session (e.g. cart).

For some we want to create a v2 because we found genuine design issues (e.g. addresses). Or provide more features and better ergonomy (e.g. async cart).

But in general we must preserve compatibility for frontends, yes.

Also note it is entirely possible to migrate to FastAPI while preserving frontend compatibility. That should be doable gradually.

What is unclear to me is to what extent we can preserve the backend apis.

@hparfr
Copy link
Contributor

hparfr commented Jun 20, 2023

My goal is to have lot of new v16 shiny stuff in v14 as well.

If possible, I want fast api in v14.

New dependency will be added to shopinvader module.
Concurrent modules with v16 compat will be created in v14. It will allow us to migrate progressively our code base.

There should be no breaking change in v14 , actual api will continue to work in v14.

@lmignon
Copy link
Collaborator

lmignon commented Jun 20, 2023

@sebastienbeau For sure a split would be welcome but the cost could be high. The creation of shopinvader_partner and could ease the cleanup of place where we explicitly use the shopinvader.partner model and replace-it by the res.partnermodel. I've the feeling that shopinvader_product will be more difficult to extract.
The most important point is to keep the API for existing projects. The difficulty is that the lack of documentation of the existing API (no schema for most of the response, no schema for the product description, ...) could lead to unwanted breakage.

@sebastienbeau sebastienbeau changed the title v14-v16 splitting shopinvader module v14 - v16 splitting shopinvader module Jun 20, 2023
@sebastienbeau
Copy link
Contributor Author

sebastienbeau commented Jun 20, 2023

After thinking twice extracting "shopinvader.partner" seem to be complicated on the old V1 API, and also maybe not a good investment as if you use the V1 is mostly because you have a shop on locomitiveCMS and in that case the shopinvader.partner is required to the module will be installed

I am hesitating in three direction

Solution 1

Having the following modules

  • shopinvader
  • shopinvader_product (that depend on shopinvader)

with a script to install this module when shopinvader is updated

Solution 2

Having the following modules

  • shopinvader_v1 or shopinvader_base_v1 (basically the old shopinvader module but rename without shopinvader product stuff)
  • shopinvader_v1_product (add the shopinvader.product + inject in sale the product info)
  • shopinvader empty module that depend on all shopinvader_v1_* module

Solution 3 Wait for V16

No split is done on V14 as it can introduce some breaking change (xmlid of views, models, data will change and will required adaption in custom).

Then on V16 we

  • remove the shopinvader.product and shopinvader.variant from shopinvader
  • rename the module shopinvader into shopinvader_v1

On V14 if we want to use the new search-engine, we create a module "shopinvader_search_engine_futur" (or "shopinvader_search_engine_v2") that hide the "shopinvader.product/shopinvader.variant", install the required dependency and provide an helper or migrate the data, then you can uninstall shopinvader_search_engine (the shopinvader.product and shopinvader.variant will be still here but not used).

What do you think @lmignon @sbidoul @simahawk

@sebastienbeau sebastienbeau changed the title v14 - v16 splitting shopinvader module v14 - v16 splitting shopinvader module ? Jun 20, 2023
@hparfr
Copy link
Contributor

hparfr commented Jun 20, 2023

+1 on solution 2

@lmignon
Copy link
Collaborator

lmignon commented Jun 21, 2023

@sebastienbeau I would tend to go with 'solution 2'. The most important consideration to keep in mind is to have a symmetry in the module naming of actual shopinvader addon between 14 and 16 (at least for the maximum of addons) to ease the back/forward port.

@simahawk
Copy link
Contributor

At first glance I tend to go w/ option 2 but is not clear what would be moved to *_v1* and what would you keep in shopinvader. Can you elaborate a bit on this?

In general, to introduce this in v14, it's really critical to maintain full backward compat. How you say in French: "ça va sans dire" 😉

And possibly having 2 api versions working at the same time (eg: using /shopinvader/v2/* for v2) but I don't think this is going to be a problem.

What is unclear to me is to what extent we can preserve the backend apis.

This will likely be more problematic. We could probably wrap the backend api w/ a component or other tool so that both versions can use the same high level api seamlessly.

@sebastienbeau
Copy link
Contributor Author

I also tend to go to solution 2 as it allow us to have the "same" code and module on V14 and V16 so backport forwardport will be easier.

I will put somebody from Akretion on this split.

@sebastienbeau
Copy link
Contributor Author

sebastienbeau commented Jun 26, 2023

TODO list

  • rename shopinvader => shopinvader_v1_base
  • extract product code and create the module shopinvader_v1_product (depend on shopinvader_v1_base)

components

  • product_product_event_listener.py

models

  • shopinvader_binding.py
  • shopinvader_category.py
  • shopinvader_product.py
  • shopinvader_variant.py
  • seo_title_mixin.py
  • product_template.py
  • product_product.py
  • product_filter.py
  • product_category.py

data

  • ir_export_category.xml
  • ir_export_product.xml

demo (do in last, maybe too many impact on test)

  • product_attribute_value_demo.xml
  • product_category_demo.xml
  • product_filter_demo.xml
  • product_product_demo.xml

views

  • product_category_view.xml
  • product_filter_view.xml
  • product_view.xml
  • shopinvader_backend_view.xml (move only the smart button + button)
  • shopinvader_category_view.xml
  • shopinvader_menu.xml (move the product menu)
  • shopinvader_product_view.xml
  • shopinvader_variant_view.xml

services

wizard

  • shopinvader_category_binding_wizard.py
  • shopinvader_category_binding_wizard.xml
  • shopinvader_category_unbinding_wizard.py
  • shopinvader_category_unbinding_wizard.xml
  • shopinvader_variant_binding_wizard.py
  • shopinvader_variant_binding_wizard.xml
  • shopinvader_variant_unbinding_wizard.py
  • shopinvader_variant_unbinding_wizard.xml

tests

  • test_shopinvader_category.py
  • test_shopinvader_category_binding_wizard.py
  • test_shopinvader_variant_binding_wizard.py
  • test_shopinvader_variant_seo_title.py
  • test_product.py
  • test_product_filter.py

Binding the product in the common test will be useless (base module do not have restriction anymore)

cls.backend.bind_all_product()

When test are green

  • create migration script

@sebastienbeau
Copy link
Contributor Author

@qgroulard
Copy link
Contributor

Migration of shopinvader_restapi to 16 here: #1403

Copy link

There hasn't been any activity on this issue in the past 6 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days.
If you want this issue to never become stale, please ask a PSC member to apply the "no stale" label.

@github-actions github-actions bot added the stale label Oct 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants