Ivy-Apps / ivy-wallet

Ivy Wallet is an open-source money manager app for android that you can either build or download from Google Play.
https://play.google.com/store/apps/details?id=com.ivy.wallet
GNU General Public License v3.0
2.51k stars 577 forks source link

[Data Layer] Migrate the code to use the new the `TransactionRepository` #2963

Open ILIYANGERMANOV opened 5 months ago

ILIYANGERMANOV commented 5 months ago

Please confirm the following

What would you like to improve?

Because...

Description

No response

Success Criteria

ivywallet commented 5 months ago

Thank you @ILIYANGERMANOV for raising Issue #2963! 🚀 What's next? Read our Contribution Guidelines 📚.

Tagging @ILIYANGERMANOV for review & approval 👀

michalguspiel commented 5 months ago

I'm on it

ivywallet commented 5 months ago

Thank you for your interest @michalguspiel! 🎉 Issue #2963 is assigned to you. You can work on it! ✅

If you don't want to work on it now, please un-assign yourself so other contributors can take it.

Also, make sure to read our Contribution Guidelines.

michalguspiel commented 5 months ago

@ILIYANGERMANOV

I understand that

Implement TransactionRepository for handling data operations part of this task is already done.

Thus this task requires to leverage TransactionRepository in the domain layer instead of directly calling Data Access Objects?

ILIYANGERMANOV commented 5 months ago

@ILIYANGERMANOV

I understand that

Implement TransactionRepository for handling data operations part of this task is already done.

Thus this task requires to leverage TransactionRepository in the domain layer instead of directly calling Data Access Objects?

Yup, correct. It's migration DAOs usages to the repository ones. This also includes using the new domain model

michalguspiel commented 5 months ago

Okay so basically plan for this is to add internal visibility modifier to both TransactionDao and WriteTransactionDao and fix the errors.

What do you mean by new domain model?

ILIYANGERMANOV commented 5 months ago

Okay so basically plan for this is to add internal visibility modifier to both TransactionDao and WriteTransactionDao and fix the errors.

What do you mean by new domain model?

This. When you fix the errors you'll need to pass Transaction instance and not TransactionEntity. But it's exactly what you said

michalguspiel commented 4 months ago

@ILIYANGERMANOV

Starting to work with this, I'm realizing how huge of a task it is. It's going to take weeks if not months, considering the amount of my free time I have for this. By the time I'm done with this, I'll need to resolve tons of conflicts which will extend this work even further.

When migrating to the new TransactionRepository it doesn't quite make sense to map the results in the Presentation layer back to the legacy Domain model. Because of that a lot of logic needs to be refactored.

Have you any idea how to split this task to smaller chunks?

For instance: instead of Migrate from using TransactionDao and WriteTransactionDao directly to leveraging the repository We could do: Migrate from using TransactionDao directly to leveraging the TransactionRepository Migrate from using Write TransactionDao to leveraging the TransactionRepository

WDYT

ILIYANGERMANOV commented 4 months ago

@michalguspiel yes your suggestion is good 👍 You can split it to even smaller, migrate one or a few usages -> submit a PR, merge it. This way it would be easier to review

michalguspiel commented 4 months ago

What do we do about TransactionHistoryItem? I got to the point where passing history: List<TransactionHistoryItem> to transactions composable creates a big problem for refactoring.

TransactionHistoryItem is deprecated but TransactionHistoryDateDivider is not. Legacy Transaction and TransactionHistoryDateDivider both inherit from TransactionHistoryItem. How do we want to fix this in regard to new Transaction model?

I could have another PR ready soon if I would skip this problem for now by creating duplicate TransactionCard composable that uses legacy Transaction but I'd like to somehow mitigate that.

Any suggestions?

ILIYANGERMANOV commented 4 months ago

I could have another PR ready soon if I would skip this problem for now by creating duplicate TransactionCard composable that uses legacy Transaction but I'd like to somehow mitigate that.

@michalguspiel just duplicate it - that's a good strategy!