Response to CR

common/djangoapps/student/models.py

@@ -581,9 +581,15 @@ class CourseEnrollment(models.Model):
         )
 
     @classmethod
-    def enrollments_in(cls, course_id):
-        """Return a queryset of CourseEnrollment for every active enrollment in the course."""
-        return cls.objects.filter(course_id=course_id, is_active=True,)
+    def enrollments_in(cls, course_id, mode=None):
+        """
+        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.
+        """
+        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)
 
     def activate(self):
         """Makes this `CourseEnrollment` record active. Saves immediately."""

lms/djangoapps/shoppingcart/models.py

@@ -205,13 +205,13 @@ class OrderItem(models.Model):
     # this is denormalized, but convenient for SQL queries for reports, etc. user should always be = order.user
     user = models.ForeignKey(User, db_index=True)
     # this is denormalized, but convenient for SQL queries for reports, etc. status should always be = order.status
-    status = models.CharField(max_length=32, default='cart', choices=ORDER_STATUSES)
+    status = models.CharField(max_length=32, default='cart', choices=ORDER_STATUSES, db_index=True)
     qty = models.IntegerField(default=1)
     unit_cost = models.DecimalField(default=0.0, decimal_places=2, max_digits=30)
     line_desc = models.CharField(default="Misc. Item", max_length=1024)
     currency = models.CharField(default="usd", max_length=8)  # lower case ISO currency codes
-    fulfilled_time = models.DateTimeField(null=True)
-    refund_requested_time = models.DateTimeField(null=True)
+    fulfilled_time = models.DateTimeField(null=True, db_index=True)
+    refund_requested_time = models.DateTimeField(null=True, db_index=True)
     service_fee = models.DecimalField(default=0.0, decimal_places=2, max_digits=30)
     # general purpose field, not user-visible.  Used for reporting
     report_comments = models.TextField(default="")
@@ -568,3 +568,8 @@ class CertificateItem(OrderItem):
                  "Please include your order number in your e-mail. "
                  "Please do NOT include your credit card information.").format(
                      billing_email=settings.PAYMENT_SUPPORT_EMAIL)
+
+    @classmethod
+    def verified_certificates_in(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)

lms/djangoapps/shoppingcart/reports.py

@@ -9,31 +9,25 @@ from course_modes.models import CourseMode
 from decimal import Decimal
 
 
-class Report(models.Model):
+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 get_report_data, csv_report_header_row, and csv_report_row.
+    the methods report_row_generator and csv_report_header_row.
     """
 
-    def get_report_data(self, start_date, end_date, start_letter=None, end_letter=None):
+    def report_row_generator(self, start_date, end_date, start_letter=None, end_letter=None):
         """
-        Performs database queries necessary for the report.  May return either a query result
-        or a list of lists, depending on the particular type of report--see Report subclasses
-        for sample implementations.
+        Performs database queries necessary for the report.  Returns an generator of
+        lists, in which each list is a separate row of the report.
         """
         raise NotImplementedError
 
     def csv_report_header_row(self):
         """
-        Returns the appropriate header based on the report type.
-        """
-        raise NotImplementedError
-
-    def csv_report_row(self, item):
-        """
-        Given the results of get_report_data, this function generates a single row of a csv.
+        Returns the appropriate header based on the report type, in the form of a
+        list of strings.
         """
         raise NotImplementedError
 
@@ -42,23 +36,36 @@ class Report(models.Model):
         Given the string report_type, a file object to write to, and start/end date bounds,
         generates a CSV report of the appropriate type.
         """
-        items = self.get_report_data(start_date, end_date, start_letter, end_letter)
+        items = self.report_row_generator(start_date, end_date, start_letter, end_letter)
         writer = unicodecsv.writer(filelike, encoding="utf-8")
         writer.writerow(self.csv_report_header_row())
         for item in items:
-            writer.writerow(self.csv_report_row(item))
+            writer.writerow(item)
 
 
 class RefundReport(Report):
     """
     Subclass of Report, used to generate Refund Reports for finance purposes.
+
+    For each refund between a given start_date and end_date, we find the relevant
+    order number, customer name, date of transaction, date of refund, and any service
+    fees.
     """
-    def get_report_data(self, start_date, end_date, start_letter=None, end_letter=None):
-        return CertificateItem.objects.filter(
+    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')
+        for item in query:
+            yield [
+                item.order_id,
+                item.user.profile.name,
+                item.fulfilled_time,
+                item.refund_requested_time,
+                item.line_cost,
+                item.service_fee,
+            ]
 
     def csv_report_header_row(self):
         return [
@@ -70,28 +77,35 @@ class RefundReport(Report):
             "Service Fees (if any)",
         ]
 
-    def csv_report_row(self, item):
-        return [
-            item.order_id,
-            item.user.get_full_name(),
-            item.fulfilled_time,
-            item.refund_requested_time,  # TODO Change this torefund_fulfilled once we start recording that value
-            item.line_cost,
-            item.service_fee,
-        ]
-
 
 class ItemizedPurchaseReport(Report):
     """
     Subclass of Report, used to generate itemized purchase reports.
+
+    For all purchases (verified certificates, paid course registrations, etc) between
+    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 get_report_data(self, start_date, end_date, start_letter=None, end_letter=None):
-        return OrderItem.objects.filter(
+    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")
 
+        for item in query:
+            yield [
+                item.fulfilled_time,
+                item.order_id,  # pylint: disable=no-member
+                item.status,
+                item.qty,
+                item.unit_cost,
+                item.line_cost,
+                item.currency,
+                item.line_desc,
+                item.report_comments,
+            ]
+
     def csv_report_header_row(self):
         return [
             "Purchase Time",
@@ -105,47 +119,50 @@ class ItemizedPurchaseReport(Report):
             "Comments"
         ]
 
-    def csv_report_row(self, item):
-        return [
-            item.fulfilled_time,
-            item.order_id,  # pylint: disable=no-member
-            item.status,
-            item.qty,
-            item.unit_cost,
-            item.line_cost,
-            item.currency,
-            item.line_desc,
-            item.report_comments,
-        ]
-
 
 class CertificateStatusReport(Report):
     """
-    Subclass of Report, used to generate Certificate Status Reports for ed services.
+    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,
+    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 get_report_data(self, start_date, end_date, start_letter=None, end_letter=None):
+    def report_row_generator(self, start_date, end_date, start_letter=None, end_letter=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()) 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  # TODO add term (i.e. Fall 2013)?
+                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 = enrollments.filter(mode="audit").count()
-                honor_enrolled = enrollments.filter(mode="honor").count()
+                audit_enrolled = CourseEnrollment.enrollments_in(course_id, "audit").count()
+                honor_enrolled = CourseEnrollment.enrollments_in(course_id, "honor").count()
+
                 # Since every verified enrollment has 1 and only 1 cert item, let's just query those
-                verified_enrollments = CertificateItem.objects.filter(course_id=course_id, mode="verified", status="purchased")
-                verified_enrolled = verified_enrollments.count()
-                gross_rev_temp = CertificateItem.objects.filter(course_id=course_id, mode="verified", status="purchased").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)
-                refunded_enrollments = CertificateItem.objects.filter(course_id='course_id', mode="verified", status="refunded")
-                number_of_refunds = refunded_enrollments.count()
-                dollars_refunded_temp = refunded_enrollments.aggregate(Sum('unit_cost'))
-                if dollars_refunded_temp['unit_cost__sum'] is None:
+                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)
+
+                # 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']
 
                 result = [
@@ -162,7 +179,8 @@ class CertificateStatusReport(Report):
                 ]
 
                 results.append(result)
-        return results
+        for item in results:
+            yield item
 
     def csv_report_header_row(self):
         return [
@@ -178,41 +196,43 @@ class CertificateStatusReport(Report):
             "Dollars Refunded",
         ]
 
-    def csv_report_row(self, item):
-        return item
-
 
 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,
+    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 get_report_data(self, start_date, end_date, start_letter=None, end_letter=None):
+    def report_row_generator(self, start_date, end_date, start_letter=None, end_letter=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
+                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)
 
-                all_paid_certs = CertificateItem.objects.filter(course_id=course_id, status="purchased")
-
-                total_payments_collected_temp = all_paid_certs.aggregate(Sum('unit_cost'))
-                if total_payments_collected_temp['unit_cost__sum'] is None:
-                    total_payments_collected = Decimal(0.00)
-                else:
+                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']
-
-                total_service_fees_temp = all_paid_certs.aggregate(Sum('service_fee'))
-                if total_service_fees_temp['service_fee__sum'] is None:
-                    service_fees = Decimal(0.00)
-                else:
+                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)
 
-                refunded_enrollments = CertificateItem.objects.filter(course_id=course_id, status="refunded")
+                refunded_enrollments = CertificateItem.verified_certificates_in(course_id, "refunded")
                 num_refunds = refunded_enrollments.count()
 
                 amount_refunds_temp = refunded_enrollments.aggregate(Sum('unit_cost'))
@@ -232,7 +252,8 @@ class UniversityRevenueShareReport(Report):
                 ]
                 results.append(result)
 
-        return results
+        for item in results:
+            yield item
 
     def csv_report_header_row(self):
         return [
@@ -244,6 +265,3 @@ class UniversityRevenueShareReport(Report):
             "Number of Successful Refunds",
             "Total Amount of Refunds",
         ]
-
-    def csv_report_row(self, item):
-        return item

lms/djangoapps/shoppingcart/tests/test_models.py

@@ -356,32 +356,47 @@ 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]
+
+        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])))
+
     def test_purchased_items_btw_dates(self):
         report = initialize_report("itemized_purchase_report")
-        purchases = report.get_report_data(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
-        self.assertEqual(len(purchases), 2)
-        self.assertIn(self.reg.orderitem_ptr, purchases)
-        self.assertIn(self.cert_item.orderitem_ptr, purchases)
-        no_purchases = report.get_report_data(self.now + self.FIVE_MINS, self.now + self.FIVE_MINS + self.FIVE_MINS)
-        self.assertFalse(no_purchases)
+        purchases = report.report_row_generator(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
 
-    test_time = datetime.datetime.now(pytz.UTC)
+        # since there's not many purchases, just run through the generator to make sure we've got the right number
+        num_purchases = 0
+        for item in purchases:
+            num_purchases += 1
+        self.assertEqual(num_purchases, 2)
+        #self.assertIn(self.reg.orderitem_ptr, purchases)
+        #self.assertIn(self.cert_item.orderitem_ptr, purchases)
 
-    CORRECT_CSV = dedent("""
-        Purchase Time,Order ID,Status,Quantity,Unit Cost,Total Cost,Currency,Description,Comments
-        {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(test_time)))
+        no_purchases = report.report_row_generator(self.now + self.FIVE_MINS, self.now + self.FIVE_MINS + self.FIVE_MINS)
+
+        num_purchases = 0
+        for item in no_purchases:
+            num_purchases +=1
+        self.assertEqual(num_purchases, 0)
 
     def test_purchased_csv(self):
         """
         Tests that a generated purchase report CSV is as we expect
         """
         report = initialize_report("itemized_purchase_report")
-        for item in report.get_report_data(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS):
-            item.fulfilled_time = self.test_time
-            item.save()
-
         csv_file = StringIO.StringIO()
         report.make_report(csv_file, self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
         csv = csv_file.getvalue()

lms/djangoapps/shoppingcart/tests/test_reports.py

@@ -29,44 +29,36 @@ class ReportTypeTests(ModuleStoreTestCase):
     def setUp(self):
         # Need to make a *lot* of users for this one
         self.user1 = UserFactory.create()
-        self.user1.first_name = "John"
-        self.user1.last_name = "Doe"
-        self.user1.save()
+        self.user1.profile.name = "John Doe"
+        self.user1.profile.save()
 
         self.user2 = UserFactory.create()
-        self.user2.first_name = "Jane"
-        self.user2.last_name = "Deer"
-        self.user2.save()
+        self.user2.profile.name = "Jane Deer"
+        self.user2.profile.save()
 
         self.user3 = UserFactory.create()
-        self.user3.first_name = "Joe"
-        self.user3.last_name = "Miller"
-        self.user3.save()
+        self.user3.profile.name = "Joe Miller"
+        self.user3.profile.save()
 
         self.user4 = UserFactory.create()
-        self.user4.first_name = "Simon"
-        self.user4.last_name = "Blackquill"
-        self.user4.save()
+        self.user4.profile.name = "Simon Blackquill"
+        self.user4.profile.save()
 
         self.user5 = UserFactory.create()
-        self.user5.first_name = "Super"
-        self.user5.last_name = "Mario"
-        self.user5.save()
+        self.user5.profile.name = "Super Mario"
+        self.user5.profile.save()
 
         self.user6 = UserFactory.create()
-        self.user6.first_name = "Princess"
-        self.user6.last_name = "Peach"
-        self.user6.save()
+        self.user6.profile.name = "Princess Peach"
+        self.user6.profile.save()
 
         self.user7 = UserFactory.create()
-        self.user7.first_name = "King"
-        self.user7.last_name = "Bowser"
-        self.user7.save()
+        self.user7.profile.name = "King Bowser"
+        self.user7.profile.save()
 
         self.user8 = UserFactory.create()
-        self.user8.first_name = "Susan"
-        self.user8.last_name = "Smith"
-        self.user8.save()
+        self.user8.profile.name = "Susan Smith"
+        self.user8.profile.save()
 
         # Two are verified, three are audit, one honor
 
@@ -116,16 +108,29 @@ class ReportTypeTests(ModuleStoreTestCase):
         self.cart.purchase(self.user8, self.course_id)
         CourseEnrollment.unenroll(self.user8, self.course_id)
 
-        self.test_time = 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("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_str},{time_str},40,0
-            4,Susan Smith,{time_str},{time_str},40,0
-            """.format(time_str=str(self.test_time)))
+            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.test_time = datetime.datetime.now(pytz.UTC)
 
         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,0,0
+            MITx,999 Robot Super Course,6,3,1,2,80.00,0.00,2,80.00
             """.format(time_str=str(self.test_time)))
 
         self.CORRECT_UNI_REVENUE_SHARE_CSV = dedent("""
@@ -133,10 +138,16 @@ 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_get_report_data(self):
+    def test_refund_report_report_row_generator(self):
         report = initialize_report("refund_report")
-        refunded_certs = report.get_report_data(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
-        self.assertEqual(len(refunded_certs), 2)
+        refunded_certs = report.report_row_generator(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
+
+        # check that we have the right number
+        num_certs = 0
+        for cert in refunded_certs:
+            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))
 
@@ -145,11 +156,6 @@ class ReportTypeTests(ModuleStoreTestCase):
         Tests that a generated purchase report CSV is as we expect
         """
         report = initialize_report("refund_report")
-        for item in report.get_report_data(self.now - self.FIVE_MINS, self.now + self.FIVE_MINS):
-            item.fulfilled_time = self.test_time
-            item.refund_requested_time = self.test_time  # hm do we want to make these different
-            item.save()
-
         csv_file = StringIO.StringIO()
         report.make_report(csv_file, self.now - self.FIVE_MINS, self.now + self.FIVE_MINS)
         csv = csv_file.getvalue()

lms/djangoapps/shoppingcart/views.py

@@ -213,7 +213,7 @@ 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.get_report_data(start_date, end_date, start_letter, end_letter)
+        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: