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

[14.0][MIG] sale_partner_version: Migration to v14.0 #1348

Merged
merged 10 commits into from
Sep 28, 2021

Conversation

kevinkhao
Copy link
Contributor

Copy link
Member

@atchuthan atchuthan 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 👍

@kevinkhao kevinkhao force-pushed the 14.0-mig-sale_partner_version branch from 1091830 to 6437ab4 Compare March 17, 2021 08:52
@hparfr
Copy link

hparfr commented May 25, 2021

@kevinkhao can you please rebase ?

@atchuthan
Copy link
Member

@kevinkhao
There seem to be conflicts. Please resolve it

Conflicting files
sale_exception/README.rst
sale_exception/manifest.py
sale_exception/data/sale_exception_data.xml
sale_exception/i18n/es.po
sale_exception/i18n/sale_exception.pot
sale_exception/models/sale.py
sale_exception/static/description/index.html

@kevinkhao kevinkhao force-pushed the 14.0-mig-sale_partner_version branch 2 times, most recently from 551ac62 to 67492cb Compare June 1, 2021 07:42
@kevinkhao
Copy link
Contributor Author

kevinkhao commented Jun 1, 2021

@hparfr should be good
could you pls help check if this follows convention 7b3ea80

@kevinkhao kevinkhao force-pushed the 14.0-mig-sale_partner_version branch from 67492cb to 7b3ea80 Compare June 1, 2021 15:59
@hparfr
Copy link

hparfr commented Jun 1, 2021

You can may be populate the excludes key in the manifest like in account_e-invoice_generate. About the .travis file I do not know.
https://github.com/OCA/edi/blob/12.0/account_e-invoice_generate/__manifest__.py#L15

@kevinkhao kevinkhao force-pushed the 14.0-mig-sale_partner_version branch from 7b3ea80 to 9b74355 Compare June 2, 2021 07:36
@kittiu
Copy link
Member

kittiu commented Aug 9, 2021

@kevinkhao I functional test this, and it works fine. I got some trivial comments,

  • Should we remove the word (copy) from the archived address. Because sometime, it will also get printed in the form.
  • The readme is rather minimum, it took me a while to make sense of it. Can we add more explanation, i.e.,
This module uses the partner_address_version module to create a partner version when confirming a sale order.

By default, this module ensure that the address fields (Invoice Address, Shipping address) are immutable.
This works by ensure that, whenever a sale order is confirmed, the address fields of the sale order will be copied (and immediately archived), so it will used by this sale order only.

@kittiu
Copy link
Member

kittiu commented Aug 9, 2021

By the way, is this technique being implemented in other modules yet? I.e., purchase order. I am interested to make one.

@kevinkhao
Copy link
Contributor Author

  • Should we remove the word (copy) from the archived address. Because sometime, it will also get printed in the form.

I think if we remove (copy) it is less easy to differentiate between archived versions, both for a user and for other module that maybe uses the name of a partner as a key or something like that. Then we could add (archived) or something like that ?

  • The readme is rather minimum, it took me a while to make sense of it. Can we add more explanation, i.e.,

Yes, thanks for the suggestion

This module uses the partner_address_version module to create a partner version when confirming a sale order.

By default, this module ensure that the address fields (Invoice Address, Shipping address) are immutable.
This works by ensure that, whenever a sale order is confirmed, the address fields of the sale order will be copied (and immediately archived), so it will used by this sale order only.

@kittiu
Copy link
Member

kittiu commented Aug 11, 2021

I think if we remove (copy) it is less easy to differentiate between archived versions, both for a user and for other module that maybe uses the name of a partner as a key or something like that. Then we could add (archived) or something like that ?

Thanks, I see it is very subjective. Let's leave it as is.

@OCA-git-bot
Copy link
Contributor

This PR has the approved label and has been created more than 5 days ago. It should therefore be ready to merge by a maintainer (or a PSC member if the concerned addon has no declared maintainer). 🤖

@kevinkhao
Copy link
Contributor Author

@kittiu also I forgot to answer the part about using it elsewhere: as far as I know partner_address_version is not used elsewhere in OCA, we have some custom modules that use it though
https://github.com/akretion/sale-import
for creating sale orders through a rest API

@newtratip
Copy link
Member

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 14.0-ocabot-merge-pr-1348-by-newtratip-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit c4da7a7 into OCA:14.0 Sep 28, 2021
@OCA-git-bot
Copy link
Contributor

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

norlinhenrik pushed a commit to loymcom/sale-workflow that referenced this pull request Sep 20, 2023
Signed-off-by pedrobaeza
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.

9 participants