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

Create encodable for credit cards post #1488

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

Conversation

warmkesselj
Copy link
Contributor

@warmkesselj warmkesselj commented Dec 16, 2024

Summary of changes

Move /credit_cards POST to Encodable.

Checklist

  • Added a changelog entry
  • Tested and confirmed payment flows affected by this change are functioning as expected

Authors

@warmkesselj
@jwarmkessel

Sources/BraintreeCard/BTCard.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/BTCard.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/BTCard.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/CreditCardGraphQLBody.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/CreditCardPOSTBody.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/CreditCardPOSTBody.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/CreditCardPOSTBody.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/CreditCardPOSTBody.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/CreditCardGraphQLBody.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/CreditCardGraphQLBody.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/CreditCardGraphQLBody.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/CreditCardGraphQLBody.swift Outdated Show resolved Hide resolved
Sources/BraintreeCard/CreditCardPOSTBody.swift Outdated Show resolved Hide resolved

import Foundation

struct CreditCardGraphQLBody: Encodable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this swiftlint disable here instead:

Suggested change
struct CreditCardGraphQLBody: Encodable {
// swiftlint:disable nesting
struct CreditCardGraphQLBody: Encodable {

@@ -0,0 +1,139 @@
// swiftlint:disable nesting
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 remove this since we have it below as well


import Foundation

struct CreditCardGraphQLBody: Encodable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a docstring for this? Can use /// The POST body for <endpoint_here> like we have in other files.

#endif

// swiftlint:disable nesting
struct CreditCardPOSTBody: Encodable {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add a docstring for this? Can use /// The POST body for <endpoint_here> like we have in other files.

}
}

/// POST Body Model
Copy link
Contributor

Choose a reason for hiding this comment

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

Take it or leave it but not sure this comment is helpful. Can likely remove once we add the one to the top of this struct.

] as [String: Any] as NSObject)
let params = card.graphQLParameters()

XCTAssertEqual(params.variables.input.options.validate, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
XCTAssertEqual(params.variables.input.options.validate, false)
XCTAssertEqual(params.variables.input.options.validate, false)

XCTAssertEqual(params.variables.input.creditCard.number, "6111111111111111")
XCTAssertEqual(params.operationName, "TokenizeCreditCard")
XCTAssertNotNil(params.query)
XCTAssertEqual(params.variables.input.options.validate, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
XCTAssertEqual(params.variables.input.options.validate, false)
XCTAssertEqual(params.variables.input.options.validate, false)

]
] as [String: Any] as NSObject)
let params = card.graphQLParameters()
printEncodableObject(params)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? It doesn't look like we are using the output.

XCTAssertEqual(params.variables.input.creditCard.number, "8111111111111111")
XCTAssertEqual(params.operationName, "TokenizeCreditCard")
XCTAssertNotNil(params.query)
XCTAssertEqual(params.variables.input.options.validate, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
XCTAssertEqual(params.variables.input.options.validate, false)
XCTAssertEqual(params.variables.input.options.validate, false)

@@ -55,6 +56,7 @@ public class MockAPIClient: BTAPIClient {
public override func post(_ path: String, parameters: Encodable, headers: [String: String]? = nil, httpType: BTAPIClientHTTPService = .gateway, completion completionBlock: ((BTJSON?, HTTPURLResponse?, Error?) -> Void)? = nil) {
lastPOSTPath = path
lastPOSTParameters = try? parameters.toDictionary()
lastPOSTEncodableParameters = parameters
Copy link
Contributor

Choose a reason for hiding this comment

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

General question. Do we need this? I think the other encodable PRs were using lastPOSTParameters which converts the Encodable class to a dictionary so we wouldn't need to change tests. Just trying to make sure I understand why we needed to make large changes to the tests here but not in the other PRs.


struct CreditCardGraphQLBody: Encodable {

// MARK: - Internal Properties
Copy link
Contributor

Choose a reason for hiding this comment

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

imo we can remove this

var number: String?
var expirationMonth: String?
var cvv: String?
var options: Options?
Copy link
Contributor

Choose a reason for hiding this comment

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

This line is causing your integration test failures. CreditCard.options does not exist in the API. This should be removed and it's correlating struct below. Right now you have options in 2 places and it actually only exists in Input at the API layer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants