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

[FIX] lang_LT, lang_LV: negative amounts #185

Merged
merged 1 commit into from Aug 23, 2018

Conversation

oerp-odoo
Copy link
Contributor

Negative amounts were not working (when no currency is used), because
get_digits method does not expect - sign, which crashes conversion.
To avoid that, we split minus sign from number string and prepare its
word to be used with amount words.

closes: #184

Fixes # by...

Changes proposed in this pull request:

Negative amounts for LT and LV conversion was not implemented and was crashing the conversion, so fix was committed (look for above for description).

Status

  • READY
  • HOLD
  • WIP (Work-In-Progress)

How to verify this change

Unittests for LT and LV were added to verify this change that it works with negative amounts (when currency is not used).

Negative amounts were not working (when no currency is used), because
`get_digits` method does not expect `-` sign, which crashes conversion.
To avoid that, we split minus sign from number string and prepare its
word to be used with amount words.

closes: savoirfairelinux#184
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 90.814% when pulling 23102da on focusate:master into 1ca8225 on savoirfairelinux:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 90.814% when pulling 23102da on focusate:master into 1ca8225 on savoirfairelinux:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 90.814% when pulling 23102da on focusate:master into 1ca8225 on savoirfairelinux:master.

@shulcsm
Copy link
Contributor

shulcsm commented Aug 23, 2018

It'd be nice to have single codepath for both cases but this should be fine.

@oerp-odoo
Copy link
Contributor Author

oerp-odoo commented Aug 23, 2018

Yes. it was already created this way. I guess it needs refactoring. Though I did not change original logic, because I don't know the reasoning for this. Maybe its just because overridden methods use same logic, but they inherit from same class?

Another thing I noticed is that if you convert amount to words with currency specified, then it does it correctly (even without this fix). I find it odd that converting to currency uses different logic than just converting to words without currency. Shouldn't it just be adding extra words in right places to specify which currency it is (and cents)?

@shulcsm
Copy link
Contributor

shulcsm commented Aug 23, 2018

It should, library needs heavy refactoring and generalization. Currency is the "newer" codepath that is why it's correct.
IMO this change should be fine for now. It doesn't make things worse and we get the test cases if anybody attempts the refactor later on.

@oerp-odoo
Copy link
Contributor Author

Though It could be moved that method _int2word and LT and LV to_cardinal (of course would need some new name to not clash with base's to_cardinal) implementation to base.py, but if as you say it needs heavy refactoring, this change might not do much anyway.

@ventilooo ventilooo added the bug label Aug 23, 2018
@ventilooo ventilooo merged commit 39f522f into savoirfairelinux:master Aug 23, 2018
@ventilooo ventilooo added this to Closed in Support via automation Aug 23, 2018
@ventilooo
Copy link
Contributor

@oerp-odoo Thanks for your PR and bug report.
@shulcsm Thanks for your review.

I merged in it's current state. Refactoring will come after. 😉

@oerp-odoo
Copy link
Contributor Author

@ventilooo how long till updated master is released on PYPI? Or should I forget that and just use source from github directly?

@ventilooo
Copy link
Contributor

@oerp-odoo It's not automated, so yeah ! The best way to stay up to date is to use source from github directly.

Once we'll automate the release process, I'll let you know 😜

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Support
  
Closed
Development

Successfully merging this pull request may close these issues.

Negative amount not working for LT and LV
4 participants