fix: prevent duplicate push metrics and deep link handling#405
Conversation
Sample app builds 📱Below you will find the list of the latest versions of the sample apps. It's recommended to always download the latest builds of the sample apps to accurately test the pull request.
|
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## levi/reliable-open-metrics #405 +/- ##
==============================================================
- Coverage 59.45% 57.75% -1.71%
==============================================================
Files 119 126 +7
Lines 4627 4902 +275
==============================================================
+ Hits 2751 2831 +80
- Misses 1876 2071 +195 ☔ View full report in Codecov by Sentry. |
| class PushHistoryImpl: PushHistory { | ||
| private let keyValueStorage: KeyValueStorage | ||
|
|
||
| var numberOfPushesToTrack = 100 |
There was a problem hiding this comment.
What does this value represent? I think adding a comment would add value to it.
There was a problem hiding this comment.
Thanks for asking. I changed the name to maxSizeOfHistory to be more descriptive of it's purpose. Does this new name help?
| } | ||
|
|
||
| // sourcery: InjectRegister = "PushHistory" | ||
| class PushHistoryImpl: PushHistory { |
There was a problem hiding this comment.
maybe we should make it thread-safe?
There was a problem hiding this comment.
Great idea. Latest commit does this. I followed the same logic as QueueStorage uses.
| // push has already been handled. exit early | ||
| return | ||
| } | ||
| pushHistory.handledPushClick(deliveryId: parsedPush.deliveryId) |
There was a problem hiding this comment.
Did we write any tests for pushClicked anywhere? If not, can we write some?
There was a problem hiding this comment.
This feature has been heavily reliant on QA testing thus far. Automated tests around swizzling and UserNotifications has been difficult to write. I have not yet found an easy or quick way to make them.
I do agree and think we can write some for this function after QA concludes, yes.
There was a problem hiding this comment.
I am going to merge this PR to make maintenance and QA testing easier. I made a note to add these tests and codecov in the target PR should help to keep track of these missing tests.
…rics-prevent-duplicates
levibostian
deleted the
levi/reliable-open-metrics-prevent-duplicates
branch
4 participants
Merges into: #403
Prevent duplicate push metrics and deep link handling for push click handling.