-
Notifications
You must be signed in to change notification settings - Fork 2
feat(#225): Send sync duration in Firebase Analytics as a custom event and in Settings row #131
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
Conversation
π @JosephSanjaya |
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.
πππ
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.
Looks very close @JosephSanjaya .
We do need to polish the layout so users can understand.

Figma link is here:
https://www.figma.com/design/9IP5iHEq0jxxujWA9sg2n2/brainwallet-app-ui-ux?node-id=4872-2847&t=XZlj6ulgA55S8ckI-1
private const val KEY_LAST_END = "last_sync_end_timestamp" | ||
private const val KEY_DURATION_MILLIS = "duration_millis" | ||
private const val KEY_END_TIMESTAMP = "end_timestamp" | ||
private const val KEY_BLOCK_HEIGHT = "block_height" |
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.
Assuming this is ok note there are core Interfaces that may make it even more succinct
The BRTransaction Class (the core C-class running the low level wallet functions) might be helpful to review.
Here is a link (@JosephSanjaya ): https://github.com/gruntsoftware/core/blob/10ae536cab23f197b60a6feac042a07dc8d61ae0/BRTransaction.h#L86
Did you already review?
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.
Thanks for the review, currently im still on going understanding the process, i will update the PR again
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.
Added formatter for the metadata, also change the source for getLastBlockTimestamp
and getCurrentBlockHeight
from BRPeer
cc @kcw-grunt
β¦t and in Settings row This commit introduces analytics tracking for the sync process. It includes: - `TimeProvider`: A utility class for getting the current time, allowing for easier testing. - `AnalyticsSource`: An interface for logging analytics events. - `FirebaseAnalyticsSource`: An implementation of `AnalyticsSource` that uses Firebase Analytics. - `SyncAnalyticsRepository`: A repository to manage and persist sync-related analytics data, and log events. - `startSync()`: Records the start time of a sync. - `stopSync()`: Records the stop time of a sync and accumulates the duration if the sync is paused/resumed. - `completeSync()`: Persists the total sync duration, end timestamp, and a unique ID. It then logs a "user_did_complete_sync" event with these details and the current block height. - `getLastSyncMetadata()`: Retrieves the metadata of the last completed sync. - Unit tests for the new classes. - Integration of `SyncAnalyticsRepository` into `SyncManager` to call the respective methods at the start, stop, and completion of the sync process. - `FakeSharedPreferences`: A testing utility for mocking SharedPreferences.
51821be
to
75bf864
Compare
This commit introduces a `Formatter` class within `SyncAnalyticsRepository.SyncMetadata` to handle the formatting of sync metadata. The `getFormatted()` method has been removed from `SyncAnalyticsRepository.SyncMetadata` and its logic is now encapsulated in the `SyncMetadata.Formatter.format()` method. This change improves the separation of concerns and makes the formatting logic more testable. The `HomeSettingDrawerSheet` has been updated to use the new `Formatter` class. Unit tests have been added to `SyncAnalyticsRepositoryTest` to verify the correctness of the `SyncMetadata.Formatter`.
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.
LGTM
π± Description
Platform
π― Type of Change
π Changes
TimeProvider
: A utility class for getting the current time, allowing for easier testing.AnalyticsSource
: An interface for logging analytics events.FirebaseAnalyticsSource
: An implementation ofAnalyticsSource
that uses Firebase Analytics.SyncAnalyticsRepository
: A repository to manage and persist sync-related analytics data, and log events.startSync()
: Records the start time of a sync.stopSync()
: Records the stop time of a sync and accumulates the duration if the sync is paused/resumed.completeSync()
: Persists the total sync duration, end timestamp, and a unique ID. It then logs a "user_did_complete_sync" event with these details and the current block height.getLastSyncMetadata()
: Retrieves the metadata of the last completed sync.SyncAnalyticsRepository
intoSyncManager
to call the respective methods at the start, stop, and completion of the sync process.FakeSharedPreferences
: A testing utility for mocking SharedPreferences.getFormatted()
method has been removed fromSyncAnalyticsRepository.SyncMetadata
and its logic is now encapsulated in theSyncMetadata.Formatter.format()
method.HomeSettingDrawerSheet
has been updated to use the newFormatter
class.SyncAnalyticsRepositoryTest
to verify the correctness of theSyncMetadata.Formatter
.π Related Issues
π§ͺ Tests Status
πΈ Screenshots/Videos
π― Reviewers
@kcw-grunt, @josikie