-
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
refactor: user mapper #1795
refactor: user mapper #1795
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1795 +/- ##
=============================================
- Coverage 53.53% 53.48% -0.05%
+ Complexity 1514 1511 -3
=============================================
Files 960 960
Lines 35763 35740 -23
Branches 3171 3164 -7
=============================================
- Hits 19144 19114 -30
- Misses 15343 15366 +23
+ Partials 1276 1260 -16 |
// @Test | ||
// fun givenUserProfileDTOAndUserTypeEntity_whenMappingFromApiResponse_thenDaoModelIsReturned() = runTest { | ||
// // Given | ||
// val givenResponse = TestUser.USER_PROFILE_DTO | ||
// val givenUserTypeEntity = UserTypeEntity.EXTERNAL | ||
// val expectedResult = TestUser.ENTITY.copy( | ||
// phone = null, // UserProfileDTO doesn't contain the phone | ||
// connectionStatus = ConnectionEntity.State.NOT_CONNECTED // UserProfileDTO doesn't contain the connection status | ||
// ) | ||
// val (_, userMapper) = Arrangement().arrange() | ||
// // When | ||
// val result = userMapper.fromApiModelWithUserTypeEntityToDaoModel(givenResponse, givenUserTypeEntity) | ||
// // Then | ||
// assertEquals(expectedResult, result) | ||
// } |
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.
Is this intended?
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 forgot to delete it after deleting this mapping function.
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.
Decided to resurrect it since test coverage of the other function was completely missing.
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.
Looking good, but can we make it as extension like we did with IdMappers? It will be easier to know that we are not duplicating mappers? 😄
We've agreed to do it like that for all mappers going forward? I'm not doing it in this PR though, it would be a follow up PR then. |
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.
Tiny suggestions to remove comments
logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt
Outdated
Show resolved
Hide resolved
logic/src/commonMain/kotlin/com/wire/kalium/logic/data/user/UserRepository.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Vitor Hugo Schwaab <[email protected]>
Co-authored-by: Vitor Hugo Schwaab <[email protected]>
PR Submission Checklist for internal contributors
The PR Title
SQPIT-764
The PR Description
What's new in this PR?
Issues
Solutions
Notes
Should we merge PublicUserMapping and UserMapping into one?
PR Post Merge Checklist for internal contributors
References
feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764
.