Skip to content

fix: check for paid orders to show billing info #6310

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 8, 2019
Merged

fix: check for paid orders to show billing info #6310

merged 1 commit into from
Aug 8, 2019

Conversation

@uds5501
Copy link
Contributor

@uds5501 uds5501 commented Aug 5, 2019 •
edited
Loading

Fixes #6309, fossasia/open-event-frontend#3367

Changes proposed in this pull request:

  • Edit validation check
  • Set is_billing_enabled to true for paid orders

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.

houndci-bot
houndci-bot reviewed Aug 5, 2019
View reviewed changes
@auto-label auto-label bot added the fix label Aug 5, 2019
@uds5501 uds5501 force-pushed the no-billing-showing branch from 5c199ac to 52d46c2 Compare August 5, 2019 20:05
@codecov
Copy link

codecov bot commented Aug 5, 2019 •
edited
Loading

Codecov Report

Merging #6310 into development will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6310      +/-   ##
===============================================
- Coverage        65.25%   65.24%   -0.01%     
===============================================
  Files              287      287              
  Lines            14727    14728       +1     
===============================================
  Hits              9610     9610              
- Misses            5117     5118       +1
Impacted Files Coverage Δ
app/api/orders.py 28.34% <0%> (-0.1%) ⬇️

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 a7ad48c...f2a2894. Read the comment docs.

@uds5501
Copy link
Contributor Author

uds5501 commented Aug 6, 2019

@mrsaicharan1 @shreyanshdwivedi @iamareebjamal Please review

kushthedude
kushthedude previously approved these changes Aug 6, 2019
View reviewed changes
Copy link
Member

@kushthedude kushthedude left a comment

Choose a reason for hiding this comment

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

Good to go

shreyanshdwivedi
shreyanshdwivedi reviewed Aug 6, 2019
View reviewed changes
app/templates/pdf/order_invoice.html Outdated
{% endif %}<br>
</td>
{% if order.is_billing_enabled %}
{% if has_billing_info %}
Copy link
Member

@shreyanshdwivedi shreyanshdwivedi Aug 6, 2019

Choose a reason for hiding this comment

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

Just a question, why do we need a check? If order.billing_info is serving the purpose

Copy link
Contributor Author

@uds5501 uds5501 Aug 6, 2019

Choose a reason for hiding this comment

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

New paid orders aren't using is_billing_enabled anymore, that's why .

Copy link
Member

@shreyanshdwivedi shreyanshdwivedi Aug 6, 2019

Choose a reason for hiding this comment

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

@uds5501 can't we proceed to make is_billing_enabled true is order isn't free either on server or on fron so that we can maintain the consistency of data? I think this will be a better approach because we can anytime get if a order has billing info or not just by a single field.

Copy link
Contributor Author

@uds5501 uds5501 Aug 8, 2019

Choose a reason for hiding this comment

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

@shreyanshdwivedi understood, fixing this

@uds5501 uds5501 force-pushed the no-billing-showing branch from 52d46c2 to f2a2894 Compare August 8, 2019 13:39
shreyanshdwivedi
shreyanshdwivedi approved these changes Aug 8, 2019
View reviewed changes
Copy link
Member

@shreyanshdwivedi shreyanshdwivedi left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Let's move forward with this PR now

@iamareebjamal iamareebjamal merged commit 3addebe into fossasia:development Aug 8, 2019
@uds5501 uds5501 mentioned this pull request Aug 8, 2019
Closed
mrsaicharan1 pushed a commit to mrsaicharan1/open-event-server that referenced this pull request Aug 9, 2019
@uds5501 @mrsaicharan1
fix: check for paid orders to show billing info (fossasia#6310)
1ee6c53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iamareebjamal iamareebjamal Awaiting requested review from iamareebjamal

4 more reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@mrsaicharan1 mrsaicharan1 mrsaicharan1 approved these changes

@shreyanshdwivedi shreyanshdwivedi shreyanshdwivedi approved these changes

@kushthedude kushthedude kushthedude left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fix hotfix

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Billing Information not showing up in placed orders PDF

6 participants

@uds5501 @houndci-bot @mrsaicharan1 @shreyanshdwivedi @kushthedude @iamareebjamal