Skip to content
This repository has been archived by the owner on Nov 5, 2024. It is now read-only.

Fix issue 2740 #2928

Merged
merged 15 commits into from
Feb 8, 2024
Merged

Fix issue 2740 #2928

merged 15 commits into from
Feb 8, 2024

Conversation

michalguspiel
Copy link
Contributor

@michalguspiel michalguspiel commented Feb 7, 2024

Pull Request (PR) Checklist

Please check if your pull request fulfills the following requirements:

  • The PR is submitted to the main branch.
  • I've read the Contribution Guidelines and my PR doesn't break the rules.
  • I've read the Architecture Guidelines.
  • I confirm that I've run the code locally and everything works as expected.
  • 🎬 I've attached a screen recoding of the changes.

What's changed?

Loans can now be increased. Loan always displays starting amount as the last item in the list. When adding loan record, user can decide if that record is decreasing the loan amount, like every record before this PR or increase it. Amount of loan displays now the total amount of loan, meaning initial amount + each record that increased this amount.

Technicalities:

  • LoanRecord includes LoanRecordType.
  • DisplayLoan includes loanTotalAmount
  • LoanViewModel calculates total amount paid as well as loan total amount.
  • LoanScreen displays information that includes all the records about each loan.
  • LoanDetailsScreen displays a list of all records involving records that increase and decrease the total loan amount
  • LoanDetailsScreenState contains calculated loanTotalAmount
  • LoanRecordModal provides a selection if loan record decrease or increase loan amount. Provided the loan record increase total amount, Mark as interest checkbox disappears.

Screen recording with new feature:

https://youtu.be/VyvornkPsDo

Risk Factors

  • DB Migration: DB migration is always a scary thing am I right? This PR had just added one column to the table so we should be fine. I've tested it on my device.
  • Backup feature. I've added default value to LoanRecord.loanRecordType so that backup doesn't fail. Awesome that test has caught this, otherwise I wouldn't have think of this.

Does this PR closes any GitHub Issues?

Troubleshooting CI failures ❌

GitHub Actions failing? Read our CI Troubleshooting guide.

- LoanDetailsScreen displays list of all records involving records that increase and decrease total loan amount
- LoanDetailsScreenState contains calculated `loanTotalAmount`
- LoanRecordModal provides a selection if loan record decrease or increase loan amount. Provided loan record increase total amount, `Mark as interest` checkbox disappears.
- DisplayLoan includes loanTotalAmount
- LoanViewModel calculates total amount paid as well as loan total amount.
- LoanScreen displays correct information about each loan.
@ILIYANGERMANOV
Copy link
Collaborator

Hey @michalguspiel can you please fix the CI failures and I'll review afterwards? I'm most concerned about the test failures both unit and integration

michalguspiel and others added 6 commits February 8, 2024 07:08
- Tests were failing due to the missing field in backup test. Now by default `loanRecordType` in `LoanRecordEntity` has DECRESE value. This fixes the issue with the backup, and is the simplest fix. This makes sense because before this pull request all loan records were implicitly of type DECREASE.
Suppressed detekt errors that were forcing this pull request to make changes unrelated to scope of this issue or keeping this pull request inconsistent with the rest of the codebase.
- Suppressed LongMethod for LoanInfoCard
- Suppressed DataClassDefaultValues for LoanRecordEntity, since there is a few default values already, and `LoanRecordEntity.loanRecordType` default value is the easiest fix for the backup problem.
- Suppressed MagicNumber and ClassNaming for Migration class to keep it consistent with the rest of migration classes.
@michalguspiel michalguspiel marked this pull request as ready for review February 8, 2024 06:37
@michalguspiel
Copy link
Contributor Author

@ILIYANGERMANOV Ready for review.

Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM! Have some comments regarding the DB migration part:

  1. Revert 126.json
  2. Commit 127.json
  3. Add DB migration test

Nice work 💯

@ILIYANGERMANOV ILIYANGERMANOV added the keep Keep it from automatically getting closed by Stale label Feb 8, 2024
@ILIYANGERMANOV
Copy link
Collaborator

Btw @michalguspiel very well written PR description 👏

@michalguspiel
Copy link
Contributor Author

Hi @ILIYANGERMANOV would you mind reviewing again? I'm not sure if androidTest is correctly configured. I have a hard time running it differently than from the command line.

# Conflicts:
#	shared/resources/src/main/res/values/strings.xml
Copy link
Collaborator

@ILIYANGERMANOV ILIYANGERMANOV left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for making the changes 💯 Fix merge conflicts and we can merge it after ✅ from the CI

@ILIYANGERMANOV ILIYANGERMANOV merged commit f5012fb into Ivy-Apps:main Feb 8, 2024
8 checks passed
@michalguspiel michalguspiel deleted the fix-issue-2740 branch February 9, 2024 09:07
@RDjarbeng
Copy link

Hello, I have been waiting for this feature.

Will this be available on the Google Play Store now or we have to wait for an update?

@ILIYANGERMANOV
Copy link
Collaborator

Hello, I have been waiting for this feature.

Will this be available on the Google Play Store now or we have to wait for an update?

@RDjarbeng We have to wait for an update. If you want to use it now you need to download the GitHub APK. We can't update right not because main in GitHub still has critical bugs and defects

@RDjarbeng
Copy link

RDjarbeng commented Mar 2, 2024

Okay, I'll just wait for the update. I am not sure what you mean by the Github APK since I can't find an apk file in the repository. I am assuming you meant to build it using android studio.

Would be nice to have a time to expect it though. Thanks for replying.👌

@ILIYANGERMANOV
Copy link
Collaborator

Okay, I'll just wait for the update. I am not sure what you mean by the Github APK since I can't find an apk file in the repository. I am assuming you meant to build it using android studio.

Would be nice to have a time to expect it though. Thanks for replying.👌

@RDjarbeng if you're in our Telegram group, GitHub Actions automatically builds and uploads one in the Telegram channel. You can find it in the artifacts of the latest "APK" workflow run

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
keep Keep it from automatically getting closed by Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Loan Section feature
3 participants