Skip to content
This repository has been archived by the owner on Jan 4, 2023. It is now read-only.

Updates to v4 of CardFlight SDK #687

Merged
merged 8 commits into from
Mar 6, 2018
Merged

Updates to v4 of CardFlight SDK #687

merged 8 commits into from
Mar 6, 2018

Conversation

ashfurrow
Copy link
Contributor

@ashfurrow ashfurrow commented Mar 5, 2018

This PR updates Eidolon to use v4 of the CardFlight SDK. It is a work-in-progress because of some outstanding TODO items, as well as manual testing before merging. Paired with @sepans with this.

case .connecting:
return "connecting"
case .batteryStatusUpdated:
return "battery status updated"
Copy link
Contributor

Choose a reason for hiding this comment

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

Great! FWIW these bottom few need a capital at start

reader.beginSwipe()
public func transaction(_ transaction: CFTTransaction, didRequest cvm: CFTCVM) {
logger.log("Transaction requested signature from user, ignoring.")
_cardStatus.onNext("Ignoring user signature request from CardFlight")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can be a bit more nuanced, since we can determine if a signature is actually required through the cvm parameter, described here: https://github.com/CardFlight/cardflight-v4-ios/issues/3

// CardFlight SDK, the user is prompted to accept processing for card tokenization, which is provides a
// unfriendly user experience (prompting to accept a transaction that we're not actually placing). So we
// auto-accept these requests and filter out confirmation messages, which don't apply to tokenization flows,
// until this issue is fixed: https://github.com/CardFlight/cardflight-v4-ios/issues/4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @orta, when you have a moment can you double-check my thinking here? I need to make sure this is not sneaky or unethical, and make sure that the intentions of the code are made clear in the comment for other developers. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this makes sense for me

@ashfurrow
Copy link
Contributor Author

@orta okay this is good for review. Please see my note above requesting feedback.

Also note that c8fdf99 removes the "processing" label and instead we opt to display the messages that CardFlight sends us to display to the user (with "Please wait..." as a placeholder if there is no current message). This fixes a longstanding issue with users swiping before the card reader is ready, too.

Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

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

Yeah, this makes sense to me

// CardFlight SDK, the user is prompted to accept processing for card tokenization, which is provides a
// unfriendly user experience (prompting to accept a transaction that we're not actually placing). So we
// auto-accept these requests and filter out confirmation messages, which don't apply to tokenization flows,
// until this issue is fixed: https://github.com/CardFlight/cardflight-v4-ios/issues/4
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this makes sense for me


if message.hasPrefix("Card Flight Error") {
self.processingLabel.text = "ERROR PROCESSING CARD - SEE ADMIN"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

👋

@orta orta changed the title WIP: Updates to v4 of CardFlight SDK Updates to v4 of CardFlight SDK Mar 6, 2018
@ashfurrow
Copy link
Contributor Author

Okay great, I'll put this on an iPad and work with Nicole to test before a deploy. Thanks!

@ashfurrow ashfurrow merged commit 24c721a into master Mar 6, 2018
@ashfurrow ashfurrow deleted the cardflight-update branch March 6, 2018 21:07
@ashfurrow
Copy link
Contributor Author

When i tested this, it didn't work. I tried but couldn't get the v4 upgrade to work, so I'll revert this PR. I've opened an issue to request help: https://github.com/CardFlight/cardflight-v4-ios/issues/5

@ashfurrow
Copy link
Contributor Author

I've ticketed the work for CardFlight compatibility, it's trickier than it looks at first due to Xcode 10 removing libstdc++.6.0.9:
https://artsyproduct.atlassian.net/browse/PURCHASE-615

ashfurrow added a commit that referenced this pull request Oct 29, 2018
This reverts commit 24c721a, reversing
changes made to e98923f.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants