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][ADD] account_check_report #247

Merged
merged 4 commits into from
Nov 6, 2019
Merged

[10.0][ADD] account_check_report #247

merged 4 commits into from
Nov 6, 2019

Conversation

AdriaGForgeFlow
Copy link
Contributor

Add account_check_report module to v10.

This module is intended to print a report of invoice payments done through a check.
image

cc ~ @aheficent

AaronHForgeFlow
AaronHForgeFlow reviewed Apr 8, 2019
View reviewed changes
Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I seems the check amount is overlapped when having many lines. Can you check @ageficent ?

image

@AdriaGForgeFlow
Copy link
Contributor Author

I seems the check amount is overlapped when having many lines. Can you check @ageficent ?

Can you check now? Thanks!

@AdriaGForgeFlow AdriaGForgeFlow force-pushed the 10.0-add-account_check_report branch from 4e55491 to 950cf73 Compare April 9, 2019 06:17
AaronHForgeFlow
AaronHForgeFlow approved these changes Apr 9, 2019
View reviewed changes
Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

👍 It works like a charm! Thanks

LoisRForgeFlow
LoisRForgeFlow approved these changes Apr 11, 2019
View reviewed changes
Copy link

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

LGTM 👍

@AaronHForgeFlow
Copy link
Contributor

Please do not merge this until this question is answered: ForgeFlow#2

AdriaGForgeFlow
AdriaGForgeFlow commented Jun 3, 2019
View reviewed changes
Copy link
Contributor Author

@AdriaGForgeFlow AdriaGForgeFlow left a comment

Choose a reason for hiding this comment

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

Last commits LGTM 👍

@max3903 max3903 added this to the 10.0 milestone Jun 3, 2019
max3903
max3903 reviewed Jun 3, 2019
View reviewed changes
account_check_report/report/account_check_report.xml
name="account_check_report.check_report"
file="account_check_report.check_report"/>

</odoo>
Copy link
Member

Choose a reason for hiding this comment

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

Missing newline.

Otherwise 👍

@AaronHForgeFlow
Copy link
Contributor

Please do not merge this until this question is answered: Eficent#2

After the refactoring, please dismiss the above quoted comment

AaronHForgeFlow
AaronHForgeFlow requested changes Sep 2, 2019
View reviewed changes
Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

Hi @ageficent , it seems the refunded bills are excluded in the _get_paid_lines method. Can you check?

Thank you!

@AdriaGForgeFlow
Copy link
Contributor Author

Hi @ageficent , it seems the refunded bills are excluded in the _get_paid_lines method. Can you check?

Thank you!

Could you clarify a little bit the steps I need to follow to reproduce the error? I don't quite understand the issue.
Thanks!

@AaronHForgeFlow
Copy link
Contributor

Could you clarify a little bit the steps I need to follow to reproduce the error? I don't quite understand the issue.
Thanks!

Sure: Create a vendor bill and a vendor refund for the same supplier. Then create a check payment for that supplier. Then reconcile the payment with the invoice and the refund. Then, in this report I should see both, the vendor bill and the refund.

It seems there are some cases were the refund is not showing, but I am not completely sure about the reason. I will also try to take a look at this tomorrow.

@JordiBForgeFlow
Copy link
Member

@ageficent @aheficent the latest commit fixes this problem. The way to solve it is rather tricky. See my comment in the code. Do you understand it?

image

AdriaGForgeFlow
AdriaGForgeFlow commented Sep 4, 2019
View reviewed changes
Copy link
Contributor Author

@AdriaGForgeFlow AdriaGForgeFlow left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@ageficent @aheficent the latest commit fixes this problem. The way to solve it is rather tricky. See my comment in the code. Do you understand it?

Understood 👍 Thanks!

@AdriaGForgeFlow AdriaGForgeFlow force-pushed the 10.0-add-account_check_report branch from 18c6bf3 to d47d85d Compare September 9, 2019 09:51
@AaronHForgeFlow
Copy link
Contributor

@jbeficent I have done a functional review to this module and seems to work ok now. Did you find a case where the report does not work?

@AdriaGForgeFlow AdriaGForgeFlow force-pushed the 10.0-add-account_check_report branch from d47d85d to 6838301 Compare September 9, 2019 11:52
@AaronHForgeFlow
Copy link
Contributor

It seems last commit 6838301 does the correct reconciliation with refunds.

AaronHForgeFlow
AaronHForgeFlow approved these changes Oct 31, 2019
View reviewed changes
Copy link
Contributor

@AaronHForgeFlow AaronHForgeFlow left a comment

Choose a reason for hiding this comment

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

👍 Could not find anything wrong in this PR

MiquelRForgeFlow
MiquelRForgeFlow approved these changes Oct 31, 2019
View reviewed changes
Copy link
Contributor

@MiquelRForgeFlow MiquelRForgeFlow left a comment

Choose a reason for hiding this comment

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

LGTM

@AaronHForgeFlow
Copy link
Contributor

Ready to merge

@JordiBForgeFlow
Copy link
Member

/ocabot merge

@OCA-git-bot
Copy link
Contributor

Hey, thanks for contributing! Proceeding to merge this for you.
Prepared branch 10.0-ocabot-merge-pr-247-by-jbeficent-bump-no, awaiting test results.

OCA-git-bot added a commit that referenced this pull request Nov 6, 2019
@OCA-git-bot
Merge PR #247 into 10.0
b9ab857 
Signed-off-by jbeficent
@OCA-git-bot OCA-git-bot merged commit 6838301 into OCA:10.0 Nov 6, 2019
@OCA-git-bot
Copy link
Contributor

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

@MiquelRForgeFlow MiquelRForgeFlow deleted the 10.0-add-account_check_report branch November 6, 2019 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MiquelRForgeFlow MiquelRForgeFlow MiquelRForgeFlow approved these changes

@LoisRForgeFlow LoisRForgeFlow LoisRForgeFlow approved these changes

@AaronHForgeFlow AaronHForgeFlow AaronHForgeFlow approved these changes

@max3903 max3903 max3903 approved these changes

Assignees

@AdriaGForgeFlow AdriaGForgeFlow

Labels
approved merged 🎉
Projects
None yet
Milestone
10.0
Development

Successfully merging this pull request may close these issues.
7 participants
@AdriaGForgeFlow @AaronHForgeFlow @JordiBForgeFlow @OCA-git-bot @max3903 @LoisRForgeFlow @MiquelRForgeFlow