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

Move more RMF code from iOS to BSK #2967

Merged
merged 16 commits into from
Jun 20, 2024
Merged

Move more RMF code from iOS to BSK #2967

merged 16 commits into from
Jun 20, 2024

Conversation

ayoy
Copy link
Contributor

@ayoy ayoy commented Jun 18, 2024

Task/Issue URL: https://app.asana.com/0/1201621708115095/1207573461751430/f
CC: @amddg44

Description:
This change moves reusable parts of the Remote Messaging Framework code over from the iOS repository.
The moved parts are: storage class, request class, Core Data model and storage unit tests.

Steps to test this PR:
These steps are supposed to ensure that database doesn't get corrupted as a result of moving RMF code to BSK:

  1. Check out the main branch
  2. In RemoteMessagingRequest.swift, update endpoint to return https://www.jsonblob.com/api/1252947611702124544
  3. Delete the app from simulator/device to ensure app data is gone
  4. Run the app (from main with the updated URL) and complete onboarding.
  5. After the onboarding, open new tab page
  6. Verify that you see "Message 1: Placeholder Title" message.
  7. Dismiss the message and close the app.
  8. Reset the branch and switch to dominik/rmf-bsk.
  9. Update the endpoint in RemoteMessagingClient.swift to the same value as in step 2.
  10. Run the app (it's crucial that the main branch app data persists).
  11. Verify that you see "Message 2: Placeholder Title" message.
  12. 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.

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 14
  • iOS 15
  • iOS 16

Theme Testing:

  • Light theme
  • Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

@ayoy ayoy self-assigned this Jun 18, 2024
Copy link

github-actions bot commented Jun 18, 2024

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

Generated by 🚫 dangerJS against 19f58a1

@ayoy ayoy marked this pull request as ready for review June 19, 2024 12:02
@ayoy ayoy requested a review from dus7 June 19, 2024 12:04
@ayoy ayoy assigned dus7 and unassigned ayoy Jun 19, 2024
Copy link
Contributor

@dus7 dus7 left a comment

Choose a reason for hiding this comment

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

I went through the steps provided - works fine. There's one non-critical comment for your consideration.

}
}

override init(mapping: @escaping EventMapping<RemoteMessagingStoreError>.Mapping) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's beneficial to add @available(*, unavailable, message: "Use init() instead") so you get an immediate feedback when trying to use this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, great idea! Let me update it, thanks!

@@ -28,11 +28,15 @@ import RemoteMessaging
import NetworkProtection
import Subscription

struct RemoteMessaging {
struct RemoteMessagingClient {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The RemoteMessaging struct had to be renamed to avoid name clashing with RemoteMessaging BSK module. This wasn't the case until we added RemoteMessaging.bundle in the BSK module, which confused the compiler.

import Foundation
import RemoteMessaging

public class RemoteMessagingStoreErrorHandling: EventMapping<RemoteMessagingStoreError> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This class intercepts error events emitted by RemoteMessagingStore and fires pixels as needed.

}
}

override init(mapping: @escaping EventMapping<RemoteMessagingStoreError>.Mapping) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely, great idea! Let me update it, thanks!

@ayoy ayoy merged commit 555b796 into main Jun 20, 2024
14 checks passed
@ayoy ayoy deleted the dominik/rmf-bsk branch June 20, 2024 12:59
samsymons added a commit that referenced this pull request Jun 20, 2024
# By Diego Rey Mendez (1) and others
# Via GitHub
* main:
  Improve prerequisite checks for VPN start (#2970)
  Move more RMF code from iOS to BSK (#2967)
  Remove unneeded pixel after investigation (#2972)
  Add quality signal for bookmarks database (#2964)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
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