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

[database-Loan] DBflow to Room migration #2292

Merged
merged 4 commits into from
Jan 30, 2025

Conversation

itsPronay
Copy link
Contributor

@itsPronay itsPronay commented Jan 22, 2025

PART OF - https://mifosforge.jira.com/browse/MIFOSAC-333

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the static analysis check ./gradlew check or ci-prepush.sh to make sure you didn't break anything

  • If you have multiple commits please combine them into one commit by squashing them.

@itsPronay itsPronay marked this pull request as draft January 22, 2025 16:30
@itsPronay itsPronay force-pushed the room-dao branch 2 times, most recently from b36ef5c to 32bf511 Compare January 24, 2025 16:27
@itsPronay itsPronay changed the title draft: DBflow migration draft: [database-Loan] DBflow to Room migration Jan 24, 2025
@itsPronay itsPronay changed the title draft: [database-Loan] DBflow to Room migration [database-Loan] DBflow to Room migration Jan 26, 2025
@itsPronay itsPronay marked this pull request as ready for review January 26, 2025 16:46
@itsPronay itsPronay marked this pull request as draft January 30, 2025 16:35
@therajanmaurya therajanmaurya marked this pull request as ready for review January 30, 2025 16:43
@HekmatullahAmin
Copy link

@itsPronay I noticed that some data classes inherit from MifosBaseModel, which overrides toString(). If this is only for debugging purposes, I don’t think we really need it. It would be good to check with @therajanmaurya Sir since he created the class.

If he agrees to remove it, then remove it from all data classes in the database module and delete MifosBaseModel itself. This will help clean up unnecessary inheritance and keep the models lightweight.
Screenshot 2025-01-30 at 19 48 48

@itsPronay
Copy link
Contributor Author

@itsPronay I noticed that some data classes inherit from MifosBaseModel, which overrides toString(). If this is only for debugging purposes, I don’t think we really need it. It would be good to check with @therajanmaurya Sir since he created the class.

If he agrees to remove it, then remove it from all data classes in the database module and delete MifosBaseModel itself. This will help clean up unnecessary inheritance and keep the models lightweight.
Screenshot 2025-01-30 at 19 48 48

@HekmatullahAmin you are viewing the old code, which was for dbflow. News codes are in room package and all the entities were migrated by @Darkeye14 already.

In room package some data classes still use 'columnInfo' because that's not the part of the loan. Since this PR was about loan's migration i have only implemented that in loan entities.

@HekmatullahAmin
Copy link

@HekmatullahAmin you are viewing the old code, which was for dbflow. News codes are in room package and all the entities were migrated by @Darkeye14 already.

In room package some data classes still use 'columnInfo' because that's not the part of the loan. Since this PR was about loan's migration i have only implemented that in loan entities.

Got it @itsPronay ! It seems like the database module need a cleanup. Since I wasn’t working on the Android client, I wasn’t aware of the old and new code separation. Just out of curiosity—why are we still keeping the old DBFlow-related code? Could it be deleted, or did you leave it because it was out of the scope of your assigned task?

Also, @therajanmaurya Sir, it looks fine to me. You can have your final look and review.

@therajanmaurya therajanmaurya merged commit 34b0b32 into openMF:kmp-impl Jan 30, 2025
5 checks passed
@itsPronay
Copy link
Contributor Author

@he

@HekmatullahAmin you are viewing the old code, which was for dbflow. News codes are in room package and all the entities were migrated by @Darkeye14 already.
In room package some data classes still use 'columnInfo' because that's not the part of the loan. Since this PR was about loan's migration i have only implemented that in loan entities.

Got it @itsPronay ! It seems like the database module need a cleanup. Since I wasn’t working on the Android client, I wasn’t aware of the old and new code separation. Just out of curiosity—why are we still keeping the old DBFlow-related code? Could it be deleted, or did you leave it because it was out of the scope of your assigned task?

Also, @therajanmaurya Sir, it looks fine to me. You can have your final look and review.

@HekmatullahAmin, previously @Darkeye14 was working on this. When I took over, he asked me to keep the old code , probably because bhaiya told him to do so or just to ensure we didn’t break anything and could do a recheck

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

Successfully merging this pull request may close these issues.

5 participants