OCA / pos

GNU Affero General Public License v3.0
285 stars 602 forks source link

[16.0][MIG] pos_report_session_summary: Migration to version 16.0 #1229

Closed carlos-lopez-tecnativa closed 2 months ago

carlos-lopez-tecnativa commented 3 months ago

@Tecnativa TT49806

Supersedes https://github.com/OCA/pos/pull/1190

legalsylvain commented 3 months ago

/ocabot migration pos_report_session_summary

OCA-git-bot commented 3 months ago

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 commented 3 months ago

/ocabot migration pos_report_session_summary

anajuaristi commented 3 months ago

@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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

@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 commented 3 months ago

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 commented 3 months ago

@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 commented 2 months ago

@anajuaristi, could you please review again?

anajuaristi commented 2 months ago

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 commented 2 months ago

Right now it's the case, so merging:

/ocabot merge nobump

OCA-git-bot commented 2 months ago

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 commented 2 months ago

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