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

[13.0][IMP] base_ubl: hook to add the AdditionalAccountID ubl field #429

Merged
merged 1 commit into from Sep 1, 2021

Conversation

FerranCforgeFlow
Copy link
Contributor

Added a _ubl_get_additional_reference method to be able to add the AdditionalAccountID ubl field in the _ubl_add_supplier_party

@OCA-git-bot
Copy link
Contributor

Hi @FerranCforgeFlow! Thank you very much for this contribution. As the addon you are improving does not have a declared maintainer, I take the opportunity to mention that you can consider adopting it. To do so, please read the maintainer role description, and, if interested, create a pull request to add your GitHub login to the maintainers key of the addon manifest.

@FerranCforgeFlow
Copy link
Contributor Author

@bealdav could you check?

Copy link
Contributor

@RaulOForgeFlow RaulOForgeFlow left a comment

Choose a reason for hiding this comment

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

👍

@FerranCforgeFlow
Copy link
Contributor Author

@bealdav could you please check?

@FerranCforgeFlow FerranCforgeFlow force-pushed the 13.0-imp-base_ubl branch 2 times, most recently from bf818fa to b108c7e Compare August 30, 2021 09:15
Copy link
Contributor

@albariera albariera left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@bealdav bealdav left a comment

Choose a reason for hiding this comment

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

Ok, thanks
I ask myself if there is no other way to add new misc nodes dynamically (method with parameter).
It seems there many conditional additions in the code now.
But if we need to do smth, it could be in an other PR I suppose

@etobella
Copy link
Member

Wouldn't be easier to return the node customer_party_root and anyone can do its specific changes if needed?? (For example for the partner reference or the additional Account Id)

@FerranCforgeFlow FerranCforgeFlow force-pushed the 13.0-imp-base_ubl branch 2 times, most recently from c126989 to e7065d8 Compare August 30, 2021 09:51
@FerranCforgeFlow
Copy link
Contributor Author

What you are saying is actually a more solid option. I opted for the alternative of adding generic methods that people can inherit if wanted.

@FerranCforgeFlow
Copy link
Contributor Author

Could it be merged?

@etobella
Copy link
Member

Might be possible to apply the proposed change? This way it is easier and all this kind of hooks are unnecessary.

…tomization

Return both, supplier_party_root and customer_party_root in _ubl_add_supplier_party and _ubl_add_customer_party, to allow customization
@FerranCforgeFlow
Copy link
Contributor Author

Taking a look at the code, it is better to return the nodes as you said to allow customization. I have already changed it in both, customer and supplier methods. Thanks for the advice!

@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). 🤖

@etobella
Copy link
Member

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 13.0-ocabot-merge-pr-429-by-etobella-bump-minor, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Aug 31, 2021
Signed-off-by etobella
@OCA-git-bot
Copy link
Contributor

@etobella your merge command was aborted due to failed check(s), which you can inspect on this commit of 13.0-ocabot-merge-pr-429-by-etobella-bump-minor.

After fixing the problem, you can re-issue a merge command. Please refrain from merging manually as it will most probably make the target branch red.

@LoisRForgeFlow
Copy link
Contributor

@etobella could you try again the merge?

@bealdav
Copy link
Member

bealdav commented Sep 1, 2021

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 13.0-ocabot-merge-pr-429-by-bealdav-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 2bdc8e4 into OCA:13.0 Sep 1, 2021
@OCA-git-bot
Copy link
Contributor

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

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.

None yet

7 participants