-
Notifications
You must be signed in to change notification settings - Fork 25
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
Remove Reference to AccessToken in Integration Guides #163
Conversation
README.md
Outdated
@@ -86,10 +47,10 @@ To accept a certain payment method in your app, you only need to include that pa | |||
## Sample Code | |||
|
|||
```swift | |||
// STEP 0: Fetch an ACCESS_TOKEN and ORDER_ID from your server. | |||
// STEP 0: Enter a CLIENT_ID and fetch ORDER_ID from your server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// STEP 0: Enter a CLIENT_ID and fetch ORDER_ID from your server. | |
// STEP 0: Fetch a CLIENT_ID and ORDER_ID from your server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a sample cURL or link to a cURL for how merchants can perform a clientID fetch on their server?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, I don't think we have one for the function Steven wrote on for the merchant server. I can write it in as a comment until we have it in the integration guide that we can link. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A typical merchant won't need to fetch a client ID. It will most likely be injected at compile time using an environment (or hardcoded) variable for most, unless they need to dynamically set their client ID.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll eventually support multiple merchants in the Demo app e.g. to test connected partner flows, so we need to set it dynamically for PPCP e2e testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On Android, we only mention that it's needed to construct a CoreConfig
object. It is technically up to the merchant, and clientID
doesn't need to be protected like the clientSecret
does. Maybe we can do something similar here and eliminate step 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this snippet in the README at all? We have this integration covered in our docs/
section per module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be able to remove:
let paymentConfig = CoreConfig(clientID: "<CLIENT_ID>", environment: .sandbox)
is probably all that's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean this entire section under ## Sample Code
in the README (link). IMO it can be removed. We don't have an equivalent in Android and the sample code for each flow already lives in docs/
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 agreed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌊
Summary of changes
Checklist
- [ ] Added a changelog entryAuthors