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.

Sign up for GitHub

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

Jump to bottom

sync conect #281

Merged
merged 17 commits into from
Mar 31, 2023
Merged

sync conect #281

merged 17 commits into from
Mar 31, 2023

Conversation

brindy
Copy link
Contributor

@brindy brindy commented Mar 28, 2023 •
edited
Loading

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/0/1204266236802307/f
iOS PR: duckduckgo/iOS#1604
macOS PR: duckduckgo/macos-browser#1062
What kind of version bump will this require?: Major

Optional:

Tech Design URL:
CC:

Description:

Add sync connect flow.

Steps to test this PR:

iOS connect from macOS

  1. Create an account on an iOS device or simulator
  2. Launch macOS, remove existing account if needed
  3. Begin turn on sync flow and continue to sync with another device - a QR code should be visible
  4. On iOS tap on Scan QR Code
  5. Using Manually Enter Code paste in the code
  6. Wait a few seconds and the account should appear on macOS
  7. Repeat the test using the Camera

iOS connect from iOS

  1. Create an account on device A
  2. Delete any existing account from device B
  3. On Device B turn on sync and continue through to sync another device
  4. Tap show QR Code
  5. Copy the code
  6. On Device A tap on Scan QR Code
  7. Paste it in as in the macOS flow
  8. After a few seconds the account should be connected
  9. Repeat with Camera

OS Testing:

  • iOS 14
  • iOS 15
  • iOS 16
  • macOS 10.15
  • macOS 11
  • macOS 12
Internal references:

Software Engineering Expectations
Technical Design Template

@brindy brindy self-assigned this Mar 30, 2023
This was referenced Mar 30, 2023
Merged
Merged
@brindy brindy marked this pull request as ready for review March 30, 2023 14:29
@brindy brindy assigned ayoy and unassigned brindy Mar 30, 2023
ayoy
ayoy approved these changes Mar 30, 2023
View reviewed changes
Copy link
Contributor

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

Looks great! Only a couple of minor comments.

Sources/DDGSync/DDGSyncing.swift Outdated Show resolved Hide resolved
Sources/DDGSync/SyncModels.swift Outdated Show resolved Hide resolved
Sources/DDGSync/internal/RemoteConnector.swift Outdated Show resolved Hide resolved
@ayoy ayoy assigned brindy and unassigned ayoy Mar 30, 2023
samsymons
samsymons reviewed Mar 30, 2023
View reviewed changes
Sources/DDGSync/internal/RecoveryKeyTransmitter.swift
Comment on lines +49 to +54
let request = api.createRequest(url: endpoints.connect,
method: .POST,
headers: ["Authorization": "Bearer \(token)"],
parameters: [:],
body: body,
contentType: "application/json")
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nitpick, this feels to me like the user of the API code has to care too much about the format of the request (like having to format the authorization header manually, and specify the content type string).

I assume we'll be needing to make similar calls in a number of other places, so it would be nice to have it handled more succinctly from the caller's perspective. What do you think?

Copy link
Contributor Author

@brindy brindy Mar 31, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Agree the authorization header could be hidden away (other headers are already added elsewhere), but everything else is the responsibility of the api call to set, so laying it out like this shows the intent. If I were to change anything anything it might be to do something like this:

api.createAuthenticatedRequest(...)

Maybe body could be more explicitly jsonBody then there'd be no need to set the content type either, I suppose.

I think we should see what we end up doing for the actual data sync and then refactor then. I don't really want to define something now that might get changed later tbh.

@brindy brindy requested a review from a team as a code owner March 31, 2023 10:42
@Bunn Bunn removed the request for review from a team March 31, 2023 13:56
ayoy
ayoy approved these changes Mar 31, 2023
View reviewed changes
Copy link
Contributor

@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! Love to see polling mechanism in BSK as originally planned 👏

@brindy brindy merged commit 0f0e209 into main Mar 31, 2023
@brindy brindy deleted the brindy/sync-connect branch March 31, 2023 19:35
brindy added a commit to duckduckgo/macos-browser that referenced this pull request Mar 31, 2023
@brindy
sync connect (#1062)
f2dfe00 
Task/Issue URL: https://app.asana.com/0/0/1204266236802307/f
Tech Design URL:
CC:

**Description**:

Add sync connect flow.

**Steps to test this PR**:
See duckduckgo/BrowserServicesKit#281 for
details.

<!--
Tagging instructions
If this PR isn't ready to be merged for whatever reason it should be
marked with the `DO NOT MERGE` label (particularly if it's a draft)
If it's pending Product Review/PFR, please add the `Pending Product
Review` label.

If at any point it isn't actively being worked on/ready for
review/otherwise moving forward (besides the above PR/PFR exception)
strongly consider closing it (or not opening it in the first place). If
you decide not to close it, make sure it's labelled to make it clear the
PRs state and comment with more information.
-->

---
###### Internal references:
[Pull Request Review
Checklist](https://app.asana.com/0/1202500774821704/1203764234894239/f)
[Software Engineering
Expectations](https://app.asana.com/0/59792373528535/199064865822552)
[Technical Design
Template](https://app.asana.com/0/59792373528535/184709971311943)
**When ready for review, remember to post the PR in MM**
brindy added a commit to duckduckgo/iOS that referenced this pull request Mar 31, 2023
@brindy
add sync connect (#1604)
5130bcc 
Task/Issue URL: https://app.asana.com/0/0/1204266236802307/f
Tech Design URL:
CC:

Description:

Adds Sync "connect" flow.

Steps to test this PR:

See duckduckgo/BrowserServicesKit#281 for details

Copy Testing:

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

 Portrait
 Landscape
Device Testing:

 iPhone SE (1st Gen)
 iPhone 8
 iPhone X
 iPhone 14 Pro
 iPad
OS Testing:

 iOS 14
 iOS 15
 iOS 16
Theme Testing:

 Light theme
 Dark theme
samsymons added a commit that referenced this pull request Apr 4, 2023
@samsymons
Merge branch 'main' into sam/persistent-history-tracking-support
e9ccc4e 
* main:
  sync conect (#281)
  Fix message registration on script reloading (#302)
  Remove codeowner (#301)
  Feature flagging with autofill subfeatures (#270)
  Update codeowner (#297)
  Autofill change to sorting algorithm for displaying logins (#243)
  Update autofill to 6.4.3 (#279)
  move AppHTTPSUpgradeStore to BSK (#267)
  Update Package.swift syntax to Swift 5.7 (#280)
  Use JSON sync code format (#271)
  Update autofill to 6.4.1 (#273)
  Make FeatureName a struct so it can be extended from client code (#276)
  add count of all bookmarks in domain to view model (#272)
samsymons added a commit that referenced this pull request Apr 4, 2023
@samsymons
Merge branch 'main' into sam/swiftlint-plugin
d3af815 
# By Alexey Martemyanov (2) and others
# Via Alexey Martemyanov (1) and GitHub (1)
* main:
  sync conect (#281)
  Fix message registration on script reloading (#302)
  Remove codeowner (#301)
  Feature flagging with autofill subfeatures (#270)
  Update codeowner (#297)
  Autofill change to sorting algorithm for displaying logins (#243)
  Update autofill to 6.4.3 (#279)
  move AppHTTPSUpgradeStore to BSK (#267)
  Update Package.swift syntax to Swift 5.7 (#280)
  Use JSON sync code format (#271)
  Update autofill to 6.4.1 (#273)

# Conflicts:
#	Package.resolved
#	Package.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsymons samsymons samsymons left review comments

@ayoy ayoy ayoy approved these changes

@bwaresiak bwaresiak Awaiting requested review from bwaresiak

@mallexxx mallexxx Awaiting requested review from mallexxx

@THISISDINOSAUR THISISDINOSAUR Awaiting requested review from THISISDINOSAUR

@tomasstrba tomasstrba Awaiting requested review from tomasstrba

Assignees

@brindy brindy

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brindy @ayoy @samsymons