Fix ModelReceipt queries

Hybrid properties were the root cause of performance issues with various queries we were trying to run on model receipts. Now we return an SQL aggregate function instead, allowing you to build similar queries without the massive performance hit. This change uses the attendee receipt discrepancies and nonzero balance pages as a proof of concept -- if these become useable once on prod, then we can apply this change to related currently-unusable pages.

One important thing to note is that you should NOT ever use the new _sql aggregate functions from two different tables in one query -- this either creates a cartesian join or, if you specify joins, repeats the rows from earlier tables in the join and screws up calculations (e.g., doubling the item total). You can see how we handle needing aggregates from two different tables in the attendees_nonzero_balance page handler, where we create a subquery for the ReceiptItems table. This appears to work well alongside normal aggregate functions/outer joins for ReceiptTransactions.

For more information, see https://stackoverflow.com/questions/12484975/sqlalchemy-using-func-with-outerjoin-on-multiple-tables and https://blog.miguelgrinberg.com/post/nested-queries-with-sqlalchemy-orm

uber/models/attendee.py

@@ -962,24 +962,10 @@ def amount_extra_unpaid(self):
     def amount_pending(self):
         return self.active_receipt.pending_total if self.active_receipt else 0
 
-    @hybrid_property
+    @property
     def is_paid(self):
         return self.active_receipt and self.active_receipt.current_amount_owed == 0
 
-    @is_paid.expression
-    def is_paid(cls):
-        from uber.models import ModelReceipt, Group
-
-        return case([(cls.paid == c.PAID_BY_GROUP,
-                      exists().select_from(Group).where(
-                          and_(cls.group_id == Group.id,
-                               Group.is_paid == True)))],  # noqa: E712
-                    else_=(exists().select_from(ModelReceipt).where(
-                            and_(ModelReceipt.owner_id == cls.id,
-                                 ModelReceipt.owner_model == "Attendee",
-                                 ModelReceipt.closed == None,  # noqa: E711
-                                 ModelReceipt.current_amount_owed == 0))))
-
     @hybrid_property
     def amount_paid(self):
         return self.active_receipt.payment_total if self.active_receipt else 0
@@ -988,7 +974,7 @@ def amount_paid(self):
     def amount_paid(cls):
         from uber.models import ModelReceipt
 
-        return select([ModelReceipt.payment_total]).where(
+        return select([ModelReceipt.payment_total_sql]).outerjoin(ModelReceipt.receipt_txns).where(
             and_(ModelReceipt.owner_id == cls.id,
                  ModelReceipt.owner_model == "Attendee",
                  ModelReceipt.closed == None)).label('amount_paid')  # noqa: E711
@@ -1001,7 +987,7 @@ def amount_refunded(self):
     def amount_refunded(cls):
         from uber.models import ModelReceipt
 
-        return select([ModelReceipt.refund_total]).where(
+        return select([ModelReceipt.refund_total_sql]).outerjoin(ModelReceipt.receipt_txns).where(
             and_(ModelReceipt.owner_id == cls.id,
                  ModelReceipt.owner_model == "Attendee")).label('amount_refunded')
 

uber/models/commerce.py

@@ -2,11 +2,12 @@
 import stripe
 
 from pytz import UTC
+from pockets import classproperty
 from pockets.autolog import log
 from residue import JSON, CoerceUTF8 as UnicodeText, UTCDateTime, UUID
-from sqlalchemy import and_, case, func, or_, select
+from sqlalchemy import func, or_
 
-from sqlalchemy.ext.hybrid import hybrid_property
+from sqlalchemy.sql.functions import coalesce
 from sqlalchemy.schema import ForeignKey
 from sqlalchemy.types import Boolean, Integer
 from sqlalchemy.dialects.postgresql.json import JSONB
@@ -148,59 +149,50 @@ def refundable_txns(self):
     def pending_total(self):
         return sum([txn.amount for txn in self.receipt_txns if txn.is_pending_charge])
 
-    @hybrid_property
+    @property
     def payment_total(self):
         return sum([txn.amount for txn in self.receipt_txns
                     if not txn.cancelled and txn.amount > 0 and (txn.charge_id or txn.intent_id == '')])
 
-    @payment_total.expression
-    def payment_total(cls):
-        return select([func.sum(ReceiptTransaction.amount)]).where(
-            and_(ReceiptTransaction.receipt_id == cls.id,
-                 ReceiptTransaction.cancelled == None,  # noqa: E711
-                 ReceiptTransaction.amount > 0)).where(
-                     or_(ReceiptTransaction.charge_id != None,  # noqa: E711
-                         ReceiptTransaction.intent_id == '')).label('payment_total')
+    @classproperty
+    def payment_total_sql(cls):
+        return coalesce(func.sum(ReceiptTransaction.amount).filter(
+            ReceiptTransaction.amount > 0, ReceiptTransaction.cancelled == None).filter(
+                or_(ReceiptTransaction.charge_id != None,
+                    ReceiptTransaction.intent_id == '')), 0)
 
-    @hybrid_property
+    @property
     def refund_total(self):
         return sum([txn.amount for txn in self.receipt_txns if txn.amount < 0]) * -1
 
-    @refund_total.expression
-    def refund_total(cls):
-        return select([func.sum(ReceiptTransaction.amount) * -1]
-                      ).where(and_(ReceiptTransaction.amount < 0, ReceiptTransaction.receipt_id == cls.id)
-                              ).label('refund_total')
+    @classproperty
+    def refund_total_sql(cls):
+        return coalesce(func.sum(ReceiptTransaction.amount).filter(
+            ReceiptTransaction.amount < 0) * -1, 0)
 
     @property
-    def has_at_con_payments(self):
-        return any([txn for txn in self.receipt_txns if txn.method == c.SQUARE])
-
-    @hybrid_property
-    def current_amount_owed(self):
-        return max(0, self.current_receipt_amount)
+    def item_total(self):
+        return sum([(item.amount * item.count) for item in self.receipt_items])
 
-    @current_amount_owed.expression
-    def current_amount_owed(cls):
-        return case([(cls.current_receipt_amount > 0, cls.current_receipt_amount)],
-                    else_=0)
+    @classproperty
+    def item_total_sql(cls):
+        return coalesce(func.sum(ReceiptItem.amount * ReceiptItem.count), 0)
+
+    @property
+    def txn_total(self):
+        return self.payment_total - self.refund_total
 
-    @hybrid_property
+    @property
     def current_receipt_amount(self):
         return self.item_total - self.txn_total
 
-    @hybrid_property
-    def item_total(self):
-        return sum([(item.amount * item.count) for item in self.receipt_items])
-
-    @item_total.expression
-    def item_total(cls):
-        return select([func.sum(ReceiptItem.amount * ReceiptItem.count)]
-                      ).where(ReceiptItem.receipt_id == cls.id).label('item_total')
+    @property
+    def current_amount_owed(self):
+        return max(0, self.current_receipt_amount)
 
-    @hybrid_property
-    def txn_total(self):
-        return self.payment_total - self.refund_total
+    @property
+    def has_at_con_payments(self):
+        return any([txn for txn in self.receipt_txns if txn.method == c.SQUARE])
 
     @property
     def total_str(self):

uber/models/group.py

@@ -355,20 +355,10 @@ def total_cost(self):
             return self.active_receipt.item_total / 100
         return (self.cost or self.calc_default_cost()) + self.amount_extra
 
-    @hybrid_property
+    @property
     def is_paid(self):
         return self.active_receipt and self.active_receipt.current_amount_owed == 0
 
-    @is_paid.expression
-    def is_paid(cls):
-        from uber.models import ModelReceipt
-
-        return exists().select_from(ModelReceipt).where(
-            and_(ModelReceipt.owner_id == cls.id,
-                 ModelReceipt.owner_model == "Group",
-                 ModelReceipt.closed == None,  # noqa: E711
-                 ModelReceipt.current_amount_owed == 0))
-
     @property
     def amount_unpaid(self):
         if self.is_dealer and self.status not in [c.APPROVED, c.SHARED]:
@@ -399,7 +389,7 @@ def amount_paid(self):
     def amount_paid(cls):
         from uber.models import ModelReceipt
 
-        return select([ModelReceipt.payment_total]
+        return select([ModelReceipt.payment_total_sql]).outerjoin(ModelReceipt.receipt_txns
                       ).where(and_(ModelReceipt.owner_id == cls.id,
                                    ModelReceipt.owner_model == "Group",
                                    ModelReceipt.closed == None)).label('amount_paid')  # noqa: E711
@@ -412,7 +402,7 @@ def amount_refunded(self):
     def amount_refunded(cls):
         from uber.models import ModelReceipt
 
-        return select([ModelReceipt.refund_total]
+        return select([ModelReceipt.refund_total_sql]).outerjoin(ModelReceipt.receipt_txns
                       ).where(and_(ModelReceipt.owner_id == cls.id,
                                    ModelReceipt.owner_model == "Group")).label('amount_refunded')
 

uber/site_sections/reg_reports.py

@@ -1,6 +1,7 @@
 from collections import defaultdict
 from sqlalchemy import or_, and_
 from sqlalchemy.sql import func
+from sqlalchemy.sql.functions import coalesce
 
 from uber.config import c
 from uber.decorators import all_renderable, log_pageview, streamable
@@ -38,74 +39,43 @@ def found_how(self, session):
             key=lambda s: s.lower())}
 
     @log_pageview
-    def attendee_receipt_discrepancies(self, session, include_pending=False, page=1):
-        '''
-        select model_receipt.owner_id
-        from model_receipt
-        join receipt_item on receipt_item.receipt_id = model_receipt.id
-        join attendee on attendee.id = model_receipt.owner_id
-        where
-            model_receipt.closed is null
-            and model_receipt.owner_model = 'Attendee'
-        group by attendee.id, attendee.default_cost, attendee.badge_status
-        having
-            attendee.default_cost * 100 != sum(receipt_item.amount * receipt_item.count)
-            and (attendee.badge_status NOT IN (175104371, 192297957, 229301191, 169050145, 91398854, 177900276));
-        '''
-
-        page = int(page)
-        if page <= 0:
-            offset = 0
-        else:
-            offset = (page - 1) * 50
-
+    def attendee_receipt_discrepancies(self, session, include_pending=False):
         if include_pending:
             filter = or_(Attendee.badge_status.in_([c.PENDING_STATUS, c.AT_DOOR_PENDING_STATUS]),
                                Attendee.is_valid == True)  # noqa: E712
         else:
             filter = Attendee.is_valid == True  # noqa: E712
-
-        receipt_query = (
-            session.query(
-                ModelReceipt.owner_id
-            )
-            .join(ReceiptItem, ReceiptItem.receipt_id == ModelReceipt.id)
-            .join(Attendee, Attendee.id == ModelReceipt.owner_id)
-            .filter(
-                ModelReceipt.closed.is_(None),
-                ModelReceipt.owner_model == 'Attendee'
-            )
-            .group_by(Attendee.id, Attendee.default_cost, Attendee.badge_status, ModelReceipt.id)
-            .having(
-                and_(
-                    Attendee.default_cost_cents != func.sum(ReceiptItem.amount * ReceiptItem.count),
-                    filter
-                )
-            )
-        )
-
-        count = receipt_query.count()
-
-        receipt_owners = [x[0] for x in receipt_query.limit(50).offset(offset)]
 
-        receipt_query = session.query(Attendee).join(Attendee.active_receipt).filter(Attendee.id.in_(receipt_owners))
+        attendees = session.query(Attendee).filter(filter).join(  # noqa: E712
+                Attendee.active_receipt).outerjoin(ModelReceipt.receipt_items).group_by(
+                    ModelReceipt.id).group_by(Attendee.id).having(Attendee.default_cost_cents != ModelReceipt.item_total_sql)
 
         return {
-            'current_page': page,
-            'pages': (count // 50) + 1,
-            'attendees': receipt_query,
+            'attendees': attendees,
             'include_pending': include_pending,
         }
 
     @log_pageview
-    def attendees_nonzero_balance(self, session, include_no_receipts=False):
-        attendees = session.query(Attendee,
-                                  ModelReceipt).join(Attendee.active_receipt
-                                                     ).filter(Attendee.default_cost_cents == ModelReceipt.item_total,
-                                                              ModelReceipt.current_receipt_amount != 0)
+    def attendees_nonzero_balance(self, session, include_discrepancies=False):
+        item_subquery = session.query(ModelReceipt.owner_id, ModelReceipt.item_total_sql.label('item_total')
+                                      ).join(ModelReceipt.receipt_items).group_by(ModelReceipt.owner_id).subquery()
+
+        if include_discrepancies:
+            filter = True
+        else:
+            filter = Attendee.default_cost_cents != item_subquery.c.item_total
+
+        attendees_and_totals = session.query(
+            Attendee, ModelReceipt.payment_total_sql, ModelReceipt.refund_total_sql, item_subquery.c.item_total
+            ).filter(Attendee.is_valid == True).join(Attendee.active_receipt).outerjoin(
+                ModelReceipt.receipt_txns).join(item_subquery, Attendee.id == item_subquery.c.owner_id).group_by(
+                    ModelReceipt.id).group_by(Attendee.id).group_by(item_subquery.c.item_total).having(
+                        and_((ModelReceipt.payment_total_sql - ModelReceipt.refund_total_sql) != item_subquery.c.item_total,
+                             filter))
 
         return {
-            'attendees': attendees.filter(Attendee.is_valid == True)  # noqa: E712
+            'attendees_and_totals': attendees_and_totals,
+            'include_discrepancies': include_discrepancies,
         }
 
     @log_pageview

uber/templates/reg_reports/attendee_receipt_discrepancies.html

@@ -4,24 +4,14 @@
 {% include 'discrepancy_reports_header.html' with context %}
 <div class="card">
     <div class="card-body">
-        <p>
+        <div class="d-flex justify-content-end mb-3">
         {% if include_pending %}
-            <a href="attendee_receipt_discrepancies?page={{ current_page }}" class="btn btn-secondary">Hide Pending</a>
+            <a href="attendee_receipt_discrepancies" class="btn btn-secondary">Hide Pending Attendees</a>
         {% else %}
-            <a href="attendee_receipt_discrepancies?include_pending=True&page={{ current_page }}" class="btn btn-outline-secondary">Show Pending</a>
+            <a href="attendee_receipt_discrepancies?include_pending=True" class="btn btn-outline-secondary">Show Pending Attendees</a>
         {% endif %}
-        {% set page_base_url = "attendee_receipt_discrepancies?include_pending=True&" if include_pending else "attendee_receipt_discrepancies?" %}
-            <nav>
-                <ul class="pagination">
-                    <li class="page-item{% if current_page == 1 %} disabled{% endif %}"><a class="page-link" href="{{ page_base_url ~ 'page=' ~ (current_page|int - 1) }}">Previous</a></li>
-                    {% for page in range(1, pages+1) %}
-                        <li class="page-item{% if page == current_page %} active{% endif %}"><a class="page-link" href="{{ page_base_url ~ 'page=' ~ page }}">{{ page }}</a></li>
-                    {% endfor %}
-                    <li class="page-item{% if current_page == pages %} disabled{% endif %}"><a class="page-link" href="{{ page_base_url ~ 'page=' ~ (current_page|int + 1) }}">Next</a></li>
-                </ul>
-            </nav>
-        </p>
-        <table class="table table-striped">
+        </div>
+        <table class="table table-striped datatable">
         <thead>
             <tr>
             <th>Badge Status</th>
@@ -75,15 +65,6 @@
         {% endfor %}
         </tbody>
         </table>
-        <nav>
-            <ul class="pagination">
-                <li class="page-item{% if current_page == 1 %} disabled{% endif %}"><a class="page-link" href="{{ page_base_url ~ 'page=' ~ (current_page|int - 1) }}">Previous</a></li>
-                {% for page in range(1, pages+1) %}
-                    <li class="page-item{% if page == current_page %} active{% endif %}"><a class="page-link" href="{{ page_base_url ~ 'page=' ~ page }}">{{ page }}</a></li>
-                {% endfor %}
-                <li class="page-item{% if current_page == pages %} disabled{% endif %}"><a class="page-link" href="{{ page_base_url ~ 'page=' ~ (current_page|int + 1) }}">Next</a></li>
-            </ul>
-        </nav>
     </div>
 </div>
 {% if c.HAS_REG_ADMIN_ACCESS %}

uber/templates/reg_reports/attendees_nonzero_balance.html

@@ -4,10 +4,13 @@
 {% include 'discrepancy_reports_header.html' with context %}
 <div class="card">
     <div class="card-body">
-        <p>
-            This page only shows attendees whose receipt matches their default cost.
-            Please see the "Attendees with Receipt Discrepancies" page to view attendees who may owe money but whose default cost does not match their receipt.
-        </p>
+        <div class="d-flex justify-content-end mb-3">
+            {% if include_discrepancies %}
+            <a href="attendees_nonzero_balance" class="btn btn-primary">Exclude Attendees with Receipt Discrepancies</a>
+            {% else %}
+            <a href="attendees_nonzero_balance?include_discrepancies=True" class="btn btn-outline-primary">Include Attendees with Receipt Discrepancies</a>
+            {% endif %}
+        </div>
         <table class="table table-striped datatable">
         <thead>
             <tr>
@@ -24,7 +27,8 @@
             </tr>
         </thead>
         <tbody>
-        {% for attendee, active_receipt in attendees %}
+        {% for attendee, payment_total, refund_total, item_total in attendees_and_totals %}
+        {% set txn_total = payment_total - refund_total %}
         <tr id="{{ attendee.id }}">
             <td>
                 {{ attendee.badge_status_label }}
@@ -39,19 +43,19 @@
                 {{ attendee.badge_type_label }}
             </td>
             <td>
-                {{ (active_receipt.item_total / 100)|format_currency }}
+                {{ (item_total / 100)|format_currency }}
             </td>
             <td>
-                {{ (active_receipt.payment_total / 100)|format_currency }}
+                {{ (payment_total / 100)|format_currency }}
             </td>
             <td>
-                {{ (active_receipt.refund_total / 100)|format_currency }}
+                {{ (refund_total / 100)|format_currency }}
             </td>
             <td>
-                {{ (active_receipt.txn_total / 100)|format_currency }}
+                {{ (txn_total / 100)|format_currency }}
             </td>
             <td>
-                {{ (active_receipt.current_receipt_amount / 100)|format_currency }}
+                {{ ((item_total - txn_total) / 100)|format_currency }}
             </td>
             {% if c.HAS_REG_ADMIN_ACCESS %}
             <td>