fix: prevent deletion of ticket that has sales #7392

maze-runnar · 2020-10-30T14:55:21Z

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.

codecov · 2020-10-30T15:12:36Z

Codecov Report

Merging #7392 into development will increase coverage by 0.00%.
The diff coverage is 57.14%.

@@             Coverage Diff              @@
##           development    #7392   +/-   ##
============================================
  Coverage        64.74%   64.74%           
============================================
  Files              262      265    +3     
  Lines            13235    13319   +84     
============================================
+ Hits              8569     8624   +55     
- Misses            4666     4695   +29

Impacted Files	Coverage Δ
app/api/tickets.py	`45.28% <33.33%> (-0.88%)`	⬇️
app/models/ticket.py	`78.08% <75.00%> (-0.18%)`	⬇️
app/api/helpers/tasks.py	`16.75% <0.00%> (-0.44%)`	⬇️
app/models/event.py	`78.63% <0.00%> (-0.11%)`	⬇️
app/api/helpers/mail_recorder.py	`70.00% <0.00%> (ø)`
app/api/custom/calendars.py	`50.00% <0.00%> (ø)`
app/api/helpers/calendar/ical.py	`97.29% <0.00%> (ø)`
app/instance.py	`88.04% <0.00%> (+0.26%)`	⬆️
app/api/helpers/permissions.py	`36.20% <0.00%> (+0.30%)`	⬆️
app/api/event_invoices.py	`63.54% <0.00%> (+0.38%)`	⬆️
... and 2 more

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 5459387...ae3cfbf. Read the comment docs.

iamareebjamal · 2020-10-30T22:34:45Z

app/api/tickets.py

@@ -241,6 +241,12 @@ def before_update_object(self, ticket, data, view_kwargs):
                    {'event_id': ticket.event.id},
                    "Event having paid ticket must have a payment method",
                )
+        else:
+            if (data.get('deleted_at') and ticket.has_order_tickets):


It should only check completed, placed, pending and initializing tickets

expired, cancelled orders may prevent deletion this way which will create issues like this fossasia/open-event-frontend#5430

can I change here something to implement above condition.

@property def has_order_tickets(self): """Returns True if ticket has already placed orders. Else False. """ from app.api.helpers.db import get_count orders = Order.id.in_( OrderTicket.query.with_entities(OrderTicket.order_id) .filter_by(ticket_id=self.id) .all() ) count = get_count(Order.query.filter(orders).filter(Order.status != 'deleted')) # Count is zero if no completed orders are present return bool(count > 0)

Create a new function as this will not just check if the ticket has orders. Name it has_current orders and use

Order.query.join(TicketHolder) .filter(TicketHolder.ticket_id == self.id, or_(Order.status == 'completed', Order.status == 'placed', Order.status == 'pending', Order.status == 'initializing'))

Don't fetch all the orders like done above. Neither use get_count. Use exists query like here

open-event-server/app/models/video_stream.py

Lines 41 to 46 in d3582cb

return db.session.query(

TicketHolder.query.filter_by(event_id=event_id, user=user)

.join(Order)

.filter(or_(Order.status == 'completed', Order.status == 'placed'))

.exists()

).scalar()

iamareebjamal · 2020-11-04T10:48:04Z

@maze-runnar Status?

maze-runnar · 2020-11-04T13:30:54Z

@iamareebjamal it is giving this error on deletion of ticket -

 if (data.get('deleted_at') and ticket.has_current_orders):
    raise TypeError("Boolean value of this clause is not defined")

File "/home/saurabh/Desktop/extra/open-event-server/app/api/tickets.py", line 244, in before_update_object
if (data.get('deleted_at') and ticket.has_current_orders):
File "/home/saurabh/Desktop/extra/open-event-server/.venv/lib/python3.7/site-packages/sqlalchemy/sql/elements.py", line 535, in bool
raise TypeError("Boolean value of this clause is not defined")
TypeError: Boolean value of this clause is not defined

iamareebjamal · 2020-11-04T16:05:56Z

app/models/ticket.py

+                or_(Order.status == 'completed',
+                Order.status == 'placed',
+                Order.status == 'pending',
+                Order.status == 'initializing')).exists() 


No, it has something missing from the query I wrote

Where is db.session.query(...).exists()

iamareebjamal · 2020-11-04T16:11:52Z

@maze-runnar That's because you didn't write what I commented. You can't just skip things randomly and expect everything to work

iamareebjamal · 2020-11-05T15:28:58Z

app/api/tickets.py

-                    "Can't delete a ticket that has sales",
-                )
+
+        if (ticket.has_current_orders and data.get('deleted_at')):


deleted_at should be checked first. There is no need to make an SQL query everytime there is a change in tickets. It only needs to be checked if ticket is being deleted

app/api/tickets.py

maze-runnar added 3 commits October 30, 2020 20:18

making has_order_tickets a property

e95c15e

putting condition

5bd01a6

fix

6d9e02e

iamareebjamal changed the title ~~Fix: can't delete ticket that has sales~~ fix: can't delete ticket that has sales Oct 30, 2020

auto-label bot added the fix label Oct 30, 2020

iamareebjamal reviewed Oct 30, 2020

View reviewed changes

Merge branch 'development' into patch-4

1e4a09a

maze-runnar added 4 commits November 4, 2020 17:41

adding has_current order as property

3388888

updating condition..

481d93c

import or_

e5f2901

import ticketholder

380ba37

iamareebjamal reviewed Nov 4, 2020

View reviewed changes

maze-runnar added 2 commits November 5, 2020 17:04

finalizing..

41815b8

Update ticket.py

a4bd0bf

iamareebjamal reviewed Nov 5, 2020

View reviewed changes

Update tickets.py

f509416

iamareebjamal previously approved these changes Nov 5, 2020

View reviewed changes

iamareebjamal reviewed Nov 5, 2020

View reviewed changes

app/api/tickets.py Outdated Show resolved Hide resolved

Update app/api/tickets.py

ae3cfbf

iamareebjamal dismissed their stale review via ae3cfbf November 5, 2020 16:09

iamareebjamal changed the title ~~fix: can't delete ticket that has sales~~ fix: prevent deletion of ticket that has sales Nov 5, 2020

iamareebjamal merged commit 87b58df into fossasia:development Nov 5, 2020

iamareebjamal mentioned this pull request Nov 11, 2020

Test code which was fixed in recent merges #7431

Closed

7 tasks

maze-runnar deleted the patch-4 branch December 27, 2020 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: prevent deletion of ticket that has sales #7392

fix: prevent deletion of ticket that has sales #7392

maze-runnar commented Oct 30, 2020

codecov bot commented Oct 30, 2020 •

edited

Loading

iamareebjamal Oct 30, 2020

iamareebjamal Oct 30, 2020

maze-runnar Oct 31, 2020

iamareebjamal Nov 2, 2020

iamareebjamal commented Nov 4, 2020

maze-runnar commented Nov 4, 2020 •

edited

Loading

iamareebjamal Nov 4, 2020

iamareebjamal Nov 4, 2020

iamareebjamal commented Nov 4, 2020

iamareebjamal Nov 5, 2020

	return db.session.query(
	TicketHolder.query.filter_by(event_id=event_id, user=user)
	.join(Order)
	.filter(or_(Order.status == 'completed', Order.status == 'placed'))
	.exists()
	).scalar()

fix: prevent deletion of ticket that has sales #7392

fix: prevent deletion of ticket that has sales #7392

Conversation

maze-runnar commented Oct 30, 2020

Checklist

codecov bot commented Oct 30, 2020 • edited Loading

Codecov Report

iamareebjamal Oct 30, 2020

Choose a reason for hiding this comment

iamareebjamal Oct 30, 2020

Choose a reason for hiding this comment

maze-runnar Oct 31, 2020

Choose a reason for hiding this comment

iamareebjamal Nov 2, 2020

Choose a reason for hiding this comment

iamareebjamal commented Nov 4, 2020

maze-runnar commented Nov 4, 2020 • edited Loading

iamareebjamal Nov 4, 2020

Choose a reason for hiding this comment

iamareebjamal Nov 4, 2020

Choose a reason for hiding this comment

iamareebjamal commented Nov 4, 2020

iamareebjamal Nov 5, 2020

Choose a reason for hiding this comment

codecov bot commented Oct 30, 2020 •

edited

Loading

maze-runnar commented Nov 4, 2020 •

edited

Loading