NetP Invite code screen PR 2 #1883

graeme · 2023-07-31T17:13:56Z

Task/Issue URL: https://app.asana.com/0/0/1205084446087083/f
BSK PR: duckduckgo/BrowserServicesKit#441
CC:

Description:

Second PR for the NetP Invite Code flow. You can find designs in the linked task above. This builds upon #1881 and adds to the existing Invite Code presentation logic in the NetworkProtectionInviteViewModel. It also fleshes out and designs the NetworkProtectionInviteView which has two states:

codeEntry
success

This also adds some tests which required some mocks / test helpers for types in the NetworkProtection library in BSK. As we will likely need these on macOS when we come to add more tests there, I added a NetworkProtectionTestUtils library to BSK. This PR points to that BSK branch so I will make sure to point to the BSK release once that’s approved. BSK PR linked above.

Steps to test this PR:

Check out this branch
Run the app
Ensure you’re authenticated as internal
Navigate to the Settings
Select Network Protection
If you’ve already enabled it, tap the button to clear the invite code then navigate back and select the NetP item again
Enter a random string and submit it to trigger the error label.
Now enter an invite code from Network Protection Invite Codes and submit it
The success screen will be shown. Click Get Started to launch the status view

Copy Testing:

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

Orientation Testing:

Portrait
Landscape

Device Testing:

OS Testing:

iOS 14
iOS 15
iOS 16

Theme Testing:

Light theme
Dark theme

Internal references:

Software Engineering Expectations
Technical Design Template

github-actions · 2023-07-31T17:16:37Z

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

Generated by 🚫 dangerJS against b8a6492

To be moved to BSK

graeme · 2023-07-31T18:46:07Z

DuckDuckGo/AutofillLoginDetailsView.swift

@@ -236,43 +236,6 @@ struct AutofillLoginDetailsView: View {
    }
 }

-struct ClearTextField: View {


This makes more sense in the shared DuckUI library

This confused me a bit because it was ultimately moved to a different file right next to this one.

That's fine, but just adding a comment to make sure that was the intention.

My comment was incorrect, it was actually moved into its own file in the shared UI components folder, not library. DuckUI doesn’t seem to have much in it at all. Nonetheless, I thought it didn’t make much sense in the AutofillLoginDetailsView file when it’s now being used from a couple of places.

graeme · 2023-07-31T18:47:22Z

DuckDuckGo/NetworkProtectionInviteViewModel.swift

-    private static let errorEvents: EventMapping<NetworkProtectionError> = .init { _, _, _, _ in
-
-    }
+    private let completion: () -> Void


Used to pop the nav stack and navigate to the status view

graeme · 2023-07-31T18:48:30Z

DuckDuckGo/NetworkProtectionInviteViewModel.swift

    }

+    // MARK: Dev only. Will be removed during https://app.asana.com/0/0/1205084446087078/f


I was using this same ViewModel for the basic invite code entry on the status view. Rather than move it in there, I will keep these functions here for now until I’ve added a replacement, probably in the debug menu, as part of the status view subtask.

graeme · 2023-07-31T18:49:40Z

DuckDuckGo/NetworkProtectionRootView.swift


    var body: some View {
+        let inviteViewModel = NetworkProtectionInviteViewModel(


This is not idea for now. It would be good to use a more elegant dependency injection mechanism than letting the View do it. But for now, this will do. Once the rest of the feature is more fleshed out, I’ll tidy this up.

What's a "DI mechanism"?

dependency injection (updated original comment)

graeme · 2023-07-31T18:50:28Z

DuckDuckGo/NetworkProtectionRootViewController.swift

+
+import SwiftUI
+
+final class NetworkProtectionRootViewController: UIHostingController<NetworkProtectionRootView> {


This subclass is needed so we can apply some styling to background of the root view. This will probably be a better place to manage the dependency injection in the future too.

graeme · 2023-07-31T18:51:14Z

DuckDuckGo/NetworkProtectionStatusView.swift

@@ -64,38 +64,22 @@ struct NetworkProtectionStatusView: View {
    @ViewBuilder
    func inviteCodeEntry() -> some View {
        Section {
-            VStack(alignment: .leading) {


Gets rid of the initial dev-designed invite entry on the dev-designed status view

graeme · 2023-07-31T18:52:12Z

DuckDuckGo/SettingsViewController.swift

-        let statusView = NetworkProtectionRootView()
-        let hostingController = UIHostingController(rootView: statusView)
-        hostingController.view.backgroundColor = UIColor(Color(designSystemColor: .background))
+        // This will be tidied up as part of https://app.asana.com/0/0/1205084446087078/f


I will probably pull this out into a presenter / launcher / router type class.

graeme · 2023-07-31T18:53:20Z

DuckDuckGoTests/NetworkProtectionInviteViewModelTests.swift

+        XCTAssertEqual(viewModel.currentStep, .codeEntry)
+    }
+
+    // Disabled this test but keeping it around to document the behaviour. It is failing inexplicably.


For some reason, errors thrown in the library are not caught in the main app. This does not apply in production, only the test environment. You can manually test this to be sure by entering an incorrect invite code.

diegoreymendez

@graeme - Adding some comments for your consideration. The implementation looks clean and mostly to be working fine.

Please let me know what you think about my comments, so we can move ahead - if we find agreement in the comments I'd even be happy to tackle it in a follow up if you so prefer.

Additional points:

The invite code input screen felt like it wasn't working at first and the Submit button remained enabled for a bit allowing me to click it 3 times before it worked. I'm not sure what's behind this, but I suspect it's network related... it may be a good idea to offer some visual cue that you've submitted (maybe disable the button?).
I have to long-press the "Clear invite code" button for it to highlight.

diegoreymendez · 2023-08-02T17:07:08Z

DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

-          "branch": null,
-          "revision": "f255099c2c373d7b0c484849b8903831e7b717ce",
-          "version": "71.0.0"
+          "branch": "graeme/network-protection-test-utils",


Friendly visual reminder to help you update the BSK version before merging.

diegoreymendez · 2023-08-02T17:13:00Z

DuckDuckGo/Assets.xcassets/IntiveLockSuccess.imageset/Contents.json

+{
+  "images" : [
+    {
+      "filename" : "Intive-Lock-Success-96.pdf",


Should "Intive" be "Invite"? This typo is present apparently in several file names, but I'm adding a single note here to avoid overcluttering the PR with comments.

diegoreymendez · 2023-08-02T17:14:48Z

DuckDuckGo/AutofillLoginDetailsView.swift

@@ -236,43 +236,6 @@ struct AutofillLoginDetailsView: View {
    }
 }

-struct ClearTextField: View {


This confused me a bit because it was ultimately moved to a different file right next to this one.

That's fine, but just adding a comment to make sure that was the intention.

graeme · 2023-08-02T20:00:54Z

@diegoreymendez I’ve address the typo in the InviteLockSuccess image and I’m now disabling the textfield and submit button when the request is loading. However, I’m not going to put any work into fixing the Clear Invite Code button as it’s a development button for making testing easier and it will be removed in the next task.

diegoreymendez · 2023-08-03T08:58:39Z

However, I’m not going to put any work into fixing the Clear Invite Code button as it’s a development button for making testing easier and it will be removed in the next task.

That's fine for me.

About this one though:

The invite code input screen felt like it wasn't working at first and the Submit button remained enabled for a bit allowing me to click it 3 times before it worked. I'm not sure what's behind this, but I suspect it's network related... it may be a good idea to offer some visual cue that you've submitted (maybe disable the button?).

This is also not blocking, but disabling the button may be a good way to signal to the user that a request is ongoing.

That said, since nothing is truly blocking here I'll move ahead to approve.

Task/Issue URL: https://app.asana.com/0/0/1205084446087083/f Description: First PR for the NetP Invite Code flow. You can find designs in the linked task above. This first PR is just to add a root view which will choose between either the (initially empty) Invite or the Status View.

…-code-pr-2

graeme changed the base branch from develop to graeme/netp-invite-code-pr-1 July 31, 2023 17:14

graeme force-pushed the graeme/netp-invite-code-pr-2 branch from f4a6d42 to 088c616 Compare July 31, 2023 17:59

graeme added 4 commits July 31, 2023 20:04

Add convenience init for token store

617961b

Add a root view to choose which netp view to show

d7c5acf

Create empty invite view

c8b01ed

Show root and therefore invite view from settings

7f5ced2

graeme force-pushed the graeme/netp-invite-code-pr-1 branch from 47e63f4 to 7f5ced2 Compare July 31, 2023 18:04

graeme added 22 commits July 31, 2023 20:05

Add invite lock assets

e1f20e8

Implement initial invite view

886a979

Add convenience init for redemption coordinator

076e0cb

Copy over some useful invite code from macOS

cef1497

Remove invite entry from Status View

219b29b

Add some mocks and test helpers

6456bbf

To be moved to BSK

Write tests + complete invite presentation logic

3c45169

Add success view

eef4e9a

Replace delegation with completion handler

6914b2a

Show an alert on error

27b3f09

Move ClearTextField out of Autofill

77c9c6f

Disable submit on empty string

6115e7d

Add submit button text

f3a81dc

and the text

0286d01

General tidy up

598ab51

Pop 'n' push the root view on get started

c2c8f59

Move mocks to BSK

18dbdeb

Make image correct size

023f758

Reduce padding below message text

f000125

Create NetPRootViewController for dark styling

196350f

Fix invite view dark text

e5fbf53

Remove unused colors

2429087

Add a nav title

e3c3b34

graeme force-pushed the graeme/netp-invite-code-pr-2 branch from 088c616 to 2e94e51 Compare July 31, 2023 18:11

Disable failing invite code error test

72c9596

graeme force-pushed the graeme/netp-invite-code-pr-2 branch from 2e94e51 to 72c9596 Compare July 31, 2023 18:20

graeme mentioned this pull request Jul 31, 2023

NetP invite code screen PR 1 #1881

Merged

13 tasks

graeme commented Jul 31, 2023

View reviewed changes

graeme mentioned this pull request Jul 31, 2023

Create NetworkProtectionTestUtils duckduckgo/BrowserServicesKit#441

Merged

6 tasks

graeme requested a review from diegoreymendez July 31, 2023 19:27

graeme marked this pull request as ready for review July 31, 2023 19:27

graeme changed the title ~~NetP invite code pr 2~~ NetP Invite code screen PR 2 Jul 31, 2023

diegoreymendez reviewed Aug 2, 2023

View reviewed changes

graeme added 2 commits August 2, 2023 21:56

Disable user interaction on making network request

4187e37

Fix typo in invite image name

fd3b43c

Base automatically changed from graeme/netp-invite-code-pr-1 to develop August 3, 2023 08:56

diegoreymendez approved these changes Aug 3, 2023

View reviewed changes

graeme added 4 commits August 3, 2023 11:15

Merge remote-tracking branch 'origin/develop' into graeme/netp-invite…

0d2dc8c

…-code-pr-2

Point to updated BSK release

210a435

Merge remote-tracking branch 'origin/develop' into graeme/netp-invite…

b8a6492

…-code-pr-2

graeme merged commit c1d0cba into develop Aug 3, 2023

graeme deleted the graeme/netp-invite-code-pr-2 branch August 3, 2023 10:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NetP Invite code screen PR 2 #1883

NetP Invite code screen PR 2 #1883

graeme commented Jul 31, 2023 •

edited

Loading

github-actions bot commented Jul 31, 2023 •

edited

Loading

graeme Jul 31, 2023

diegoreymendez Aug 2, 2023

graeme Aug 2, 2023

graeme Jul 31, 2023

graeme Jul 31, 2023

graeme Jul 31, 2023 •

edited

Loading

diegoreymendez Aug 1, 2023 •

edited

Loading

graeme Aug 2, 2023

graeme Jul 31, 2023 •

edited

Loading

graeme Jul 31, 2023

graeme Jul 31, 2023

graeme Jul 31, 2023

diegoreymendez left a comment •

edited

Loading

diegoreymendez Aug 2, 2023

diegoreymendez Aug 2, 2023

diegoreymendez Aug 2, 2023

graeme commented Aug 2, 2023

diegoreymendez commented Aug 3, 2023

		}

		// MARK: Dev only. Will be removed during https://app.asana.com/0/0/1205084446087078/f


		var body: some View {
		let inviteViewModel = NetworkProtectionInviteViewModel(


		import SwiftUI

		final class NetworkProtectionRootViewController: UIHostingController<NetworkProtectionRootView> {

NetP Invite code screen PR 2 #1883

NetP Invite code screen PR 2 #1883

Conversation

graeme commented Jul 31, 2023 • edited Loading

Internal references:

github-actions bot commented Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graeme Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

diegoreymendez Aug 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graeme Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graeme commented Aug 2, 2023

diegoreymendez commented Aug 3, 2023

graeme commented Jul 31, 2023 •

edited

Loading

github-actions bot commented Jul 31, 2023 •

edited

Loading

graeme Jul 31, 2023 •

edited

Loading

diegoreymendez Aug 1, 2023 •

edited

Loading

graeme Jul 31, 2023 •

edited

Loading

diegoreymendez left a comment •

edited

Loading