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

fix: ignore casing when checking bt-expose-raw-proxy-response header #74

Merged
merged 6 commits into from
Aug 9, 2024

Conversation

lcschy
Copy link
Contributor

@lcschy lcschy commented Aug 9, 2024

Description

  • Title +
  • Update simulator and xcode versions
  • Fix issue w/ checking for invalid URL would not throw
  • Update tests to match new versions

Testing required outside of automated testing?

  • Not Applicable

Screenshots (if appropriate):

  • Not Applicable

Rollback / Rollforward Procedure

  • Roll Forward
  • Roll Back

Reviewer Checklist

  • Description of Change
  • Description of outside testing if applicable.
  • Description of Roll Forward / Backward Procedure
  • Documentation updated for Change

@@ -73,12 +73,16 @@ private func encodeParamsToFormUrlEncoded(_ formParams: [String: Any]) -> String

struct HttpClientHelpers {
static func executeRequest(method: HttpMethod, url: String, payload: [String: Any]?, config: Config?, completion: @escaping ((_ request: URLResponse?, _ data: JSON?, _ error: Error?) -> Void)) -> Void {
guard let url = URL(string: url) else {
guard var components = URLComponents(string: url), components.scheme != nil else {
Copy link
Contributor Author

@lcschy lcschy Aug 9, 2024

Choose a reason for hiding this comment

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

I guess this changed w/ OS version? Super weird lol.
Somehow to it #$@$%23!@%23%5E*&*(()-=0__+badurl?!?! is a valid URL, had to make it more complex.

let shouldExposeRawProxyResponse = (response as? HTTPURLResponse)?
.allHeaderFields
.contains { (key, _) in
(key as? String)?.lowercased() == "bt-expose-raw-proxy-response"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lower casing header key.

// doubleTap doesn't work because it has a certain delay to display the menu
phoneNumberTextField.tap()
phoneNumberTextField.tap()
phoneNumberTextField.press(forDuration: 1)
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 seemed more reliable than the double tap on new simulator.

@@ -448,7 +448,7 @@ final class ProxyServiceTests: XCTestCase {
do {
let token = try result.get().body.data!.value as! [String: Any]

XCTAssertEqual(token["textFieldRef"] as! String, "https://echo.basistheory.com/get")
XCTAssertEqual(token["textFieldRef"] as! String, "http://echo.basistheory.com/get")
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 was a change directly on the echo server, so had to change here to match.

@lcschy lcschy changed the title Eng 7122 new fix: ignore casing when checking bt-expose-raw-proxy-response header Aug 9, 2024
@lcschy lcschy marked this pull request as ready for review August 9, 2024 18:02
@lcschy lcschy requested a review from a team as a code owner August 9, 2024 18:02
@@ -27,5 +27,5 @@ xcodebuild clean test \
-project ./IntegrationTester/IntegrationTester.xcodeproj \
-scheme IntegrationTester \
-configuration Debug \
-destination platform="iOS Simulator,OS=16.1,name=iPhone 14 Pro" \
-destination platform="iOS Simulator,OS=latest,name=iPhone 15 Pro" \
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine, but shouldn't we be testing against the lowest version we support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went as low as possible w/ the machine/xcode version combo rn.
We can look into what it takes to have 'lesser' iOS versions later on

kevinperaza
kevinperaza previously approved these changes Aug 9, 2024
@lcschy lcschy merged commit 17bd44d into master Aug 9, 2024
3 checks passed
@lcschy lcschy deleted the eng-7122-new branch August 9, 2024 19:26
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.

2 participants