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.

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

fix: removes discrepancies in cancelling order #6186

Merged
merged 1 commit into from Jul 30, 2019
Merged

fix: removes discrepancies in cancelling order #6186

merged 1 commit into from Jul 30, 2019

Conversation

shreyanshdwivedi
Copy link
Member

Fixes #6141

Short description of what this resolves:

There are discrepancies while cancelling order.

Changes proposed in this pull request:

  • updates send_notif_ticket_cancel in api/helpers/notification.py
  • updates the logic to raise error on field update 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 12, 2019
@fossasia fossasia deleted a comment Jul 12, 2019
@codecov
Copy link

codecov bot commented Jul 12, 2019 •
edited

Codecov Report

Merging #6186 into development will decrease coverage by 0.06%.
The diff coverage is 3.57%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6186      +/-   ##
===============================================
- Coverage        65.84%   65.78%   -0.07%     
===============================================
  Files              286      286              
  Lines            14501    14516      +15     
===============================================
+ Hits              9548     9549       +1     
- Misses            4953     4967      +14
Impacted Files Coverage Δ
app/api/helpers/notification.py 21.73% <ø> (ø) ⬆️
app/api/orders.py 29.25% <3.57%> (-1.22%) ⬇️

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 adf81d6...7d18f66. Read the comment docs.

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal @mrsaicharan1 @uds5501 please review

poush
poush reviewed Jul 15, 2019
View reviewed changes
app/api/orders.py Outdated Show resolved Hide resolved
poush
poush reviewed Jul 15, 2019
View reviewed changes
app/api/orders.py Outdated Show resolved Hide resolved
poush
poush reviewed Jul 15, 2019
View reviewed changes
app/api/orders.py Outdated Show resolved Hide resolved
poush
poush reviewed Jul 15, 2019
View reviewed changes
Copy link
Member

@poush poush left a comment

Choose a reason for hiding this comment

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

Hi, please check my comments. There are more instances of that thing.

houndci-bot
houndci-bot reviewed Jul 17, 2019
View reviewed changes
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
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
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
app/api/orders.py Outdated Show resolved Hide resolved
@fossasia fossasia deleted a comment Jul 17, 2019
@shreyanshdwivedi
Copy link
Member Author

@poush I've updated the PR. Please review

@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal @poush please review

@iamareebjamal
Copy link
Member

Add tests

@shreyanshdwivedi
Copy link
Member Author

Add tests

@iamareebjamal I couldn't find any relevant docs to help me out. Can you please tell me how to add tests for POST or PATCH request? It will help me to add tests for other parts of the project as well

@iamareebjamal
Copy link
Member

https://flask.palletsprojects.com/en/1.1.x/testing/

houndci-bot
houndci-bot reviewed Jul 25, 2019
View reviewed changes
tests/all/integration/api/test_order.py Outdated Show resolved Hide resolved
tests/all/integration/api/test_order.py Outdated Show resolved Hide resolved
tests/all/integration/api/test_order.py Outdated Show resolved Hide resolved
tests/all/integration/api/test_order.py Outdated Show resolved Hide resolved
tests/all/integration/api/test_order.py Outdated Show resolved Hide resolved
tests/all/integration/api/test_order.py Outdated Show resolved Hide resolved
@fossasia fossasia deleted a comment Jul 25, 2019
@fossasia fossasia deleted a comment Jul 25, 2019
@fossasia fossasia deleted a comment Jul 25, 2019
@fossasia fossasia deleted a comment Jul 25, 2019
@fossasia fossasia deleted a comment Jul 25, 2019
@fossasia fossasia deleted a comment Jul 25, 2019
@fossasia fossasia deleted a comment Jul 25, 2019
@auto-label auto-label bot removed the fix label Jul 25, 2019
@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal I'm unable to add tests and I'm getting confused in adding tests for such API changes. Can you please help me out or should I drop testing it and we can move forward by testing it manually?
@mrsaicharan1 @uds5501 if you've any understanding of adding tests for API changes, can you help me out by adding tests for this?

@iamareebjamal
Copy link
Member

Let's drop tests but thoroughly test for each transition

@shreyanshdwivedi
Copy link
Member Author

shreyanshdwivedi commented Jul 28, 2019 •
edited

Ok @iamareebjamal
I'll test it again and let you know. Till then placing a DO-NOT-MERGE tag

@shreyanshdwivedi shreyanshdwivedi changed the title [WIP] fix: removes discrepancies in cancelling order fix: removes discrepancies in cancelling order Jul 28, 2019
@auto-label auto-label bot added the fix label Jul 28, 2019
@fossasia fossasia deleted a comment Jul 28, 2019
@shreyanshdwivedi
Copy link
Member Author

@iamareebjamal I've tested the following -

  • Ordered ticket, both placed and pending
  • Cancelled ticket
  • Deleted ticket.

Everything worked fine. @uds5501 @kushthedude @mrsaicharan1 your input will help

@fossasia fossasia deleted a comment Jul 29, 2019
iamareebjamal
iamareebjamal reviewed Jul 29, 2019
View reviewed changes
app/api/orders.py Outdated Show resolved Hide resolved
houndci-bot
houndci-bot reviewed Jul 29, 2019
View reviewed changes
app/api/orders.py Outdated Show resolved Hide resolved
@fossasia fossasia deleted a comment Jul 29, 2019
iamareebjamal
iamareebjamal reviewed Jul 29, 2019
View reviewed changes
app/api/orders.py Outdated Show resolved Hide resolved
iamareebjamal
iamareebjamal reviewed Jul 29, 2019
View reviewed changes
app/api/orders.py Outdated Show resolved Hide resolved
@shreyanshdwivedi
fix: removes discrepancies in cancelling order
7d18f66 
add test

fix hound

fix test

remove tests
@shreyanshdwivedi
Copy link
Member Author

@uds5501 @mrsaicharan1 please review

@fossasia fossasia deleted a comment Jul 30, 2019
@iamareebjamal iamareebjamal merged commit 30cc028 into fossasia:development Jul 30, 2019
mrsaicharan1 pushed a commit to mrsaicharan1/open-event-server that referenced this pull request Aug 9, 2019
@shreyanshdwivedi @mrsaicharan1
fix: removes discrepancies in cancelling order (fossasia#6186)
78fbee4
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@poush poush poush left review comments

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

@iamareebjamal iamareebjamal iamareebjamal approved these changes

@uds5501 uds5501 uds5501 approved these changes

@kushthedude kushthedude kushthedude approved these changes

Assignees
No one assigned
Labels
fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Discrepancies in cancelling order
6 participants
@shreyanshdwivedi @iamareebjamal @poush @houndci-bot @uds5501 @kushthedude