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] pos_report_session_summary: Migration to version 16.0 #1229

Merged
merged 18 commits into from
Aug 29, 2024

Conversation

carlos-lopez-tecnativa
Copy link
Contributor

@carlos-lopez-tecnativa carlos-lopez-tecnativa commented Aug 1, 2024

@Tecnativa TT49806

Supersedes #1190

@legalsylvain
Copy link
Contributor

/ocabot migration pos_report_session_summary

@OCA-git-bot OCA-git-bot added this to the 16.0 milestone Aug 1, 2024
@OCA-git-bot
Copy link
Contributor

The migration issue (#849) has not been updated to reference the current pull request because a previous pull request (#1190) is not closed.
Perhaps you should check that there is no duplicate work.
CC @zamberjo

@pedrobaeza
Copy link
Member

/ocabot migration pos_report_session_summary

@OCA-git-bot OCA-git-bot mentioned this pull request Aug 2, 2024
37 tasks
@anajuaristi
Copy link

@carlos-lopez-tecnativa @pedrobaeza
Just a functional question. We are testing the PR and it works perfectly but we see that only cash payment methods are shown on summary.
IMHO all payment methods should be shown since it's a session sumary and not a cash closing summary.
What do you think about it?

@carlos-lopez-tecnativa
Copy link
Contributor Author

@carlos-lopez-tecnativa @pedrobaeza Just a functional question. We are testing the PR and it works perfectly but we see that only cash payment methods are shown on summary. IMHO all payment methods should be shown since it's a session sumary and not a cash closing summary. What do you think about it?

@anajuaristi I think what you propose is great; however, the detail is that we would now only use pos.payment and no longer account.bank.statement.line, is that correct? Since Odoo only creates statement lines for cash-type journals and only at the close of a session. @pedrobaeza , what do you think?

@pedrobaeza
Copy link
Member

If I'm not wrong, when you close the session, you get full details, isn't it? So that's a limitation on how Odoo works now.

@carlos-lopez-tecnativa
Copy link
Contributor Author

If I'm not wrong, when you close the session, you get full details, isn't it? So that's a limitation on how Odoo works now.

@pedrobaeza while the session is open, account.bank.statement.line(absl) records are not created, which is why the report only shows payments of type cash. When the session is closed, Odoo creates records in absl, but only for journals of type "cash." For other payment methods, account.move.line records are created instead of absl.

In previous versions, Odoo created absl for all journal types (both cash and bank), but now it only creates them for cash journals.

@pedrobaeza
Copy link
Member

Well, I didn't understand you correctly then the previous one. We obviously need to show everything. The problem I understood is that there's certain information duplicated when the session is closed. We don't want duplicates nor missing information. If we have to sacrifice that the information is not complete until the session is closed, then that's something that can be assumed (but put it on the ROADMAP).

@carlos-lopez-tecnativa
Copy link
Contributor Author

@pedrobaeza, let me know if, for this report, we should exclude account.bank.statement.line and if all data should be sourced from pos.payment. @anajuaristi, what do you think?

Remember that these fields in pos.session can't be used, and amounts must be recomputed only in the reports to show the summary. For context, refer to the Odoo code here: https://github.com/odoo/odoo/blob/ead5c4f365c6cb981b55f5ee26906e42fa4d449d/addons/point_of_sale/models/pos_session.py#L70-L89

When Session is closed
image

Bank statements only include payment type "cash":
image

pos.payments includes all payment types:
image

@pedrobaeza
Copy link
Member

If pos.payment includes everything, then let's use it. I'm just talking out of what you comment though. Anyway, I insist that the goal is to have a summary of all the payments, no matter the source. If this needs to be limited after the session is closed, then limit it.

@carlos-lopez-tecnativa
Copy link
Contributor Author

@pedrobaeza @anajuaristi
I've updated the roadmap because pos.payment does not include all the necessary information (e.g., Cash In/Out transactions do not generate a pos.payment). This makes it challenging to use pos.payment for including all payment methods. Please let me know if we can merge this module for now with these limitations or if you have any suggestions?.

@carlos-lopez-tecnativa
Copy link
Contributor Author

@anajuaristi, could you please review again?

@anajuaristi
Copy link

anajuaristi commented Aug 29, 2024

If pos.payment includes everything, then let's use it. I'm just talking out of what you comment though. Anyway, I insist that the goal is to have a summary of all the payments, no matter the source. If this needs to be limited after the session is closed, then limit it.

👍

About this " pos.payment does not include all the necessary information (e.g., Cash In/Out transactions do not generate a pos.payment)..."
in/out cash movement is not pos.payment so IMHO it's correct that is not shown as pos.payment. It's completely diferent but if several payment modes are allowed on pos (bank, credit car, cash.. etc) Final report should show all pos.payment independently of payment mode.

p.d: sorry for delay answering, I just come back from holidays

@pedrobaeza
Copy link
Member

Right now it's the case, so merging:

/ocabot merge nobump

@OCA-git-bot
Copy link
Contributor

What a great day to merge this nice PR. Let's do it!
Prepared branch 16.0-ocabot-merge-pr-1229-by-pedrobaeza-bump-nobump, awaiting test results.

@OCA-git-bot OCA-git-bot merged commit d84488e into OCA:16.0 Aug 29, 2024
7 checks passed
@OCA-git-bot
Copy link
Contributor

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

@pedrobaeza pedrobaeza deleted the 16.0-mig-pos_report_session_summary branch August 29, 2024 18:45
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.