fix: count query of sold tickets #6693

codedsun · 2019-12-23T20:04:40Z

Short description of what this resolves:

Count query of sold tickets. Solution: Calculate only placed orders + initializing orders which were created under order expiry time

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.

app/api/attendees.py

codecov · 2019-12-23T20:07:32Z

Codecov Report

Merging #6693 into development will increase coverage by 0.06%.
The diff coverage is 97.22%.

@@              Coverage Diff               @@
##           development   #6693      +/-   ##
==============================================
+ Coverage        65.44%   65.5%   +0.06%     
==============================================
  Files              300     300              
  Lines            15297   15324      +27     
==============================================
+ Hits             10011   10038      +27     
  Misses            5286    5286

Impacted Files	Coverage Δ
app/models/order.py	`90% <100%> (ø)`	⬆️
app/factories/attendee.py	`100% <100%> (ø)`	⬆️
tests/all/integration/api/helpers/test_order.py	`98.41% <100%> (+0.79%)`	⬆️
app/api/attendees.py	`40% <85.71%> (+2.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 f27e08d...bee05bf. Read the comment docs.

app/api/attendees.py

iamareebjamal · 2019-12-23T20:17:03Z

app/api/attendees.py

+        order_expiry_time = get_settings()['order_expiry_time']
+        if get_count(db.session.query(TicketHolder.id)
+                    .filter(TicketHolder.ticket_id == int(data['ticket']),
+                            TicketHolder.state == 'placed', TicketHolder.deleted_at.is_(None),


placed or completed

@iamareebjamal This is done wrong as TicketHolder has a state which represents its address and not a placed state.It represents a state of address. I have to change this to order and then check for order status

codedsun · 2019-12-23T20:17:37Z

@iamareebjamal What command should I run locally to get the hound violations here on my local?

iamareebjamal · 2019-12-23T20:17:53Z

Please add tests as well

iamareebjamal · 2019-12-23T20:18:43Z

What command should I run locally to get the hound violations here only?

Not sure

codedsun · 2019-12-25T00:23:23Z

I am away from keyboard for today, will update the PR as soon as I get back in the evening

kushthedude · 2019-12-25T04:43:23Z

app/api/attendees.py

+                    .filter(TicketHolder.ticket_id == int(data['ticket']),
+                            TicketHolder.state == 'placed', TicketHolder.deleted_at.is_(None),
+                            or_(TicketHolder.state ==' initializing', TicketHolder.created_at <
+                            datetime.datetime.utcnow()+datetime.timedelta(minutes=order_expiry_time))))\


Why check the order expiry time condition? If the order is not created under it, Order is labelled expired. It is not required IMO.
Simply counting the Placed & Completed orders in the before the condition is enough.

We need to count the currently reserved tickets. Or else more tickets than max tickets will be bought. See the discussion in the issue

kushthedude · 2019-12-25T04:46:31Z

@iamareebjamal Why not to count the placed orders and tickets in here

open-event-server/app/api/order_statistics/events.py

Line 37 in f27e08d

    
           total = db.session.query(func.sum(OrderTicket.quantity.label('sum'))).join(Order.order_tickets).filter(

. here we can event correct the sales statistics all over the web app. As of now we are not showing the sales data and ticket count of Offline Tickets in the dashboard?

iamareebjamal · 2019-12-25T11:08:21Z

We are showing it, but separately. And yes, this is where it will be placed finally. This PR is work in progress

codedsun · 2019-12-26T19:57:18Z

@iamareebjamal For the test, shall I write functions with the multiple status of orders? or seperate.

Also while creating attendee I can't change the order status, so shall I run the only create the order or shall I create the attendee.?

iamareebjamal · 2019-12-27T12:48:57Z

For the test, shall I write functions with the multiple status of orders? or seperate.

What?

Also while creating attendee I can't change the order status, so shall I run the only create the order or shall I create the attendee.?

You need to create both

codedsun · 2019-12-27T12:51:22Z

@iamareebjamal - While creating both, 2 orders and 1 attendee is created as in attendee factory an order factory reference is also their.

What?

For the query to test, I am creating a single function with multiple attendees and I am running this query to check that count is valid or not.

codedsun · 2019-12-27T13:00:44Z

@iamareebjamal - Guide me

iamareebjamal · 2019-12-27T13:04:52Z

While creating both, 2 orders and 1 attendee is created as in attendee factory an order factory reference is also their.

Split it and use base

https://github.com/fossasia/open-event-server/blob/development/app/factories/session.py

iamareebjamal · 2019-12-27T13:10:39Z

For the query to test, I am creating a single function with multiple attendees and I am running this query to check that count is valid or not.

Create 2 attendees with no order ID, 6 with completed order, 1 with placed, 4 with initializing/initialized and not expired (created_at within order expiry time), 3 with initializing but expired created_at, 5 order status == expired

The count should be 11

codedsun · 2019-12-27T13:16:53Z

While creating both, 2 orders and 1 attendee is created as in attendee factory an order factory reference is also their.

Split it and use base

https://github.com/fossasia/open-event-server/blob/development/app/factories/session.py

Shall this be done in another PR? as it requires changes everywhere it's used

iamareebjamal · 2019-12-27T13:32:10Z

It requires changes in just one file. attendee factory

codedsun · 2019-12-27T13:37:27Z

Ok got that

tests/all/integration/api/helpers/test_order.py

app/factories/attendee.py

app/api/attendees.py

codedsun · 2019-12-27T16:18:27Z

@iamareebjamal Review.

app/api/attendees.py

tests/all/integration/api/helpers/test_order.py

iamareebjamal · 2019-12-28T09:57:40Z

app/api/attendees.py

+                    Order.status == 'completed',
+                    and_(Order.status == 'initializing',
+                         Order.created_at + datetime.timedelta(
+                             minutes=30 + order_expiry_time) > datetime.datetime.utcnow()))).count()


No, initializing = order expiry time
initialized = 30 minutes + order expiry time

We have a initialized state too🙄

codedsun · 2019-12-28T09:57:43Z

@iamareebjamal What i see here is something else.

open-event-server/app/api/schema/orders.py

Lines 72 to 75 in f27e08d

    
           status = fields.Str( 
        
               validate=validate.OneOf( 
        
                   choices=["initializing", "pending", "cancelled", "completed", "placed", "expired"] 
        
               ))

iamareebjamal · 2019-12-28T09:58:30Z

That's wrong. There is no pending state in production DB

kushthedude · 2019-12-28T10:04:45Z

That's wrong. There is no pending state in production DB

Then what is the status getting assigned to pending orders or orders with online payment due?

tests/all/integration/api/helpers/test_order.py

codedsun · 2019-12-28T10:09:49Z

@iamareebjamal Check, I have created also 2 attendees with the same initialized state in the test and checked for them also. The test is passing 👍

iamareebjamal · 2019-12-28T10:31:59Z

Then what is the status getting assigned to pending orders or orders with online payment due

Initialized AFAIK

iamareebjamal · 2019-12-28T10:32:48Z

Waiting for verification of a few things. Will merge then

codedsun · 2019-12-28T10:34:00Z

@iamareebjamal Sure, thanks a lot. It was fun solving this. I am taking other issues now.

kushthedude · 2019-12-28T10:39:29Z

They are assigned pending,Also the reason for pending not be present in db is because it either gets converted into completed after successful payment or to expired after failing to pay within 30 min of creation. I dont know what is the purpose of initialized status, as FE is not labeling any order with initialized.

…

On Sat, 28 Dec, 2019, 16:04 Suneet Srivastava, ***@***.***> wrote: @iamareebjamal <https://github.com/iamareebjamal> Sure, thanks a lot. It was fun solving this. I am taking other issues now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6693?email_source=notifications&email_token=AKQMTLWFSJDQSGRVGBJ3ZSTQ24TRTA5CNFSM4J6XTT3KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHYG6OI#issuecomment-569405241>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AKQMTLUQK6XLZXCN7AOYO3TQ24TRTANCNFSM4J6XTT3A> .

iamareebjamal · 2019-12-28T10:47:52Z

Alright, please change initialized to pending @codedsun

I just confirmed.

Don't know how but there are 257 orders in initialized state in production

tests/all/integration/api/helpers/test_order.py

codedsun · 2019-12-28T10:56:18Z

@iamareebjamal Check now!

iamareebjamal · 2020-01-04T11:44:02Z

app/models/order.py

                 transaction_id=None,
                 paid_via=None,
                 is_billing_enabled=False,
+                 created_at=datetime.datetime.now(datetime.timezone.utc),


This is the problem. created_at is static - set to the starting time of the server #6703

@kushthedude @codedsun

Making the PR

auto-label bot added the fix label Dec 23, 2019

houndci-bot reviewed Dec 23, 2019

View reviewed changes

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

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

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

codedsun force-pushed the sold-query branch from 26b5d4e to 765e216 Compare December 23, 2019 20:07

houndci-bot reviewed Dec 23, 2019

View reviewed changes

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

codedsun force-pushed the sold-query branch from 765e216 to a09f1a2 Compare December 23, 2019 20:14

houndci-bot reviewed Dec 23, 2019

View reviewed changes

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

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

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

codedsun force-pushed the sold-query branch from a09f1a2 to d7654e2 Compare December 23, 2019 20:16

iamareebjamal reviewed Dec 23, 2019

View reviewed changes

kushthedude reviewed Dec 25, 2019

View reviewed changes

codedsun force-pushed the sold-query branch from d7654e2 to cdcb0ac Compare December 27, 2019 16:01

houndci-bot reviewed Dec 27, 2019

View reviewed changes

codedsun force-pushed the sold-query branch from cdcb0ac to f36ae02 Compare December 27, 2019 16:22

houndci-bot reviewed Dec 27, 2019

View reviewed changes

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

codedsun force-pushed the sold-query branch from f662ee2 to 7485c12 Compare December 28, 2019 09:53

houndci-bot reviewed Dec 28, 2019

View reviewed changes

tests/all/integration/api/helpers/test_order.py Outdated Show resolved Hide resolved

codedsun force-pushed the sold-query branch from 7485c12 to 89346a2 Compare December 28, 2019 09:54

iamareebjamal reviewed Dec 28, 2019

View reviewed changes

codedsun force-pushed the sold-query branch from 89346a2 to eb3e56b Compare December 28, 2019 10:05

houndci-bot reviewed Dec 28, 2019

View reviewed changes

tests/all/integration/api/helpers/test_order.py Outdated Show resolved Hide resolved

tests/all/integration/api/helpers/test_order.py Outdated Show resolved Hide resolved

tests/all/integration/api/helpers/test_order.py Outdated Show resolved Hide resolved

codedsun force-pushed the sold-query branch 2 times, most recently from cc4d153 to f2587d9 Compare December 28, 2019 10:08

iamareebjamal previously approved these changes Dec 28, 2019

View reviewed changes

codedsun dismissed iamareebjamal’s stale review via 7569715 December 28, 2019 10:50

codedsun force-pushed the sold-query branch from f2587d9 to 7569715 Compare December 28, 2019 10:50

houndci-bot reviewed Dec 28, 2019

View reviewed changes

tests/all/integration/api/helpers/test_order.py Outdated Show resolved Hide resolved

fix:count query of sold tickets

bee05bf

codedsun force-pushed the sold-query branch from 7569715 to bee05bf Compare December 28, 2019 10:51

iamareebjamal approved these changes Dec 28, 2019

View reviewed changes

iamareebjamal changed the title ~~fix:count query of sold tickets~~ fix: count query of sold tickets Dec 28, 2019

iamareebjamal merged commit 7e94006 into fossasia:development Dec 28, 2019

iamareebjamal reviewed Jan 4, 2020

View reviewed changes

kushthedude mentioned this pull request Jan 4, 2020

Urgent: Stripe is not working #6716

Closed

iamareebjamal mentioned this pull request Mar 2, 2020

fix: Ticket sold count query #6871

Merged

fix: count query of sold tickets #6693

fix: count query of sold tickets #6693

Uh oh!

Conversation

codedsun commented Dec 23, 2019

Short description of what this resolves:

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Dec 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

iamareebjamal Dec 23, 2019

Choose a reason for hiding this comment

Uh oh!

codedsun Dec 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codedsun commented Dec 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iamareebjamal commented Dec 23, 2019

Uh oh!

iamareebjamal commented Dec 23, 2019

Uh oh!

codedsun commented Dec 25, 2019

Uh oh!

kushthedude Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

iamareebjamal Dec 25, 2019

Choose a reason for hiding this comment

Uh oh!

kushthedude commented Dec 25, 2019

Uh oh!

iamareebjamal commented Dec 25, 2019

Uh oh!

codedsun commented Dec 26, 2019

Uh oh!

iamareebjamal commented Dec 27, 2019

Uh oh!

codedsun commented Dec 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codedsun commented Dec 27, 2019

Uh oh!

iamareebjamal commented Dec 27, 2019

Uh oh!

iamareebjamal commented Dec 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codedsun commented Dec 27, 2019

Uh oh!

iamareebjamal commented Dec 27, 2019

Uh oh!

codedsun commented Dec 27, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codedsun commented Dec 27, 2019

Uh oh!

Uh oh!

Uh oh!

iamareebjamal Dec 28, 2019

Choose a reason for hiding this comment

Uh oh!

kushthedude Dec 28, 2019

codecov bot commented Dec 23, 2019 •

edited

Loading

codedsun Dec 26, 2019 •

edited

Loading

codedsun commented Dec 23, 2019 •

edited

Loading

codedsun commented Dec 27, 2019 •

edited

Loading

iamareebjamal commented Dec 27, 2019 •

edited

Loading

iamareebjamal commented Dec 28, 2019 •

edited

Loading