Skip to content

Conversation

@gr8bit
Copy link
Contributor

@gr8bit gr8bit commented Jan 28, 2018

Consists of two parts to support in-app payment calls:

  1. renamed config option appsecret to app_secret and added app_idconfig option to be able to supply app_access_token() method (with deprecation warnings)
  2. added payment, dispute and refunds calls

@gr8bit
Copy link
Contributor Author

gr8bit commented Jan 28, 2018

Also added: functions to encode/decode Facebook's "signed requests". Refactored signature generation a little.

lib/facebook.ex Outdated
* https://developers.facebook.com/docs/graph-api/reference/payment/dispute
"""
@spec dispute(object_id, access_token, dispute_reason) :: resp
def dispute(payment_id, access_token, reason) do
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer that having the name payment_dispute to make it clear when reading code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point

lib/facebook.ex Outdated
* https://developers.facebook.com/docs/graph-api/reference/payment/refunds
"""
# credo:disable-for-lines:1 Credo.Check.Readability.MaxLineLength
@spec refunds(object_id, access_token, currency, amount, refunds_reason) :: resp
Copy link
Owner

Choose a reason for hiding this comment

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

same here, payment_refunds

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do

lib/facebook.ex Outdated

@doc """
Decodes a signed request from a client SDK (in-app payments), verifies the
signature and -if valid- returns its contents.
Copy link
Owner

Choose a reason for hiding this comment

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

why the dashes around if valid? IIRC dashes don't do any special kind of formatting, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These should have been brackets, I changed the sentence a bit to make it more understandable.

{:ok, signature} <- Base.url_decode64(signature_str),
_signature_verification = ^signature <- signature(payload_str),
{:ok, payload} <- Base.url_decode64(payload_str),
{:ok, payload} <- JSON.decode(payload)
Copy link
Owner

Choose a reason for hiding this comment

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

IMO this should be doable in a simpler way.
What's the purpose of the guard in the function? If signed_request is not a binary and you call that function then there won't be another function executed instead.
Also why the with with another guard (if you split a binary it should still be a binary)?

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'm quite new to Elixir, maybe it really can be done better: I used the inner guards to ensure there won't be nil values. Which, you're right, is complete nonsense in the case of split() as it doesn't return nil values. I'll remove them.
However, guard'ing the function itself prevents it from being called with a wrong value and raise an exception instead of trying to call split() with a wrong argument. What do you think about that? How could I do it better?

Copy link
Owner

@mweibel mweibel left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. In general these seem to be sensible additions and changes 👍

@mweibel
Copy link
Owner

mweibel commented Feb 6, 2018

:/ didn't pay attention when squash merging those commits manually. They're in master now but without your author info. Sucks, sorry!

@mweibel mweibel closed this Feb 6, 2018
@mweibel
Copy link
Owner

mweibel commented Feb 6, 2018

Published version 0.18.0 with both of your changes.

@gr8bit gr8bit deleted the payments branch February 10, 2018 22:06
@gr8bit
Copy link
Contributor Author

gr8bit commented Feb 10, 2018

@mweibel no worries, I'm glad I can switch to the official version again :) I work with facebook.ex on a daily basis at the moment, so there may be more PRs. I hope you don't mind ;)

@gr8bit gr8bit restored the payments branch February 10, 2018 22:31
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