feat: Single endpoint for creating creating an order #6635

prateekj117 · 2019-11-30T17:21:44Z

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 · 2019-11-30T17:25:48Z

Codecov Report

Merging #6635 into development will increase coverage by 0.23%.
The diff coverage is 93.38%.

@@               Coverage Diff               @@
##           development    #6635      +/-   ##
===============================================
+ Coverage        65.15%   65.39%   +0.23%     
===============================================
  Files              299      300       +1     
  Lines            15319    15315       -4     
===============================================
+ Hits              9981    10015      +34     
+ Misses            5338     5300      -38

Impacted Files	Coverage Δ
app/models/event.py	`78.7% <ø> (-0.23%)`	⬇️
app/api/routes.py	`100% <ø> (ø)`
app/api/helpers/system_mails.py	`100% <ø> (ø)`	⬆️
app/views/blueprints.py	`58.2% <ø> (ø)`
app/api/schema/events.py	`91.33% <ø> (-0.46%)`	⬇️
app/api/schema/users.py	`100% <ø> (ø)`	⬆️
app/api/tickets.py	`47.16% <0%> (-1.38%)`	⬇️
app/api/helpers/csv_jobs_util.py	`88.88% <100%> (+2.92%)`	⬆️
tests/all/integration/api/helpers/test_errors.py	`92.85% <100%> (-3.58%)`	⬇️
tests/all/unit/api/helpers/test_exceptions.py	`93.33% <100%> (ø)`
... and 75 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 98108a2...5559090. Read the comment docs.

iamareebjamal · 2019-12-01T16:38:03Z

app/api/custom/orders.py

+    if result.errors:
+        return jsonify(result.errors)
+    for ticket in tickets:
+        ticket_info = db.session.query(Ticket).filter_by(id=int(ticket['id']), deleted_at=None).first()


Do this in a single query filter(ids in ticket_ids, event_id=event_id, deleted_at=None)

iamareebjamal · 2019-12-01T16:38:23Z

app/api/custom/orders.py

+            db.session.add(attendee)
+            db.session.commit()
+            db.session.refresh(attendee)
+            attendee_list.append(attendee.id)


Append attendee directly

iamareebjamal · 2019-12-01T16:38:58Z

app/api/custom/orders.py

+                           error="Ticket with id {} belongs to a different Event.".format(ticket['id']))
+        if (ticket_info.quantity - get_count(db.session.query(TicketHolder.id).filter_by(
+              ticket_id=int(ticket['id']), deleted_at=None))) < ticket['quantity']:
+            return jsonify(status='Order Unsuccessful', error='Ticket already sold out.')


These checks should be in calculate amount function itself

iamareebjamal · 2019-12-01T16:39:12Z

app/api/custom/orders.py

+        for ticket_amount in range(ticket['quantity']):
+            attendee = TicketHolder(**result[0], event_id=int(data['event_id']), ticket_id=int(ticket['id']))
+            db.session.add(attendee)
+            db.session.commit()


Commit once after the loop

iamareebjamal · 2019-12-01T16:39:44Z

app/api/custom/orders.py

+    db.session.refresh(order)
+    order_tickets = {}
+    for attendee_id in attendee_list:
+        holder = TicketHolder.query.filter(TicketHolder.id == attendee_id).one()


No need to refetch the ticket holder

iamareebjamal · 2019-12-01T16:39:57Z

app/api/custom/orders.py

+    for attendee_id in attendee_list:
+        holder = TicketHolder.query.filter(TicketHolder.id == attendee_id).one()
+        holder.order_id = order.id
+        save_to_db(holder)


Again, commit once after the loop

iamareebjamal · 2019-12-01T16:40:12Z

app/api/custom/orders.py

+
+    for ticket in order_tickets:
+        od = OrderTicket(order_id=order.id, ticket_id=ticket, quantity=order_tickets[ticket])
+        save_to_db(od)


commit once after loop

iamareebjamal · 2019-12-01T16:40:33Z

app/api/custom/orders.py

+        save_to_db(od)
+
+    order.quantity = order.tickets_count
+    return jsonify("Order successful.")


Newly created order should be returned

iamareebjamal · 2019-12-03T18:55:23Z

As asked previously, never use update branch feature unless just before merging. The triggers and CI is ran again without any change for no reason and maintainers get a notification thinking that something has changed

prateekj117 · 2019-12-04T20:56:40Z

@iamareebjamal Done. I am thinking to refactor the whole calculate-order function in another PR.

iamareebjamal · 2019-12-04T22:06:41Z

app/api/custom/orders.py

+    if result.errors:
+        return jsonify(result.errors)
+    ticket_ids = [ticket['id'] for ticket in tickets]
+    ticket_info_list = db.session.query(Ticket).filter(Ticket.id.in_(ticket_ids)).all()


This will allow deleted tickets as well. And as previously instructed, event filtering should be done here itself

app/api/custom/orders.py

prateekj117 · 2019-12-09T07:55:01Z

@iamareebjamal Done the required changes.

iamareebjamal · 2019-12-09T13:51:17Z

app/api/custom/orders.py

+    if not args:
+        data = request.get_json()
+    else:
+        data = json.loads(args[0])


As mentioned previously, this should not be done

@iamareebjamal #6635 (comment)

No benefit of merging an intermediate state PR

@iamareebjamal I will start working on that PR as soon as this gets merged. I wanted to keep them separate as this PR already contains a big change. And the second change is more of a refactor.

Yes, that's why it can be taken in this PR only. It's not a huge change compared to what has been done already. It makes no sense to break something first and then fix later in a separate PR

iamareebjamal · 2019-12-09T13:52:00Z

app/api/custom/orders.py

+        for ticket_information in ticket_info_list:
+            if ticket_information.id == int(ticket['id']):
+                ticket_info = ticket_information
+                break


What is this doing?

You have unnecessariy made an O(n) function to ne O(n^2)

prateekj117 · 2019-12-10T16:18:16Z

@iamareebjamal Did the changes. Please review.

iamareebjamal · 2019-12-10T16:28:17Z

app/api/custom/orders.py

+    ticket_ids = [int(ticket['id']) for ticket in tickets]
+    ticket_info_list = db.session.query(Ticket).filter(Ticket.id.in_(ticket_ids)).filter_by(event_id=data['event_id'],
+                                                                                            deleted_at=None).all()
+    ticket_ids_found = [ticket_information.id for ticket_information in ticket_info_list]


Suggested change

ticket_ids_found = [ticket_information.id for ticket_information in ticket_info_list]

ticket_ids_found = {ticket_information.id for ticket_information in ticket_info_list}

iamareebjamal · 2019-12-10T16:29:38Z

app/api/custom/orders.py

+    if result.errors:
+        return jsonify(result.errors)
+    ticket_ids = [int(ticket['id']) for ticket in tickets]
+    ticket_info_list = db.session.query(Ticket).filter(Ticket.id.in_(ticket_ids)).filter_by(event_id=data['event_id'],


Why not tickets or ticket_list

iamareebjamal · 2019-12-10T16:30:41Z

app/api/custom/orders.py

+    ticket_ids_found = [ticket_information.id for ticket_information in ticket_info_list]
+    tickets_not_found = list(set(ticket_ids) - set(ticket_ids_found))
+    if tickets_not_found:
+        return jsonify(status='Order Unsuccessful', error='Ticket with id {} was not found.'.format(tickets_not_found))


There can be multiple tickets not found. Also, send event ID as well. Ticket 324 may exist but not under the event requested

@iamareebjamal I am sending the list of all the tickets not found. Regarding example of ticket 324, I am already passing event_id as well which fetching the tickets, so those tickets will also throw tickets not found.

It will throw: Ticket with ID 324, 325, 326 was not found

Everyone wants to see: Tickets with ID 324, 325, 326 were not found in Event 432

Hence, again #6635 (comment)

It will throw: Ticket with ID 324, 325, 326 was not found in Event 432

Everyone wants to see: Tickets with ID 324, 325, 326 were not found in Event 432

iamareebjamal · 2019-12-10T16:31:30Z

app/api/custom/orders.py

+        return jsonify(status='Order Unsuccessful', error='Ticket with id {} was not found.'.format(tickets_not_found))
+    for ticket in tickets:
+        ticket_info = None
+        for ticket_information in ticket_info_list:


Nested for loop = Automatically not approved. There is no need of nested for loop here

iamareebjamal · 2019-12-10T16:32:58Z

app/api/custom/orders.py

+    ticket_ids_found = [ticket_information.id for ticket_information in ticket_info_list]
+    tickets_not_found = list(set(ticket_ids) - set(ticket_ids_found))
+    if tickets_not_found:
+        return jsonify(status='Order Unsuccessful', error='Ticket with id {} was not found.'.format(tickets_not_found))


This will send HTTP status code as 200, which means OK. Which means order was created successfully

iamareebjamal · 2019-12-10T16:33:08Z

app/api/custom/orders.py

+                ticket_info = ticket_information
+        if (ticket_info.quantity - get_count(db.session.query(TicketHolder.id).filter_by(
+              ticket_id=int(ticket['id']), deleted_at=None))) < ticket['quantity']:
+            return jsonify(status='Order Unsuccessful', error='Ticket already sold out.')


This will send HTTP status code as 200, which means OK. Which means order was created successfully

iamareebjamal · 2019-12-10T16:33:23Z

app/api/custom/orders.py

+    json_api_attendee = {"data": {"attributes": data['attendee'], "type": "attendee"}}
+    result = schema.load(json_api_attendee)
+    if result.errors:
+        return jsonify(result.errors)


This will send HTTP status code as 200, which means OK. Which means order was created successfully

prateekj117 · 2019-12-10T17:16:46Z

@iamareebjamal Done.

iamareebjamal · 2019-12-10T17:32:15Z

#6635 (comment)

prateekj117 · 2019-12-10T17:36:00Z

@iamareebjamal Did those changes.

iamareebjamal · 2019-12-10T18:06:59Z

Read again

app/api/custom/orders.py

iamareebjamal · 2019-12-14T10:02:41Z

app/api/helpers/order.py

@@ -213,5 +223,5 @@ def calculate_order_amount(tickets, discount_code):
            'ticket_fee': round(ticket_fee, 2),
            'sub_total': round(sub_total, 2)
        })
-    return dict(tax_included=tax_included, total_amount=round(total_amount, 2), total_tax=round(total_tax, 2),
-                total_discount=round(total_discount, 2), tickets=ticket_list)
+    return jsonify(dict(tax_included=tax_included, total_amount=round(total_amount, 2), total_tax=round(total_tax, 2),


This should not return JSON response. This function should have no idea about request or response or anything. This function does not know that it is part of flask application. This should be a pure python function, otherwise what was the point of extracting it out?

iamareebjamal · 2019-12-14T10:09:19Z

app/api/helpers/order.py

@@ -134,7 +135,16 @@ def create_onsite_attendees_for_order(data):
    del data['on_site_tickets']


-def calculate_order_amount(tickets, discount_code):
+def calculate_order_amount(data):


Don't change the function signature, it was fine. This should take tickets and discount code only.

Create a wrapper function if you want to

houndci-bot · 2019-12-14T10:09:37Z

app/api/helpers/order.py

@@ -214,4 +224,4 @@ def calculate_order_amount(tickets, discount_code):
            'sub_total': round(sub_total, 2)
        })
    return dict(tax_included=tax_included, total_amount=round(total_amount, 2), total_tax=round(total_tax, 2),
-                total_discount=round(total_discount, 2), tickets=ticket_list)
+                        total_discount=round(total_discount, 2), tickets=ticket_list)


continuation line over-indented for visual indent

houndci-bot · 2019-12-14T10:09:38Z

app/api/helpers/order.py

@@ -1,16 +1,17 @@
 import logging
 from datetime import timedelta, datetime, timezone

-from flask import render_template
+from flask import render_template, jsonify


'flask.jsonify' imported but unused

houndci-bot · 2019-12-14T10:09:38Z

app/api/custom/orders.py

@@ -1,22 +1,26 @@
-from flask import Blueprint, jsonify, request
+import json


'json' imported but unused

app/api/custom/orders.py

niranjan94 · 2020-01-04T10:10:02Z

Here is an overview of what got changed by this pull request:

Issues
======
+ Solved 2
- Added 2
           

Complexity increasing per file
==============================
- app/api/custom/orders.py  10
- app/api/helpers/order.py  2

See the complete overview on Codacy

app/api/custom/orders.py

prateekj117 added 3 commits November 29, 2019 17:19

Create attendee using custom orders API

a20ac00

Create an initial order.

748b796

Required changes

6acd3d3

prateekj117 added feature Work in Progress labels Nov 30, 2019

prateekj117 changed the title ~~Single endpoint for creating creating an order.~~ feat: Single endpoint for creating creating an order. Nov 30, 2019

iamareebjamal reviewed Dec 1, 2019

View reviewed changes

Merge branch 'development' into new-orders-api

b984817

Required changes.

a18ad86

iamareebjamal reviewed Dec 4, 2019

View reviewed changes

app/api/custom/orders.py Show resolved Hide resolved

Required changes.

6174428

iamareebjamal reviewed Dec 9, 2019

View reviewed changes

prateekj117 added 2 commits December 10, 2019 21:41

Required changes.

a2652e1

Required changes.

f7f017f

prateekj117 removed the Work in Progress label Dec 10, 2019

iamareebjamal reviewed Dec 10, 2019

View reviewed changes

prateekj117 added 2 commits December 10, 2019 22:39

Required changes.

c2daa41

Add Event ID in Error Message.

cdaf783

Correct error message

3968527

iamareebjamal reviewed Dec 14, 2019

View reviewed changes

app/api/custom/orders.py Show resolved Hide resolved

prateekj117 added 2 commits December 14, 2019 15:01

Refactor calculate_order_amount function.

f791455

Required changes.

09e7cb8

iamareebjamal reviewed Dec 14, 2019

View reviewed changes

Required changes.

cc220fe

houndci-bot reviewed Dec 14, 2019

View reviewed changes

Required changes.

98108a2

iamareebjamal previously approved these changes Dec 14, 2019

View reviewed changes

iamareebjamal changed the title ~~feat: Single endpoint for creating creating an order.~~ feat: Single endpoint for creating creating an order Dec 14, 2019

Merge branch 'development' into new-orders-api

5559090

iamareebjamal dismissed their stale review via 5559090 January 4, 2020 10:06

houndci-bot reviewed Jan 4, 2020

View reviewed changes

app/api/custom/orders.py Show resolved Hide resolved

niranjan94 reviewed Jan 4, 2020

View reviewed changes

app/api/custom/orders.py Show resolved Hide resolved

niranjan94 reviewed Jan 4, 2020

View reviewed changes

app/api/custom/orders.py Show resolved Hide resolved

iamareebjamal merged commit e758cfc into fossasia:development Jan 5, 2020

	ticket_ids_found = [ticket_information.id for ticket_information in ticket_info_list]
	ticket_ids_found = {ticket_information.id for ticket_information in ticket_info_list}

		@@ -1,22 +1,26 @@
		from flask import Blueprint, jsonify, request
		import json

feat: Single endpoint for creating creating an order #6635

feat: Single endpoint for creating creating an order #6635

Conversation

prateekj117 commented Nov 30, 2019

Checklist

codecov bot commented Nov 30, 2019 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamareebjamal commented Dec 3, 2019

prateekj117 commented Dec 4, 2019

Choose a reason for hiding this comment

prateekj117 commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekj117 commented Dec 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prateekj117 commented Dec 10, 2019

iamareebjamal commented Dec 10, 2019

prateekj117 commented Dec 10, 2019

iamareebjamal commented Dec 10, 2019

iamareebjamal Dec 14, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niranjan94 commented Jan 4, 2020

codecov bot commented Nov 30, 2019 •

edited

iamareebjamal Dec 14, 2019 •

edited