-
Notifications
You must be signed in to change notification settings - Fork 12
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
Basic NetP remote messaging #1665
Basic NetP remote messaging #1665
Conversation
…ove-hardcoded-message # By Fernando Bunn (4) and others # Via Sam Symons (2) and GitHub (1) * develop: (23 commits) Add run attempt to WORKFLOW_URL for reporting failed tests (#1671) Fix share in News (#1668) Set version to 1.57.1. Update BSK version for previous cherry-pick. The connection tester no longer disables on-demand (#1631) Home Button Implementation (#1649) Fix issue not setting maintenance date (#1663) Update DebugUI (#1659) Fix DBP on close notification (#1652) Ask for full disk access (NSAppDataUsageDescription) (#1639) Log user out of Sync when SyncOperation receives a HTTP 401 (#1666) fix first responder, improve logging, minor refactoring (#1657) Fix unit tests (#1662) Add child broker json files (#1660) Update HomePageContinueSetUpModel to not save a hardcoded Privacy Config reference (#1637) Update OpenSSL to 3.1.2000. (#1636) Onboarding flow fix for macOS Sonoma (#1655) Update profile address copy (#1658) Add WireGuard NetP Error Pixels (#1641) update bsk for bookmarks view model changes (#1640) ... # Conflicts: # DuckDuckGo/HomePage/Model/HomePageContinueSetUpModel.swift
…message' into sam/add-netp-messaging-support-part-2-add-remote-fetching # By Fernando Bunn (4) and others # Via Sam Symons (3) and GitHub (1) * sam/add-netp-messaging-support-part-1-remove-hardcoded-message: (23 commits) Add run attempt to WORKFLOW_URL for reporting failed tests (#1671) Fix share in News (#1668) Set version to 1.57.1. Update BSK version for previous cherry-pick. The connection tester no longer disables on-demand (#1631) Home Button Implementation (#1649) Fix issue not setting maintenance date (#1663) Update DebugUI (#1659) Fix DBP on close notification (#1652) Ask for full disk access (NSAppDataUsageDescription) (#1639) Log user out of Sync when SyncOperation receives a HTTP 401 (#1666) fix first responder, improve logging, minor refactoring (#1657) Fix unit tests (#1662) Add child broker json files (#1660) Update HomePageContinueSetUpModel to not save a hardcoded Privacy Config reference (#1637) Update OpenSSL to 3.1.2000. (#1636) Onboarding flow fix for macOS Sonoma (#1655) Update profile address copy (#1658) Add WireGuard NetP Error Pixels (#1641) update bsk for bookmarks view model changes (#1640) ... # Conflicts: # DuckDuckGo/HomePage/Model/HomePageContinueSetUpModel.swift # DuckDuckGo/Statistics/PixelEvent.swift # DuckDuckGo/Statistics/PixelParameters.swift # UnitTests/HomePage/ContinueSetUpModelTests.swift
Hey @samsymons - I can't make "Initial Card" come up for me. I'm following the instructions. Any thoughts of things I could try? |
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.
Works as described in the testing steps.
A few suggestions:
- It would be nice if resetting NetP also reset both the activation date, and the survey card.
- I haven't tested pixels since there aren't specific testing instructions. Do you think you could sum up testing instructions for those so I can double check?
- Could you expand a bit on the "rate limited operation" concept? I'm having a bit of trouble understanding the rate limiting bit in the context of this PR. Just an explanation of why thats needed / how it's necessary.
I think that should address all the feedback 🙏 |
Note that SwiftLint is failing on this PR because CI has updated it. I'll open a PR to fix lint warnings against develop in a separate PR. |
...ts/DeveloperIDTarget/NetworkProtectionRemoteMessaging/NetworkProtectionRemoteMessaging.swift
Outdated
Show resolved
Hide resolved
DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionAppEvents.swift
Show resolved
Hide resolved
Added some additional review comments. The optional ones can be safely ignored... the 1 that I think is slightly more important is the one about thread safety, but it should also be easy to address. |
…-remote-fetching * develop: Encapsulate handling one profile at the Repository level (#1676) Refactor MouseOverView/Button; fix bkm bar hlight glitch (#1667) Replace old privacy page with new - MacOS (#1611) Fix SwiftLint 0.53.0 warnings (#1683) fix crash in address bar selection macOS 12 (#1679) Fix of tab bar gap in Xcode 15 (#1680) Pixel m_mac_user_has_pinned_tab removed (#1675) Upload .xcresult artifacts on PR test failure (#1678) Prevent preferredRunDate from being set in the future (#1670) Fix Microsoft teams links opening in macOS browser (#1669) Bump version to 1.58.0 (61) Update embedded files Add underlying error info to the config fetch error (#1672)
…-remote-fetching # By Alexey Martemyanov (1) and others # Via GitHub * develop: Add scan and optout support for child sites (#1681) Connection interruption simulation option (#1686) Resolve naming inconsistencies (#1692) Improve Asana integration for failed PR checks (#1673) Favicon store refactoring (#1674) Update BSK with autofill 8.4.1 (#1687) # Conflicts: # DuckDuckGo/NetworkProtection/AppTargets/BothAppTargets/NetworkProtectionAppEvents.swift
…-pixels # Via GitHub * develop: Basic NetP remote messaging (#1665) # Conflicts: # DuckDuckGo/Main/View/MainViewController.swift # DuckDuckGo/Statistics/PixelParameters.swift
Task/Issue URL: https://app.asana.com/0/0/1205553443275690/f
Tech Design URL:
CC:
Description:
This PR adds basic remote messaging support for NetP.
This feature pings a new URL for messages to display, where each message can optionally include a number of days that NetP has to be active for it to be displayed, and an optional URL to open when the user clicks the action button.
This feature will be used by us to deliver surveys to users, and to tell them when the beta is ending.
This feature was built for NetP, so each card just uses a NetP icon.
Steps to test this PR:
First, activate NetP:
Now, test the remote messages:
How to test pixels:
m.mac.netp.remote.message.displayed.456.d
m.mac.netp.remote.message.opened.789
m.mac.netp.remote.message.dismissed.123
for each of themInternal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation