Skip to content

Commit

Permalink
fix: wrong handling of conversation access role (WPB-10746) (#2975) (#…
Browse files Browse the repository at this point in the history
…2978)

* fix: add @JsonNames to get both values (current and deprecated) from api

* test: adjust and add a new test to harden this fix

* fix: add missing fallback field to default ConversationResponse



* fix: add both accessRole and accessRoleV2 to api response models

* fix: add proper mapping for v3 access role and v2- access_role_v2

* fix: add v3 implementation for fetchConversationsListDetails

* fix: remove unused access_role from api v2-

* fix: add ConversationResponseDTO for V3

* fix: add tests for conversation response with different access roles for V2 and V3

---------

Signed-off-by: alexandreferris <ferris.alexandre@gmail.com>
Co-authored-by: Alexandre Ferris <ferris.alexandre@gmail.com>
Co-authored-by: Yamil Medina <yamilmedina@users.noreply.github.com>
  • Loading branch information
3 people authored Aug 29, 2024
1 parent 9f4a150 commit 3618918
Show file tree
Hide file tree
Showing 13 changed files with 596 additions and 23 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,8 @@ internal class ConversationMapperImpl(
lastNotificationDate = null,
lastModifiedDate = apiModel.lastEventTime.toInstant(),
access = apiModel.access.map { it.toDAO() },
accessRole = apiModel.accessRole.map { it.toDAO() },
accessRole = (apiModel.accessRole ?: ConversationAccessRoleDTO.DEFAULT_VALUE_WHEN_NULL)
.map { it.toDAO() },
receiptMode = receiptModeMapper.fromApiToDaoModel(apiModel.receiptMode),
messageTimer = apiModel.messageTimer,
userMessageTimer = null, // user picked self deletion timer is only persisted locally
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,11 @@ object ConversationResponseJson {
conversationResponse, conversationResponseSerializer
)

val v0 = ValidJsonProvider(
conversationResponse, conversationResponseSerializerWithDeprecatedAccessRole
fun v0(accessRole: Set<ConversationAccessRoleDTO>? = null) = ValidJsonProvider(
conversationResponse.copy(
accessRole = accessRole ?: conversationResponse.accessRole
),
conversationResponseSerializerWithDeprecatedAccessRole
)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,15 @@ import com.wire.kalium.network.api.model.ConversationAccessRoleDTO
object CreateConversationRequestJson {

private val createConversationRequest = CreateConversationRequest(
listOf(QualifiedIDSamples.one),
qualifiedUsers = listOf(QualifiedIDSamples.one),
name = "NameOfThisGroupConversation",
listOf(ConversationAccessDTO.PRIVATE),
listOf(ConversationAccessRoleDTO.TEAM_MEMBER),
ConvTeamInfo(false, "teamID"),
0,
ReceiptMode.DISABLED,
"WIRE_MEMBER",
ConvProtocol.PROTEUS,
access = listOf(ConversationAccessDTO.PRIVATE),
accessRole = listOf(ConversationAccessRoleDTO.TEAM_MEMBER),
convTeamInfo = ConvTeamInfo(false, "teamID"),
messageTimer = 0,
receiptMode = ReceiptMode.DISABLED,
conversationRole = "WIRE_MEMBER",
protocol = ConvProtocol.PROTEUS,
creatorClient = null
)

Expand Down Expand Up @@ -72,8 +72,10 @@ object CreateConversationRequestJson {
""".trimMargin()
}

val v3 = ValidJsonProvider(
createConversationRequest
fun v3(accessRole: List<ConversationAccessRoleDTO>? = null) = ValidJsonProvider(
createConversationRequest.copy(
accessRole = accessRole ?: createConversationRequest.accessRole
)
) {
"""
|{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,3 +35,10 @@ data class ConversationResponseDTO(
@SerialName("not_found") val conversationsNotFound: List<ConversationId>,
@SerialName("failed") val conversationsFailed: List<ConversationId>,
)

@Serializable
data class ConversationResponseDTOV3(
@SerialName("found") val conversationsFound: List<ConversationResponseV3>,
@SerialName("not_found") val conversationsNotFound: List<ConversationId>,
@SerialName("failed") val conversationsFailed: List<ConversationId>,
)
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ data class ConversationResponse(
val access: Set<ConversationAccessDTO>,

@SerialName("access_role_v2")
val accessRole: Set<ConversationAccessRoleDTO> = ConversationAccessRoleDTO.DEFAULT_VALUE_WHEN_NULL,
val accessRole: Set<ConversationAccessRoleDTO>?,

@SerialName("receipt_mode")
val receiptMode: ReceiptMode
Expand Down Expand Up @@ -145,8 +145,11 @@ data class ConversationResponseV3(
@SerialName("access")
val access: Set<ConversationAccessDTO>,

@SerialName("access_role")
val accessRole: Set<ConversationAccessRoleDTO>?,

@SerialName("access_role_v2")
val accessRole: Set<ConversationAccessRoleDTO> = ConversationAccessRoleDTO.DEFAULT_VALUE_WHEN_NULL,
val accessRoleV2: Set<ConversationAccessRoleDTO>?,

@SerialName("receipt_mode")
val receiptMode: ReceiptMode,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class ApiModelMapperImpl : ApiModelMapper {
response.lastEventTime,
response.mlsCipherSuiteTag,
response.access,
response.accessRole,
accessRole = response.accessRole ?: response.accessRoleV2 ?: ConversationAccessRoleDTO.DEFAULT_VALUE_WHEN_NULL,
response.receiptMode
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ package com.wire.kalium.network.api.v3.authenticated

import com.wire.kalium.network.AuthenticatedNetworkClient
import com.wire.kalium.network.api.authenticated.conversation.ConversationResponse
import com.wire.kalium.network.api.authenticated.conversation.ConversationResponseDTO
import com.wire.kalium.network.api.authenticated.conversation.ConversationResponseDTOV3
import com.wire.kalium.network.api.authenticated.conversation.ConversationResponseV3
import com.wire.kalium.network.api.authenticated.conversation.ConversationsDetailsRequest
import com.wire.kalium.network.api.authenticated.conversation.CreateConversationRequest
import com.wire.kalium.network.api.authenticated.conversation.UpdateConversationAccessRequest
import com.wire.kalium.network.api.authenticated.conversation.UpdateConversationAccessResponse
Expand All @@ -44,6 +47,23 @@ internal open class ConversationApiV3 internal constructor(
private val apiModelMapper: ApiModelMapper = ApiModelMapperImpl()
) : ConversationApiV2(authenticatedNetworkClient) {

override suspend fun fetchConversationsListDetails(
conversationsIds: List<ConversationId>
): NetworkResponse<ConversationResponseDTO> =
wrapKaliumResponse<ConversationResponseDTOV3> {
httpClient.post("$PATH_CONVERSATIONS/$PATH_CONVERSATIONS_LIST") {
setBody(ConversationsDetailsRequest(conversationsIds = conversationsIds))
}
}.mapSuccess {
ConversationResponseDTO(
conversationsFound = it.conversationsFound.map { conversationFound ->
apiModelMapper.fromApiV3(conversationFound)
},
conversationsNotFound = it.conversationsNotFound,
conversationsFailed = it.conversationsFailed
)
}

/**
* returns 201 when a new conversation is created or 200 if the conversation already existed
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -464,7 +464,7 @@ internal class ConversationApiV0Test : ApiTest() {
const val PATH_RECEIPT_MODE = "receipt-mode"
const val PATH_CODE = "code"
const val PATH_TYPING_NOTIFICATION = "typing"
val CREATE_CONVERSATION_RESPONSE = ConversationResponseJson.v0.rawJson
val CREATE_CONVERSATION_RESPONSE = ConversationResponseJson.v0().rawJson
val CREATE_CONVERSATION_REQUEST = CreateConversationRequestJson.v0
val CREATE_CONVERSATION_IDS_REQUEST = ConversationListIdsResponseJson.validRequestIds
val UPDATE_ACCESS_ROLE_REQUEST = UpdateConversationAccessRequestJson.v0
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,209 @@
/*
* Wire
* Copyright (C) 2024 Wire Swiss GmbH
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see http://www.gnu.org/licenses/.
*/
package com.wire.kalium.api.v2

import com.wire.kalium.api.ApiTest
import com.wire.kalium.mocks.responses.AnyResponseProvider
import com.wire.kalium.mocks.responses.conversation.ConversationDetailsResponse
import com.wire.kalium.network.api.model.ConversationAccessRoleDTO
import com.wire.kalium.network.api.v2.authenticated.ConversationApiV2
import com.wire.kalium.network.utils.mapSuccess
import io.ktor.http.HttpStatusCode
import kotlinx.coroutines.test.runTest
import kotlin.test.Test
import kotlin.test.assertEquals

internal class ConversationResponseTest : ApiTest() {

@Test
fun givenConversationResponseWithOnlyAccessRoleV2_whenMappingToConversation_thenItMapsCorrectly() = runTest {
val networkClient = mockAuthenticatedNetworkClient(
conversationResponseWithAccessRoleV2.rawJson,
statusCode = HttpStatusCode.OK
)

val conversationApi = ConversationApiV2(networkClient)

val response = conversationApi.fetchConversationsListDetails(listOf())

response.mapSuccess {
assertEquals(
setOf(
ConversationAccessRoleDTO.TEAM_MEMBER,
ConversationAccessRoleDTO.SERVICE
),
it.conversationsFound.first().accessRole
)
}
}

@Test
fun givenConversationResponseWithBothAccessRole_whenMappingToConversation_thenItMapsCorrectly() = runTest {
val networkClient = mockAuthenticatedNetworkClient(
conversationResponseWithBothAccessRole.rawJson,
statusCode = HttpStatusCode.OK
)

val conversationApi = ConversationApiV2(networkClient)

val response = conversationApi.fetchConversationsListDetails(listOf())

response.mapSuccess {
assertEquals(
setOf(
ConversationAccessRoleDTO.TEAM_MEMBER,
ConversationAccessRoleDTO.SERVICE
),
it.conversationsFound.first().accessRole
)
}
}

private companion object {
val conversationResponseWithAccessRoleV2 = AnyResponseProvider(data = "") {
"""
|{
| "failed": [],
| "found": [
| {
| "access": [
| "invite"
| ],
| "access_role_v2": [
| "team_member",
| "service"
| ],
| "creator": "f4680835-2cfe-4d4d-8491-cbb201bd5c2b",
| "id": "ebafd3d4-1548-49f2-ac4e-b2757e6ca44b",
| "last_event": "0.0",
| "last_event_time": "1970-01-01T00:00:00.000Z",
| "members": {
| "others": [
| {
| "conversation_role": "wire_member",
| "id": "22dfd5cc-11ae-4a9d-9046-ba27585f4613",
| "qualified_id": {
| "domain": "bella.wire.link",
| "id": "22dfd5cc-11ae-4a9d-9046-ba27585f4613"
| },
| "status": 0
| }
| ],
| "self": {
| "conversation_role": "wire_admin",
| "hidden": false,
| "hidden_ref": null,
| "id": "f4680835-2cfe-4d4d-8491-cbb201bd5c2b",
| "otr_archived": false,
| "otr_archived_ref": null,
| "otr_muted_ref": null,
| "otr_muted_status": null,
| "qualified_id": {
| "domain": "anta.wire.link",
| "id": "f4680835-2cfe-4d4d-8491-cbb201bd5c2b"
| },
| "service": null,
| "status": 0,
| "status_ref": "0.0",
| "status_time": "1970-01-01T00:00:00.000Z"
| }
| },
| "message_timer": null,
| "name": "test-anta-grp",
| "protocol": "proteus",
| "qualified_id": {
| "domain": "anta.wire.link",
| "id": "ebafd3d4-1548-49f2-ac4e-b2757e6ca44b"
| },
| "receipt_mode": null,
| "team": null,
| "type": 0
| }
| ],
| "not_found": []
|}
""".trimMargin()
}

val conversationResponseWithBothAccessRole = AnyResponseProvider(data = "") {
"""
|{
| "failed": [],
| "found": [
| {
| "access": [
| "invite"
| ],
| "access_role": "activated",
| "access_role_v2": [
| "team_member",
| "service"
| ],
| "creator": "f4680835-2cfe-4d4d-8491-cbb201bd5c2b",
| "id": "ebafd3d4-1548-49f2-ac4e-b2757e6ca44b",
| "last_event": "0.0",
| "last_event_time": "1970-01-01T00:00:00.000Z",
| "members": {
| "others": [
| {
| "conversation_role": "wire_member",
| "id": "22dfd5cc-11ae-4a9d-9046-ba27585f4613",
| "qualified_id": {
| "domain": "bella.wire.link",
| "id": "22dfd5cc-11ae-4a9d-9046-ba27585f4613"
| },
| "status": 0
| }
| ],
| "self": {
| "conversation_role": "wire_admin",
| "hidden": false,
| "hidden_ref": null,
| "id": "f4680835-2cfe-4d4d-8491-cbb201bd5c2b",
| "otr_archived": false,
| "otr_archived_ref": null,
| "otr_muted_ref": null,
| "otr_muted_status": null,
| "qualified_id": {
| "domain": "anta.wire.link",
| "id": "f4680835-2cfe-4d4d-8491-cbb201bd5c2b"
| },
| "service": null,
| "status": 0,
| "status_ref": "0.0",
| "status_time": "1970-01-01T00:00:00.000Z"
| }
| },
| "message_timer": null,
| "name": "test-anta-grp",
| "protocol": "proteus",
| "qualified_id": {
| "domain": "anta.wire.link",
| "id": "ebafd3d4-1548-49f2-ac4e-b2757e6ca44b"
| },
| "receipt_mode": null,
| "team": null,
| "type": 0
| }
| ],
| "not_found": []
|}
""".trimMargin()
}
}
}
Loading

0 comments on commit 3618918

Please sign in to comment.