-
Notifications
You must be signed in to change notification settings - Fork 6
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: repositoriesShouldNotAccessFeature in MLSConversationRepository [WPB-6326] #2736
fix: repositoriesShouldNotAccessFeature in MLSConversationRepository [WPB-6326] #2736
Conversation
Test Results2 170 tests - 868 2 164 ✔️ - 770 8s ⏱️ - 3m 34s Results for commit df441cc. ± Comparison against base commit fac18ff. This pull request removes 3038 and adds 2170 tests. Note that renamed tests count towards both.
This pull request removes 104 skipped tests and adds 6 skipped tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
Datadog ReportBranch report: ✅ 0 Failed, 2934 Passed, 104 Skipped, 14m 29.03s Wall Time |
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.
Nice to dedicate some time to fix this.
But I think just moving the file and adding "internal" to the name isn't enough.
This is just moving a "UseCase"
to data
, which is not a pattern we should follow.
Unlike real use cases, this one is not public, it shouldn't be used by the apps.
It's an internal, reusable piece of logic that other use cases (and even repositories) are using. So removing this suffix would be better.
@@ -35,15 +36,18 @@ interface CheckRevocationListUseCase { | |||
suspend operator fun invoke(url: String): Either<CoreFailure, ULong?> | |||
} | |||
|
|||
internal class CheckRevocationListUseCaseImpl( | |||
internal class CheckRevocationListInternalUseCaseImpl( |
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.
I think we should reserve the name UseCase
for exposing functionality to the apps.
It isn't gonna be public and usable by the apps. So no need to be named a "UseCase" when it isn't.
internal class CheckRevocationListInternalUseCaseImpl( | |
internal class RevocationListCheckerImpl( |
Also, no need to be a invoke
function either.
It could be a check
or whatever.
…in_mls_conversation_repository
What's new in this PR?
Issues
MLSConversationRepository was accessing the useCase from Feature module
Causes (Optional)
It was done like that
Solutions
Move
CheckRevocationListUseCase
into Logic module and update it to not use UseCases from the Feature.This UseCase is used only in kalium, so it doesn't affect Android part.