-
Notifications
You must be signed in to change notification settings - Fork 27
csgn #460
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
base: master
Are you sure you want to change the base?
csgn #460
Conversation
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.
Pull Request Overview
This PR appears to implement campaign-based notifications functionality with enhanced geofence handling and notification parsing capabilities.
- Enhanced geofence management with individual metadata-based notification handling
- Added notification parsing and creation functionality for campaign notifications
- Refactored color parsing to use extension functions for consistency
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| RadarGeofence.kt | Added blank line for formatting |
| RadarNotificationHelper.kt | Added notification parsing/sending methods and updated color parsing |
| RadarLocationReceiver.kt | Split geofence handling logic and added notification processing |
| RadarLocationManager.kt | Modified geofence management to handle individual metadata |
| Radar.kt | Added geofence replacement call |
| AndroidManifest.xml | Added new intent filter actions |
| MyRadarReceiver.kt | Disabled notification functionality |
| MainActivity.kt | Updated API key and added debug logging |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
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.
Just noting that the behaviors are not explicitly tested.
I also concede that adding test coverage for this is non trivial due to the extensive mocking required.
Given the urgency of this ask, maybe as a fast follow up?
|
|
||
| return | ||
| } | ||
| // track successful |
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.
do we this this new comment?
| if (geofences.size == 0) { | ||
| logger.d("No synced geofences") | ||
| return@mapNotNull RadarAbstractLocationClient.RadarAbstractGeofence( | ||
| requestId = geofence._id, |
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.
requestId changed to geofence._id instead of radar_sync_1, radar_sync_2, etc. because we want to know which geofence it is
Client side geofence notification for Android
Test step: (using mock GPS)
With no frequency cap: Turn on override frequency cap on dashboard
There is an issue with this: