feat: Add paypal configuration for Android Paypal Payment #6297

anhanh11001 · 2019-08-03T07:37:14Z

Fixes: #5961

Short description of what this resolves:

Add PayPal configuration to verify Paypal payment for Android Paypal integration

Changes proposed in this pull request:

Add Verifying Paypal Payment for Android
I have read the Contribution & Best practices Guide and my PR follows them.
My branch is up-to-date with the Upstream development branch.
[] The unit tests pass locally with my changes
[] I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

All the functions created/modified in this PR contain relevant docstrings.

codecov · 2019-08-03T08:01:23Z

Codecov Report

Merging #6297 into development will decrease coverage by 0.12%.
The diff coverage is 13.88%.

@@               Coverage Diff               @@
##           development    #6297      +/-   ##
===============================================
- Coverage        65.29%   65.16%   -0.13%     
===============================================
  Files              287      287              
  Lines            14775    14810      +35     
===============================================
+ Hits              9647     9651       +4     
- Misses            5128     5159      +31

Impacted Files	Coverage Δ
app/api/helpers/payment.py	`23.7% <11.11%> (-3.82%)`	⬇️
app/api/orders.py	`27.91% <22.22%> (-0.17%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f2607b0...e703ddb. Read the comment docs.

mrsaicharan1 · 2019-08-03T16:39:43Z

app/api/helpers/payment.py

+        """
+        Make sure same payment does not reused
+        """
+        verified_paypal_payment = "paypal_verified_payments.txt"


Umm, why can't you scan through the model to search for the used payment ID?

I'm kinda new to server-side issue so I might need more advices. What do you mean by "scan through the model"? Besides, codecov/patch and codecov/project are failing, what should I do then? @mrsaicharan1

@anhanh11001 Yeah, so those checks are ok. Leave them. Travis is important. Scanning through the model as in scanning through the DB table. (SQLAlchemy). Read the SQLalchemy docs and read up on how to query the model & others.

In layman terms, would you rather read data from a table in a DB or read the values from a txt file?

D:\Coding Project\11. Fossasia\open-event-server\venv\lib\site-packages\envparse.py:195: UserWarning: Could not any envfile. warnings.warn('Could not any envfile.') `DATABASE_URL` either not exported or empty

I'm getting this when migrating new table, what should I do?

This is a warning, not an error

app/api/helpers/payment.py

anhanh11001 · 2019-08-08T04:08:53Z

@mrsaicharan1 please review

mrsaicharan1

Travis is failing

anhanh11001 · 2019-08-08T10:09:23Z

@mrsaicharan1 updated, travis is not failing now

mrsaicharan1

@anhanh11001 For this purpose, query the order model for the paypal id.

app/models/paypal_payment_id.py

mrsaicharan1 · 2019-08-08T11:48:19Z

Also, remove the new model which was created

anhanh11001 · 2019-08-09T09:36:10Z

@iamareebjamal please review, this is pending for a long time..

iamareebjamal · 2019-08-09T09:42:30Z

app/models/order.py

        self.deleted_at = deleted_at
        self.order_notes = order_notes
        self.tickets_pdf_url = tickets_pdf_url
+        self.paypal_token = paypal_token


Why is this needed?

so that I can create an Order to save paypal payment ID. @mrsaicharan1 commented above that I can find paypal payment ID in Order model to save it, but I couldn't find it Order model so I thought he meant paypal_token.

@mrsaicharan1 Please comment

Orders should never be created with a paypal_token. Consider this as imitating a paid order through paypal. After an order is created, only after proper authorisation from PayPal's API, the attribute should be updated.
All you need to do is verify it right? So please remove this.

This is not needed anywhere AFAIK

mrsaicharan1 · 2019-08-09T12:09:18Z

@anhanh11001 So have a look here. https://github.com/paypal/PayPal-Python-SDK
The workflow is as follows:

Create a payment on the server side
Authorize it
Execute the payment

As you're trying to verify if the payment ID has been used or not, you'll need to hit the paypal API to check if the payment has been CREATED like this. There is paypal token which resides in the order model but it would be better if you would verify it with PayPal REST API itself rather than checking on our order model.
Just think about it in this way, the paypal token might've been used by some other project/application, who knows? So, always verify through the API when it comes to payment IDs/tokens.

payment = paypalrestsdk.Payment.find("PAY-57363176S1057143SKE2HO3A")
if payment is None:
    return 'No payment found'

Post that, according to what you're trying to accomplish, to verify the payment status, just check for the approved status(which has already been done)

iamareebjamal · 2019-08-09T14:57:31Z

@mrsaicharan1 Yes, so where does this payment ID PAY-57363176S1057143SKE2HO3A is stored?

anhanh11001 · 2019-08-09T15:44:11Z

@mrsaicharan1 I was following this example code: https://github.com/paypal/PayPal-Python-SDK/blob/master/samples/mobile_backend/paypal_client.py#L37 maybe you can take a look as well.

In Android, we use Android SDK to make the payment to pay and receive a proof of payment (which contain payment ID). Server side integration for Android will use that proof of payment to verify the order one more time to provide tickets for user.

Besides, I don't quite understand what does it mean by "paypal token". is it also the same with "paypal payment id"

anhanh11001 · 2019-08-09T15:50:52Z

@iamareebjamal maybe can I just first follow : https://github.com/paypal/PayPal-Python-SDK/blob/master/samples/mobile_backend/paypal_client.py#L37 this to store the payment ID to a text file first to get this issue done so that harshit and I can work and do some testing on Android and then come back to this later to save it to a model.

iamareebjamal · 2019-08-09T17:13:09Z

I can't merge an incomplete implementation. You're free to host your server on a temporary heroku instance

anhanh11001 · 2019-08-09T17:20:46Z

I can't merge an incomplete implementation. You're free to host your server on a temporary heroku instance

ok, I'll wait for @mrsaicharan1 feedback

mrsaicharan1 · 2019-08-09T17:24:50Z

Okay, so I wasn't aware what was happening on the android side so just to verify it for one more time, just scan the order model if there's an order associated with that PayPal token or not. That's it.

…

On Fri, 9 Aug 2019 at 10:50 PM, Duc Le Tran ***@***.***> wrote: I can't merge an incomplete implementation. You're free to host your server on a temporary heroku instance ok, I'll wait for @mrsaicharan1 <https://github.com/mrsaicharan1> feedback — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6297?email_source=notifications&email_token=AGAHUW7HZAKYTUQV6ANU7YTQDWRPDA5CNFSM4IJCOSKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD37IULY#issuecomment-519997999>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGAHUW6W3EFY5EA7FQUBZD3QDWRPDANCNFSM4IJCOSKA> .

anhanh11001 · 2019-08-09T17:29:21Z

Okay, so I wasn't aware what was happening on the android side so just to verify it for one more time, just scan the order model if there's an order associated with that PayPal token or not. That's it.
…
On Fri, 9 Aug 2019 at 10:50 PM, Duc Le Tran @.***> wrote: I can't merge an incomplete implementation. You're free to host your server on a temporary heroku instance ok, I'll wait for @mrsaicharan1 https://github.com/mrsaicharan1 feedback — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6297?email_source=notifications&email_token=AGAHUW7HZAKYTUQV6ANU7YTQDWRPDA5CNFSM4IJCOSKKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD37IULY#issuecomment-519997999>, or mute the thread https://github.com/notifications/unsubscribe-auth/AGAHUW6W3EFY5EA7FQUBZD3QDWRPDANCNFSM4IJCOSKA .

then please review this PR @mrsaicharan1

mrsaicharan1 · 2019-08-09T20:32:08Z

@mrsaicharan1 Yes, so where does this payment ID PAY-57363176S1057143SKE2HO3A is stored?

It will be stored in PayPal records & the same ID is being stored in our order model as paypal_token

app/api/helpers/payment.py

app/api/orders.py

mrsaicharan1 · 2019-08-09T20:35:27Z

@anhanh11001 Also, is this working locally? Have you tested it?

anhanh11001 · 2019-08-11T05:29:52Z

@mrsaicharan1 Do you know which file I can use to put my client ID and client secret in for testing ?

mrsaicharan1 · 2019-08-11T05:34:12Z

@mrsaicharan1 Do you know which file I can use to put my client ID and client secret in for testing ?

We never store keys in files but if you just want to test it locally, you can replace the code where you find get_settings()['client_id'] with your key.
you'll find it here -> app/api/helpers/payment.py(configure_paypal method)
Just keep in mind that you don't push this code with the actual keys set in the code

anhanh11001 · 2019-08-14T02:49:27Z

@anhanh11001 Also, is this working locally? Have you tested it?

tested

liveHarshit · 2019-08-14T06:58:43Z

@anhanh11001 Also, is this working locally? Have you tested it?

tested

Okay, so I think it is ready to merge now?

mrsaicharan1 · 2019-08-14T10:40:19Z

app/api/helpers/payment.py

+    @staticmethod
+    def used_payment(payment_id):
+        """
+        Make sure same payment does not reused


function to check for recycling of payment IDs

Orders should never be created with a paypal_token. Consider this as imitating a paid order through PayPal. After an order is created, only after proper authorization from PayPal's API, the attribute should be updated.
All you need to do is verify it right? So please remove this.

So how should I save a used payment ID? You told me to save it in the Order model, please clarify this for me? Thank you

mrsaicharan1 · 2019-08-14T10:46:49Z

app/models/order.py

        self.deleted_at = deleted_at
        self.order_notes = order_notes
        self.tickets_pdf_url = tickets_pdf_url
+        self.paypal_token = paypal_token


Orders should never be created with a paypal_token. Consider this as imitating a paid order through paypal. After an order is created, only after proper authorisation from PayPal's API, the attribute should be updated.
All you need to do is verify it right? So please remove this.

mrsaicharan1 · 2019-08-14T14:28:03Z

You check if the PayPal token is associated with any of the orders

…

On Wed, 14 Aug 2019 at 7:56 PM, Duc Le Tran ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/api/helpers/payment.py <#6297 (comment)> : > + return False, 'Payment amount does not match order' + elif currency_server != order.event.payment_currency: + return False, 'Payment currency does not match order' + if sale_state != 'completed': + return False, 'Sale not completed' + elif PayPalPaymentsManager.used_payment(payment_id): + return False, 'Payment already been verified' + else: + return True, None + except paypalrestsdk.ResourceNotFound: + return False, 'Payment Not Found' + + @staticmethod + def used_payment(payment_id): + """ + Make sure same payment does not reused Orders should never be created with a paypal_token. Consider this as imitating a paid order through PayPal. After an order is created, only after proper authorization from PayPal's API, the attribute should be updated. All you need to do is verify it right? So please remove this. So how should I save a used payment ID? You told me to save it in the Order model, please clarify this for me? Thank you — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6297?email_source=notifications&email_token=AGAHUW3EJTFTR7DVIV5XDDLQEQIZLA5CNFSM4IJCOSKKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCBRQFOQ#discussion_r313904186>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGAHUW5XNXYJBCMS2UKIHSLQEQIZLANCNFSM4IJCOSKA> .

iamareebjamal · 2019-08-14T15:35:09Z

You are not even using the paypal token anywhere. There is no migration. So if you confirmed it to be working, then it can be removed safely without any issue

Fixes: #5961

anhanh11001 · 2019-08-16T01:36:39Z

@mrsaicharan1 @iamareebjamal please review

mrsaicharan1

@anhanh11001 This looks good but I have a question. What order object are you passing into the used_payment function?
Is the order object being created on android-client side with the amount, related event etc and then being passed into it?

iamareebjamal · 2019-08-17T14:29:49Z

@anhanh11001 Please respond

anhanh11001 · 2019-08-18T00:05:20Z

@anhanh11001 This looks good but I have a question. What order object are you passing into the used_payment function?
Is the order object being created on android-client side with the amount, related event etc and then being passed into it?

Yes @mrsaicharan1

iamareebjamal · 2019-08-18T01:34:37Z

Let's merge this so that you guys can test on debug server

auto-label bot added the feature label Aug 3, 2019

mrsaicharan1 suggested changes Aug 3, 2019

View reviewed changes

anhanh11001 mentioned this pull request Aug 4, 2019

feat: Add paypal fossasia/open-event-attendee-android#2223

Merged

houndci-bot reviewed Aug 7, 2019

View reviewed changes

app/api/helpers/payment.py Outdated Show resolved Hide resolved

fossasia deleted a comment Aug 8, 2019

mrsaicharan1 reviewed Aug 8, 2019

View reviewed changes

mrsaicharan1 suggested changes Aug 8, 2019

View reviewed changes

app/models/paypal_payment_id.py Outdated Show resolved Hide resolved

fossasia deleted a comment Aug 8, 2019

iamareebjamal reviewed Aug 9, 2019

View reviewed changes

mrsaicharan1 suggested changes Aug 9, 2019

View reviewed changes

app/api/helpers/payment.py Outdated Show resolved Hide resolved

app/api/orders.py Outdated Show resolved Hide resolved

mrsaicharan1 suggested changes Aug 14, 2019

View reviewed changes

Summary: Add Paypal verification for Android client

47374f7

Fixes: #5961

mrsaicharan1 reviewed Aug 17, 2019

View reviewed changes

iamareebjamal changed the title ~~feat Add paypal configuration for Android Paypal Payment~~ feat: Add paypal configuration for Android Paypal Payment Aug 18, 2019

iamareebjamal approved these changes Aug 18, 2019

View reviewed changes

Merge branch 'development' into 5961_add_paypal_mobile

e703ddb

iamareebjamal merged commit ead0ba0 into fossasia:development Aug 18, 2019

feat: Add paypal configuration for Android Paypal Payment #6297

feat: Add paypal configuration for Android Paypal Payment #6297

Uh oh!

Conversation

anhanh11001 commented Aug 3, 2019

Short description of what this resolves:

Changes proposed in this pull request:

Uh oh!

codecov bot commented Aug 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anhanh11001 commented Aug 8, 2019

Uh oh!

mrsaicharan1 left a comment

Choose a reason for hiding this comment

Uh oh!

anhanh11001 commented Aug 8, 2019

Uh oh!

mrsaicharan1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrsaicharan1 commented Aug 8, 2019

Uh oh!

anhanh11001 commented Aug 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrsaicharan1 commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamareebjamal commented Aug 9, 2019

Uh oh!

anhanh11001 commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anhanh11001 commented Aug 9, 2019

Uh oh!

iamareebjamal commented Aug 9, 2019

Uh oh!

anhanh11001 commented Aug 9, 2019

Uh oh!

mrsaicharan1 commented Aug 9, 2019 via email

Uh oh!

anhanh11001 commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrsaicharan1 commented Aug 9, 2019

Uh oh!

Uh oh!

Uh oh!

mrsaicharan1 commented Aug 9, 2019

Uh oh!

anhanh11001 commented Aug 11, 2019

Uh oh!

mrsaicharan1 commented Aug 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 3, 2019 •

edited

Loading

mrsaicharan1 commented Aug 9, 2019 •

edited

Loading

anhanh11001 commented Aug 9, 2019 •

edited

Loading

anhanh11001 commented Aug 9, 2019 •

edited

Loading

mrsaicharan1 commented Aug 11, 2019 •

edited

Loading

anhanh11001 commented Aug 16, 2019 •

edited

Loading

anhanh11001 commented Aug 18, 2019 •

edited

Loading