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

Safari version warning label hidden if it's version is higher or equal to 15 #574

Merged
merged 2 commits into from
May 6, 2022

Conversation

tomasstrba
Copy link
Contributor

Task/Issue URL: https://app.asana.com/0/1177771139624306/1202116285049224/f

Description:
The import flow doesn't contain the Safari version warning if the current version requirement is met.

Steps to test this PR:

  1. On macOS Monterey: Go to "Import bookmarks and Passwords..", choose "Safari". Make sure there is no warning "Requires Safari 15 or later"
  2. On older macOS with Safari version 14 or older: Go to "Import bookmarks and Passwords..", choose "Safari". Make sure the warning "Requires Safari 15 or later" is present

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

@samsymons samsymons self-assigned this May 5, 2022
Copy link
Collaborator

@samsymons samsymons left a comment

Choose a reason for hiding this comment

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

LGTM, though I think we can improve the SafariVersionReader in a few ways:

  1. We could update it to look up Safari's path at runtime using NSWorkspace.shared.absolutePathForApplication(withBundleIdentifier: safariBundleID), since I don't trust that Safari will always be installed at that location
  2. It could take a dependency injected path, defaulting to the path provided by NSWorkspace
  3. Then, we could bundle in different copies of the Safari Info.plist and pass their paths in during unit tests, to really verify that the version checks are accurate.

All of these are nice to haves that aren't directly related to the code edited in this PR, so feel free to disregard them. 🙂

@samsymons samsymons assigned tomasstrba and unassigned samsymons May 6, 2022
@tomasstrba tomasstrba merged commit e5ee593 into develop May 6, 2022
@tomasstrba tomasstrba deleted the tom/safari-label branch May 6, 2022 11:14
@tomasstrba
Copy link
Contributor Author

Thanks @samsymons! Agree with all improvements of SafariVersionReader 👍

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