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

fix: orders with amount>0 cannot be completed without payment #6270

Merged
merged 1 commit into from Aug 5, 2019

Conversation

shreyanshdwivedi
Copy link
Member

Fixes #6258

Short description of what this resolves:

When the Stripe/Paypal is not linked but it is selected as a payment option, the completed tickets are generated without payment

Changes proposed in this pull request:

  • Adds checks to raise an error if payment_mode == free for order with amount>0
  • Verifies if the Stripe or Paypal payment is actually made if it's opted as payment_mode

Checklist

  • 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.

@auto-label auto-label bot added the fix label Jul 30, 2019
@codecov
Copy link

codecov bot commented Jul 30, 2019

Codecov Report

Merging #6270 into development will decrease coverage by 0.04%.
The diff coverage is 9.09%.

Impacted file tree graph

@@              Coverage Diff               @@
##           development   #6270      +/-   ##
==============================================
- Coverage        65.54%   65.5%   -0.05%     
==============================================
  Files              286     286              
  Lines            14589   14600      +11     
==============================================
+ Hits              9562    9563       +1     
- Misses            5027    5037      +10
Impacted Files Coverage Δ
app/api/orders.py 28.52% <9.09%> (-0.73%) ⬇️

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 9fc0c20...c53f8b5. Read the comment docs.

app/api/orders.py Outdated Show resolved Hide resolved
app/api/orders.py Outdated Show resolved Hide resolved
app/api/orders.py Outdated Show resolved Hide resolved
@shreyanshdwivedi
Copy link
Member Author

I will like to point out few things -

  • The root cause of the error was that it was sending free order status for orders with amount>0 too and it was unchecked.
    So, I included this check -
    if data.get('payment_mode') == 'free' and data.get('amount') > 0:
            raise ConflictException({'pointer': '/data/attributes/payment-mode'},
                                    "payment-mode cannot be free for order with amount > 0")
  • All the props used in function is_payment_valid() for checking stripe and paypal payment are dump only. They are not even called when a normal payment is carried out as the function charge_stripe_order_payment and charge_paypal_order_payment saves the order object there itself so no PATCH request.
    However now, if a client sets payment_mode to stripe/paypal and status to completed, these props will be checked from order object (not data as they are dump_only).
    @iamareebjamal please have a look. Updated the PR with these things in consideration

@shreyanshdwivedi
Copy link
Member Author

@uds5501 @kushthedude @mrsaicharan1 it will be great if you guys can test out this PR

@iamareebjamal
Copy link
Member

It should be unprocessable entity, not conflict

@shreyanshdwivedi
Copy link
Member Author

It should be unprocessable entity, not conflict

Made the change

@fossasia fossasia deleted a comment Aug 1, 2019
@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal please review

@iamareebjamal iamareebjamal merged commit 5a0f9e3 into fossasia:development Aug 5, 2019
mrsaicharan1 pushed a commit to mrsaicharan1/open-event-server that referenced this pull request Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Paid tickets can be received without actually paying
7 participants