search

coopiteasy / addons

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

[ADD] customer balance on ticket #237

Closed remytms closed 2 years ago

remytms commented 2 years ago

Description

This features depends on this PR: https://github.com/coopiteasy/addons/pull/235

I don't know how to automate tests for that.

Odoo task (if applicable)

task

Checklist before approval

codecov-commenter commented 2 years ago

Codecov Report

Merging #237 (f1cf043) into 12.0 (0165aaa) will increase coverage by 0.16%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             12.0     #237      +/-   ##
==========================================
+ Coverage   70.11%   70.28%   +0.16%     
==========================================
  Files         115      117       +2     
  Lines        1596     1605       +9     
  Branches      311      312       +1     
==========================================
+ Hits         1119     1128       +9     
  Misses        443      443              
  Partials       34       34
Impacted Files Coverage Δ
account_customer_wallet/models/res_partner.py 92.85% <100.00%> (+0.75%) :arrow_up:
pos_customer_wallet/models/res_partner.py 90.47% <100.00%> (ø)
...customer_wallet_partner_is_user/models/__init__.py 100.00% <100.00%> (ø)
...tomer_wallet_partner_is_user/models/res_partner.py 100.00% <100.00%> (ø)

:mega: Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

remytms commented 2 years ago

@carmenbianca I agree with you, I'm in discussion with Pol to see if this is_customer_wallet_user is needed, because for me it brings only confusion and bug cases.

polchampion commented 2 years ago

@remytms @carmenbianca I agree that is_customer_waller_user as envisaged here isn't in keeping with the customer_wallet philosphy and poses practical problems - but the customers want a way to hide the total to non-users.

I anticipate the customers will want is_customer_waller_user to also hide the balance on the pos, and generally prevent non-users from using the wallet. A future development might also automatically set is_customer_waller_user to True when there is a movement on the customer wallet account for the corresponding partner. These improvements would come at a cost of complexity that seems too high compared to the benefit.

I see three options.

  1. Keep this PR as is and see about future devs later.
  2. Add is_customer_waller_user to account_customer_wallet BUT change the present PR to always display the balance on the ticket. Create an additional module that overcharges this behaviour and hides the ticket balance based on is_customer_waller_user.
  3. DO NOT add is_customer_waller_user to account_customer_wallet and make a now module for it. Change pos_customer_wallet to always display the balance on the ticket. Use another module to hide the balance based on is_customer_waller_user.

What would you recommend?

polchampion commented 2 years ago

@remytms @carmenbianca I would value your input on this (see above) in order to discuss it with Rémy and decide what to do.

carmenbianca commented 2 years ago

@polchampion I think option 2 or 3 both work.

polchampion commented 2 years ago

@remytms @carmenbianca I agree that is_customer_waller_user as envisaged here isn't in keeping with the customer_wallet philosphy and poses practical problems - but the customers want a way to hide the total to non-users.

I anticipate the customers will want is_customer_waller_user to also hide the balance on the pos, and generally prevent non-users from using the wallet. A future development might also automatically set is_customer_waller_user to True when there is a movement on the customer wallet account for the corresponding partner. These improvements would come at a cost of complexity that seems too high compared to the benefit.

I see three options.

1. Keep this PR as is and see about future devs later.

2. Add `is_customer_waller_user` to `account_customer_wallet` BUT change the present PR to always display the balance on the ticket. Create an additional module that overcharges this behaviour and hides the ticket balance based on `is_customer_waller_user`.

3. DO NOT add `is_customer_waller_user` to `account_customer_wallet` and make a now module for it. Change `pos_customer_wallet` to always display the balance on the ticket. Use another module to hide the balance based on `is_customer_waller_user`.

What would you recommend?

As discussed with @remytms we go for option 3 and a non-computed field. If and when a future development is requested to 1) make this field computed and 2) prevent pos customers from using the wallet based on it, then we might move the field to account_customer_wallet.

carmenbianca commented 2 years ago

Rebased this PR to remove is_customer_wallet_user functionality. I will re-introduce it in new commits. If for some reason I don't get to that and someone else begins work on this; I still have the is_customer_wallet_user commit tagged locally.

carmenbianca commented 2 years ago

Rebased on top of 12.0.

carmenbianca commented 2 years ago

/ocabot merge minor

github-grap-bot commented 2 years ago

This PR looks fantastic, let's merge it! Prepared branch 12.0-ocabot-merge-pr-237-by-carmenbianca-bump-minor, awaiting test results.

github-grap-bot commented 2 years ago

Congratulations, your PR was merged at 731bd7cf3d75d41abcc1da5b03696da6e743fb4d. Thanks a lot for contributing to coopiteasy. ❤️