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

Removed Venmo fallbacktoWeb #1434

Open
wants to merge 7 commits into
base: v7
Choose a base branch
from
Open

Removed Venmo fallbacktoWeb #1434

wants to merge 7 commits into from
+15 −29

Conversation

stechiu
Copy link
Collaborator

@stechiu stechiu commented Oct 15, 2024 •
edited
Loading

Removed Venmo fallbacktoWeb opt-in to default to universal link

Summary of changes

  • Removed fallbacktoWeb and replaced with universal link

Checklist

  • Added a changelog entry

Authors

List GitHub usernames for everyone who contributed to this pull request.

@stechiu stechiu requested a review from a team as a code owner October 15, 2024 00:28
jaxdesmarais
jaxdesmarais reviewed Oct 15, 2024
View reviewed changes
Copy link
Contributor

@jaxdesmarais jaxdesmarais left a comment

Choose a reason for hiding this comment

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

We will want to change the base branch to v7. I think we can also clean up the logic in BTVenmoAppSwitchRedirectURL to only use universal links.

Sources/BraintreeVenmo/BTVenmoClient.swift Outdated
@@ -67,7 +67,7 @@ import BraintreeCore
@objc(tokenizeWithVenmoRequest:completion:)
// swiftlint:disable:next function_body_length cyclomatic_complexity
public func tokenize(_ request: BTVenmoRequest, completion: @escaping (BTVenmoAccountNonce?, Error?) -> Void) {
linkType = request.fallbackToWeb ? .universal : .deeplink
linkType = .universal
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just hardcode this on line 47 above vs setting it here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I was overthinking

@scannillo
Copy link
Contributor

Like Jax mentioned, since this change removes something from the SDKs public interface, it will break a merchant app who might be integrating with this property. So this PR should target v7 branch

@stechiu stechiu changed the base branch from main to v7 October 16, 2024 17:10
@stechiu
Copy link
Collaborator Author

stechiu commented Oct 16, 2024

Like Jax mentioned, since this change removes something from the SDKs public interface, it will break a merchant app who might be integrating with this property. So this PR should target v7 branch

I totally forgot to change the target branch. Just fixed!

@stechiu
Copy link
Collaborator Author

stechiu commented Oct 16, 2024

TODO: Fix failing tests

jaxdesmarais
jaxdesmarais reviewed Oct 17, 2024
View reviewed changes
Copy link
Contributor

@jaxdesmarais jaxdesmarais left a comment

Choose a reason for hiding this comment

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

Looks like when we changed the base branch, that branch wasn't up to date with the latest from main. We will need to merge the latest from main into v7 then merge the latest from v7 into your branch to remove that portion of the diff from this PR.

@stechiu
Copy link
Collaborator Author

stechiu commented Oct 17, 2024

Looks like when we changed the base branch, that branch wasn't up to date with the latest from main. We will need to merge the latest from main into v7 then merge the latest from v7 into your branch to remove that portion of the diff from this PR.

Updated. How often do we merge changes from main branch to feature branches?

@jaxdesmarais
Copy link
Contributor

Updated. How often do we merge changes from main branch to feature branches?

I normally try to update long lived feature branches once a week. If a large feature is merged in we may want to update it sooner, but that's generally what I've done previously.

@stechiu stechiu changed the base branch from v7 to main October 17, 2024 20:36
@stechiu stechiu changed the base branch from main to v7 October 17, 2024 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaxdesmarais jaxdesmarais jaxdesmarais left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stechiu @scannillo @jaxdesmarais