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

Update the Fireproof checkmark in the Save Credentials view controller #555

Merged
merged 4 commits into from
Apr 26, 2022

Conversation

samsymons
Copy link
Collaborator

@samsymons samsymons commented Apr 25, 2022

Task/Issue URL: https://app.asana.com/0/1177771139624306/1202118895326543/f
Tech Design URL:
CC:

Description:

This PR updates the Save Credentials VC to accurately reflect whether a domain is already fireproof, and to change that in the database if the checkmark changes. That is, if a domain is already fireproof and you un-check that box before saving credentials, it will now remove it from the Fireproof store.

This PR has also taken a pass over the debug menu, moving all of the "Reset *" options into their own "Reset Data" submenu, and adding a new "UI Triggers" submenu for triggering pieces of the UI which can be annoying to iterate on if you need to run through a bunch of steps on a website. There's only one option in this menu for now, which is to trigger the Save Credentials VC.

Note: I went back and forth on a full refactor of this popover, but have since been given another task which requires making larger changes to this UI, so I've decided to just open this PR now with the simplest changes possible in order to get this fixed.

Steps to test this PR:

  1. Make sure you don't have any credentials for example.com in Autofill, and that example.com is not Fireproof
  2. Go to Debug -> UI Triggers -> Show Save Credentials Popover
  3. Verify that the Fireproof checkbox is not checked
  4. Check the Fireproof checkbox, and save the credentials - you should now have some creds for example.com in Autofill and it should be Fireproof
  5. Delete the credentials from Autofill, since you can't save the same credentials repeatedly
  6. Open the popover via the Debug menu again, verify that the Fireproof checkbox is checked this time
  7. Uncheck the Fireproof checkbox and save the credentials
  8. Open the popover one last time, the Fireproof checkbox should not be checked any more

Testing checklist:

  • Test with Release configuration
  • Test proper deallocation of tabs
  • Make sure committed submodule changes are desired

Internal references:

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

Copy link
Collaborator

@Bunn Bunn left a comment

Choose a reason for hiding this comment

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

✅ PR test flow is working correctly
✅ Tested with both debug and release config
✅ LGTM

I've left a comment with a small suggestion (that might not be very good haha), would like to know your opinion on it.

Comment on lines -511 to -513
var legacyStore = LocalStatisticsStore.LegacyStatisticsStore()
legacyStore.atb = "fake-atb-value"
legacyStore.installDate = Date()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also I added this for the lock screen ages ago, but it was mostly a way to test the upgrade flow during PR review - that is long since resolved and never needs testing again, so I've removed it.

@samsymons samsymons merged commit db86137 into develop Apr 26, 2022
@samsymons samsymons deleted the sam/save-credentials-fireproof-fix branch April 26, 2022 02:56
samsymons added a commit that referenced this pull request Apr 27, 2022
# By Alexey Martemyanov (3) and others
# Via GitHub
* develop:
  Lazy load background tabs at app startup (#553)
  Update the Fireproof checkmark in the Save Credentials view controller (#555)
  Support config v2 (#528)
  Fullscreen video fixing (#541)
  Add data import failure pixels (#552)
  Update BSK to fix autofill on Catalina (#551)
  fix contrast bug on Catalina / Big Sur (#546)
  Disable download reload on page tab reactivation/session restoration (#516)
  Add "New Window" item to App Dock menu (#544)

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
samsymons added a commit that referenced this pull request Apr 29, 2022
Task/Issue URL: https://app.asana.com/0/1177771139624306/1202118895326543/f
Tech Design URL:
CC:

Description:

This PR adds an additional check that was missed in #555. I've re-used the same Asana task as a reference, since this is directly a part of those changes, which themselves have not yet shipped.
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