fau-fablab / FabLabKasse

FabLabKasse, a Point-of-Sale Software for FabLabs and other public and trust-based workshops
https://fablabkasse.readthedocs.io
GNU General Public License v3.0
15 stars 4 forks source link

FIX for recent crash - faucardpayment #150

Closed BDrescher closed 6 years ago

BDrescher commented 6 years ago

A crash can occur if the new balance of the card used equals 0 after payment. This is due to a simply wrong if clause and default values for the variables card_number, new_balance and old_balance.

In the previous case, the new balance had been equal to 0 which caused an assertion later on.

Feel free to add suggestions for the code and/or correct me if I did not fix the issue correctly. I had not been able to test the system with this commit version yet.

codecov-io commented 6 years ago

Codecov Report

Merging #150 into development will decrease coverage by 0.02%. The diff coverage is 0%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development     #150      +/-   ##
===============================================
- Coverage        27.17%   27.15%   -0.03%     
===============================================
  Files               62       62              
  Lines             7636     7642       +6     
===============================================
  Hits              2075     2075              
- Misses            5561     5567       +6
Impacted Files Coverage Δ
FabLabKasse/faucardPayment/FAUcardPaymentThread.py 14.87% <0%> (-0.29%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update f738a73...24fd370. Read the comment docs.

mgmax commented 6 years ago

I think this would work, but I have two suggestions for improvement:

It would be better to use None instead of -1, so that accidental addition properly fails (None + 5 raises TypeError, whereas -1 + 5 = 4)

Two other functions still use 0 as magic value:

Logging the status and balance values in FAUCardPaymentThread.run() would also be very helpful.

BDrescher commented 6 years ago

@mgmax Thank you for the contribution. Using None as a initialization value is a great idea. Seems like I am still thinking in constant variable type definitions.

I've additionally added debug logging to the FAUCardPaymentThread.run() function as suggested. If this kind of logging is too much, just point at superfluous entries.

mgmax commented 6 years ago

I think both _read_card and _decrease_balance can never return a default value, they either raise an exception or return valid values.