search

coopiteasy / addons

Various modules that extend or improve Odoo Community.
GNU Affero General Public License v3.0
21 stars 17 forks source link

[16.0][MIG] pos_customer_wallet #300

Open carmenbianca opened 1 year ago

carmenbianca commented 1 year ago

Description

Odoo task (if applicable)

https://gestion.coopiteasy.be/web#id=10799&action=475&active_id=492&model=project.task&view_type=form&menu_id=536

Checklist before approval

codecov-commenter commented 1 year ago

Codecov Report

Merging #300 (8772350) into 16.0 (b640076) will decrease coverage by 3.22%. Report is 19 commits behind head on 16.0. The diff coverage is 86.00%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##             16.0     #300      +/-   ##
==========================================
- Coverage   97.73%   94.52%   -3.22%     
==========================================
  Files          12       26      +14     
  Lines         221      347     +126     
  Branches       30       39       +9     
==========================================
+ Hits          216      328     +112     
- Misses          1       15      +14     
  Partials        4        4
Files Changed Coverage Δ
pos_customer_wallet/models/pos_session.py 40.00% <40.00%> (ø)
pos_customer_wallet/models/pos_config.py 58.33% <58.33%> (ø)
account_customer_wallet/tests/common.py 94.11% <100.00%> (+0.17%) :arrow_up:
pos_customer_wallet/__init__.py 100.00% <100.00%> (ø)
pos_customer_wallet/models/__init__.py 100.00% <100.00%> (ø)
pos_customer_wallet/models/pos_payment_method.py 100.00% <100.00%> (ø)
pos_customer_wallet/models/res_partner.py 100.00% <100.00%> (ø)
pos_customer_wallet/tests/__init__.py 100.00% <100.00%> (ø)
pos_customer_wallet/tests/common.py 100.00% <100.00%> (ø)
pos_customer_wallet/tests/test_balance.py 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

carmenbianca commented 1 year ago

I think this is ported as far as I'm going to be able to port it for the moment.

Some notes:

legalsylvain commented 1 year ago

Thanks a lot !

I should probably write tests for the JavaScript…

;-)

I removed base_suspend_security as a dependency. @legalsylvain I know that you introduced this dependency, and I think it had to do with multi-company support, but I cannot well remember why it was there. There should be a test for the use case.

let's start like that, and see if we face some issue.

legalsylvain commented 1 year ago

Is it ready to review ?

carmenbianca commented 1 year ago

@legalsylvain yes, i think!

carmenbianca commented 1 year ago

Problem found during functional testing: The balance is not reduced when making POS payments with the child of a partner. Needs further investigation.

carmenbianca commented 1 year ago

Ignore previous comment. I was testing the wrong payment method ('Customer Account' instead of 'Customer Wallet')

I did find another bug, though. After closing the session, all changes to the customer wallet balance disappear, as if the POS session never happened.

carmenbianca commented 1 year ago

Interesting. The account.move.lines generated by closing the POS do not contain partner_ids. This is going to be painful to research.

legalsylvain commented 1 year ago

Hi @carmenbianca. Thanks for porting this module.

Just did some tests.

Globally it works !

Regarding your problem.

Interesting. The account.move.lines generated by closing the POS do not contain partner_ids. This is going to be painful to research.

It is partially wrong. for the payment account move, if you check the checkbox split_transactions AND set the outstanding account : it will :

Ex : payment of 5€ with wallet :

image

BUT : if you sale "wallet product", AFAIK, the account move does'nt contain the partner, if it is not possible to credit the wallet, saling wallet in the PoS.

Ex : credit 999€, saling wallet product : image

Do you know how we could fix it ?

carmenbianca commented 1 year ago

Thanks for the tip on split_transactions @legalsylvain

BUT : if you sale "wallet product", AFAIK, the account move does'nt contain the partner, if it is not possible to credit the wallet, saling wallet in the PoS.

Ex : credit 999€, saling wallet product :

Do you know how we could fix it ?

I've been looking at this for a while. I'm not sure what a good solution is. When closing the session, it correctly gets the income account from the wallet product for each pos.order.line that sells a wallet product. However, the amount for every such pos.order.line is eventually accumulated/aggregated into a single partner-less account.move.line on the session's account move.

There exists no split_transactions that we can use here, and the code does not seem to have the right hooks for us to force such a behaviour.

Furthermore, the session's account move is on the POS journal, not on the customer wallet journal. If we were to succeed in splitting the lines on the session's account move, we might still run afoul of some expectations made in account_customer_wallet. Trivially fixed, I think, but important to keep in mind.

Some solutions that I can think of, but haven't thought through:

  1. Like above, try to force split_transactions behaviour on the session's account move. There are no good hooks for this, so we might have to do it retroactively. I'm not sure I like the idea of retroactively adjusting account moves.
  2. Do some creative accounting. After the POS session is closed, create enough account moves to solve the problem, reading from the pos.order.lines.
  3. Don't do anything. Or rather, include the pos.order.lines (with wallet products) when calculating the balance. We already do some form of this for open pos sessions. Furthermore, the customer wallet is already credited the correct amount when closing the session.

I will expand later.

carmenbianca commented 1 year ago

Now is later.

All three options proposed above have some flaws.

A full example. Find below the account.move of the POS session, where Alice bought 800€ of wallet product, and Bob bought 200€:

Account Partner Debit Credit
Account Receivable (PoS) 1000
Customer Wallet 1000

Option 1: Split on the pos session's account move

I've explained the flaws of option 1 fairly well. The account move is on the wrong journal, retroactively editing a closed account move seems like a bad idea, and there are no good hooks to make this happen.

But if we did make it happen, we would turn the example account move into this:

Account Partner Debit Credit
Account Receivable (PoS) 1000
Customer Wallet Alice 800
Customer Wallet Bob 200

Option 2: Do creative accounting after the pos session closes

Option 2 is fine-ish, if a little verbose and difficult to retroactively comprehend. You have to undo the account.move.line against the customer wallet account and create new per-partner lines.

Given the example, we would create one or two new moves that look a little like this:

Two moves against a temporary account

Account Partner Debit Credit
Temporary 1000
Customer Wallet 1000

and

Account Partner Debit Credit
Temporary 1000
Customer Wallet Alice 800
Customer Wallet Bob 200

One move against the wallet account itself

Not sure if this is possible.

Account Partner Debit Credit
Customer Wallet 1000
Customer Wallet Alice 800
Customer Wallet Bob 200

Option 3: Compute the balance from pos orders; don't touch the account moves any further

Option 3 seems fairly easy, but I'm not certain it's a good idea. One ramification is that you cannot uninstall pos_customer_wallet without data loss to the balances. This is not presently the case if all POS sessions are closed, I think.

But overall, I just feel like it would be (mis)using pos orders for the wrong purpose.

legalsylvain commented 1 year ago

Thanks a lot @carmenbianca for all the investigations !

Option 3 Well. I don't think option 3 is OK : For the reason you mentionned, about the uninstallation, but also because the information, regarding the credit should be in account.move.line. It's an important information. If some bridges are done with other accounting software, the data should be exported. (for exemple, in a software for accountant experts).

The account move is on the wrong journal,

I'm not sure. for me saling wallet product should go in sale journal. Don't you think ? I'm pretty sure it is the current behaviour in v12.

Regarding option 2, I think writing a single extra account move is OK in a accounting point of view.

For the time being, I think option 1 has my preference, but I read the code that prepare the account move lines, and it is just horrible. ;-) So maybe option 2 will be mandatory.

What is your deadline for this developpement ? we could make a little code sprint during the oca days?

regards.

carmenbianca commented 1 year ago

@legalsylvain We can definitely sprint on this at OCA Days!

I agree with you on option 3.

carmenbianca commented 5 months ago

@legalsylvain I wrote an implementation for this finally! It was tricky to get right, but it implements option 1.

It needs heaps more tests before this can be merged, but I thought I'd notify you.

legalsylvain commented 4 months ago

I have to review this one ! thanks for the ping !

polchampion commented 3 weeks ago

@carmenbianca à combien d'heures estimes-tu la finalisation du portage ?