Skip to content

[MERGE][REF] delivery: extract stock logic in separate module#110686

Closed
vava-odoo wants to merge 7 commits into
odoo:masterfrom
odoo-dev:master-delivery_split_from_sale_and_stock-vava
Closed

[MERGE][REF] delivery: extract stock logic in separate module#110686
vava-odoo wants to merge 7 commits into
odoo:masterfrom
odoo-dev:master-delivery_split_from_sale_and_stock-vava

Conversation

@vava-odoo

@vava-odoo vava-odoo commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

It's currently impossible to set fixed delivery costs in eCommerce without installing Inventory. This is a problem for one-app free instances on our SaaS, which would need to pay for a feature that should be part of the basic eCommerce features bundle.

Delivery is now split from inventory/stock module to allow for a soft free integration into website_sale. This PR makes it possible to define fixed-cost delivery methods in eCommerce without installing Inventory by adding a direct dependency of website_sale on delivery. The bridge between those two modules is therefore included in the former.

Steps are:

  • extract the Inventory logic from the delivery module into a new stock_delivery module;
  • move website_sale_delivery into website_sale;
  • adapt dependencies to delivery and website_sale_delivery;
  • remove bridge module with loyalty directly in website_sale_loyalty;
  • move website_sale_delivery_mondialrelay to website_sale_mondialrelay.

task-3074497
task-3203210

See also:

@robodoo

robodoo commented Jan 23, 2023

Copy link
Copy Markdown
Contributor

Pull request status dashboard

@C3POdoo C3POdoo added the RD research & development, internal work label Jan 23, 2023
@vava-odoo vava-odoo force-pushed the master-delivery_split_from_sale_and_stock-vava branch 12 times, most recently from 83d1956 to 771d608 Compare January 26, 2023 16:56
@vava-odoo vava-odoo force-pushed the master-delivery_split_from_sale_and_stock-vava branch 16 times, most recently from 8d5d9a0 to 5fbadfa Compare February 6, 2023 10:54
@vava-odoo vava-odoo force-pushed the master-delivery_split_from_sale_and_stock-vava branch from 6ddf05c to e2494de Compare March 1, 2023 17:30

@vava-odoo vava-odoo left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @Demesmaeker !
I mostly agree on your comments. Made a lot of changes.
Only the one about po files: since I remove the bridge, I remove the entire folder in github. Can't really do otherwise.

Comment thread addons/delivery/models/delivery_carrier.py Outdated
Comment thread addons/delivery/models/delivery_carrier.py Outdated
Comment thread addons/delivery/models/delivery_carrier.py Outdated
Comment thread addons/delivery/__manifest__.py Outdated
Comment thread addons/delivery/models/delivery_carrier.py Outdated
Comment thread addons/delivery/models/delivery_carrier.py Outdated
@vava-odoo vava-odoo force-pushed the master-delivery_split_from_sale_and_stock-vava branch 6 times, most recently from df9ed3e to f08cb5a Compare March 3, 2023 08:46

@Demesmaeker Demesmaeker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello there 🦆 , you have website_delivery_sendcloud that still depends on (and then import from) website_sale_delivery ;)

Also, in deliver_carrier from stock_delivery, it looks like you only made cosmetic changes to respect the max 100 chars line, but you missed some lines.

And finally: Internal Server Error on the runbot, couldn't test it yet

Still have a few files to check, but let's already post it =]

Comment thread addons/delivery/data/delivery_data.xml Outdated
Comment thread addons/delivery/models/delivery_carrier.py Outdated
Comment thread addons/delivery/models/delivery_carrier.py Outdated
Comment thread addons/delivery/security/ir.model.access.csv Outdated
Comment thread addons/delivery/security/ir.model.access.csv Outdated
Comment thread addons/website_sale/data/website_sale_delivery_demo.xml Outdated
Comment thread addons/website_sale/controllers/delivery.py Outdated
Comment thread addons/website_sale/controllers/main.py Outdated
Comment thread addons/website_sale/controllers/main.py Outdated
Comment thread addons/website_sale/controllers/main.py Outdated

@Feyensv Feyensv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very quick look, didn't see anything big on the fly so only linting comments for now :D

Comment thread addons/delivery/models/delivery_carrier.py Outdated
Comment thread addons/delivery/models/delivery_carrier.py Outdated
Comment thread addons/delivery/security/ir.model.access.csv Outdated
Comment thread addons/delivery/views/delivery_view.xml Outdated
Comment thread addons/stock_delivery/__manifest__.py Outdated
Comment thread addons/website_sale/models/sale_order.py Outdated
Comment thread addons/website_sale/views/website_sale_delivery_views.xml Outdated
Comment thread addons/website_sale_mondialrelay/views/res_config_settings_views.xml Outdated
Comment thread addons/website_sale_mondialrelay/views/delivery_carrier_views.xml Outdated
Comment thread addons/stock_delivery/models/sale_order.py Outdated

@Demesmaeker Demesmaeker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello

I started testing this task, here are some problems that I encountered that should be solved:

Installing only ecommerce:

  • Not a single shipping method set as "published" on the website, yet I could select Standard Delivery
  • I tried to publish Standard Delivery, I couldn't. Nor any of the others, not even a newly created one.

I stopped testing there. You made a lot of changes since it was last thoroughly tested, it might be wise to do it again to avoid both obvious and tricky bugs. =]

edit: as these are known problems, I started again, sorry for that ;)

Comment thread addons/delivery/models/delivery_carrier.py Outdated
Comment thread addons/website_sale/data/website_sale_delivery_demo.xml Outdated
Comment thread addons/stock_delivery/security/ir.model.access.csv Outdated
Comment thread addons/website_sale/models/sale_order.py Outdated

@Demesmaeker Demesmaeker left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bugs I found were all also presents in master, improvements I could think of are for moved and untouched lines, so... lgtm, as long as someone from the stock team can also confirm/test that, and @chevalierv confirms the express checkout part. =]

Comment thread addons/website_sale/models/sale_order.py Outdated

@ticodoo ticodoo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, nice work! I only have nitpick comments (i.e. they can all be ignored). General flows look like they all still work fine. I only tested the sale + stock parts. I didn't touch the website stuff since I don't know their flows.

Comment thread addons/delivery/__manifest__.py Outdated
Comment thread addons/delivery/models/delivery_carrier.py Outdated
Comment thread addons/stock_delivery/views/delivery_view.xml Outdated

@Feyensv Feyensv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick comm, will take a func look tomorrow 👌

Comment thread addons/website_sale/views/website_sale_delivery_views.xml Outdated

@Feyensv Feyensv left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@robodoo rebase-merge r+
@robodoo delegate+

@robodoo

robodoo commented Mar 10, 2023

Copy link
Copy Markdown
Contributor

Merge method set to rebase and merge, using the PR as merge commit message.

@robodoo

robodoo commented Mar 10, 2023

Copy link
Copy Markdown
Contributor

@Feyensv you may want to rebuild or fix this PR as it has failed CI.

@AntoineVDV

Copy link
Copy Markdown
Contributor

@robodoo override=ci/security
Reason: The concerned code is moved from one module to another and left unchanged.
Link to ci/security build (master): https://runbot.odoo.com/runbot/build/24910293

split-delivery

@robodoo

robodoo commented Mar 10, 2023

Copy link
Copy Markdown
Contributor

@vava-odoo @Feyensv linked pull request(s) odoo/upgrade#4362 not ready. Linked PRs are not staged until all of them are ready.

Extract the Inventory logic from the `delivery` module into a new
`stock_delivery` module. This will allow to integrate the basic delivery
features into the website_sale app in a one-app free database.

task-3074497
It's currently impossible to set fixed delivery costs in eCommerce
without installing Inventory. This is a problem for one-app free
instances on our SaaS, which would need to pay for a feature that
should be part of the basic eCommerce features bundle.

This commit makes it possible to define fixed-cost delivery methods in
eCommerce without installing Inventory by adding a direct dependency of
`website_sale` on `delivery`. The bridge between those two modules is
therefore included in the former.
*: mondialrelay, stock_picking_batch
**: delivery_mondialrelay, loyalty_delivery, picking

Adapt dependency to `website_sale` directly instead of
`website_sale_delivery`.
In ecommerce, the form vue of shipping methods should be improved to
easily set up fixed delivery costs. This commit improves the
user-experience and solves bugs on the fields displayed.

task-3203210
@Feyensv

Feyensv commented Mar 13, 2023

Copy link
Copy Markdown
Contributor

@robodoo r+

@robodoo

robodoo commented Mar 14, 2023

Copy link
Copy Markdown
Contributor

@vava-odoo @Feyensv linked pull request(s) odoo/enterprise#36609 not ready. Linked PRs are not staged until all of them are ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

16.2 RD research & development, internal work

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants