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 support for the Person resource #1332

Merged
merged 1 commit into from
Oct 31, 2018
Merged

Add support for the Person resource #1332

merged 1 commit into from
Oct 31, 2018

Conversation

remi-stripe
Copy link
Contributor

@remi-stripe remi-stripe commented Oct 13, 2018

cc @stripe/api-libraries
r? @ob-stripe

public bool SsnLast4Provided { get; set; }

[JsonProperty("verification")]
public LegalEntityVerification Verification { get; set; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we take this as an opportunity to rename some of those classes to not be so specific since they will soon be shared across Person and Account.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd keep the current name for now. A person is also a legal entity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah but the concept of LegalEntity is going away soon that's why I was asking since we will have to rename it

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's cross that bridge when we get to it? This PR doesn't have breaking changes, so I don't see any upside to doing this now rather than later.

@ob-stripe ob-stripe force-pushed the integration-next-major-version branch 2 times, most recently from c6704cd to e9fd90d Compare October 22, 2018 16:02
@ob-stripe ob-stripe changed the base branch from integration-next-major-version to master October 22, 2018 19:06
@remi-stripe remi-stripe force-pushed the remi-add-person branch 2 times, most recently from f1ad803 to 43c30d1 Compare October 25, 2018 21:01
public string Phone { get; set; }

[JsonProperty("relationship")]
public Relationship Relationship { get; set; }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be named PersonRelationship to avoid future conflicts? I'm trying to not over-prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'd rename Relationship to PersonRelationship and Requirements to PersonRequirements to match the OpenAPI names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Requirements will be shared with Accounts so I'd rather not prefix it Person

@remi-stripe
Copy link
Contributor Author

r? @ob-stripe
This should be ready for release (baring the new stripe-mock that we can do tomorrow)

@remi-stripe remi-stripe force-pushed the remi-add-person branch 2 times, most recently from 16bc53c to b0fca33 Compare October 26, 2018 15:33
@remi-stripe remi-stripe changed the title [WIP] Add support for the Person resource Add support for the Person resource Oct 26, 2018
Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

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

LGTM overall, some minor comments.

public string Phone { get; set; }

[JsonProperty("relationship")]
public Relationship Relationship { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'd rename Relationship to PersonRelationship and Requirements to PersonRequirements to match the OpenAPI names.

public class RelationshipListOptions : INestedOptions
{
[JsonProperty("account_opener")]
public bool? AccountOpener { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

This property isn't present in the OpenAPI spec or supported by the API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is, just not in this release but it was overlooked and since I know it will be supported it seemed cleaner to just release now.

@@ -19,7 +19,7 @@ public void Deserialize()

Assert.NotNull(evt.Data);
Assert.NotNull(evt.Data.Object);
Assert.IsType<Customer>(evt.Data.Object);
Assert.IsType<Plan>(evt.Data.Object);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should just remove this assertion and add tests with local fixtures to ensure that resources in events are correctly deserialized.

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

Choose a reason for hiding this comment

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

Thank You👌

public bool SsnLast4Provided { get; set; }

[JsonProperty("verification")]
public LegalEntityVerification Verification { get; set; }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd keep the current name for now. A person is also a legal entity.

@remi-stripe
Copy link
Contributor Author

@ob-stripe PTAL

@remi-stripe remi-stripe merged commit 6782114 into master Oct 31, 2018
@remi-stripe remi-stripe deleted the remi-add-person branch October 31, 2018 18:21
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.

3 participants