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

Prompt to store cards and identities #461

Merged
merged 38 commits into from
Mar 29, 2022

Conversation

samsymons
Copy link
Collaborator

@samsymons samsymons commented Mar 10, 2022

Task/Issue URL: https://app.asana.com/0/0/1201938691568506/f
Tech Design URL:
CC: @brindy

Description:

This PR updates the macOS browser to support saving and storing payment methods and identities.

Steps to test this PR:

  1. Test that prompting to save credentials still works as expected.
  2. Test that prompting to save payment methods works as expected. You can use this site to test.
  3. Test that prompting to save identities works as expected. I've been using the Amazon address form to test this, but Emanuele might have suggestions for other ways to test.
  4. Test that disabling the three new settings in preferences causes each of these prompt types to no longer appear.
  5. Test that the Settings gear icon on each prompt takes you to preferences.

Test websites:

Testing checklist:

  • Test with Release configuration

Internal references:

Software Engineering Expectations
Technical Design Template
When ready for review, remember to post the PR in MM

samsymons added 14 commits March 2, 2022 12:59
# By Tomas Strba (6) and others
# Via GitHub (1) and Tomas Strba (1)
* develop:
  Logins+ authentication (#438)
  Update privacy dashboard to main (#451)
  Version 0.19.1
  Remove mouse event listeners on browser close (#448)
  Find in Page Hotfix + refactoring of forced unwrapping (#450)
  Memory leak fixed (#449)
  Version 0.19.0
  Top autofill (#432)
  Option to add new notes or edit existing is disabled (#446)
  Use our own autoconsent fork (#444)
  New Feedback Form (#424)
  Update privacy dashboard (#440)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
#	DuckDuckGo/SecureVault/View/PasswordManager.storyboard
# By Dominik Kapusta (2) and others
# Via GitHub
* develop:
  Animate dashboard resizing (#454)
  Dashboard in background tabs (#458)
  Fix resubmitting search with spaces (#456)
  Use smarter encryption feature from the BSK (#441)
  Use pageZoom instead of magnification when adjusting web view zoom level (#455)
  Version 0.19.2

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
return label
}

static func optionalLabel(titled title: String?) -> NSTextField? {
Copy link
Collaborator Author

@samsymons samsymons Mar 10, 2022

Choose a reason for hiding this comment

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

Don't love this function signature, am open to suggestions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically we can do this:

identity.addressStreet
    .flatMap(NSTextField.label(titled:))
    .map(identityStackView.addArrangedSubview(_:))

Here we use Optional.flatMap which is a no-op for .none and otherwise produces a non-optional NSTextField, which is then added to the stack view using map. And then we don't need optionalLabel and a variant of addArrangedSubview that takes an optional. We can even go further and do this:

[
    identity.addressStreet,
    identity.addressStreet2, 
    identity.addressCity,
    identity.addressProvince,
    identity.addressPostalCode,
    identity.addressCountryCode
]
    .forEach { value in
        value.flatMap(NSTextField.label(titled:))
            .map(identityStackView.addArrangedSubview(_:))
    }

I personally consider such code "clever", which does not necessarily mean "good", so it's a matter of deciding if we think it's understandable to the reader. Perhaps it's a valid alternative to having optionalLabel kind of API :)

Copy link
Contributor

@brindy brindy left a comment

Choose a reason for hiding this comment

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

  1. https://www.roboform.com/filling-test-all-fields
    Saw this form create a prompt once, but couldn't get it to do it again even after clearing data and running a clean build. It also didn't offer to re-fill them when I came back to the page when I did have some data saved.

  2. Disabling the prompts worked, but turning them on again I wasn't getting prompts for the address or credit cards. Also tried fire button and restarting the browser.

  3. Would be good if the test outline had some definitive test sites we can use. That might mean creating them in our privacy test pages if there aren't some good public ones.

Looks like Emanuelle also has some issue so going to leave this for now. Ping me when it's ready for another look

@brindy brindy assigned brindy and samsymons and unassigned brindy Mar 10, 2022
* develop:
  New Tab Page (#433)
  Make website URLs clickable in Logins+ (#462)
  Correction of URL if it is missing one slash '/' (#459)
* develop:
  fix launch link when no windows are open (#470)
  Version 0.20.1
  Update storyboards to use correct color names. (#468)
  Fix suggestion selection crash (#469)
  Version 0.20.0
  Dev tools visibility fix (#457)
  Fix BrowserTabViewController leakage (#463)
  Remove the upgrade logic. (#460)
# By Tomas Strba (3) and others
# Via GitHub
* develop:
  Testing Checklist Updates (#480)
  Xcode Clean Up (#479)
  External App Scheme permission (#419)
  Update dashboard to correct version (#477)
  Update BSK to fix percent encoding query parameters (#475)
  bump version 0.21.0
  add alternate for paste and match with style (#474)
  First reponder issues when switching tabs (#464)
  Pre-beta copy updates (#472)
  Improvements to data clearing (#467)
  Open feedback form upon tapping "Send Feedback" in the About page (#473)
  new tab tweaks (#471)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved
#	DuckDuckGo/Secure Vault/View/SaveIdentityPopover.swift
#	DuckDuckGo/Secure Vault/View/SaveIdentityViewController.swift
#	DuckDuckGo/Secure Vault/View/SavePaymentMethodPopover.swift
#	DuckDuckGo/Secure Vault/View/SavePaymentMethodViewController.swift
@ayoy ayoy assigned ayoy and unassigned samsymons Mar 25, 2022
@ayoy ayoy self-requested a review March 25, 2022 07:07
Signed-off-by: Emanuele Feliziani <[email protected]>
Copy link
Collaborator

@ayoy ayoy left a comment

Choose a reason for hiding this comment

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

LGTM! I tested it in various ways and it does the job[0] 🚀 Great stuff! Please have a look into my comments for a possible alternative to optionalLabel.

[0] - Actually, I found that the CC info is not saved on Catalina. Saving identities and passwords work fine. On Big Sur it all works fine. I believe it's something on the JS side though, what do you think? Having said that, the Logins+ UI on Catalina has more issues (not related to this project) - perhaps it's just a separate topic to handle with a separate project. Or it will wait until we drop Catalina support 🤔

@ayoy ayoy assigned samsymons and unassigned ayoy Mar 25, 2022
@samsymons
Copy link
Collaborator Author

samsymons commented Mar 28, 2022

Good catch on the Catalina issue, I took a look through the code and also think that this is something on the JS side. This is one I'm going to write up as a separate task before merging, and then try to set up my spare laptop with Catalina so that I can test it later in the week.

Catalina support is something I would personally love to drop, it causes us quite a few headaches 😩

samsymons added a commit to duckduckgo/BrowserServicesKit that referenced this pull request Mar 29, 2022
Task/Issue URL: https://app.asana.com/0/0/1201938691568505/f
iOS PR: duckduckgo/iOS#1086
macOS PR: duckduckgo/macos-browser#461

Optional:

CC: @brindy

Description:

This PR updates BrowserServicesKit to handle the new pmHandlerStoreData call. As a part of this, it runs some logic to look up whether the provided payment method or address already exist in the database.

Steps to test this PR:

See macOS PR for more information
@samsymons
Copy link
Collaborator Author

samsymons commented Mar 29, 2022

I've written up a task for myself to get 10.15 set up and to do a deep dive into Logins+ issues, starting with this CC issue you found. 🙂 I've set it due Wednesday, but it might be later in the week depending on how busy the next couple days are.

@samsymons
Copy link
Collaborator Author

The push build is failing on a test that is known to be flaky, but the PR build has succeeded. I ran this test locally, and confirmed tests are green (log attached). I'm going to run this one more time just in case, but if it fails again I'm going to merge it.

Test DuckDuckGo Privacy Browser_2022-03-28T21-42-39.txt

@samsymons samsymons merged commit 3e4702a into develop Mar 29, 2022
@samsymons samsymons deleted the sam/autofill-cards-and-identities branch March 29, 2022 05:01
samsymons added a commit that referenced this pull request Mar 29, 2022
* develop:
  Prompt to store cards and identities (#461)
  Version 0.21.1
  inject CTL user script atDocumentEnd (#482)
  Fire button and shield button animations (#476)
  Add tds etag to breakage reports (#466)
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.

4 participants