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

Add data import failure pixels #552

Merged
merged 6 commits into from
Apr 22, 2022
Merged

Add data import failure pixels #552

merged 6 commits into from
Apr 22, 2022

Conversation

samsymons
Copy link
Collaborator

@samsymons samsymons commented Apr 21, 2022

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

Description:

This PR adds a new pixel which reports the import source, import type, and error message when data import fails.

The format and parameters of this pixel are found here.

The pixel from this PR will be used to drill down into where the importer is failing and why next week, and translate into some fixes. One thing we're missing is the actual versions of the files we're reading from, which will be added in a follow-up later.

Steps to test this PR:

First: turn on pixel logging in Logging.swift.

Next: It's hard to set up the import failure conditions needed, so I recommend picking one of the import classes and adding a hardcoded failure to it. This won't help with testing whether the actual importer classes themselves are returning the right error at the right time, but I haven't modified those here and plan to expand their test suites in a PR to ensure that all cases are covered next week.

You can use ChromiumDataImporter.swift and add the following line to the top of the importData function (of course, please tweak the failure type to test that the pixel is accurate):

completion(.failure(.bookmarks(.browserNeedsToBeClosed)))
return

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
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.

Did a bit of a smoke test, checked importing still works, etc. Difficult to test the scenarios of course, but it LGTM. Just one comment about the logging strings to use, I don't have a strong opinion.

DuckDuckGo/Data Import/DataImport.swift Outdated Show resolved Hide resolved
@brindy brindy assigned samsymons and unassigned brindy Apr 21, 2022
@samsymons samsymons merged commit e026e2f into develop Apr 22, 2022
@samsymons samsymons deleted the sam/data-import-pixels branch April 22, 2022 01:22
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
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