Fix & cleanup course run seats in publisher

course_discovery/apps/publisher/constants.py

@@ -17,3 +17,4 @@ PARTNER_COORDINATOR_GROUP_NAME = 'Partner Coordinators'
 
 # Waffle flags
 PUBLISHER_ENTITLEMENTS_WAFFLE_SWITCH = 'publisher_entitlements'
+PUBLISHER_CREATE_AUDIT_SEATS_FOR_VERIFIED_COURSE_RUNS = 'publisher_create_audit_seats_for_verified_course_runs'

course_discovery/apps/publisher/forms.py

@@ -15,6 +15,7 @@ from course_discovery.apps.course_metadata.choices import CourseRunPacing
 from course_discovery.apps.course_metadata.models import LevelType, Organization, Person, Subject
 from course_discovery.apps.ietf_language_tags.models import LanguageTag
 from course_discovery.apps.publisher.choices import CourseRunStateChoices, PublisherUserRole
+from course_discovery.apps.publisher.constants import PUBLISHER_CREATE_AUDIT_SEATS_FOR_VERIFIED_COURSE_RUNS
 from course_discovery.apps.publisher.mixins import LanguageModelSelect2Multiple, get_user_organizations
 from course_discovery.apps.publisher.models import (
     Course, CourseEntitlement, CourseRun, CourseRunState, CourseState, CourseUserRole, OrganizationExtension,
@@ -392,6 +393,10 @@ class SeatForm(BaseForm):
         model = Seat
 
     def save(self, commit=True, course_run=None, changed_by=None):  # pylint: disable=arguments-differ
+        """Saves the seat for a particular course run."""
+        # Background -- EDUCATOR-2337 (Should be removed once the data is consistent)
+        self.validate_and_remove_seats(course_run)
+
         # When seat is save make sure its prices and others fields updated accordingly.
         seat = super(SeatForm, self).save(commit=False)
         if seat.type in [Seat.HONOR, Seat.AUDIT]:
@@ -413,12 +418,12 @@ class SeatForm(BaseForm):
         if commit:
             seat.save()
 
-            if waffle.switch_is_active('publisher_create_audit_seats_for_verified_course_runs'):
+            if waffle.switch_is_active(PUBLISHER_CREATE_AUDIT_SEATS_FOR_VERIFIED_COURSE_RUNS):
                 course_run = seat.course_run
                 audit_seats = course_run.seats.filter(type=Seat.AUDIT)
 
                 # Ensure that course runs with a verified seat always have an audit seat
-                if seat.type in (Seat.CREDIT, Seat.VERIFIED,):
+                if seat.type in Seat.PAID_AND_AUDIT_APPLICABLE_SEATS:
                     if not audit_seats.exists():
                         course_run.seats.create(type=Seat.AUDIT, price=0, upgrade_deadline=None)
                         logger.info('Created audit seat for course run [%d]', course_run.id)
@@ -448,6 +453,22 @@ class SeatForm(BaseForm):
         seat.credit_hours = None
         seat.credit_price = 0.00
 
+    def validate_and_remove_seats(self, course_run):
+        """
+        Remove course run seats if the data is bogus
+
+        Temporary call to remove the duplicate course run seats -- EDUCATOR-2337
+        Background: There was a bug in the system, where course run seats being duplicated,
+        in order to cleanup, remove all the existing seats.
+        """
+        if not course_run:
+            return
+        all_course_run_seats = course_run.seats.all()
+        seats_data_is_bogus = all_course_run_seats.count() > 2
+        if seats_data_is_bogus:
+            logger.info('Removing bogus course run [%d] seats [%d]', course_run.id, all_course_run_seats.count())
+            all_course_run_seats.delete()
+
 
 class CourseEntitlementForm(BaseForm):
     MODE_CHOICES = [

course_discovery/apps/publisher/models.py

@@ -25,15 +25,15 @@ from course_discovery.apps.course_metadata.models import LevelType, Organization
 from course_discovery.apps.course_metadata.utils import UploadToFieldNamePath
 from course_discovery.apps.ietf_language_tags.models import LanguageTag
 from course_discovery.apps.publisher import emails
-from course_discovery.apps.publisher.choices import (CourseRunStateChoices, CourseStateChoices, InternalUserRole,
-                                                     PublisherUserRole)
+from course_discovery.apps.publisher.choices import (
+    CourseRunStateChoices, CourseStateChoices, InternalUserRole, PublisherUserRole
+)
 from course_discovery.apps.publisher.utils import is_email_notification_enabled, is_internal_user, is_publisher_admin
+
 from course_discovery.apps.publisher.validators import ImageMultiSizeValidator
 
 logger = logging.getLogger(__name__)
 
-PAID_SEATS = ['verified', 'professional']
-
 
 class ChangedByMixin(models.Model):
     changed_by = models.ForeignKey(User, null=True, blank=True)
@@ -430,6 +430,11 @@ class CourseRun(TimeStampedModel, ChangedByMixin):
         seats = self.seats.filter(type__in=[Seat.AUDIT, Seat.VERIFIED, Seat.PROFESSIONAL, Seat.CREDIT])
         return all([seat.is_valid_seat for seat in seats]) if seats else False
 
+    @property
+    def paid_seats(self):
+        """ Return course run paid seats """
+        return self.seats.filter(type__in=Seat.PAID_SEATS)
+
     def get_absolute_url(self):
         return reverse('publisher:publisher_course_run_detail', kwargs={'pk': self.id})
 
@@ -441,6 +446,8 @@ class Seat(TimeStampedModel, ChangedByMixin):
     PROFESSIONAL = 'professional'
     NO_ID_PROFESSIONAL = 'no-id-professional'
     CREDIT = 'credit'
+    PAID_SEATS = [VERIFIED, PROFESSIONAL, CREDIT]
+    PAID_AND_AUDIT_APPLICABLE_SEATS = [CREDIT, VERIFIED]
 
     SEAT_TYPE_CHOICES = (
         (HONOR, _('Honor')),

course_discovery/apps/publisher/tests/test_views.py

@@ -34,8 +34,8 @@ from course_discovery.apps.publisher.choices import (
     CourseRunStateChoices, CourseStateChoices, InternalUserRole, PublisherUserRole
 )
 from course_discovery.apps.publisher.constants import (
-    ADMIN_GROUP_NAME, INTERNAL_USER_GROUP_NAME, PROJECT_COORDINATOR_GROUP_NAME, PUBLISHER_ENTITLEMENTS_WAFFLE_SWITCH,
-    REVIEWER_GROUP_NAME
+    ADMIN_GROUP_NAME, INTERNAL_USER_GROUP_NAME, PROJECT_COORDINATOR_GROUP_NAME,
+    PUBLISHER_CREATE_AUDIT_SEATS_FOR_VERIFIED_COURSE_RUNS, PUBLISHER_ENTITLEMENTS_WAFFLE_SWITCH, REVIEWER_GROUP_NAME
 )
 from course_discovery.apps.publisher.models import (
     Course, CourseEntitlement, CourseRun, CourseRunState, CourseState, OrganizationExtension, Seat
@@ -554,25 +554,36 @@ class CreateCourseRunViewTests(SiteMixin, TestCase):
     def test_existing_run_and_seat_data_auto_populated(self):
         """
         Verify that existing course run and seat data auto populated on new course run form.
+
+        Create a course run with a verified seat (for a verified and credit seat, audit seat
+        is automatically created), then try to create a new run of the same course and verify
+        the previous course run's seats and pacing is automatically populated.
         """
+        verified_seat_price = 550.0
         latest_run = self.course.course_runs.latest('created')
-        factories.SeatFactory(course_run=latest_run, type=Seat.VERIFIED, price=550.0)
-        latest_seat = latest_run.seats.latest('created')
+        factories.SeatFactory(course_run=latest_run, type=Seat.VERIFIED, price=verified_seat_price)
+        factories.SeatFactory(course_run=latest_run, type=Seat.AUDIT)
+
+        # Verified seat should be preselected
+        verified_seat = latest_run.seats.get(type=Seat.VERIFIED)
         response = self.client.get(self.create_course_run_url_new)
         response_content = BeautifulSoup(response.content)
 
         pacing_type_attribute = response_content.find(
             "input", {"value": latest_run.pacing_type, "type": "radio", "name": "pacing_type"}
         )
-        seat_type_attribute = response_content.find("option", {"value": latest_seat.type})
+        seat_type_attribute = response_content.find("option", {"value": verified_seat.type})
         price_attribute = response_content.find(
-            "input", {"value": latest_seat.price, "id": "id_price", "step": "0.01", "type": "number"}
+            "input", {"value": verified_seat.price, "id": "id_price", "step": "0.01", "type": "number"}
         )
 
         # Verify that existing course run and seat values auto populated on form.
-        assert pacing_type_attribute is not None
-        assert seat_type_attribute is not None
-        assert price_attribute is not None
+        self.assertIsNotNone(pacing_type_attribute)
+        self.assertIn('checked=""', str(pacing_type_attribute))
+        self.assertIsNotNone(seat_type_attribute)
+        self.assertIn('selected=""', str(seat_type_attribute))
+        self.assertIsNotNone(price_attribute)
+        self.assertIn(str(verified_seat_price), str(price_attribute))
 
     def test_credit_type_without_price(self):
         """ Verify that without credit price course-run cannot be created with credit seat type. """
@@ -664,7 +675,7 @@ class CreateCourseRunViewTests(SiteMixin, TestCase):
         assign_perm(
             OrganizationExtension.VIEW_COURSE_RUN, self.organization_extension.group, self.organization_extension
         )
-        toggle_switch('publisher_create_audit_seats_for_verified_course_runs', True)
+        toggle_switch(PUBLISHER_CREATE_AUDIT_SEATS_FOR_VERIFIED_COURSE_RUNS, True)
 
         self.course.entitlements.create(mode=entitlement_mode, price=entitlement_price)
         post_data = {'start': '2018-02-01 00:00:00', 'end': '2018-02-28 00:00:00', 'pacing_type': 'instructor_paced'}
@@ -3072,7 +3083,7 @@ class CourseRunEditViewTests(SiteMixin, TestCase):
         assign_perm(OrganizationExtension.EDIT_COURSE_RUN, self.group, self.organization_extension)
         assign_perm(OrganizationExtension.VIEW_COURSE_RUN, self.group, self.organization_extension)
 
-        # assert edit page is loading sucesfully.
+        # assert edit page is loading successfully.
         self.edit_page_url = reverse('publisher:publisher_course_runs_edit', kwargs={'pk': self.new_course_run.id})
         response = self.client.get(self.edit_page_url)
         self.assertEqual(response.status_code, 200)
@@ -3105,6 +3116,19 @@ class CourseRunEditViewTests(SiteMixin, TestCase):
         response = self.client.get(url)
         self.assertContains(response, 'CERTIFICATE TYPE AND PRICE', status_code=200)
 
+    def login_with_course_user_role(self, course_run, role=PublisherUserRole.CourseTeam):
+        self.client.logout()
+        user = course_run.course.course_user_roles.get(role=role).user
+        self.client.login(username=user.username, password=USER_PASSWORD)
+        return user
+
+    def assert_seats(self, course_run, expected_seat_count, expected_seats):
+        """Helper method to assert expected course run seats"""
+        course_run_seats = Seat.objects.filter(course_run=course_run).order_by('type')
+        self.assertEqual(course_run_seats.count(), expected_seat_count)
+        seat_types = [seat.type for seat in course_run_seats]
+        self.assertEqual(seat_types, expected_seats)
+
     def test_edit_page_with_two_seats(self):
         """
         Verify that if a course run has both audit and verified seats, Verified seat is displayed
@@ -3220,16 +3244,12 @@ class CourseRunEditViewTests(SiteMixin, TestCase):
 
     def test_update_course_run_with_seat(self):
         """ Verify that course run edit page create seat object also if not exists previously."""
-
-        self.client.logout()
-        user = self.new_course_run.course.course_user_roles.get(role=PublisherUserRole.CourseTeam).user
-        self.client.login(username=user.username, password=USER_PASSWORD)
+        user = self.login_with_course_user_role(self.new_course_run)
         self.assertNotEqual(self.course_run.changed_by, user)
 
         # post data without seat
         data = {'image': ''}
         updated_dict = self._post_data(data, self.new_course, self.new_course_run)
-
         updated_dict['type'] = Seat.PROFESSIONAL
         updated_dict['price'] = 10.00
 
@@ -3242,12 +3262,52 @@ class CourseRunEditViewTests(SiteMixin, TestCase):
             target_status_code=200
         )
 
-        course_run = CourseRun.objects.get(id=self.new_course_run.id)
+        self.assert_seats(self.new_course_run, 1, [Seat.PROFESSIONAL])
+        self.assertEqual(self.new_course_run.seats.first().price, 10)
+        self.assertEqual(self.new_course_run.seats.first().history.all().count(), 1)
+
+    def test_update_course_run_create_duplicate_seats(self):
+        """
+        Tests that course run seats are not duplicated when editing.
+        Course run seats should remain consistent when updating seats.
+        """
+        self.login_with_course_user_role(self.new_course_run)
+        data = {'image': '', 'type': Seat.VERIFIED, 'price': 10.00}
+        toggle_switch(PUBLISHER_CREATE_AUDIT_SEATS_FOR_VERIFIED_COURSE_RUNS, True)
+        post_data = self._post_data(data, self.new_course, self.new_course_run)
+
+        self.client.post(self.edit_page_url, post_data)
+        self.assert_seats(self.new_course_run, 2, [Seat.AUDIT, Seat.VERIFIED])
+
+        post_data['price'] = 20.00
+        self.client.post(self.edit_page_url, post_data)
+        self.assert_seats(self.new_course_run, 2, [Seat.AUDIT, Seat.VERIFIED])
+        self.assertEqual(self.new_course_run.paid_seats.first().price, 20.00)
+
+    @ddt.data(Seat.VERIFIED, Seat.CREDIT)
+    def test_cleanup_course_run_seats_bogus_data(self, seat_type):
+        """
+        Test that bogus course run seat data is corrected upon saving the
+        course run.
+
+        Some data in publisher has duplicate course seats, this tests that we are able
+        to correct the data upon course run save.
+        """
+        # Login and verify the course run has no seats.
+        self.login_with_course_user_role(self.new_course_run)
+        self.assert_seats(self.new_course_run, 0, [])
 
-        self.assertEqual(course_run.seats.first().type, Seat.PROFESSIONAL)
-        self.assertEqual(course_run.seats.first().price, 10)
+        # create some bogus seats for the course run & verify the seats exists
+        __ = [self.new_course_run.seats.create(type=seat_type) for _ in range(10)]
+        self.assert_seats(self.new_course_run, 10, [seat_type] * 10)
 
-        self.assertEqual(course_run.seats.first().history.all().count(), 1)
+        # Make course run save post request and verify the correct course seats.
+        toggle_switch(PUBLISHER_CREATE_AUDIT_SEATS_FOR_VERIFIED_COURSE_RUNS, True)
+        data = {'image': '', 'type': Seat.VERIFIED, 'price': 100.00}
+        post_data = self._post_data(data, self.new_course, self.new_course_run)
+        self.client.post(self.edit_page_url, post_data)
+        self.assert_seats(self.new_course_run, 2, [Seat.AUDIT, Seat.VERIFIED])
+        self.assertEqual(self.new_course_run.paid_seats.first().price, 100.00)
 
     def test_update_course_run_for_course_that_uses_entitlements(self):
         """ Verify that a user cannot change Seat data when editing courseruns for courses that use entitlements. """
@@ -3840,7 +3900,7 @@ class CreateRunFromDashboardViewTests(SiteMixin, TestCase):
         assign_perm(
             OrganizationExtension.VIEW_COURSE_RUN, self.organization_extension.group, self.organization_extension
         )
-        toggle_switch('publisher_create_audit_seats_for_verified_course_runs', True)
+        toggle_switch(PUBLISHER_CREATE_AUDIT_SEATS_FOR_VERIFIED_COURSE_RUNS, True)
 
         self.course.entitlements.create(mode=entitlement_mode, price=entitlement_price)
         post_data = {

course_discovery/apps/publisher/views.py

@@ -30,13 +30,17 @@ from course_discovery.apps.publisher.choices import CourseRunStateChoices, Cours
 from course_discovery.apps.publisher.constants import PUBLISHER_ENTITLEMENTS_WAFFLE_SWITCH
 from course_discovery.apps.publisher.dataloader.create_courses import process_course
 from course_discovery.apps.publisher.emails import send_email_for_published_course_run_editing
-from course_discovery.apps.publisher.forms import (AdminImportCourseForm, CourseEntitlementForm, CourseForm,
-                                                   CourseRunForm, CourseSearchForm, SeatForm)
-from course_discovery.apps.publisher.models import (PAID_SEATS, Course, CourseEntitlement, CourseRun, CourseRunState,
-                                                    CourseState, CourseUserRole, OrganizationExtension, PublisherUser,
-                                                    Seat, UserAttributes)
-from course_discovery.apps.publisher.utils import (get_internal_users, has_role_for_course, is_internal_user,
-                                                   is_project_coordinator_user, is_publisher_admin, make_bread_crumbs)
+from course_discovery.apps.publisher.forms import (
+    AdminImportCourseForm, CourseEntitlementForm, CourseForm, CourseRunForm, CourseSearchForm, SeatForm
+)
+from course_discovery.apps.publisher.models import (
+    Course, CourseEntitlement, CourseRun, CourseRunState, CourseState, CourseUserRole, OrganizationExtension,
+    PublisherUser, Seat, UserAttributes
+)
+from course_discovery.apps.publisher.utils import (
+    get_internal_users, has_role_for_course, is_internal_user, is_project_coordinator_user, is_publisher_admin,
+    make_bread_crumbs
+)
 from course_discovery.apps.publisher.wrappers import CourseRunWrapper
 
 logger = logging.getLogger(__name__)
@@ -582,15 +586,23 @@ class CreateCourseRunView(mixins.LoginRequiredMixin, mixins.PublisherUserRequire
 
         new_run.save()
 
-    def _initialize_seat_form(self, last_run=None):
+    def _initialize_seat_form(self, last_run):
         initial_seat_data = {}
-        if last_run:
-            try:
-                latest_seat = last_run.seats.latest('created')
-                initial_seat_data = model_to_dict(latest_seat)
-                del initial_seat_data['id'], initial_seat_data['course_run'], initial_seat_data['changed_by']
-            except Seat.DoesNotExist:
-                pass
+        if not last_run:
+            return self.seat_form(initial=initial_seat_data)
+
+        def _get_latest_seat():
+            """Returns latest course run seat. Paid seats are Preferred"""
+            if last_run.paid_seats:
+                return last_run.paid_seats.latest()
+            return last_run.seats.latest()
+
+        try:
+            latest_seat = _get_latest_seat()
+            initial_seat_data = model_to_dict(latest_seat)
+            del initial_seat_data['id'], initial_seat_data['course_run'], initial_seat_data['changed_by']
+        except Seat.DoesNotExist:
+            pass
 
         return self.seat_form(initial=initial_seat_data)
 
@@ -787,6 +799,11 @@ class CourseRunEditView(mixins.LoginRequiredMixin, mixins.PublisherPermissionMix
             'organizations': mixins.get_user_organizations(user)
         }
 
+    def get_latest_course_run_seat(self, course_run):
+        if course_run.paid_seats:
+            return course_run.paid_seats.first()
+        return course_run.seats.first()
+
     def get(self, request, *args, **kwargs):
         context = self.get_context_data()
         course_run = context.get('course_run')
@@ -804,11 +821,8 @@ class CourseRunEditView(mixins.LoginRequiredMixin, mixins.PublisherPermissionMix
         )
 
         if not course.uses_entitlements:
-            course_run_paid_seat = course_run.seats.filter(type__in=PAID_SEATS).first()
-            if course_run_paid_seat:
-                context['seat_form'] = self.seat_form(instance=course_run_paid_seat)
-            else:
-                context['seat_form'] = self.seat_form(instance=course_run.seats.first())
+            course_run_seat = self.get_latest_course_run_seat(course_run)
+            context['seat_form'] = self.seat_form(instance=course_run_seat)
 
         start_date = course_run.start.strftime("%B %d, %Y") if course_run.start else None
         context['breadcrumbs'] = make_bread_crumbs(
@@ -841,16 +855,14 @@ class CourseRunEditView(mixins.LoginRequiredMixin, mixins.PublisherPermissionMix
             form_data_is_valid = form_data_is_valid and not seat_data_in_form
             seat_form = None
         else:
-            seat_form = self.seat_form(request.POST, instance=course_run.seats.first())
+            seat_form = self.seat_form(request.POST, instance=self.get_latest_course_run_seat(course_run))
             form_data_is_valid = form_data_is_valid and seat_form.is_valid()
             context['seat_form'] = seat_form
 
         if form_data_is_valid:
             try:
                 with transaction.atomic():
-
                     course_run = run_form.save(changed_by=user)
-
                     run_form.save_m2m()
 
                     # If price-type comes with request then save the seat object.