Skip to content
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

Add notification status to firebase #36

Merged
merged 7 commits into from
Mar 7, 2024

Conversation

bryant-jimenez
Copy link
Contributor

@bryant-jimenez bryant-jimenez commented Mar 7, 2024

Timestamp Notification Status to Firebase, Update Notification Handling

♻️ Current situation & Problem

With the release of Spezi 1.2, new support for Notifications was added, breaking the current implementation of push notifications. Additionally, we required support for notification handling on the client side, for background, foreground, and user-actions regarding notifications. To log data about notification status, such as "received" and "opened", these handlers were necessary, alongside updates to the standard for the delivery of timestamps to firestore and dealing with notifications for the path. "Sent" status is logged to firestore on the server side.

⚙️ Release Notes

Updates

  • Migrated to Spezi 1.2 and updated support for notifications, as outlined in the Spezi documentation. This required updating many of the functions and ordering of necessary steps to conform to the FirebaseMessaging protocol.
  • New support for using getPath() in PrismaStandard for anything related to the notifications collection in firestore, as well as the type of data "logs" or schedule"
  • Extended the standard for PushNotifications, adding functions for addNotificationOpened() and addNotificationReceived() to Firestore
  • Added new support for specifying timezones to the .toISOFormat() method extension of the Date() class

Next Steps

  • Currently, notification handling distinguishes between foreground notifications and clicking on the notification, meaning that in firestore "opening" and "receiving" can't happen at the same time. Will follow up with the Spezi team on how to get this to work

📚 Documentation

Please ensure that you properly document any additions in conformance to Spezi Documentation Guide.
You can use this section to describe your solution, but we encourage contributors to document your reasoning and changes using in-line documentation.

✅ Testing

Please ensure that the PR meets the testing requirements set by CodeCov and that new functionality is appropriately tested.
This section describes important information about the tests and why some elements might not be testable.

📝 Code of Conduct & Contributing Guidelines

By submitting creating this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:

@bryant-jimenez bryant-jimenez self-assigned this Mar 7, 2024
Copy link

codecov bot commented Mar 7, 2024

Codecov Report

Attention: Patch coverage is 1.17647% with 84 lines in your changes are missing coverage. Please review.

Project coverage is 36.19%. Comparing base (2873e1e) to head (1efe818).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #36      +/-   ##
==========================================
- Coverage   37.38%   36.19%   -1.18%     
==========================================
  Files          42       43       +1     
  Lines        1576     1622      +46     
==========================================
- Hits          589      587       -2     
- Misses        987     1035      +48     
Files Coverage Δ
Prisma/Onboarding/OnboardingFlow.swift 96.00% <100.00%> (-0.42%) ⬇️
Prisma/PrismaDelegate.swift 96.52% <ø> (+3.26%) ⬆️
Prisma/Onboarding/NotificationPermissions.swift 2.23% <0.00%> (-2.12%) ⬇️
Prisma/Standard/PrismaStandard+Questionnaire.swift 0.00% <0.00%> (ø)
Prisma/Standard/PrismaModule.swift 0.00% <0.00%> (ø)
Prisma/Standard/PrismaStandard+HealthKit.swift 6.67% <0.00%> (ø)
Prisma/Standard/PrismaStandard.swift 3.83% <0.00%> (-0.07%) ⬇️
Prisma/Standard/PrismaStandard+Extension.swift 0.00% <0.00%> (ø)
...ma/Standard/PrismaStandard+PushNotifications.swift 0.00% <0.00%> (ø)
Prisma/PushNotifications/PushNotifications.swift 8.34% <0.00%> (-4.82%) ⬇️

... and 1 file with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2873e1e...1efe818. Read the comment docs.

Copy link
Contributor

@carolinentran carolinentran left a comment

Choose a reason for hiding this comment

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

Looks good to me. The code is well-commented and you broke down the large tasks well.

@bryant-jimenez bryant-jimenez merged commit f41a4ce into main Mar 7, 2024
7 checks passed
@bryant-jimenez bryant-jimenez deleted the bryant/notification-status-to-firebase branch March 7, 2024 10:43
bryant-jimenez added a commit that referenced this pull request Mar 12, 2024
# *Handling Background Notifications*

## ♻️ Current situation & Problem
- #31
- #36

Currently, Prisma is only configured to handle notifications in the
foreground. Handling notifications requires writing timestamps to
Firestore when notifications are received and opened on the user's
device, but this was not possible with the current configuration of
background notification handling due to Apple throttling the amount of
requests per hour [(see this
link)](https://developer.apple.com/documentation/usernotifications/pushing-background-updates-to-your-app).


## ⚙️ Release Notes 

As a workaround, we utilize the UNNotificationServiceExtension class to
provide an entry-point for pushing updates to Firestore on the arrival
of background notifications. Normally we would use this extension to
modify the notification’s content or download content related to the
extension, but we are taking advantage of the fact that we are allowed a
30 second period to do so, in order to write to Firestore. Doing so
required [creating a shared keychain for cross-app
authentication](https://firebase.google.com/docs/auth/ios/single-sign-on),
allowing access of the auth state for Firebase between the application
and the extension, utilized for authorizing writes to Firestore using
the same instance of Firebase as the application upon notification
arrival.

The following additions/changes were made to the Prisma application:

- Added the Notification Service Extension
`PrismaPushNotificationsExtension/NotificationsService.swift`
- `didReceive` implements functionality for writing to Firestore, using
the logs collection path passed in the notification payload. Because the
main application and app extension run independently of each other, we
needed to introduce an authorization mechanism that is checked in the
function before actually doing any writes.

- We also added a keychain access group, allowing for keychain sharing
between the main app and the extension. We subsequently implemented the
authorization function `authorizeAccessGroupForCurrentUser()` which
checks for the shared access group instance for a user, documented in
`PrismaStandard.swift`.
- This function is called within both the Home view and the
AccountOnboarding flow, ensuring that a user will always need to be
authorized and logged in in order to receive background notification
updates and have those writable to Firestore.



## 📚 Documentation
*Please ensure that you properly document any additions in conformance
to [Spezi Documentation
Guide](https://github.com/StanfordSpezi/.github/blob/main/DOCUMENTATIONGUIDE.md).*
*You can use this section to describe your solution, but we encourage
contributors to document your reasoning and changes using in-line
documentation.*


## ✅ Testing
*Please ensure that the PR meets the testing requirements set by CodeCov
and that new functionality is appropriately tested.*
*This section describes important information about the tests and why
some elements might not be testable.*


## 📝 Code of Conduct & Contributing Guidelines 

By submitting creating this pull request, you agree to follow our [Code
of
Conduct](https://github.com/CS342/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/CS342/.github/blob/main/CONTRIBUTING.md):
- [ x] I agree to follow the [Code of
Conduct](https://github.com/CS342/.github/blob/main/CODE_OF_CONDUCT.md)
and [Contributing
Guidelines](https://github.com/CS342/.github/blob/main/CONTRIBUTING.md).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants