[PM-26736] Fix push notification logout reason serialization #6116
Conversation
andrebispo5
requested review from
a team,
brian-livefront and
david-livefront
as code owners
|
Claude finished @andrebispo5's task —— View job Code Review Complete ✅I've reviewed the changes in this PR that fix push notification logout reason serialization. Overall, this is a well-implemented fix that properly addresses the serialization issues and aligns with backend expectations. SummaryThis PR makes three focused changes to fix how
Detailed Assessment✅ What's Working WellArchitectural Compliance:
Serialization Implementation:
Field Naming:
Test Coverage:
📋 Review Status UpdatePrevious reviews identified the need to update test data - this has been properly addressed in commit "Reason": 0instead of the old: "PushNotificationLogOutReason": "0"RecommendationsNo blocking issues found ✅The implementation is solid and follows all established patterns in the codebase. Optional Future Considerations💭 Extensibility: Currently only @SerialName("1")
ANOTHER_REASON,📝 Documentation: The PR description clearly explains the changes. The KDoc on ConclusionThis PR successfully fixes the push notification logout reason serialization bug. The changes are:
Recommendation: Ready for merge pending any final team review preferences. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| /** | ||
| * Enumerated values to represent the possible reasons for a log out push notification | ||
| */ | ||
| @Serializable(with = PushNotificationLogOutReasonSerializer::class) |
There was a problem hiding this comment.
👍 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.
| @Keep | ||
| private class PushNotificationLogOutReasonSerializer : | ||
| BaseEnumeratedIntSerializer<PushNotificationLogOutReason>( | ||
| className = "PushNotificationLogOutReason", | ||
| values = PushNotificationLogOutReason.entries.toTypedArray(), | ||
| ) |
There was a problem hiding this comment.
👍 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.
| val date: ZonedDateTime?, | ||
|
|
||
| @JsonNames("PushNotificationLogOutReason", "pushNotificationLogOutReason") | ||
| @JsonNames("Reason", "reason") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Can we update all the test JSON to reflect these changes
|
Great job! No new security vulnerabilities introduced in this pull request |
| "UserId": "078966a2-93c2-4618-ae2a-0a2394c88d37", | ||
| "Date": "2023-10-27T12:00:00.000Z", | ||
| "PushNotificationLogOutReason": "0" | ||
| "Reason": 0 |
There was a problem hiding this comment.
👍 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 integer0(correct format forBaseEnumeratedIntSerializer)
This aligns with how other serialized enums work in the codebase (e.g., NotificationType).
| /** | ||
| * Enumerated values to represent the possible reasons for a log out push notification | ||
| */ | ||
| @Serializable(with = PushNotificationLogOutReasonSerializer::class) |
There was a problem hiding this comment.
👍 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.
| KDF_CHANGE, | ||
| } | ||
|
|
||
| @Keep |
There was a problem hiding this comment.
👍 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.
| val date: ZonedDateTime?, | ||
|
|
||
| @JsonNames("PushNotificationLogOutReason", "pushNotificationLogOutReason") | ||
| @JsonNames("Reason", "reason") |
There was a problem hiding this comment.
👍 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).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6116 +/- ##
=======================================
Coverage 84.90% 84.90%
=======================================
Files 723 724 +1
Lines 52753 52768 +15
Branches 7659 7665 +6
=======================================
+ Hits 44789 44802 +13
Misses 5277 5277
- Partials 2687 2689 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-23280
https://bitwarden.atlassian.net/browse/PM-26736
📔 Objective
This pull request updates how the
PushNotificationLogOutReasonis handled in push notification payloads, improving serialization and aligning field names with backend expectations. The main changes focus on serialization support and field renaming for consistency.⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes