This repository has been archived by the owner on Feb 24, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 16
Basic NetP remote messaging #1665
Merged
samsymons
merged 29 commits into
develop
from
sam/add-netp-messaging-support-part-2-add-remote-fetching
Oct 2, 2023
Merged
Basic NetP remote messaging #1665
samsymons
merged 29 commits into
develop
from
sam/add-netp-messaging-support-part-2-add-remote-fetching
Oct 2, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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)
diegoreymendez
approved these changes
Sep 28, 2023
…-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
samsymons
added a commit
that referenced
this pull request
Oct 2, 2023
…-pixels # Via GitHub * develop: Basic NetP remote messaging (#1665) # Conflicts: # DuckDuckGo/Main/View/MainViewController.swift # DuckDuckGo/Statistics/PixelParameters.swift
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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