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

allow updates to existing cards #133

Merged
merged 1 commit into from
Jan 21, 2020

Conversation

sherrardb
Copy link
Collaborator

i left this stalled a while ago, but i figured i'd dust it off for inclusion in the next release. discussed preciously in #100 .

@sherrardb sherrardb mentioned this pull request Dec 24, 2019
@sherrardb
Copy link
Collaborator Author

i have updated this branch based on issues outlined in #100

Copy link
Collaborator

@andrewsolomon andrewsolomon left a comment

Choose a reason for hiding this comment

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

I think it would be helpful to add comments explaining the logic of post_card for the next maintainer. The reason I say this is because I look at all the if conditionals and I think:

  • What happens if $card is not a ref, but not a token either (also - it would be good to explain what the /^tok_... means
  • What happens if $card is a Token but doesn't have an id
  • What happens if it's a ref, but not a Net::Stripe::Token and not a HASH? I suspect you're going to say "then it falls through to line 593" but it would be nice to make it clear that all possibilities are covered.

@sherrardb
Copy link
Collaborator Author

I think it would be helpful to add comments explaining the logic of post_card for the next maintainer. The reason I say this is because I look at all the if conditionals and I think:

  • What happens if $card is not a ref, but not a token either (also - it would be good to explain what the /^tok_... means
  • What happens if $card is a Token but doesn't have an id
  • What happens if it's a ref, but not a Net::Stripe::Token and not a HASH? I suspect you're going to say "then it falls through to line 593" but it would be nice to make it clear that all possibilities are covered.

welllll... several times i was tempted to massage the structure of that sub to clarify and tighten, and i have been trying to resist the urge. but since you asked, i'll have at it :-)

@sherrardb
Copy link
Collaborator Author

@andrewsolomon i would appreciate your thoughts on

  • the structure of the chained conditionals
  • the lack of a terminal else/die, even though technically it shouldn't be necessary
  • the structure and wording of the error messages, which are mostly mock-ups for reference
    TIA

@andrewsolomon
Copy link
Collaborator

@andrewsolomon i would appreciate your thoughts on

  • the structure of the chained conditionals
  • the lack of a terminal else/die, even though technically it shouldn't be necessary
  • the structure and wording of the error messages, which are mostly mock-ups for reference
    TIA

I agree that there's lots of scope for making the code more robust. As long as you're confident that your testing covers any changes you make you should feel free to make the code better :-)

@sherrardb
Copy link
Collaborator Author

sherrardb commented Jan 16, 2020

upon review (of my own code :-)) i see that i managed to both remove the code path that handles Net::Stripe::Card objects (remove appropriately, per #100) and re-add it later. my only guess as to why is that i also added a test case for Net::Stripe::Card objects, and therefore ended up trying to "fix" a brokenness that should not have been fixed.

@sherrardb sherrardb force-pushed the f-cannot-update-existing-card branch 2 times, most recently from 5e86d7e to 9e2afff Compare January 21, 2020 17:41
 * add update_card() method to allow updates to card address, expiration, metadata, etc for existing customer cards
 * update convert_to_form_fields() to handle customer card metadata
 * add unit tests to confirm card metdata update
 * correct errant 'rw' on metadata attribute in `Net::Stripe::Card`
@sherrardb sherrardb force-pushed the f-cannot-update-existing-card branch from 9e2afff to d84ba41 Compare January 21, 2020 17:43
@sherrardb sherrardb merged commit 9f5c06b into lukec:master Jan 21, 2020
@sherrardb sherrardb deleted the f-cannot-update-existing-card branch January 21, 2020 17:49
sherrardb added a commit that referenced this pull request Jan 21, 2020
@sherrardb sherrardb restored the f-cannot-update-existing-card branch January 21, 2020 17:56
sherrardb added a commit to sherrardb/stripe-perl that referenced this pull request Jan 22, 2020
 * remove invalid argument types from post_card(), per <lukec#138>
 * remove dead code paths from post_card() and make conditional structure more explicit, per discussion in <lukec#133>
 * add unit tests for all calling forms, per <lukec#139>
 * update the POD
@sherrardb sherrardb mentioned this pull request Jan 22, 2020
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