-
-
Notifications
You must be signed in to change notification settings - Fork 221
Add possibility ot add a default value to update balance #2532
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
Conversation
| balance = new BigDecimal(String.valueOf(jsonCard.getDouble("balance"))); | ||
| } | ||
|
|
||
| BigDecimal defaultBalanceChange = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, I only figured out how to test the Catima importer, so this might be a mistake.
| String unparsedDefaultBalanceChange = CSVHelpers.extractString(DBHelper.LoyaltyCardDbIds.DEFAULT_BALANCE_CHANGE, record, null); | ||
| if (unparsedDefaultBalanceChange != null) { | ||
| try { | ||
| defaultBalanceChange = new BigDecimal(unparsedDefaultBalanceChange); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In contrast to the balance, I didn't do the extractString again here when creating the BigDecimal. Is there a reason why this is done for the balance?
|
Hi, thanks for contributing. This seems quite a big change, even needing a database migration, for a fairly niche use case. I'm not sure if the extra UI complexity and setting fits in Catima's philosophy of being simple to use and understand (if anything, I argue we already have too many settings and the UI is overly complex at some points). Am I correct in that this only saves 1 single tap in a flow that's already only 3 taps? As I understand it:
Given how little difference this makes in daily usage, yet it adds a fair bit of extra code complexity, I feel tempted to say I'd rather not accept this contribution. The fact that it adds extra Java and Android XML code while we already need to migrate everything to Kotlin Compose (#2483) which is a huge rewrite (and thus: more to rewrite after this) adds to that decision. I hope this is not too discouraging, I definitely appreciate seeing someone contribute, it's a fairly rare but very welcome sight :) |
|
A fairly niche use case, but also a fairly nice one ;) I agree that the added complexity is certainly not worth a merge. It was still worth digging into the app and developing it for myself, so that I can use a slightly different app version. That's the power of Open Source, I guess. However, the Kotlin Compose rewrite also sounds interesting. I'm not that familiar with it yet, but if you have a good entry point where I could contribute, just let me know @TheLastProject ! :) |
|
@F462 I'm messing around with Jetpack Compose a bit in #2489, still trying to learn it. However, we first need to use Kotlin in activities. So the first step is basically rewriting every single activity in Kotlin, so the UI can become Compose after. If you feel like it, help in converting any of the non-claimed activities (basically every single one) to Kotlin would be quite welcome: #2483 :) |
First of all, thank you for the great project!
I use this app to manage my entrance card to go swimming. Thereby, when buying the card, it allows 12 entries, while obviously each entry decreases the balance by 1.
So, at the moment, after each entry I have to open the balance update dialog, enter a "1" and then press the positive button.
To avoid entering the, for the card well-known, value "1" each time, I added in this PR the possibility to provide the card with a default value to change the balance.
By the way: it seems as if the contribution guidelines are outdated, since the tasks
testReleaseUnitTestandlintReleasedon't exist anymore.