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

16.0 mig sale delivery state #2770

Merged
merged 29 commits into from
Nov 24, 2023

Conversation

sebastienbeau
Copy link
Member

@sebastienbeau sebastienbeau commented Nov 9, 2023

Following the discussion here : #2361

As Odoo have introduced the state "delivery_status" in V16, we believed that the module was useless.
But as the native implementation is broken so we still need this module.
see comment here : #2361 (comment)

In order to not add any extra new fields, the feature is split in 3 modules

  • sale_delivery_state (this add the field but it's not visible in the view)
  • sale_stock_delivery_state (glue module to overwrite the implementation of the computed field delivery_status)
  • sale_delivery_state_view (glue module when sale_stock is not installed, it will show the field)

It's sad to have to do 3 modules for this, but we do not have choice...

@sebastienbeau
Copy link
Member Author

@rousseldenis @lmignon needed for having clean state on shopinvader

@sebastienbeau sebastienbeau force-pushed the 16.0-MIG-sale_delivery_state branch from 25ed927 to 0d875dc Compare November 9, 2023 14:13
@sebastienbeau
Copy link
Member Author

@yostashiro seem to be good idea, I going to change it today
@flotho a review will be welcome (maybe just after the change propose by @yostashiro)

@sebastienbeau sebastienbeau force-pushed the 16.0-MIG-sale_delivery_state branch from 17ab6e0 to 5ed05ed Compare November 15, 2023 11:21
@sebastienbeau sebastienbeau force-pushed the 16.0-MIG-sale_delivery_state branch from 5ed05ed to aabc76a Compare November 15, 2023 11:22
@sebastienbeau
Copy link
Member Author

@flotho @yostashiro the PR is ready for the review. I will squash my commit before merging it

Copy link
Contributor

@lmignon lmignon left a comment

Choose a reason for hiding this comment

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

Code review only

@sebastienbeau
Copy link
Member Author

@flotho @yostashiro ping, thanks in advance for your feedback

@sebastienbeau
Copy link
Member Author

@Kev-Roche can you review?

Copy link
Member

@yostashiro yostashiro left a comment

Choose a reason for hiding this comment

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

@sebastienbeau Would you consider adding a migration script?

Comment on lines 17 to 20

This module do not add the "views" for the field "delivery_status".
If you have the sale_stock module installed please installed "sale_stock_delivery_state".
If not install the module "sale_delivery_state_view".
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
This module do not add the "views" for the field "delivery_status".
If you have the sale_stock module installed please installed "sale_stock_delivery_state".
If not install the module "sale_delivery_state_view".
When the 'sale_stock' module is installed, the glue module 'sale_stock_delivery_state' should also be installed; this module is designed to override the compute method of the delivery status field from 'sale_stock'.

@sebastienbeau
Copy link
Member Author

@yostashiro I have added a post-init-hook. This should solve

  • migration from 14 to 16 => the field is recomputed so we have the right value
  • new installation on v16 => after the installation on an db that have sale_stock, the field is recomputed so all existing Sale Order will have the right value.

It's ok for you ?

@yostashiro
Copy link
Member

@sebastienbeau Thanks for the update. I think sale_delivery_state still requires a migration script for the scenario of upgrading a database with this module but no sale_stock. Simply moving post_init_hook() from sale_stock_delivery_state to sale_delivery_state might do the job, but I wasn't perfectly sure.

@sebastienbeau
Copy link
Member Author

sebastienbeau commented Nov 24, 2023

@yostashiro When migrating a database to V16, as the field "delivery_status" have changed of name ("delivery_state" before),
Odoo will consider it as a new field and will compute it and fill the value on all existing sale order.
So everything should work without any extra logic.

@yostashiro
Copy link
Member

@yostashiro When migrating a database to V16, as the field "delivery_status" have changed of name ("delivery_state" before), Odoo will consider it as a new field and will compute it and fill the value on all existing sale order. So everything should work without any extra logic.

@sebastienbeau Ah, I see. Missed that point. Thanks for the explanation!

@sebastienbeau
Copy link
Member Author

Thanks for the review let's merge it

/ocabot merge path

@OCA-git-bot
Copy link
Contributor

Hi @sebastienbeau. Your command failed:

Invalid options for command merge: path.

Ocabot commands

  • ocabot merge major|minor|patch|nobump
  • ocabot rebase
  • ocabot migration {MODULE_NAME}

More information

@sebastienbeau
Copy link
Member Author

/ocabot merge patch

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-2770-by-sebastienbeau-bump-patch, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit a0392e5 into OCA:16.0 Nov 24, 2023
7 of 8 checks passed
@OCA-git-bot
Copy link
Contributor

Congratulations, your PR was merged at e8df2b1. Thanks a lot for contributing to OCA. ❤️

@ValentinVinagre
Copy link
Contributor

Hey @pedrobaeza @sebastienbeau @yostashiro @HaraldPanten @etobella
Hello,
I thought that Oca was going to go down the path of using Odoo functionality. Will the native Odoo functionality be left aside in the end? According to the conversation with Pedrobaeza, it seemed that the way forward was going to be to use the native way of Odoo.

The discusion: OCA/purchase-workflow#2081

@pedrobaeza
Copy link
Member

I prefer Odoo native feature, but there's a lot of people in OCA, and they can think different. At least a mention in the README should be done about the differences between this module and the standard. Or maybe they didn't know about the core feature?

@HaraldPanten
Copy link
Contributor

I think they knew about the native Odoo feature, but the decision was to keep the OCA one, because it's more complete than de native one.

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

Successfully merging this pull request may close these issues.