More response to CR

common/djangoapps/student/models.py

@@ -10,10 +10,12 @@ file and check it in at the same time as your model changes. To do that,
 2. ./manage.py lms schemamigration student --auto description_of_your_change
 3. Add the migration file created in edx-platform/common/djangoapps/student/migrations/
 """
+import crum
 from datetime import datetime
 import hashlib
 import json
 import logging
+from pytz import UTC
 import uuid
 
 from django.conf import settings
@@ -25,16 +27,13 @@ from django.dispatch import receiver, Signal
 import django.dispatch
 from django.forms import ModelForm, forms
 from django.core.exceptions import ObjectDoesNotExist
-
-from course_modes.models import CourseMode
-import lms.lib.comment_client as cc
-from pytz import UTC
-import crum
-
 from track import contexts
 from track.views import server_track
 from eventtracking import tracker
 
+from course_modes.models import CourseMode
+import lms.lib.comment_client as cc
+
 unenroll_done = Signal(providing_args=["course_enrollment"])
 log = logging.getLogger(__name__)
 AUDIT_LOG = logging.getLogger("audit")
@@ -581,15 +580,17 @@ class CourseEnrollment(models.Model):
         )
 
     @classmethod
-    def enrollments_in(cls, course_id, mode=None):
+    def enrollment_counts(cls, course_id):
         """
-        Return a queryset of CourseEnrollment for every active enrollment in the course course_id.
-        Returns only CourseEnrollments with the given mode, if a mode is supplied by the caller.
+        Returns a dictionary that stores the total enrollment count for a course, as well as the
+        enrollment count for each individual mode.
         """
-        if mode is None:
-            return cls.objects.filter(course_id=course_id, is_active=True,)
-        else:
-            return cls.objects.filter(course_id=course_id, is_active=True, mode=mode)
+        d = {}
+        d['total'] = cls.objects.filter(course_id=course_id, is_active=True).count()
+        d['honor'] = cls.objects.filter(course_id=course_id, is_active=True, mode='honor').count()
+        d['audit'] = cls.objects.filter(course_id=course_id, is_active=True, mode='audit').count()
+        d['verified'] = cls.objects.filter(course_id=course_id, is_active=True, mode='verified').count()
+        return d
 
     def activate(self):
         """Makes this `CourseEnrollment` record active. Saves immediately."""

common/djangoapps/util/query.py

@@ -0,0 +1,4 @@
+from django.conf import settings
+
+def use_read_replica_if_available(queryset):
+    return queryset.using("read_replica") if "read_replica" in settings.DATABASES else queryset

lms/djangoapps/shoppingcart/migrations/0007_auto__add_field_orderitem_service_fee.py

@@ -8,57 +8,30 @@ from django.db import models
 class Migration(SchemaMigration):
 
     def forwards(self, orm):
-        # Adding model 'RefundReport'
-        db.create_table('shoppingcart_refundreport', (
-            ('report_ptr', self.gf('django.db.models.fields.related.OneToOneField')(to=orm['shoppingcart.Report'], unique=True, primary_key=True)),
-        ))
-        db.send_create_signal('shoppingcart', ['RefundReport'])
-
-        # Adding model 'Report'
-        db.create_table('shoppingcart_report', (
-            ('id', self.gf('django.db.models.fields.AutoField')(primary_key=True)),
-        ))
-        db.send_create_signal('shoppingcart', ['Report'])
-
-        # Adding model 'CertificateStatusReport'
-        db.create_table('shoppingcart_certificatestatusreport', (
-            ('report_ptr', self.gf('django.db.models.fields.related.OneToOneField')(to=orm['shoppingcart.Report'], unique=True, primary_key=True)),
-        ))
-        db.send_create_signal('shoppingcart', ['CertificateStatusReport'])
-
-        # Adding model 'ItemizedPurchaseReport'
-        db.create_table('shoppingcart_itemizedpurchasereport', (
-            ('report_ptr', self.gf('django.db.models.fields.related.OneToOneField')(to=orm['shoppingcart.Report'], unique=True, primary_key=True)),
-        ))
-        db.send_create_signal('shoppingcart', ['ItemizedPurchaseReport'])
-
-        # Adding model 'UniversityRevenueShareReport'
-        db.create_table('shoppingcart_universityrevenuesharereport', (
-            ('report_ptr', self.gf('django.db.models.fields.related.OneToOneField')(to=orm['shoppingcart.Report'], unique=True, primary_key=True)),
-        ))
-        db.send_create_signal('shoppingcart', ['UniversityRevenueShareReport'])
-
         # Adding field 'OrderItem.service_fee'
         db.add_column('shoppingcart_orderitem', 'service_fee',
                       self.gf('django.db.models.fields.DecimalField')(default=0.0, max_digits=30, decimal_places=2),
                       keep_default=False)
 
+        # Adding index on 'OrderItem', fields ['status']
+        db.create_index('shoppingcart_orderitem', ['status'])
+
+        # Adding index on 'OrderItem', fields ['fulfilled_time']
+        db.create_index('shoppingcart_orderitem', ['fulfilled_time'])
+
+        # Adding index on 'OrderItem', fields ['refund_requested_time']
+        db.create_index('shoppingcart_orderitem', ['refund_requested_time'])
+
 
     def backwards(self, orm):
-        # Deleting model 'RefundReport'
-        db.delete_table('shoppingcart_refundreport')
+        # Removing index on 'OrderItem', fields ['refund_requested_time']
+        db.delete_index('shoppingcart_orderitem', ['refund_requested_time'])
 
-        # Deleting model 'Report'
-        db.delete_table('shoppingcart_report')
+        # Removing index on 'OrderItem', fields ['fulfilled_time']
+        db.delete_index('shoppingcart_orderitem', ['fulfilled_time'])
 
-        # Deleting model 'CertificateStatusReport'
-        db.delete_table('shoppingcart_certificatestatusreport')
-
-        # Deleting model 'ItemizedPurchaseReport'
-        db.delete_table('shoppingcart_itemizedpurchasereport')
-
-        # Deleting model 'UniversityRevenueShareReport'
-        db.delete_table('shoppingcart_universityrevenuesharereport')
+        # Removing index on 'OrderItem', fields ['status']
+        db.delete_index('shoppingcart_orderitem', ['status'])
 
         # Deleting field 'OrderItem.service_fee'
         db.delete_column('shoppingcart_orderitem', 'service_fee')
@@ -108,14 +81,6 @@ class Migration(SchemaMigration):
             'mode': ('django.db.models.fields.SlugField', [], {'max_length': '50'}),
             'orderitem_ptr': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['shoppingcart.OrderItem']", 'unique': 'True', 'primary_key': 'True'})
         },
-        'shoppingcart.certificatestatusreport': {
-            'Meta': {'object_name': 'CertificateStatusReport', '_ormbases': ['shoppingcart.Report']},
-            'report_ptr': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['shoppingcart.Report']", 'unique': 'True', 'primary_key': 'True'})
-        },
-        'shoppingcart.itemizedpurchasereport': {
-            'Meta': {'object_name': 'ItemizedPurchaseReport', '_ormbases': ['shoppingcart.Report']},
-            'report_ptr': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['shoppingcart.Report']", 'unique': 'True', 'primary_key': 'True'})
-        },
         'shoppingcart.order': {
             'Meta': {'object_name': 'Order'},
             'bill_to_cardtype': ('django.db.models.fields.CharField', [], {'max_length': '32', 'blank': 'True'}),
@@ -139,15 +104,15 @@ class Migration(SchemaMigration):
         'shoppingcart.orderitem': {
             'Meta': {'object_name': 'OrderItem'},
             'currency': ('django.db.models.fields.CharField', [], {'default': "'usd'", 'max_length': '8'}),
-            'fulfilled_time': ('django.db.models.fields.DateTimeField', [], {'null': 'True'}),
+            'fulfilled_time': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}),
             'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'}),
             'line_desc': ('django.db.models.fields.CharField', [], {'default': "'Misc. Item'", 'max_length': '1024'}),
             'order': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['shoppingcart.Order']"}),
             'qty': ('django.db.models.fields.IntegerField', [], {'default': '1'}),
-            'refund_requested_time': ('django.db.models.fields.DateTimeField', [], {'null': 'True'}),
+            'refund_requested_time': ('django.db.models.fields.DateTimeField', [], {'null': 'True', 'db_index': 'True'}),
             'report_comments': ('django.db.models.fields.TextField', [], {'default': "''"}),
             'service_fee': ('django.db.models.fields.DecimalField', [], {'default': '0.0', 'max_digits': '30', 'decimal_places': '2'}),
-            'status': ('django.db.models.fields.CharField', [], {'default': "'cart'", 'max_length': '32'}),
+            'status': ('django.db.models.fields.CharField', [], {'default': "'cart'", 'max_length': '32', 'db_index': 'True'}),
             'unit_cost': ('django.db.models.fields.DecimalField', [], {'default': '0.0', 'max_digits': '30', 'decimal_places': '2'}),
             'user': ('django.db.models.fields.related.ForeignKey', [], {'to': "orm['auth.User']"})
         },
@@ -163,18 +128,6 @@ class Migration(SchemaMigration):
             'course_id': ('django.db.models.fields.CharField', [], {'unique': 'True', 'max_length': '128', 'db_index': 'True'}),
             'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'})
         },
-        'shoppingcart.refundreport': {
-            'Meta': {'object_name': 'RefundReport', '_ormbases': ['shoppingcart.Report']},
-            'report_ptr': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['shoppingcart.Report']", 'unique': 'True', 'primary_key': 'True'})
-        },
-        'shoppingcart.report': {
-            'Meta': {'object_name': 'Report'},
-            'id': ('django.db.models.fields.AutoField', [], {'primary_key': 'True'})
-        },
-        'shoppingcart.universityrevenuesharereport': {
-            'Meta': {'object_name': 'UniversityRevenueShareReport', '_ormbases': ['shoppingcart.Report']},
-            'report_ptr': ('django.db.models.fields.related.OneToOneField', [], {'to': "orm['shoppingcart.Report']", 'unique': 'True', 'primary_key': 'True'})
-        },
         'student.courseenrollment': {
             'Meta': {'ordering': "('user', 'course_id')", 'unique_together': "(('user', 'course_id'),)", 'object_name': 'CourseEnrollment'},
             'course_id': ('django.db.models.fields.CharField', [], {'max_length': '255', 'db_index': 'True'}),

lms/djangoapps/shoppingcart/models.py

@@ -1,13 +1,12 @@
+from collections import namedtuple
 from datetime import datetime
+from decimal import Decimal
 import pytz
 import logging
 import smtplib
 import unicodecsv
 
-from model_utils.managers import InheritanceManager
-from collections import namedtuple
 from boto.exception import BotoServerError  # this is a super-class of SESError and catches connection errors
-
 from django.dispatch import receiver
 from django.db import models
 from django.conf import settings
@@ -16,7 +15,9 @@ from django.core.mail import send_mail
 from django.contrib.auth.models import User
 from django.utils.translation import ugettext as _
 from django.db import transaction
+from django.db.models import Sum
 from django.core.urlresolvers import reverse
+from model_utils.managers import InheritanceManager
 
 from xmodule.modulestore.django import modulestore
 from xmodule.course_module import CourseDescriptor
@@ -26,6 +27,7 @@ from course_modes.models import CourseMode
 from edxmako.shortcuts import render_to_string
 from student.views import course_from_id
 from student.models import CourseEnrollment, unenroll_done
+from util.query import use_read_replica_if_available
 
 from verify_student.models import SoftwareSecurePhotoVerification
 
@@ -40,8 +42,6 @@ ORDER_STATUSES = (
     ('refunded', 'refunded'),
 )
 
-
-
 # we need a tuple to represent the primary key of various OrderItem subclasses
 OrderItemSubclassPK = namedtuple('OrderItemSubclassPK', ['cls', 'pk'])  # pylint: disable=C0103
 
@@ -570,6 +570,25 @@ class CertificateItem(OrderItem):
                      billing_email=settings.PAYMENT_SUPPORT_EMAIL)
 
     @classmethod
-    def verified_certificates_in(cls, course_id, status):
+    def verified_certificates_count(cls, course_id, status):
         """Return a queryset of CertificateItem for every verified enrollment in course_id with the given status."""
-        return CertificateItem.objects.filter(course_id=course_id, mode='verified', status=status)
+        return use_read_replica_if_available(
+            CertificateItem.objects.filter(course_id=course_id, mode='verified', status=status).count())
+
+    # TODO combine these three methods into one
+    @classmethod
+    def verified_certificates_monetary_field_sum(cls, course_id, status, field_to_aggregate):
+        """
+        Returns a Decimal indicating the total sum of field_to_aggregate for all verified certificates with a particular status.
+
+        Sample usages: 
+        - status 'refunded' and field_to_aggregate 'unit_cost' will give the total amount of money refunded for course_id
+        - status 'purchased' and field_to_aggregate 'service_fees' gives the sum of all service fees for purchased certificates
+        etc
+        """
+        query = use_read_replica_if_available(
+            CertificateItem.objects.filter(course_id=course_id, mode='verified', status='purchased').aggregate(Sum(field_to_aggregate)))[field_to_aggregate + '__sum']
+        if query is None:
+            return Decimal(0.00)
+        else:
+            return query

lms/djangoapps/shoppingcart/reports.py

@@ -1,44 +1,49 @@
-from shoppingcart.models import CertificateItem, OrderItem
-from django.db import models
-from django.db.models import Sum
-import unicodecsv
-from django.conf import settings
-from courseware.courses import get_course_by_id
-from student.models import CourseEnrollment
-from course_modes.models import CourseMode
 from decimal import Decimal
+import unicodecsv
+
+from django.db import models
+from django.conf import settings
+
+from courseware.courses import get_course_by_id
+from course_modes.models import CourseMode
+from shoppingcart.models import CertificateItem, OrderItem
+from student.models import CourseEnrollment
+from util.query import use_read_replica_if_available
 
 
-class Report(Object):
+class Report(object):
     """
     Base class for making CSV reports related to revenue, enrollments, etc
 
     To make a different type of report, write a new subclass that implements
-    the methods report_row_generator and csv_report_header_row.
+    the methods rows and header.
     """
 
-    def report_row_generator(self, start_date, end_date, start_letter=None, end_letter=None):
+    def rows(self, start_date, end_date, start_word=None, end_word=None):
         """
-        Performs database queries necessary for the report.  Returns an generator of
+        Performs database queries necessary for the report and eturns an generator of
         lists, in which each list is a separate row of the report.
+
+        Arguments are start_date (datetime), end_date (datetime), start_word (str),
+        and end_word (str).  Date comparisons are start_date <= [date of item] < end_date.
         """
         raise NotImplementedError
 
-    def csv_report_header_row(self):
+    def header(self):
         """
         Returns the appropriate header based on the report type, in the form of a
         list of strings.
         """
         raise NotImplementedError
 
-    def make_report(self, filelike, start_date, end_date, start_letter=None, end_letter=None):
+    def write_csv(self, filelike, start_date, end_date, start_word=None, end_word=None):
         """
-        Given the string report_type, a file object to write to, and start/end date bounds,
+        Given a file object to write to and {start/end date, start/end letter} bounds,
         generates a CSV report of the appropriate type.
         """
-        items = self.report_row_generator(start_date, end_date, start_letter, end_letter)
+        items = self.rows(start_date, end_date, start_word, end_word)
         writer = unicodecsv.writer(filelike, encoding="utf-8")
-        writer.writerow(self.csv_report_header_row())
+        writer.writerow(self.header())
         for item in items:
             writer.writerow(item)
 
@@ -51,12 +56,14 @@ class RefundReport(Report):
     order number, customer name, date of transaction, date of refund, and any service
     fees.
     """
-    def report_row_generator(self, start_date, end_date, start_letter=None, end_letter=None):
-        query = CertificateItem.objects.select_related('user__profile').filter(
-            status="refunded",
-            refund_requested_time__gte=start_date,
-            refund_requested_time__lt=end_date,
-        ).order_by('refund_requested_time')
+    def rows(self, start_date, end_date, start_word=None, end_word=None):
+        query = use_read_replica_if_available(
+            CertificateItem.objects.select_related('user__profile').filter(
+                status="refunded",
+                refund_requested_time__gte=start_date,
+                refund_requested_time__lt=end_date,
+            ).order_by('refund_requested_time'))
+
         for item in query:
             yield [
                 item.order_id,
@@ -67,7 +74,7 @@ class RefundReport(Report):
                 item.service_fee,
             ]
 
-    def csv_report_header_row(self):
+    def header(self):
         return [
             "Order Number",
             "Customer Name",
@@ -86,12 +93,13 @@ class ItemizedPurchaseReport(Report):
     a given start_date and end_date, we find that purchase's time, order ID, status,
     quantity, unit cost, total cost, currency, description, and related comments.
     """
-    def report_row_generator(self, start_date, end_date, start_letter=None, end_letter=None):
-        query = OrderItem.objects.filter(
-            status="purchased",
-            fulfilled_time__gte=start_date,
-            fulfilled_time__lt=end_date,
-        ).order_by("fulfilled_time")
+    def rows(self, start_date, end_date, start_word=None, end_word=None):
+        query = use_read_replica_if_available(
+            OrderItem.objects.filter(
+                status="purchased",
+                fulfilled_time__gte=start_date,
+                fulfilled_time__lt=end_date,
+            ).order_by("fulfilled_time"))
 
         for item in query:
             yield [
@@ -106,7 +114,7 @@ class ItemizedPurchaseReport(Report):
                 item.report_comments,
             ]
 
-    def csv_report_header_row(self):
+    def header(self):
         return [
             "Purchase Time",
             "Order ID",
@@ -124,65 +132,55 @@ class CertificateStatusReport(Report):
     """
     Subclass of Report, used to generate Certificate Status Reports for Ed Services.
 
-    For each course in each university whose name is within the range start_letter and end_letter,
+    For each course in each university whose name is within the range start_word and end_word,
     inclusive, (i.e., the letter range H-J includes both Ithaca College and Harvard University), we
     calculate the total enrollment, audit enrollment, honor enrollment, verified enrollment, total
     gross revenue, gross revenue over the minimum, and total dollars refunded.
     """
-    def report_row_generator(self, start_date, end_date, start_letter=None, end_letter=None):
+    def rows(self, start_date, end_date, start_word=None, end_word=None):
         results = []
-        for course_id in settings.COURSE_LISTINGS['default']:
-            # If the first letter of the university is between start_letter and end_letter, then we include
+        for course_id in course_ids_between(start_word, end_word):
+            # If the first letter of the university is between start_word and end_word, then we include
             # it in the report.  These comparisons are unicode-safe.
-            if (start_letter.lower() <= course_id.lower()) and (end_letter.lower() >= course_id.lower()) and (get_course_by_id(course_id) is not None):
-                cur_course = get_course_by_id(course_id)
-                university = cur_course.org
-                course = cur_course.number + " " + cur_course.display_name_with_default  # TODO add term (i.e. Fall 2013)?
-                enrollments = CourseEnrollment.enrollments_in(course_id)
-                total_enrolled = enrollments.count()
-                audit_enrolled = CourseEnrollment.enrollments_in(course_id, "audit").count()
-                honor_enrolled = CourseEnrollment.enrollments_in(course_id, "honor").count()
+            cur_course = get_course_by_id(course_id)
+            university = cur_course.org
+            course = cur_course.number + " " + cur_course.display_name_with_default  # TODO add term (i.e. Fall 2013)?
+            counts = CourseEnrollment.enrollment_counts(course_id)
+            total_enrolled = counts['total']
+            audit_enrolled = counts['audit']
+            honor_enrolled = counts['honor']
 
-                # Since every verified enrollment has 1 and only 1 cert item, let's just query those
-                verified_enrollments = CertificateItem.verified_certificates_in(course_id, 'purchased')
-                if verified_enrollments is None:
-                    verified_enrolled = 0
-                    gross_rev = Decimal(0.00)
-                    gross_rev_over_min = Decimal(0.00)
-                else:
-                    verified_enrolled = verified_enrollments.count()
-                    gross_rev_temp = verified_enrollments.aggregate(Sum('unit_cost'))
-                    gross_rev = gross_rev_temp['unit_cost__sum']
-                    gross_rev_over_min = gross_rev - (CourseMode.objects.get(course_id=course_id, mode_slug="verified").min_price * verified_enrolled)
+            if counts['verified'] == 0:
+                verified_enrolled = 0
+                gross_rev = Decimal(0.00)
+                gross_rev_over_min = Decimal(0.00)
+            else:
+                verified_enrolled = counts['verified']
+                gross_rev = CertificateItem.verified_certificates_monetary_field_sum(course_id, 'purchased', 'unit_cost')
+                gross_rev_over_min = gross_rev - (CourseMode.min_course_price_for_currency(course_id, 'usd') * verified_enrolled)
 
-                # should I be worried about is_active here?
-                refunded_enrollments = CertificateItem.verified_certificates_in(course_id, 'refunded')
-                if refunded_enrollments is None:
-                    number_of_refunds = 0
-                    dollars_refunded = Decimal(0.00)
-                else:
-                    number_of_refunds = refunded_enrollments.count()
-                    dollars_refunded_temp = refunded_enrollments.aggregate(Sum('unit_cost'))
-                    dollars_refunded = dollars_refunded_temp['unit_cost__sum']
+            # should I be worried about is_active here?
+            number_of_refunds = CertificateItem.verified_certificates_count(course_id, 'refunded')
+            if number_of_refunds == 0:
+                dollars_refunded = Decimal(0.00)
+            else:
+                dollars_refunded = CertificateItem.verified_certificates_monetary_field_sum(course_id, 'refunded', 'unit_cost')
 
-                result = [
-                    university,
-                    course,
-                    total_enrolled,
-                    audit_enrolled,
-                    honor_enrolled,
-                    verified_enrolled,
-                    gross_rev,
-                    gross_rev_over_min,
-                    number_of_refunds,
-                    dollars_refunded
-                ]
+            result = [
+                university,
+                course,
+                total_enrolled,
+                audit_enrolled,
+                honor_enrolled,
+                verified_enrolled,
+                gross_rev,
+                gross_rev_over_min,
+                number_of_refunds,
+                dollars_refunded
+            ]
+            yield result
 
-                results.append(result)
-        for item in results:
-            yield item
-
-    def csv_report_header_row(self):
+    def header(self):
         return [
             "University",
             "Course",
@@ -201,61 +199,35 @@ class UniversityRevenueShareReport(Report):
     """
     Subclass of Report, used to generate University Revenue Share Reports for finance purposes.
 
-    For each course in each university whose name is within the range start_letter and end_letter,
+    For each course in each university whose name is within the range start_word and end_word,
     inclusive, (i.e., the letter range H-J includes both Ithaca College and Harvard University), we calculate
     the total revenue generated by that particular course.  This includes the number of transactions,
     total payments collected, service fees, number of refunds, and total amount of refunds.
     """
-    def report_row_generator(self, start_date, end_date, start_letter=None, end_letter=None):
+    def rows(self, start_date, end_date, start_word=None, end_word=None):
         results = []
-        for course_id in settings.COURSE_LISTINGS['default']:
-            # If the first letter of the university is between start_letter and end_letter, then we include
-            # it in the report.  These comparisons are unicode-safe.
-            if (start_letter.lower() <= course_id.lower()) and (end_letter.lower() >= course_id.lower()):
-                try:
-                    cur_course = get_course_by_id(course_id)
-                except:
-                    break
-                university = cur_course.org
-                course = cur_course.number + " " + cur_course.display_name_with_default
-                num_transactions = 0  # TODO clarify with billing what transactions are included in this (purchases? refunds? etc)
+        for course_id in course_ids_between(start_word, end_word):
+            cur_course = get_course_by_id(course_id)
+            university = cur_course.org
+            course = cur_course.number + " " + cur_course.display_name_with_default
+            num_transactions = 0  # TODO clarify with billing what transactions are included in this (purchases? refunds? etc)
+            total_payments_collected = CertificateItem.verified_certificates_monetary_field_sum(course_id, 'purchased', 'unit_cost')
+            service_fees = CertificateItem.verified_certificates_monetary_field_sum(course_id, 'purchased', 'service_fee')
+            num_refunds = CertificateItem.verified_certificates_count(course_id, "refunded")
+            amount_refunds = CertificateItem.verified_certificates_monetary_field_sum(course_id, 'refunded', 'unit_cost')
 
-                all_paid_certs = CertificateItem.verified_certificates_in(course_id, "purchased")
-                try:
-                    total_payments_collected_temp = all_paid_certs.aggregate(Sum('unit_cost'))
-                    total_payments_collected = total_payments_collected_temp['unit_cost__sum']
-                except:
-                    total_payments_collected = Decimal(0.00)
-                try:
-                    total_service_fees_temp = all_paid_certs.aggregate(Sum('service_fee'))
-                    service_fees = total_service_fees_temp['service_fee__sum']
-                except:
-                    service_fees = Decimal(0.00)
+            result = [
+                university,
+                course,
+                num_transactions,
+                total_payments_collected,
+                service_fees,
+                num_refunds,
+                amount_refunds
+            ]
+            yield result
 
-                refunded_enrollments = CertificateItem.verified_certificates_in(course_id, "refunded")
-                num_refunds = refunded_enrollments.count()
-
-                amount_refunds_temp = refunded_enrollments.aggregate(Sum('unit_cost'))
-                if amount_refunds_temp['unit_cost__sum'] is None:
-                    amount_refunds = Decimal(0.00)
-                else:
-                    amount_refunds = amount_refunds_temp['unit_cost__sum']
-
-                result = [
-                    university,
-                    course,
-                    num_transactions,
-                    total_payments_collected,
-                    service_fees,
-                    num_refunds,
-                    amount_refunds
-                ]
-                results.append(result)
-
-        for item in results:
-            yield item
-
-    def csv_report_header_row(self):
+    def header(self):
         return [
             "University",
             "Course",
@@ -265,3 +237,14 @@ class UniversityRevenueShareReport(Report):
             "Number of Successful Refunds",
             "Total Amount of Refunds",
         ]
+
+def course_ids_between(start_word, end_word):
+    """ 
+    Returns a list of all valid course_ids that fall alphabetically between start_word and end_word.
+    These comparisons are unicode-safe. 
+    """
+    valid_courses = []
+    for course_id in settings.COURSE_LISTINGS['default']:
+        if (start_word.lower() <= course_id.lower()) and (end_word.lower() >= course_id.lower()) and (get_course_by_id(course_id) is not None):
+            valid_courses.append(course_id)
+    return valid_courses

lms/djangoapps/shoppingcart/tests/test_models.py

@@ -356,26 +356,25 @@ class ItemizedPurchaseReportTest(ModuleStoreTestCase):
         self.cart.purchase()
         self.now = datetime.datetime.now(pytz.UTC)
 
-        # We can't modify the values returned by report_row_generator directly, since it's a generator, but
-        # we need the times on CORRECT_CSV and the generated report to match.  So, we extract the times from
-        # the report_row_generator and place them in CORRECT_CSV.
-        self.time_str = {}
-        report = initialize_report("itemized_purchase_report")
-        purchases = report.report_row_generator(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
-        num_of_item = 0
-        for item in purchases:
-            num_of_item += 1
-            self.time_str[num_of_item] = item[0]
+        paid_reg = PaidCourseRegistration.objects.get(course_id=self.course_id, user=self.user)
+        paid_reg.fulfilled_time = self.now
+        paid_reg.refund_requested_time = self.now
+        paid_reg.save()
+
+        cert = CertificateItem.objects.get(course_id=self.course_id, user=self.user)
+        cert.fulfilled_time = self.now
+        cert.refund_requested_time = self.now
+        cert.save()
 
         self.CORRECT_CSV = dedent("""
             Purchase Time,Order ID,Status,Quantity,Unit Cost,Total Cost,Currency,Description,Comments
-            {time_str1},1,purchased,1,40,40,usd,Registration for Course: Robot Super Course,Ba\xc3\xbc\xe5\x8c\x85
-            {time_str2},1,purchased,1,40,40,usd,"Certificate of Achievement, verified cert for course Robot Super Course",
-            """.format(time_str1=str(self.time_str[1]), time_str2=str(self.time_str[2])))
+            {time_str},1,purchased,1,40,40,usd,Registration for Course: Robot Super Course,Ba\xc3\xbc\xe5\x8c\x85
+            {time_str},1,purchased,1,40,40,usd,"Certificate of Achievement, verified cert for course Robot Super Course",
+            """.format(time_str=str(self.now)))
 
     def test_purchased_items_btw_dates(self):
         report = initialize_report("itemized_purchase_report")
-        purchases = report.report_row_generator(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
+        purchases = report.rows(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
 
         # since there's not many purchases, just run through the generator to make sure we've got the right number
         num_purchases = 0
@@ -385,7 +384,7 @@ class ItemizedPurchaseReportTest(ModuleStoreTestCase):
         #self.assertIn(self.reg.orderitem_ptr, purchases)
         #self.assertIn(self.cert_item.orderitem_ptr, purchases)
 
-        no_purchases = report.report_row_generator(self.now + self.FIVE_MINS, self.now + self.FIVE_MINS + self.FIVE_MINS)
+        no_purchases = report.rows(self.now + self.FIVE_MINS, self.now + self.FIVE_MINS + self.FIVE_MINS)
 
         num_purchases = 0
         for item in no_purchases:
@@ -398,7 +397,7 @@ class ItemizedPurchaseReportTest(ModuleStoreTestCase):
         """
         report = initialize_report("itemized_purchase_report")
         csv_file = StringIO.StringIO()
-        report.make_report(csv_file, self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
+        report.write_csv(csv_file, self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
         csv = csv_file.getvalue()
         csv_file.close()
         # Using excel mode csv, which automatically ends lines with \r\n, so need to convert to \n

lms/djangoapps/shoppingcart/tests/test_reports.py

@@ -3,20 +3,21 @@ Tests for the Shopping Cart Models
 """
 import StringIO
 from textwrap import dedent
+import pytz
+import datetime
 
 from django.conf import settings
 from django.test.utils import override_settings
-from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
-from xmodule.modulestore.tests.factories import CourseFactory
+
+from course_modes.models import CourseMode
 from courseware.tests.tests import TEST_DATA_MONGO_MODULESTORE
 from shoppingcart.models import (Order, CertificateItem)
 from shoppingcart.reports import ItemizedPurchaseReport, CertificateStatusReport, UniversityRevenueShareReport, RefundReport
+from shoppingcart.views import initialize_report, REPORT_TYPES
 from student.tests.factories import UserFactory
 from student.models import CourseEnrollment
-from course_modes.models import CourseMode
-from shoppingcart.views import initialize_report, REPORT_TYPES
-import pytz
-import datetime
+from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
+from xmodule.modulestore.tests.factories import CourseFactory
 
 
 @override_settings(MODULESTORE=TEST_DATA_MONGO_MODULESTORE)
@@ -28,37 +29,37 @@ class ReportTypeTests(ModuleStoreTestCase):
 
     def setUp(self):
         # Need to make a *lot* of users for this one
-        self.user1 = UserFactory.create()
-        self.user1.profile.name = "John Doe"
-        self.user1.profile.save()
+        self.first_verified_user = UserFactory.create()
+        self.first_verified_user.profile.name = "John Doe"
+        self.first_verified_user.profile.save()
 
-        self.user2 = UserFactory.create()
-        self.user2.profile.name = "Jane Deer"
-        self.user2.profile.save()
+        self.second_verified_user = UserFactory.create()
+        self.second_verified_user.profile.name = "Jane Deer"
+        self.second_verified_user.profile.save()
 
-        self.user3 = UserFactory.create()
-        self.user3.profile.name = "Joe Miller"
-        self.user3.profile.save()
+        self.first_audit_user = UserFactory.create()
+        self.first_audit_user.profile.name = "Joe Miller"
+        self.first_audit_user.profile.save()
 
-        self.user4 = UserFactory.create()
-        self.user4.profile.name = "Simon Blackquill"
-        self.user4.profile.save()
+        self.second_audit_user = UserFactory.create()
+        self.second_audit_user.profile.name = "Simon Blackquill"
+        self.second_audit_user.profile.save()
 
-        self.user5 = UserFactory.create()
-        self.user5.profile.name = "Super Mario"
-        self.user5.profile.save()
+        self.third_audit_user = UserFactory.create()
+        self.third_audit_user.profile.name = "Super Mario"
+        self.third_audit_user.profile.save()
 
-        self.user6 = UserFactory.create()
-        self.user6.profile.name = "Princess Peach"
-        self.user6.profile.save()
+        self.honor_user = UserFactory.create()
+        self.honor_user.profile.name = "Princess Peach"
+        self.honor_user.profile.save()
 
-        self.user7 = UserFactory.create()
-        self.user7.profile.name = "King Bowser"
-        self.user7.profile.save()
+        self.first_refund_user = UserFactory.create()
+        self.first_refund_user.profile.name = "King Bowser"
+        self.first_refund_user.profile.save()
 
-        self.user8 = UserFactory.create()
-        self.user8.profile.name = "Susan Smith"
-        self.user8.profile.save()
+        self.second_refund_user = UserFactory.create()
+        self.second_refund_user.profile.name = "Susan Smith"
+        self.second_refund_user.profile.save()
 
         # Two are verified, three are audit, one honor
 
@@ -79,55 +80,53 @@ class ReportTypeTests(ModuleStoreTestCase):
         course_mode2.save()
 
         # User 1 & 2 will be verified
-        self.cart1 = Order.get_cart_for_user(self.user1)
+        self.cart1 = Order.get_cart_for_user(self.first_verified_user)
         CertificateItem.add_to_order(self.cart1, self.course_id, self.cost, 'verified')
         self.cart1.purchase()
 
-        self.cart2 = Order.get_cart_for_user(self.user2)
+        self.cart2 = Order.get_cart_for_user(self.second_verified_user)
         CertificateItem.add_to_order(self.cart2, self.course_id, self.cost, 'verified')
         self.cart2.purchase()
 
         # Users 3, 4, and 5 are audit
-        CourseEnrollment.enroll(self.user3, self.course_id, "audit")
-        CourseEnrollment.enroll(self.user4, self.course_id, "audit")
-        CourseEnrollment.enroll(self.user5, self.course_id, "audit")
+        CourseEnrollment.enroll(self.first_audit_user, self.course_id, "audit")
+        CourseEnrollment.enroll(self.second_audit_user, self.course_id, "audit")
+        CourseEnrollment.enroll(self.third_audit_user, self.course_id, "audit")
 
         # User 6 is honor
-        CourseEnrollment.enroll(self.user6, self.course_id, "honor")
+        CourseEnrollment.enroll(self.honor_user, self.course_id, "honor")
 
         self.now = datetime.datetime.now(pytz.UTC)
 
         # Users 7 & 8 are refunds
-        self.cart = Order.get_cart_for_user(self.user7)
+        self.cart = Order.get_cart_for_user(self.first_refund_user)
         CertificateItem.add_to_order(self.cart, self.course_id, self.cost, 'verified')
         self.cart.purchase()
-        CourseEnrollment.unenroll(self.user7, self.course_id)
+        CourseEnrollment.unenroll(self.first_refund_user, self.course_id)
 
-        self.cart = Order.get_cart_for_user(self.user8)
+        self.cart = Order.get_cart_for_user(self.second_refund_user)
         CertificateItem.add_to_order(self.cart, self.course_id, self.cost, 'verified')
-        self.cart.purchase(self.user8, self.course_id)
-        CourseEnrollment.unenroll(self.user8, self.course_id)
-
-        # We can't modify the values returned by report_row_generator directly, since it's a generator, but
-        # we need the times on CORRECT_CSV and the generated report to match.  So, we extract the times from
-        # the report_row_generator and place them in CORRECT_CSV.
-        self.time_str = {}
-        report = initialize_report("refund_report")
-        refunds = report.report_row_generator(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
-        time_index = 0
-        for item in refunds:
-            self.time_str[time_index] = item[2]
-            time_index += 1
-            self.time_str[time_index] = item[3]
-            time_index += 1
-        self.CORRECT_REFUND_REPORT_CSV = dedent("""
-            Order Number,Customer Name,Date of Original Transaction,Date of Refund,Amount of Refund,Service Fees (if any)
-            3,King Bowser,{time_str0},{time_str1},40,0
-            4,Susan Smith,{time_str2},{time_str3},40,0
-            """.format(time_str0=str(self.time_str[0]), time_str1=str(self.time_str[1]), time_str2=str(self.time_str[2]), time_str3=str(self.time_str[3])))
+        self.cart.purchase(self.second_refund_user, self.course_id)
+        CourseEnrollment.unenroll(self.second_refund_user, self.course_id)
 
         self.test_time = datetime.datetime.now(pytz.UTC)
 
+        first_refund = CertificateItem.objects.get(id=3)
+        first_refund.fulfilled_time = self.test_time
+        first_refund.refund_requested_time = self.test_time
+        first_refund.save()
+
+        second_refund = CertificateItem.objects.get(id=4)
+        second_refund.fulfilled_time = self.test_time
+        second_refund.refund_requested_time = self.test_time
+        second_refund.save()
+
+        self.CORRECT_REFUND_REPORT_CSV = dedent("""
+            Order Number,Customer Name,Date of Original Transaction,Date of Refund,Amount of Refund,Service Fees (if any)
+            3,King Bowser,{time_str},{time_str},40,0
+            4,Susan Smith,{time_str},{time_str},40,0
+            """.format(time_str=str(self.test_time)))
+
         self.CORRECT_CERT_STATUS_CSV = dedent("""
             University,Course,Total Enrolled,Audit Enrollment,Honor Code Enrollment,Verified Enrollment,Gross Revenue,Gross Revenue over the Minimum,Number of Refunds,Dollars Refunded
             MITx,999 Robot Super Course,6,3,1,2,80.00,0.00,2,80.00
@@ -138,9 +137,9 @@ class ReportTypeTests(ModuleStoreTestCase):
             MITx,999 Robot Super Course,0,80.00,0.00,2,80.00
             """.format(time_str=str(self.test_time)))
 
-    def test_refund_report_report_row_generator(self):
+    def test_refund_report_rows(self):
         report = initialize_report("refund_report")
-        refunded_certs = report.report_row_generator(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
+        refunded_certs = report.rows(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
 
         # check that we have the right number
         num_certs = 0
@@ -148,8 +147,8 @@ class ReportTypeTests(ModuleStoreTestCase):
             num_certs += 1
         self.assertEqual(num_certs, 2)
 
-        self.assertTrue(CertificateItem.objects.get(user=self.user7, course_id=self.course_id))
-        self.assertTrue(CertificateItem.objects.get(user=self.user8, course_id=self.course_id))
+        self.assertTrue(CertificateItem.objects.get(user=self.first_refund_user, course_id=self.course_id))
+        self.assertTrue(CertificateItem.objects.get(user=self.second_refund_user, course_id=self.course_id))
 
     def test_refund_report_purchased_csv(self):
         """
@@ -157,7 +156,7 @@ class ReportTypeTests(ModuleStoreTestCase):
         """
         report = initialize_report("refund_report")
         csv_file = StringIO.StringIO()
-        report.make_report(csv_file, self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
+        report.write_csv(csv_file, self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
         csv = csv_file.getvalue()
         csv_file.close()
         # Using excel mode csv, which automatically ends lines with \r\n, so need to convert to \n
@@ -166,13 +165,13 @@ class ReportTypeTests(ModuleStoreTestCase):
     def test_basic_cert_status_csv(self):
         report = initialize_report("certificate_status")
         csv_file = StringIO.StringIO()
-        report.make_report(csv_file, self.now - self.FIVE_MINS, self.now + self.FIVE_MINS, 'A', 'Z')
+        report.write_csv(csv_file, self.now - self.FIVE_MINS, self.now + self.FIVE_MINS, 'A', 'Z')
         csv = csv_file.getvalue()
         self.assertEqual(csv.replace('\r\n', '\n').strip(), self.CORRECT_CERT_STATUS_CSV.strip())
 
     def test_basic_uni_revenue_share_csv(self):
         report = initialize_report("university_revenue_share")
         csv_file = StringIO.StringIO()
-        report.make_report(csv_file, self.now - self.FIVE_MINS, self.now + self.FIVE_MINS, 'A', 'Z')
+        report.write_csv(csv_file, self.now - self.FIVE_MINS, self.now + self.FIVE_MINS, 'A', 'Z')
         csv = csv_file.getvalue()
         self.assertEqual(csv.replace('\r\n', '\n').strip(), self.CORRECT_UNI_REVENUE_SHARE_CSV.strip())

lms/djangoapps/shoppingcart/tests/test_views.py

@@ -365,26 +365,6 @@ class CSVReportViewsTest(ModuleStoreTestCase):
         self.assertIn(_("There was an error in your date input.  It should be formatted as YYYY-MM-DD"),
                       response.content)
 
-    @patch('shoppingcart.views.render_to_response', render_mock)
-    @override_settings(PAYMENT_REPORT_MAX_ITEMS=0)
-    def test_report_csv_too_long(self):
-        PaidCourseRegistration.add_to_order(self.cart, self.course_id)
-        self.cart.purchase()
-        self.login_user()
-        self.add_to_download_group(self.user)
-        response = self.client.post(reverse('payment_csv_report'), {'start_date': '1970-01-01',
-                                                                    'end_date': '2100-01-01',
-                                                                    'requested_report': 'itemized_purchase_report'})
-
-        ((template, context), unused_kwargs) = render_mock.call_args
-        self.assertEqual(template, 'shoppingcart/download_report.html')
-        self.assertTrue(context['total_count_error'])
-        self.assertFalse(context['date_fmt_error'])
-        self.assertIn(_("There are too many results in your report.") + " (>0)", response.content)
-
-    # just going to ignored the date in this test, since we already deal with date testing
-    # in test_models.py
-
     CORRECT_CSV_NO_DATE_ITEMIZED_PURCHASE = ",1,purchased,1,40,40,usd,Registration for Course: Robot Super Course,"
 
     def test_report_csv_itemized(self):
@@ -398,7 +378,7 @@ class CSVReportViewsTest(ModuleStoreTestCase):
                                                                     'requested_report': report_type})
         self.assertEqual(response['Content-Type'], 'text/csv')
         report = initialize_report(report_type)
-        self.assertIn(",".join(report.csv_report_header_row()), response.content)
+        self.assertIn(",".join(report.header()), response.content)
         self.assertIn(self.CORRECT_CSV_NO_DATE_ITEMIZED_PURCHASE, response.content)
 
     def test_report_csv_university_revenue_share(self):
@@ -412,7 +392,7 @@ class CSVReportViewsTest(ModuleStoreTestCase):
                                                                     'requested_report': report_type})
         self.assertEqual(response['Content-Type'], 'text/csv')
         report = initialize_report(report_type)
-        self.assertIn(",".join(report.csv_report_header_row()), response.content)
+        self.assertIn(",".join(report.header()), response.content)
         # TODO add another test here
 
 

lms/djangoapps/shoppingcart/views.py

@@ -11,11 +11,11 @@ from django.core.urlresolvers import reverse
 from django.views.decorators.csrf import csrf_exempt
 from django.contrib.auth.decorators import login_required
 from edxmako.shortcuts import render_to_response
-from student.models import CourseEnrollment
 from shoppingcart.reports import RefundReport, ItemizedPurchaseReport, UniversityRevenueShareReport, CertificateStatusReport
+from student.models import CourseEnrollment
+from .exceptions import ItemAlreadyInCartException, AlreadyEnrolledInCourseException, CourseDoesNotExistException, ReportTypeDoesNotExistException
 from .models import Order, PaidCourseRegistration, OrderItem
 from .processors import process_postpay_callback, render_purchase_form_html
-from .exceptions import ItemAlreadyInCartException, AlreadyEnrolledInCourseException, CourseDoesNotExistException, ReportTypeDoesNotExistException
 
 log = logging.getLogger("shoppingcart")
 
@@ -213,20 +213,12 @@ def csv_report(request):
             return _render_report_form(start_str, end_str, start_letter, end_letter, report_type, date_fmt_error=True)
 
         report = initialize_report(report_type)
-        items = report.report_row_generator(start_date, end_date, start_letter, end_letter)
-
-        # TODO add this back later as a query-est function or something
-        try:
-            if items.count() > settings.PAYMENT_REPORT_MAX_ITEMS:
-            # Error case: too many items would be generated in the report and we're at risk of timeout
-                return _render_report_form(start_str, end_str, start_letter, end_letter, report_type, total_count_error=True)
-        except:
-            pass
+        items = report.rows(start_date, end_date, start_letter, end_letter)
 
         response = HttpResponse(mimetype='text/csv')
         filename = "purchases_report_{}.csv".format(datetime.datetime.now(pytz.UTC).strftime("%Y-%m-%d-%H-%M-%S"))
         response['Content-Disposition'] = 'attachment; filename="{}"'.format(filename)
-        report.make_report(response, start_date, end_date, start_letter, end_letter)
+        report.write_csv(response, start_date, end_date, start_letter, end_letter)
         return response
 
     elif request.method == 'GET':

lms/envs/aws.py

@@ -170,7 +170,6 @@ PAID_COURSE_REGISTRATION_CURRENCY = ENV_TOKENS.get('PAID_COURSE_REGISTRATION_CUR
 
 # Payment Report Settings
 PAYMENT_REPORT_GENERATOR_GROUP = ENV_TOKENS.get('PAYMENT_REPORT_GENERATOR_GROUP', PAYMENT_REPORT_GENERATOR_GROUP)
-PAYMENT_REPORT_MAX_ITEMS = ENV_TOKENS.get('PAYMENT_REPORT_MAX_ITEMS', PAYMENT_REPORT_MAX_ITEMS)
 
 # Bulk Email overrides
 BULK_EMAIL_DEFAULT_FROM_EMAIL = ENV_TOKENS.get('BULK_EMAIL_DEFAULT_FROM_EMAIL', BULK_EMAIL_DEFAULT_FROM_EMAIL)

lms/envs/common.py

@@ -555,8 +555,6 @@ PAID_COURSE_REGISTRATION_CURRENCY = ['usd', '$']
 
 # Members of this group are allowed to generate payment reports
 PAYMENT_REPORT_GENERATOR_GROUP = 'shoppingcart_report_access'
-# Maximum number of rows the report can contain
-PAYMENT_REPORT_MAX_ITEMS = 10000
 
 ################################# open ended grading config  #####################
 

lms/templates/shoppingcart/download_report.html

@@ -12,12 +12,6 @@
       ${_("There was an error in your date input.  It should be formatted as YYYY-MM-DD")}
     </section>
   % endif
-  % if total_count_error:
-    <section class="error_msg">
-      ${_("There are too many results in your report.")} (>${settings.PAYMENT_REPORT_MAX_ITEMS}).
-      ${_("Try making the date range smaller.")}
-    </section>
-  % endif
   <form method="post">
     <p>${_("These reports are delimited by start and end dates.")}</p>
     <label for="start_date">${_("Start Date: ")}</label>