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_product_variant_attribute_tax: Migration to version 16.0 #354

Merged

Conversation

carolinafernandez-tecnativa
Copy link

@carolinafernandez-tecnativa carolinafernandez-tecnativa commented May 16, 2024

@Tecnativa
TT46598

@pedrobaeza @pilarvargas-tecnativa

@pedrobaeza
Copy link
Member

/ocabot migration sale_product_variant_attribute_tax

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone May 17, 2024
@OCA-git-bot OCA-git-bot mentioned this pull request May 17, 2024
9 tasks
Ernesto Tejeda and others added 3 commits June 26, 2024 10:07
@victoralmau victoralmau force-pushed the 16.0-mig-sale_product_variant_attribute_tax branch from 453582f to 4948595 Compare June 26, 2024 08:07
@pedrobaeza
Copy link
Member

cc @carolinafernandez-tecnativa

@carolinafernandez-tecnativa carolinafernandez-tecnativa force-pushed the 16.0-mig-sale_product_variant_attribute_tax branch from 4948595 to 5f8688f Compare July 4, 2024 16:02
@pedrobaeza
Copy link
Member

@victoralmau is it OK for you?

@victoralmau
Copy link
Member

Thinking again about this functionality, I don't understand not included in sale_variant_configurator (is probably related to i don't understand the need for product_variant_attribute_tax because you could always use the template taxes, but that's another debate).

@pedrobaeza
Copy link
Member

Let me explain this feature, that is somehow independent of sale_variant_configurator, but right now it's linked for not doing 2 modules.

The question is that the product taxes are defined at template level, so you can't have some variants with an specific tax. The use case of our customer is that one attribute makes that all the variants with such attribute has different taxes. This module + the other one covers this gap adding the taxes at attribute level for having to define it only once, instead of searching for other solution like having taxes at template level that forces you to define the specific taxes for each of the variants that are affected.

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

  • Please squash the first 3 commits into one.
  • You must split your migration into 2 commits: the pre-commit one + the real migration one.
  • Attend the rest of the inline comments.

@victoralmau
Copy link
Member

Let me explain this feature, that is somehow independent of sale_variant_configurator, but right now it's linked for not doing 2 modules.

The question is that the product taxes are defined at template level, so you can't have some variants with an specific tax. The use case of our customer is that one attribute makes that all the variants with such attribute has different taxes. This module + the other one covers this gap adding the taxes at attribute level for having to define it only once, instead of searching for other solution like having taxes at template level that forces you to define the specific taxes for each of the variants that are affected.

Ok, now I understand the requirement, thanks for clarifying.

@pedrobaeza
Copy link
Member

@carolinafernandez-tecnativa can you please finish this. You can include my previous reasoning as CONTEXT.rst in the readme.

@carolinafernandez-tecnativa carolinafernandez-tecnativa force-pushed the 16.0-mig-sale_product_variant_attribute_tax branch 2 times, most recently from b60f7a2 to 2124d2a Compare July 12, 2024 18:18
@carolinafernandez-tecnativa
Copy link
Author

@carolinafernandez-tecnativa can you please finish this. You can include my previous reasoning as CONTEXT.rst in the readme.

done

@carolinafernandez-tecnativa
Copy link
Author

  • Please squash the first 3 commits into one.
  • You must split your migration into 2 commits: the pre-commit one + the real migration one.
  • Attend the rest of the inline comments.

pending to do

@carolinafernandez-tecnativa
Copy link
Author

@victoralmau could you please help me with this?thanks!

@victoralmau victoralmau force-pushed the 16.0-mig-sale_product_variant_attribute_tax branch from 2124d2a to c2d9eb1 Compare July 15, 2024 10:12
@carolinafernandez-tecnativa
Copy link
Author

ping @pedrobaeza

@carolinafernandez-tecnativa carolinafernandez-tecnativa force-pushed the 16.0-mig-sale_product_variant_attribute_tax branch from c2d9eb1 to 9ab2d7f Compare July 30, 2024 08:59
@carolinafernandez-tecnativa
Copy link
Author

ping @pedrobaeza

Copy link
Member

@pedrobaeza pedrobaeza left a comment

Choose a reason for hiding this comment

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

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 16.0-ocabot-merge-pr-354-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 4dc5721 into OCA:16.0 Aug 2, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 16.0-mig-sale_product_variant_attribute_tax branch August 2, 2024 07:54
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.

5 participants