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.

Sign up for GitHub

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

Jump to bottom

[18.0][MIG] product_pricelist_fixed_currency_rate: Migration to 18.0 #1793

Merged
merged 10 commits into from
Dec 23, 2024
Merged

[18.0][MIG] product_pricelist_fixed_currency_rate: Migration to 18.0 #1793

merged 10 commits into from
Dec 23, 2024

Conversation

HeliconiaSolutions
Copy link

No description provided.

LoisRForgeFlow and others added 9 commits December 2, 2024 10:18
@LoisRForgeFlow @HeliconiaSolutions
[ADD] product_pricelist_fixed_currency_rate
4f9ccc0
@HeliconiaSolutions
[UPD] Update product_pricelist_fixed_currency_rate.pot
67d81c1
@mymage @HeliconiaSolutions
Added translation using Weblate (Italian)
810a89c
@mymage @HeliconiaSolutions
Translated using Weblate (Italian)
09f48ff 
Currently translated at 100.0% (12 of 12 strings)

Translation: product-attribute-16.0/product-attribute-16.0-product_pricelist_fixed_currency_rate
Translate-URL: https://translation.odoo-community.org/projects/product-attribute-16-0/product-attribute-16-0-product_pricelist_fixed_currency_rate/it/
@Ivorra78 @HeliconiaSolutions
Added translation using Weblate (Spanish)
a51596e
@Ivorra78 @HeliconiaSolutions
Translated using Weblate (Spanish)
86cdc25 
Currently translated at 100.0% (12 of 12 strings)

Translation: product-attribute-16.0/product-attribute-16.0-product_pricelist_fixed_currency_rate
Translate-URL: https://translation.odoo-community.org/projects/product-attribute-16-0/product-attribute-16-0-product_pricelist_fixed_currency_rate/es/
@kobros-tech @HeliconiaSolutions
[MIG] product_pricelist_fixed_currency_rate: Migration to 17.0
af98cb1
@HeliconiaSolutions
[UPD] Update product_pricelist_fixed_currency_rate.pot
671a292
@OCA-git-bot @HeliconiaSolutions
[BOT] post-merge updates
9efd078
@HeliconiaSolutions
Copy link
Author

/ocabot migration product_pricelist_fixed_currency_rate

@OCA-git-bot
Copy link
Contributor

Sorry @HeliconiaSolutions you are not allowed to mark the addon tobe migrated.

To do so you must either have push permissions on the repository, or be a declared maintainer of all modified addons.

If you wish to adopt an addon and become it's maintainer, open a pull request to add your GitHub login to the maintainers key of its manifest.

@HeliconiaSolutions
Copy link
Author

Hi @LoisRForgeFlow,

Could you please review this pull request when you have a moment? I would appreciate any feedback or suggestions for improvement.

Thank you for your time!

@LoisRForgeFlow
Copy link
Contributor

Hi @HeliconiaSolutions

Thanks for your contribution, before reviewing I would like that CI runs its checks, I cannot accept the workflow so we need to ask @OCA/product-maintainers to do so.

At the same time, I'm not sure what is the OCA policy regarding contributions from a company account, not sure if it is required that contributions come from individual accounts. @pedrobaeza @etobella maybe one of you can clarify this?

Kind regards,

@pedrobaeza
Copy link
Member

There's no strict prohibition of these accounts, but first of all, the CLA should be signed, and second, I don't like these techniques, but it's just a personal POV.

@rousseldenis
Copy link
Contributor

@LoisRForgeFlow Already stated about that on other PR's. Doing a PR with a global account is ok IMHO. But for commits, I tend to say they should have beeen done with an individual account.

@pedrobaeza The CLA bot was looking at commiters logins. And as only individuals are mentioned in CLAS (even there is a company one), the bot was blocking such PR's.

@pedrobaeza
Copy link
Member

Well, apart from not seeing the CLA bot warning since a while, I think it's incorrect to not warn in these cases. A possible workaround is to put this login in a child account or similar to act like an individual.

@LoisRForgeFlow
Copy link
Contributor

Yes, it is also a while since I see one of those CLA warnings

@pedrobaeza I also find it a bit weird to contribute with a company account. I see problems with responsibility and attribution; especially internally it must generate quite some headaches when the company have quite a few developers/contributors.

@LoisRForgeFlow Already stated about that on other PR's. Doing a PR with a global account is ok IMHO. But for commits, I tend to say they should have beeen done with an individual account.

This can be a good intermediate approach, to require that at least commits come from an individual.

@pedrobaeza
Copy link
Member

You have GitHub organizations for pushing the branches, so there's no sense in pushing under a company account.

@LoisRForgeFlow
Copy link
Contributor

Then, we are in a all-or-nothing decision and from what you said before, this has been already discussed and the decision (maybe with no full agreement) was that it is allowed to collaborate with company accounts. Am I getting it right?

@HeliconiaSolutions
Copy link
Author

There's no strict prohibition of these accounts, but first of all, the CLA should be signed, and second, I don't like these techniques, but it's just a personal POV.

@pedrobaeza, we’ve signed the entity CLA, which might be why you haven’t seen the CLA warning from the OCA bot.

@HeliconiaSolutions
Copy link
Author

Yes, it is also a while since I see one of those CLA warnings

@pedrobaeza I also find it a bit weird to contribute with a company account. I see problems with responsibility and attribution; especially internally it must generate quite some headaches when the company have quite a few developers/contributors.

@LoisRForgeFlow Already stated about that on other PR's. Doing a PR with a global account is ok IMHO. But for commits, I tend to say they should have beeen done with an individual account.

This can be a good intermediate approach, to require that at least commits come from an individual.

@LoisRForgeFlow @pedrobaeza
Thanks for sharing your thoughts, and we completely understand your concerns. We decided to adopt this approach because we've observed several companies doing the same, and it helps centralize contributions in a more unified manner. By using the company account, we aim to streamline the process and ensure that the entire organization shares responsibility and attribution for the work done.

That said, we're fully open to both individual and company-based contributions. We believe each approach has its merits, and we're happy to adapt based on what works best for the team and the project.

@pedrobaeza
Copy link
Member

Using the organization GitHub remote, any in the company can push to that branch, so about the flows, you won't have any restriction.

@HeliconiaSolutions HeliconiaSolutions mentioned this pull request Dec 17, 2024
Open
26 tasks
@HeliconiaSolutions
Copy link
Author

@pedrobaeza @LoisRForgeFlow @rousseldenis Thanks for the sharing the information.

As per our recent discussions, we have implemented the necessary changes on our side and have also signed the updated Individual CLA. Additionally, we’ve updated our profile to reflect these changes. To further support the OCA community, we’ve become proud members of OCA for 2025.

We’re excited about the opportunities for collaboration and look forward to working together with the entire OCA community to build even better open-source solutions.

@LoisRForgeFlow
Copy link
Contributor

Hi @HeliconiaSolutions

Thanks for that.

I would kindly as again @OCA/product-maintainers to accept the CI workflow of this PR, which is the first automatic review that is needed in order to move forward with this pull request.

Thanks!

sebalix
sebalix reviewed Dec 20, 2024
View reviewed changes
Copy link
Contributor

@sebalix sebalix left a comment

Choose a reason for hiding this comment

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

LG, minor comment

product_pricelist_fixed_currency_rate/tests/test_product_pricelist_fixed_currency_rate.py Outdated
Comment on lines 15 to 20
[
{
"name": "Test",
"currency_id": cls.env.ref("base.EUR").id,
}
]
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO this change (in the whole test file) is not required (create accepts a dictionary), and introduces diff that could be annoying when porting commits.

@HeliconiaSolutions HeliconiaSolutions force-pushed the 18.0-mig-product_pricelist_fixed_currency_rate branch from bb99f6b to de3f741 Compare December 23, 2024 05:59
@HeliconiaSolutions
Copy link
Author

Hello @sebalix,

Thanks for the feedback! I see your point that the change might not be necessary, especially since create already accepts a dictionary. I have reverted the changes to avoid adding unnecessary diffs and potential conflicts when porting commits.

LoisRForgeFlow
LoisRForgeFlow approved these changes Dec 23, 2024
View reviewed changes
Copy link
Contributor

@LoisRForgeFlow LoisRForgeFlow left a comment

Choose a reason for hiding this comment

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

Functional and code review LGTM

@LoisRForgeFlow
Copy link
Contributor

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

This PR looks fantastic, let's merge it!
Prepared branch 18.0-ocabot-merge-pr-1793-by-LoisRForgeFlow-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 906678f into OCA:18.0 Dec 23, 2024
6 of 7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@HeliconiaSolutions HeliconiaSolutions mentioned this pull request Dec 30, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LoisRForgeFlow LoisRForgeFlow LoisRForgeFlow approved these changes

@sebalix sebalix sebalix approved these changes

Assignees
No one assigned
Labels
approved merged 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@HeliconiaSolutions @OCA-git-bot @LoisRForgeFlow @pedrobaeza @rousseldenis @sebalix @mymage @Ivorra78 @kobros-tech