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 initial networking layer #5

Closed
wants to merge 14 commits into from
Closed

Conversation

momo-ozawa
Copy link
Contributor

Description

Resolves wordpress-mobile/WordPress-iOS#16863

This PR adds the initial networking layer:

  • Added InAppPurchasesService to call the fetch product skus / create order endpoints
  • Added initial InAppPurchasesServiceTests
  • Added InAppPurchasesAPIRouter to organize the endpoints
  • Added Alamofire as a package dependency

Notes

  • I will add more tests to InAppPurchasesService in a later PR

How to test

  1. Go to Edit Scheme > Run > Options > StoreKit Configuration
  2. Choose Configuration.storekit
  3. Run the iOS app
  4. Tap on Rocket Fuel to purchase
  5. Tap Confirm
  6. Tap OK
  7. On Xcode debugger menu, click on the Manage StoreKit Transactions menu item (See example below)

Screen Shot 2021-07-19 at 16 29 19

  1. ✅ There should be a record of Rocket Fuel being purchased (See example below)

Screen Shot 2021-07-19 at 16 32 32

@leandroalonso
Copy link
Member

leandroalonso commented Jul 29, 2021

@momo-ozawa These tests are failing:

testPaymentQueueUpdatedTransactions_WhenTransactionStateIsPurchased_CallsFinishTransaction
testPaymentQueueUpdatedTransactions_WhenTransactionStateIsRestored_CallsFinishTransaction

Copy link

@emilylaguna emilylaguna left a comment

Choose a reason for hiding this comment

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

Hiya! Overall this works really well! I left a couple comments below.

In addition the tests that are failing, I found the macOS example doesn't build because navigationBarItems is for iOS only.

Also, I'm unsure we need to use Alamofire since the requests themselves are pretty simple, and we should be able to use URLSession's instead. Unless there was a specific reason you chose to use AF?


iapService.createOrder(
identifier: product.productIdentifier,
price: Int(truncating: product.price),

Choose a reason for hiding this comment

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

The value of the price should be in cents, if the price is 5.99 then it should be converted to: 599.

In theory this should work unless there's a better way:

Int(price.floatValue * 100)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. cd1fb1f

Thank you for updating the API docs as well! 🙇‍♀️


guard let appStoreReceiptURL = Bundle.main.appStoreReceiptURL,
FileManager.default.fileExists(atPath: appStoreReceiptURL.path) else {
print("Could not find app store receipt")

Choose a reason for hiding this comment

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

Should we handle the returns as errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! I'm thinking of creating a separate PR for error handling.

I think this PR might get big if I changed the CompletionCallbacks from (T) -> Void to something like (Result<T, Error>) -> Void.

Choose a reason for hiding this comment

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

Makes sense to me!

"object": {
"pins": [
{
"package": "Alamofire",
Copy link
Member

Choose a reason for hiding this comment

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

@momo-ozawa there's any special reason why we need Alamofire?

I think it would be good to keep this library without dependencies and as much light as possible. Nowadays URLSession is very powerful so I think we can go with it.

This can be refactored on a future PR though.

@leandroalonso
Copy link
Member

@momo-ozawa as we talked, let's ignore the failing tests for now and then we can get back to those later.

For the purchased / restored tests to pass, we need to build a receipt stub and a mock iapService.
@momo-ozawa
Copy link
Contributor Author

@leandroalonso @emilylaguna
I added a separate task for the macOS build on the project issue.

I'll be removing Alamofire in favor of URLSession in my next PR 🙇‍♀️

Copy link

@emilylaguna emilylaguna left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@momo-ozawa
Copy link
Contributor Author

Closing this PR due to merge conflicts -- I will create a new PR!

@momo-ozawa momo-ozawa deleted the task/add-networking branch August 27, 2021 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mobile IAP Package: Create Networking stack
3 participants