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

Remote Messaging Framework for macOS #3031

Merged
merged 30 commits into from
Jul 9, 2024
Merged

Remote Messaging Framework for macOS #3031

merged 30 commits into from
Jul 9, 2024

Conversation

ayoy
Copy link
Contributor

@ayoy ayoy commented Jul 3, 2024

Task/Issue URL: https://app.asana.com/0/72649045549333/1202913520695928/f
CC: @amddg44 @samsymons

Description:
This change adjusts RMF implementation to updates in BSK that add support for RMF on macOS.

  • RMF is now controlled by a feature flag, enabled by default on iOS.
  • Remote messaging config matcher creation has been moved into a standalone provider class
    that is otherwise separated from the RMF client.
  • RemoteMessagingClient now implements a protocol from BSK that provides implementation
    for fetching and processing the config. The API is not static and the instance of the client is owned
    by AppDelegate.
  • RemoteMessagingStore and HomePageConfiguration were moved out of AppDependencyProvider
    to have better control over their instantiation time and to allow for dependency injection.

Steps to test this PR:

  1. In RemoteMessagingClient.swift, update endpoint to return https://www.jsonblob.com/api/1258315611053613056 for debug builds.
  2. Delete the app from simulator/device to ensure app data is gone
  3. Run the app and complete onboarding.
  4. After the onboarding, open new tab page
  5. Verify that you see "Message 1: Placeholder Title" message.
  6. Don't dismiss the message and restart the app.
  7. Verify that you see "Message 1: Placeholder Title" message again. Dismiss the message and restart the app.
  8. Verify that you see "Message 2: Placeholder Title" message.
  9. Dismiss the message and restart the app 3 more times, verify that on each subsequent run you're seeing message 3, 4 and 5. After dismissing message 5 and restarting the app, no message should be shown.

Definition of Done (Internal Only):

Copy Testing:

  • Use of correct apostrophes in new copy, ie rather than '

Orientation Testing:

  • Portrait
  • Landscape

Device Testing:

  • iPhone SE (1st Gen)
  • iPhone 8
  • iPhone X
  • iPhone 14 Pro
  • iPad

OS Testing:

  • iOS 15
  • iOS 16
  • iOS 17

Theme Testing:

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

@ayoy ayoy marked this pull request as ready for review July 4, 2024 07:19
@ayoy ayoy requested a review from jaceklyp July 4, 2024 07:20
@ayoy ayoy assigned jaceklyp and unassigned ayoy Jul 4, 2024
Copy link

github-actions bot commented Jul 4, 2024

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS against 1ef7733

surveyActionMapper = DefaultRemoteMessagingSurveyURLBuilder(statisticsStore: statisticsStore, subscription: nil)
}

let dismissedMessageIds = store.fetchDismissedRemoteMessageIds()
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny thing, but I think we once established these acronyms should be all capitalized, so IDs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I personally hate that too :D just haven't updated it because it's been like that. Happy do to it since I've moved/updated the entire protocol anyway. Thanks for pointing it out!

@ayoy ayoy merged commit a68d0f9 into main Jul 9, 2024
14 checks passed
@ayoy ayoy deleted the dominik/rmf-macos branch July 9, 2024 06:41
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.

None yet

2 participants