Skip to content

[PM-26736] Fix push notification logout reason serialization #6116

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.

Sign up for GitHub

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

Merged
andrebispo5 merged 2 commits into from
Nov 3, 2025
Merged

[PM-26736] Fix push notification logout reason serialization #6116

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7035d98
[PM-26736] Fix push notification logout reason serialization
andrebispo5 Nov 3, 2025
f5e1988
[PM-26736] Update tests.
andrebispo5 Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/src/main/kotlin/com/x8bit/bitwarden/data/platform/manager/model/NotificationPayload.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ sealed class NotificationPayload {
@JsonNames("Date", "date")
val date: ZonedDateTime?,

@JsonNames("PushNotificationLogOutReason", "pushNotificationLogOutReason")
@JsonNames("Reason", "reason")
Copy link
Contributor

@claude claude bot Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Critical Issue: Test data needs to be updated

This field name change from "PushNotificationLogOutReason" to "Reason" breaks existing test data. The test file PushManagerTest.kt at line 978 still uses the old field name:

private val LOGOUT_KDF_NOTIFICATION_MAP = mapOf(
    ...
    "payload" to """{
      "UserId": "078966a2-93c2-4618-ae2a-0a2394c88d37",
      "Date": "2023-10-27T12:00:00.000Z",
      "PushNotificationLogOutReason": "0"  // ❌ Should be "Reason": "0"
    }""",
)

Action Required: Update the test payload to use "Reason": "0" to match the new serialization field name. This will cause deserialization to fail in tests otherwise.

📝 Note: The @JsonNames annotation includes both "Reason" and "reason" (lowercase), which is good for backward compatibility with different casing from the backend. However, the old field name "PushNotificationLogOutReason" is completely different and won't be recognized.

Copy link
Collaborator

@david-livefront david-livefront Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we update all the test JSON to reflect these changes

Copy link
Contributor

@claude claude bot Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good field rename from "PushNotificationLogOutReason" to "Reason" - this aligns with backend expectations and is more concise.

📝 The @JsonNames annotation correctly provides both "Reason" and "reason" for case-insensitive matching, following the same pattern as other fields in this class (lines 29, 42, 53, 57).

val pushNotificationLogOutReason: PushNotificationLogOutReason?,
) : NotificationPayload()

Expand Down
11 changes: 11 additions & 0 deletions ...in/kotlin/com/x8bit/bitwarden/data/platform/manager/model/PushNotificationLogOutReason.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,22 @@
package com.x8bit.bitwarden.data.platform.manager.model

import androidx.annotation.Keep
import com.bitwarden.core.data.serializer.BaseEnumeratedIntSerializer
import kotlinx.serialization.SerialName
import kotlinx.serialization.Serializable

/**
* Enumerated values to represent the possible reasons for a log out push notification
*/
@Serializable(with = PushNotificationLogOutReasonSerializer::class)
Copy link
Contributor

@claude claude bot Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Excellent addition of the @Serializable annotation with custom serializer. This properly enables kotlinx.serialization support and uses BaseEnumeratedIntSerializer to handle integer-based enum serialization, consistent with other enums in the codebase like NotificationType.

Copy link
Contributor

@claude claude bot Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Perfect addition of proper kotlinx.serialization support! The @Serializable annotation with PushNotificationLogOutReasonSerializer follows the established pattern used throughout the codebase (see NotificationType.kt:11 for reference).

Using BaseEnumeratedIntSerializer ensures this enum is serialized as an integer value, which is consistent with how the backend sends the data.

enum class PushNotificationLogOutReason {
@SerialName("0")
KDF_CHANGE,
}

@Keep
Copy link
Contributor

@claude claude bot Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good use of @Keep annotation! This prevents R8/ProGuard from removing the serializer during code optimization, which is critical for runtime serialization. This matches the pattern in NotificationType.kt:65.

private class PushNotificationLogOutReasonSerializer :
BaseEnumeratedIntSerializer<PushNotificationLogOutReason>(
className = "PushNotificationLogOutReason",
values = PushNotificationLogOutReason.entries.toTypedArray(),
)
Comment on lines +17 to +22
Copy link
Contributor

@claude claude bot Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Good use of @Keep annotation to prevent R8/ProGuard from removing this serializer during code optimization. This follows the same pattern as NotificationType and other enums using BaseEnumeratedIntSerializer in the codebase.

2 changes: 1 addition & 1 deletion app/src/test/kotlin/com/x8bit/bitwarden/data/platform/manager/PushManagerTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,7 +975,7 @@ private val LOGOUT_KDF_NOTIFICATION_MAP = mapOf(
"payload" to """{
"UserId": "078966a2-93c2-4618-ae2a-0a2394c88d37",
"Date": "2023-10-27T12:00:00.000Z",
"PushNotificationLogOutReason": "0"
"Reason": 0
Copy link
Contributor

@claude claude bot Nov 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Excellent fix! The test data has been properly updated to match the new field name and format:

  • Field renamed from "PushNotificationLogOutReason" to "Reason" (matching backend expectations)
  • Value changed from string "0" to integer 0 (correct format for BaseEnumeratedIntSerializer)

This aligns with how other serialized enums work in the codebase (e.g., NotificationType).

}""",
)

Expand Down
Loading