-
Notifications
You must be signed in to change notification settings - Fork 4
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 for unwanted deletion of non-synced new TEI when its 2nd enrollment is cancelled #179
Fix for unwanted deletion of non-synced new TEI when its 2nd enrollment is cancelled #179
Conversation
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.
thanks @alex-vt
It seems a good fix, however I've found some issues about clean code to improve.
I've added comments in the code
teiRepository.blockingDelete() | ||
} else { | ||
enrollmentObjectRepository.blockingDelete() | ||
} | ||
analyticsHelper.setEvent(DELETE_AND_BACK, CLICK, DELETE_AND_BACK) | ||
} | ||
|
||
private fun isTeiInNoOtherProgram(): Boolean = |
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.
This application follow some bad practices, and we would like to follow best practices when we add customizations or fixes.
From a presenter we avoid use directly d2.
We try to use d2 in an uncoupled way using repositories, the advantage is if they change d2 in the future, the change in encapsulated in the repositories instead of to change a lot of places in presenters or somewhere.
In this case, is a better option to move isTeiInNoOtherProgram function to some repository (enrollmentRepository seems the best option here)
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.
Thank you for the suggestion. EnrollmentRepository
and other repositories in EnrollmentPresenterImpl
do not have the direct D2
dependency. Passing isTeiInNoOtherProgram
deeper would probably be unproportional to how limited its usage is. I see that such narrow context functions are extracted into utils, such as https://github.com/EyeSeeTea/dhis2-android-capture-app/blob/develop-simprints/app/src/main/java/org/dhis2/data/biometrics/utils/GetTeiByUid.kt that's also used in EnrollmentPresenterImpl
. I've extracted isTeiInNoOtherProgram
in a similar way.
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.
@alex-vt EnrollmentRepository doesn't have direct D2 dependency, but it has EnrollmentConfiguration dependency and this one has D2 dependency.
It's possible to move your isTeiInNoOtherProgram function to EnrollmentConfiguration to use from EnrollmentRepository.
So we avoid use d2 from presenter to add this fix even passing as parameter to a new function used from presenter.
app/src/test/java/org/dhis2/usescases/enrollment/EnrollmentPresenterImplTest.kt
Show resolved
Hide resolved
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.
thanks @alex-vt
I've leaved a reply to your comment
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.
thanks @alex-vt
Description
An issue of the logic of cleanup when a newly created, not synced yet TEI is being enrolled into another program, and the user backs out of the 2nd enrollment screen by discarding the changes in the pop-up bottom sheet dialog. The issue occurred in
app/src/main/java/org/dhis2/usescases/enrollment/EnrollmentPresenterImpl.kt
indeleteAllSavedData
.Solution description
A check is added in
deleteAllSavedData
to delete the entire non-synced TEI only when there are no enrollments in other programs than the one being cancelled.Covered unit test cases
Unit tests for the new behavior of
deleteAllSavedData
added toapp/src/test/java/org/dhis2/usescases/enrollment/EnrollmentPresenterImplTest.kt
.