-
Notifications
You must be signed in to change notification settings - Fork 997
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
[17.0.0, breaking] Update API version from 2015-10-12 to 2019-05-16 #1254
Conversation
@@ -8,31 +8,54 @@ | |||
|
|||
#import "STPConnectAccountParams.h" | |||
|
|||
#import "STPLegalEntityParams.h" | |||
|
|||
NS_ASSUME_NONNULL_BEGIN | |||
|
|||
@implementation STPConnectAccountParams | |||
|
|||
@synthesize additionalAPIParameters; | |||
|
|||
- (instancetype)initWithTosShownAndAccepted:(BOOL)wasAccepted |
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.
since we're changing these signatures anyway, any reason to even have the wasAccepted
argument?
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 think we need it to differentiate between if the customer accepted the ToS or if the user didn't show a ToS (ie YES vs nil).
@@ -1,5 +1,8 @@ | |||
## Migration Guides | |||
|
|||
### Migrating from versions < 17.0.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.
I'm kind of inclined not to have another major version bump so soon...Maybe 16.1.0
? This should only be a breaking change for Connect users right? Thoughts @yuki-stripe @davidme-stripe
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'm not familiar with how versioning has worked here in the past, but moving from the 2015 API sounds like an appropriate reason to increment the major version number.
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.
Definitely wary of fatiguing our users...there are the (minor) enum changes that Source users should update with, too. Outside of that, it should be unnoticed.
Either way, I'd like to ship this only after some confidence that 16.* won't have more bugfixes.
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.
👍 Let's hold this patch until 16.0.* has settled and then go from there
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.
looks good after clarifcation on empty test file
If a user uploaded an ID document photo from their camera roll or file browser which could not be decoded by the server, the user would have no way to select a new photo to upload because the "Select" button would not display on the screen. This fix resets the `DocumentUploader` when the ViewController's `reset` method is called after the user encounters an error state, enabling the "Select" buttons so they can upload a new photo.
Summary
Relevant changes since then:
(Full changelog: https://paper.dropbox.com/doc/Update-iOS-API-version-to-2019-05-16--Ahqa8fWMvo2AGLYlL1kISzoTAg-hFrNrJFf1f4dL3ZlT62YK)
Motivation
Testing
Re-ran all the functional tests
STPPinManagementFunctionalTest
, which is in beta (so presumably has the same behavior for all API versions). I couldn't re-record it anyways, I think it needs to be reworked to include manual steps like {Payment, Setup}IntentFunctionalTest.