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

Basic NetP remote messaging #1665

Merged

Conversation

samsymons
Copy link
Collaborator

@samsymons samsymons commented Sep 22, 2023

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:

  1. Reset NetP state completely
  2. Turn on the NetP waitlist active and waitlist beta feature flags in the debug menu
  3. Join the waitlist
  4. After you join the waitlist, use the debug menu to bypass the invite code flow and let you into the beta by entering your own invite code (let me know if that doesn't work, we haven't really used that feature since the ship review)

Now, test the remote messages:

  1. Background and foreground the app a couple times, to give the app a chance to trigger the remote message fetch and refresh the new tab page
  2. Open the new tab page
  3. Check that you have a single new message in the new tab page titled "Initial Card"
  4. Go to the Debug menu's Network Protection setting and pick "Set Activation Date to 10 Days Ago" from the NetP Activation Override menu
  5. Go back to the new tab page and check that two new cards have appeared
  6. Check that you can click the survey link on the last card and it opens a new tab, then when you go back to the new tab page it's gone
  7. Test dismissing the other two messages via the X or Dismiss buttons
  8. Open a new window/tab and verify that the messages are gone
CleanShot 2023-09-21 at 20 44 19@2x

How to test pixels:

  1. Turn on Logging in the Debug menu via Debug -> Logging -> Pixel
  2. Set your activation date to 10 days via Debug -> Network Protection -> Override NetP Activation Date -> Set Activation Date to 10 Days Ago
  3. Reset cards via Debug -> Network Protection -> Reset Remote Messages
  4. Reset daily pixel state via Debug -> Reset Data -> Reset Daily Pixels
  5. Background and foreground the app a couple times to have it trigger remote message fetch and update the new tab page
  6. Once cards have appeared, you should see three pixel log entries like m.mac.netp.remote.message.displayed.456.d
  7. Click the "Take Survey" button on the third card and you should see m.mac.netp.remote.message.opened.789
  8. Click the X button on the first and the Dismiss button on the second card, and you should see logs like m.mac.netp.remote.message.dismissed.123 for each of them

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@github-actions
Copy link
Contributor

github-actions bot commented Sep 22, 2023

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

Generated by 🚫 dangerJS against 793c4f3

@samsymons samsymons changed the base branch from sam/add-netp-messaging-support-part-1-remove-hardcoded-message to develop September 24, 2023 20:07
@samsymons samsymons changed the title Basic NetP Remote Messaging, Part 2: Add remote messaging support Basic NetP remote messaging Sep 24, 2023
…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
@diegoreymendez
Copy link
Contributor

Hey @samsymons - I can't make "Initial Card" come up for me. I'm following the instructions. Any thoughts of things I could try?

Copy link
Contributor

@diegoreymendez diegoreymendez left a 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.

@diegoreymendez diegoreymendez self-requested a review September 26, 2023 09:48
@samsymons
Copy link
Collaborator Author

@diegoreymendez

  1. I've updated the reset option for NetP to remove activation state and messages
  2. I've added a new debug menu option to reset Daily Pixel state
  3. I've added a option to reset remote messages specifically, in case you want to reset those without resetting all of NetP
  4. I added a little doc comment to the rate limited operation class - let me know if it's clear, please don't hesitate to push back if it's not, or if you think the class name/design could use improvement
  5. I added a section to the description titled How to test pixels which should give you everything you need to test them, let me know if anything there doesn't work

I think that should address all the feedback 🙏

@samsymons
Copy link
Collaborator Author

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.

@diegoreymendez
Copy link
Contributor

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
…-remote-fetching

# By Juan Manuel Pereira (2) and others
# Via GitHub
* develop:
  Update Sparkle to 2.5.0 (#1645)
  Opt-out pixels improvements (#1694)
  Run DBP tests on the CI (#1682)
  Fix for Autofill sections not sorting correctly (#1677)

# Conflicts:
#	DuckDuckGo/Statistics/PixelEvent.swift
@samsymons samsymons merged commit 075c821 into develop Oct 2, 2023
13 checks passed
@samsymons samsymons deleted the sam/add-netp-messaging-support-part-2-add-remote-fetching branch October 2, 2023 05:18
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 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.

2 participants