Skip to content
This repository has been archived by the owner on Apr 14, 2023. It is now read-only.

Fix ambiguous behavior in generate_token method #209

Merged
merged 2 commits into from
Oct 27, 2020
Merged

Fix ambiguous behavior in generate_token method #209

merged 2 commits into from
Oct 27, 2020

Conversation

skukx
Copy link
Contributor

@skukx skukx commented Jan 29, 2019

The SolidusPaypalBraintree::Gateway#generate_token method is ambiguous
on success and failure. Both results will return a string. To be more
explicit, this method should raise an error which can be handled by the
app.

Closes #208

kennyadsl
kennyadsl reviewed Feb 2, 2019
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Left same comments of the ones left in #212 (just noticed this one and got the git mess there 👍 )

app/models/solidus_paypal_braintree/gateway.rb Outdated Show resolved Hide resolved
app/controllers/solidus_paypal_braintree/client_tokens_controller.rb Outdated Show resolved Hide resolved
@kennyadsl
Copy link
Member

@skukx thanks! Last thing: I'd also squash everything in a single commit, what do you think?

@skukx
Copy link
Contributor Author

skukx commented Feb 4, 2019

@kennyadsl yeah, I'll do that now

@skukx skukx force-pushed the 208-ambiguous-behavior-for-generate-token-method branch from 92d164b to 40b99a9 Compare February 4, 2019 17:29
ericsaupe
ericsaupe approved these changes Feb 4, 2019
View reviewed changes
Copy link

@ericsaupe ericsaupe left a comment

Choose a reason for hiding this comment

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

Thanks!

@skukx
Copy link
Contributor Author

skukx commented Feb 4, 2019

@ericsaupe I'm curious if force pushing affects travis CI. Getting those random failures again

@kennyadsl kennyadsl changed the title Fix ambiguous behavior in generate_tokenn method Fix ambiguous behavior in generate_token method Feb 11, 2019
@skukx skukx force-pushed the 208-ambiguous-behavior-for-generate-token-method branch from 40b99a9 to 687b697 Compare February 12, 2019 16:53
tvdeyen
tvdeyen suggested changes May 16, 2019
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Thanks, this is helpful, but I would rather not use exceptions as control flow. Could we rewrite this without exceptions?

app/controllers/solidus_paypal_braintree/client_tokens_controller.rb Outdated Show resolved Hide resolved
@tvdeyen
Copy link
Member

tvdeyen commented May 16, 2019

@ericsaupe I'm curious if force pushing affects travis CI. Getting those random failures again

Yes, force pushes trigger a new build on Travis. The random failures are because of the nature of the "not so ideal written" integrations tests.

@seand7565
Copy link
Contributor

@skukx Hey there, from the future! I would like to merge this in soon. Do you have a moment to give this a rebase? Thanks!

@skukx
Copy link
Contributor Author

skukx commented Oct 26, 2020

@skukx Hey there, from the future! I would like to merge this in soon. Do you have a moment to give this a rebase? Thanks!

@seand7565 Yes, I'll do that asap

@skukx
Fix ambiguous behavior in generate_tokenn method
460e0c6 
The `SolidusPaypalBraintree::Gateway#generate_token` method is ambiguous
on success and failure. Both results will return a string. To be more
explicit, this method should raise an error which can be handled by the
app.
@skukx skukx force-pushed the 208-ambiguous-behavior-for-generate-token-method branch from 9168828 to 8d1d921 Compare October 26, 2020 18:52
@seand7565
Copy link
Contributor

@skukx Hey there, from the future! I would like to merge this in soon. Do you have a moment to give this a rebase? Thanks!

@seand7565 Yes, I'll do that asap

Fantastic, thanks! Looks like there's just two small linting issues and this should be good to go.

@skukx skukx force-pushed the 208-ambiguous-behavior-for-generate-token-method branch from 8d1d921 to f28ef52 Compare October 26, 2020 19:47
@kennyadsl
Copy link
Member

This is another breaking change, people relying on that specific response even for failing cases. We will definitely need to release a new version with a clear changelog for this issue.

tvdeyen
tvdeyen approved these changes Oct 27, 2020
View reviewed changes
Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

Code wise 👌

I agree with Alberto that this is a breaking change, but in a good sense. The former behavior was just wrong.

@kennyadsl kennyadsl merged commit ee96c84 into solidusio:master Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ericsaupe ericsaupe ericsaupe approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

@tvdeyen tvdeyen tvdeyen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ambiguous behavior for generate token method
5 participants
@skukx @kennyadsl @tvdeyen @seand7565 @ericsaupe