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

[10.0] [IMP] base_transaction_id: Add transaction_id on account.payment #183

Merged
merged 3 commits into from Nov 2, 2020
Merged

[10.0] [IMP] base_transaction_id: Add transaction_id on account.payment #183

merged 3 commits into from Nov 2, 2020

Conversation

lmignon
Copy link
Sponsor

@lmignon lmignon commented Sep 13, 2017 •
edited

This change is required to ease the reconciliation of the account.move
created from the account.payment.

@lmignon lmignon force-pushed the 10.0-transaction_id_account_payment-lmi branch 2 times, most recently from e4df4c1 to 4867609 Compare September 13, 2017 08:51
@lmignon lmignon mentioned this pull request Sep 13, 2017
Merged
2 tasks
dvdhinesh
dvdhinesh approved these changes Sep 27, 2017
View reviewed changes
Copy link
Member

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

@adrienpeiffer adrienpeiffer added this to the 10.0 milestone Oct 2, 2017
adrienpeiffer
adrienpeiffer reviewed Oct 2, 2017
View reviewed changes
base_transaction_id/models/account_payment.py
vals = super(AccountPayment, self)._get_liquidity_move_line_vals(
amount)
if self.transaction_id:
vals['name'] = self.transaction_id
Copy link

Choose a reason for hiding this comment

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

wouldn't it be better to fill transaction_ref (as is done in this module) ?

Copy link
Sponsor

Choose a reason for hiding this comment

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

transaction_id is actually the transaction reference ...

atchuthan
atchuthan approved these changes Feb 7, 2018
View reviewed changes
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 👍

@Cedric-Pigeon
Copy link

Travis is red because of another addon

@Cedric-Pigeon Cedric-Pigeon force-pushed the 10.0-transaction_id_account_payment-lmi branch from f1e8585 to 95676b1 Compare March 29, 2018 09:30
@Cedric-Pigeon Cedric-Pigeon force-pushed the 10.0-transaction_id_account_payment-lmi branch from 95676b1 to bd43501 Compare September 30, 2019 10:04
@Cedric-Pigeon
Copy link

@OCA/accounting-maintainers Does anyone can merge this one?

rousseldenis
rousseldenis approved these changes Oct 1, 2019
View reviewed changes
Copy link
Sponsor

@rousseldenis rousseldenis 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

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

@rousseldenis
Copy link
Sponsor

@sbidoul This one is ready and in production for years

@sbidoul
Copy link
Member

sbidoul commented Nov 5, 2019

@sbidoul This one is ready and in production for years

@rousseldenis @lmignon is it correct that we'll abandon this approach in 13 (and maybe even 12)? In which case it might be better to not merge it?

@rousseldenis
Copy link
Sponsor

@sbidoul This one is ready and in production for years

@rousseldenis @lmignon is it correct that we'll abandon this approach in 13 (and maybe even 12)? In which case it might be better to not merge it?

Yes, but the module exists already in 10.0 branch. This is an enhancement/fix for the actual installs.

@gurneyalex
Copy link
Member

/ocabot merge minor

@OCA-git-bot
Copy link
Contributor

On my way to merge this fine PR!
Prepared branch 10.0-ocabot-merge-pr-183-by-gurneyalex-bump-minor, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit 0a5c2b3 into OCA:10.0 Nov 2, 2020
@OCA-git-bot
Copy link
Contributor

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

@sbidoul sbidoul deleted the 10.0-transaction_id_account_payment-lmi branch November 2, 2020 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adrienpeiffer adrienpeiffer adrienpeiffer left review comments

@rousseldenis rousseldenis rousseldenis approved these changes

@gurneyalex gurneyalex gurneyalex approved these changes

@atchuthan atchuthan atchuthan approved these changes

@dvdhinesh dvdhinesh dvdhinesh approved these changes

Assignees
No one assigned
Labels
approved merged 🎉 needs review ready to merge
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@lmignon @Cedric-Pigeon @OCA-git-bot @rousseldenis @sbidoul @gurneyalex @atchuthan @dvdhinesh @adrienpeiffer