Skip to content

feat: enable admin to delete any order #6172

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
Closed
wants to merge 1 commit into from
Closed

feat: enable admin to delete any order #6172

wants to merge 1 commit into from

Conversation

@uds5501
Copy link
Contributor

@uds5501 uds5501 commented Jul 11, 2019 •
edited
Loading

Fixes #6173

Short description of what this resolves:

Allow admin to soft delete any order

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 feature label Jul 11, 2019
@iamareebjamal
Copy link
Member

iamareebjamal commented Jul 11, 2019

And what happens on deleting order? Refund?

@codecov
Copy link

codecov bot commented Jul 11, 2019 •
edited
Loading

Codecov Report

Merging #6172 into development will increase coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@               Coverage Diff               @@
##           development    #6172      +/-   ##
===============================================
+ Coverage        65.26%   65.27%   +0.01%     
===============================================
  Files              287      287              
  Lines            14713    14710       -3     
===============================================
  Hits              9602     9602              
+ Misses            5111     5108       -3
Impacted Files Coverage Δ
app/api/orders.py 28.7% <0%> (+0.27%) ⬆️

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 e390b56...cd5ae28. Read the comment docs.

@uds5501
Copy link
Contributor Author

uds5501 commented Jul 11, 2019

@iamareebjamal No, the project has no refund ability yet.
This PR is just to ensure that admin can delete an order in any case, just the way a co-organizer can.

@iamareebjamal
Copy link
Member

iamareebjamal commented Jul 11, 2019

But I see no changes related to admin

@uds5501
Copy link
Contributor Author

uds5501 commented Jul 11, 2019

@iamareebjamal sorry for that, forgot to change admin permissions.
Apart from that, i've added the changes.

@iamareebjamal iamareebjamal mentioned this pull request Jul 11, 2019
Merged
abhinavk96
abhinavk96 suggested changes Jul 11, 2019
View reviewed changes
Copy link
Contributor

@abhinavk96 abhinavk96 left a comment

Choose a reason for hiding this comment

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

Shouldn't you be modifying the before_delete_hook if you really want to delete paid/placed orders?

@uds5501
Copy link
Contributor Author

uds5501 commented Jul 12, 2019

@iamareebjamal

  • Since refund feature has not yet been implemented, should the admin be allowed to delete completed and placed orders?

  • @CosmicCoder96 I tried deleting few orders and observing their behaviour, they go through before_update hook. I am trying to ensure soft deletion so that the orders can be restored. (just like deleting and restoring events)

shreyanshdwivedi
shreyanshdwivedi reviewed Jul 15, 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.

Please fix merge conflicts @uds5501
Also, I think that as it is soft deletion, the field is getting updated so adding check in before_update_object is fine. The data is passed through this function too.

iamareebjamal
iamareebjamal reviewed Jul 16, 2019
View reviewed changes
app/api/orders.py
@@ -1,2 +1,2 @@
import logging
from datetime import datetime
Copy link
Member

@iamareebjamal iamareebjamal Jul 16, 2019

Choose a reason for hiding this comment

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

It was better before IMO

Copy link
Contributor Author

@uds5501 uds5501 Jul 16, 2019

Choose a reason for hiding this comment

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

@iamareebjamal arlgiht, changing it.

shreyanshdwivedi
shreyanshdwivedi reviewed Aug 6, 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.

Please resolve conflicts and squash your commits

iamareebjamal
iamareebjamal previously approved these changes Aug 7, 2019
View reviewed changes
@fossasia fossasia deleted a comment Aug 7, 2019
@uds5501 uds5501 force-pushed the admin-delete-action branch from 3c41266 to cd5ae28 Compare August 7, 2019 05:11
@uds5501
Copy link
Contributor Author

uds5501 commented Aug 7, 2019

@iamareebjamal @shreyanshdwivedi squashed the commits and resolved the conflicts, please review now.

@fossasia fossasia deleted a comment Aug 7, 2019
mrsaicharan1
mrsaicharan1 suggested changes Aug 9, 2019
View reviewed changes
app/api/orders.py
else:
check_event_user_ticket_holders(order, data, element)
if data[element] and data[element] != getattr(order, element, None):
if element not in ['status', 'deleted_at']:
Copy link
Member

@mrsaicharan1 mrsaicharan1 Aug 9, 2019

Choose a reason for hiding this comment

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

let's push this to the previous line instead of additional nesting.

@kushthedude
Copy link
Member

kushthedude commented Aug 16, 2019

@uds5501 Are we good closing it ?

@uds5501
Copy link
Contributor Author

uds5501 commented Aug 16, 2019

@kushthedude Yes. Not a priority right now.

@uds5501 uds5501 closed this Aug 16, 2019
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

@shreyanshdwivedi shreyanshdwivedi Awaiting requested review from shreyanshdwivedi

2 more reviewers

@abhinavk96 abhinavk96 abhinavk96 requested changes

@mrsaicharan1 mrsaicharan1 mrsaicharan1 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

feature

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Allow admin to soft delete orders

6 participants

@uds5501 @iamareebjamal @kushthedude @abhinavk96 @mrsaicharan1 @shreyanshdwivedi