Skip to content

Conversation

@shreyanshdwivedi
Copy link
Member

Fixes #6137

Short description of what this resolves:

Make billing information mandatory for paid orders

Changes proposed in this pull request:

  • Adds checks in before_create_object and before_update_object in api/orders.py

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 14, 2019
@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #6192 into development will decrease coverage by 0.02%.
The diff coverage is 14.28%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6192      +/-   ##
===============================================
- Coverage        65.29%   65.27%   -0.03%     
===============================================
  Files              287      287              
  Lines            14704    14711       +7     
===============================================
+ Hits              9601     9602       +1     
- Misses            5103     5109       +6
Impacted Files Coverage Δ
app/api/orders.py 28.43% <14.28%> (-0.33%) ⬇️

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 5949dc6...2a98f9b. Read the comment docs.

@codecov
Copy link

codecov bot commented Jul 15, 2019

Codecov Report

Merging #6192 into development will decrease coverage by 0.03%.
The diff coverage is 0%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6192      +/-   ##
===============================================
- Coverage           66%   65.96%   -0.04%     
===============================================
  Files              288      288              
  Lines            14521    14529       +8     
===============================================
  Hits              9584     9584              
- Misses            4937     4945       +8
Impacted Files Coverage Δ
app/api/orders.py 29.61% <0%> (-0.85%) ⬇️

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 faf208e...4ba2903. Read the comment docs.

@iamareebjamal
Copy link
Member

I think you made a method which checks the payment mode, use that. If there's no method, create one to avoid repetition

@shreyanshdwivedi
Copy link
Member Author

Yeah that function was to check if any of the payment option is enabled in event, both online and offline form of payments.
But here I need to check if the order is paid via our platform i.e. stripe, omise, alipay or paypal. When order is crrated, I don't think that we can check it via a method. However while updating, we might check via a method.

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal should I add a new function just to check if payment options are from stripe, omise, alipay or paypal?

@iamareebjamal
Copy link
Member

I see repeated code. There should be one function called from 2 places instead of repetition

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal made a function check_billing_info to remove the code duplication. Please have a look

@iamareebjamal
Copy link
Member

Have a peer review

@shreyanshdwivedi
Copy link
Member Author

@mrsaicharan1 @uds5501 @kushthedude please review

@kushthedude
Copy link
Member

@shreyanshdwivedi There is one hound violation.

@shreyanshdwivedi shreyanshdwivedi force-pushed the billingInfo branch 3 times, most recently from b0a8678 to b0ba2bc Compare July 25, 2019 20:44
@shreyanshdwivedi
Copy link
Member Author

@uds5501 @mrsaicharan1 @kushthedude please review

uds5501
uds5501 previously approved these changes Jul 26, 2019
mrsaicharan1
mrsaicharan1 previously approved these changes Jul 26, 2019
Copy link
Member

@mrsaicharan1 mrsaicharan1 left a comment

Choose a reason for hiding this comment

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

LGTM

kushthedude
kushthedude previously approved these changes Jul 26, 2019
prateekj117
prateekj117 previously approved these changes Jul 26, 2019
Copy link
Member

@prateekj117 prateekj117 left a comment

Choose a reason for hiding this comment

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

LGTM

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal please review. Updated the exception

@shreyanshdwivedi
Copy link
Member Author

@CosmicCoder96 I have a query. The issue for which I opened this PR doesn't state whether the billing Info is required for only orders paid via our platform or even for offline paid event. Do you want it for both online as well as offline paid events?

iamareebjamal
iamareebjamal previously approved these changes Jul 27, 2019
@shreyanshdwivedi
Copy link
Member Author

Keep it on hold till @CosmicCoder96 replies to #6192 (comment)

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal I've just updated the check to include all type of paid orders - data.get('amount') > 0 as we are following this on FE too.

@iamareebjamal iamareebjamal merged commit 7f74bd1 into fossasia:development Aug 7, 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.

Make billing information mandatory for paid orders

7 participants