Fixed bug on program admin page

The previous override of save_model() was largely unnecessary and broke because it attempts to set m2m fields on unsaved models. The override now relies on the super method (which performs the correct behavior) and simply does error handling.

Our use of Select2 has also been cleaned up to make our monkey patching clearer.

ECOM-6238

course_discovery/apps/course_metadata/admin.py

@@ -87,38 +87,47 @@ class ProgramAdmin(admin.ModelAdmin):
         'authoring_organizations', 'credit_backing_organizations'
     )
     fields += filter_horizontal
-    save_error = None
+    save_error = False
 
     def custom_course_runs_display(self, obj):
         return mark_safe('<br>'.join([str(run) for run in obj.course_runs]))
 
     custom_course_runs_display.short_description = _('Included course runs')
 
+    def _redirect_course_run_update_page(self, obj):
+        """ Returns a response redirect to a page where the user can update the
+        course runs for the program being edited.
+
+        Returns:
+            HttpResponseRedirect
+        """
+        return HttpResponseRedirect(reverse('admin_metadata:update_course_runs', kwargs={'pk': obj.pk}))
+
     def response_add(self, request, obj, post_url_continue=None):
         if self.save_error:
             return self.response_post_save_add(request, obj)
         else:
-            return HttpResponseRedirect(reverse('admin_metadata:update_course_runs', kwargs={'pk': obj.pk}))
+            return self._redirect_course_run_update_page(obj)
 
     def response_change(self, request, obj):
         if self.save_error:
             return self.response_post_save_change(request, obj)
         else:
             if any(status in request.POST for status in ['_continue', '_save']):
-                return HttpResponseRedirect(reverse('admin_metadata:update_course_runs', kwargs={'pk': obj.pk}))
+                return self._redirect_course_run_update_page(obj)
             else:
                 return HttpResponseRedirect(reverse('admin:course_metadata_program_add'))
 
     def save_model(self, request, obj, form, change):
         try:
-            # courses are ordered by django id, but form.cleaned_data is ordered correctly
-            obj.courses = form.cleaned_data.get('courses')
-            obj.authoring_organizations = form.cleaned_data.get('authoring_organizations')
-            obj.credit_backing_organizations = form.cleaned_data.get('credit_backing_organizations')
-            obj.save()
+            super().save_model(request, obj, form, change)
             self.save_error = False
-        except ProgramPublisherException as ex:
-            messages.add_message(request, messages.ERROR, ex.message)
+        except ProgramPublisherException:
+            # TODO Redirect the user back to the form so that he/she can try again.
+            logger.exception('An error occurred while publishing the program [%s] to the marketing site.', obj.uuid)
+            msg = _('An error occurred while publishing the program to the marketing site. Please try again. '
+                    'If the error persists, please contact the Engineering Team.')
+            messages.add_message(request, messages.ERROR, msg)
             self.save_error = True
 
     class Media:

course_discovery/apps/course_metadata/forms.py

-from dal import autocomplete, widgets
+from dal import autocomplete
 from django import forms
 from django.core.exceptions import ValidationError
 from django.forms.utils import ErrorList
@@ -8,31 +8,30 @@ from course_discovery.apps.course_metadata.choices import ProgramStatus
 from course_discovery.apps.course_metadata.models import Program, CourseRun
 
 
-class HackDjangoAutocompleteMixin(object):
-    # It seems to me there is an issue with the select 2 widget in django autocomplete.
-    # When the widget loads selected choices it loads them in order of django id, not the order
-    # they are stored in in the database. This workaround works, but not sure what approach
-    # would be less hacky. Perhaps opening a PR to the django autocomplete repo if this is
-    # fact an issue?
+def filter_choices_to_render_with_order_preserved(self, selected_choices):
+    """
+    Preserves ordering of selected_choices when creating the choices queryset.
 
-    class QuerySetSelectMixin2(widgets.WidgetMixin):
-        def filter_choices_to_render(self, selected_choices):
-            # preserve ordering of selected_choices in queryset
-            # https://codybonney.com/creating-a-queryset-from-a-list-while-preserving-order-using-django/
-            clauses = ' '.join(['WHEN id={} THEN {}'.format(pk, i) for i, pk in enumerate(selected_choices)])
-            ordering = 'CASE {} END'.format(clauses)
-            self.choices.queryset = self.choices.queryset.filter(
-                pk__in=[c for c in selected_choices if c]
-            ).extra(select={'ordering': ordering}, order_by=('ordering',))
+    See https://codybonney.com/creating-a-queryset-from-a-list-while-preserving-order-using-django.
 
-    widgets.QuerySetSelectMixin = QuerySetSelectMixin2
+    django-autocomplete's definition of this method on QuerySetSelectMixin loads selected choices in
+    order of primary key instead of the order in which the choices are actually stored.
+    """
+    clauses = ' '.join(['WHEN id={} THEN {}'.format(pk, i) for i, pk in enumerate(selected_choices)])
+    ordering = 'CASE {} END'.format(clauses)
+    self.choices.queryset = self.choices.queryset.filter(
+        pk__in=[c for c in selected_choices if c]
+    ).extra(select={'ordering': ordering}, order_by=('ordering',))
 
 
-class ProgramAdminForm(HackDjangoAutocompleteMixin, forms.ModelForm):
+class ProgramAdminForm(forms.ModelForm):
     class Meta:
         model = Program
         fields = '__all__'
 
+        # Monkey patch filter_choices_to_render with our own definition which preserves ordering.
+        autocomplete.ModelSelect2Multiple.filter_choices_to_render = filter_choices_to_render_with_order_preserved
+
         widgets = {
             'courses': autocomplete.ModelSelect2Multiple(
                 url='admin_metadata:course-autocomplete',

course_discovery/apps/course_metadata/tests/test_admin.py

@@ -4,19 +4,25 @@ import ddt
 from django.core.urlresolvers import reverse
 from django.test import TestCase, LiveServerTestCase
 from selenium import webdriver
-from selenium.webdriver.common.action_chains import ActionChains
+from selenium.webdriver.common.by import By
+from selenium.webdriver.support import expected_conditions as EC
+from selenium.webdriver.support.ui import Select
+from selenium.webdriver.support.wait import WebDriverWait
 
+from course_discovery.apps.core.models import Partner
+from course_discovery.apps.core.tests.factories import UserFactory, USER_PASSWORD
+from course_discovery.apps.core.tests.helpers import make_image_file
 from course_discovery.apps.course_metadata.choices import ProgramStatus
 from course_discovery.apps.course_metadata.forms import ProgramAdminForm
+from course_discovery.apps.course_metadata.models import Program, ProgramType
 from course_discovery.apps.course_metadata.tests import factories
-from course_discovery.apps.core.tests.factories import UserFactory, USER_PASSWORD
-from course_discovery.apps.core.tests.helpers import make_image_file
 
 
 # pylint: disable=no-member
 @ddt.ddt
 class AdminTests(TestCase):
     """ Tests Admin page."""
+
     def setUp(self):
         super(AdminTests, self).setUp()
         self.user = UserFactory(is_staff=True, is_superuser=True)
@@ -185,71 +191,144 @@ class AdminTests(TestCase):
 
 class ProgramAdminFunctionalTests(LiveServerTestCase):
     """ Functional Tests for Admin page."""
+    create_view_name = 'admin:course_metadata_program_add'
+    edit_view_name = 'admin:course_metadata_program_change'
+
+    @classmethod
+    def setUpClass(cls):
+        super().setUpClass()
+        cls.browser = webdriver.Firefox()
+        cls.browser.implicitly_wait(10)
+        cls.browser.set_window_size(1024, 768)
+
+    @classmethod
+    def tearDownClass(cls):
+        cls.browser.quit()
+        super().tearDownClass()
+
+    @classmethod
+    def _build_url(cls, path):
+        """ Returns a URL for the live test server. """
+        return cls.live_server_url + path
+
+    @classmethod
+    def _wait_for_page_load(cls, body_class):
+        """ Wait for the page to load. """
+        WebDriverWait(cls.browser, 2).until(
+            EC.presence_of_element_located((By.CSS_SELECTOR, 'body.' + body_class))
+        )
+
     def setUp(self):
-        super(ProgramAdminFunctionalTests, self).setUp()
-        self.user = UserFactory(is_staff=True, is_superuser=True)
-        self.client.login(username=self.user.username, password=USER_PASSWORD)
+        super().setUp()
+
         self.course_runs = factories.CourseRunFactory.create_batch(2)
         self.courses = [course_run.course for course_run in self.course_runs]
 
         self.excluded_course_run = factories.CourseRunFactory(course=self.courses[0])
         self.program = factories.ProgramFactory(
-            courses=self.courses, excluded_course_runs=[self.excluded_course_run]
+            courses=self.courses, excluded_course_runs=[self.excluded_course_run], status=ProgramStatus.Unpublished
         )
-        self.browser = webdriver.Firefox()
-
-        # Get Page
-        domain = self.live_server_url
-        url = reverse('admin:course_metadata_program_change', args=(self.program.id,))
-        self.browser.get(domain + url)
-
-        # Login
-        username = self.browser.find_element_by_id('id_username')
-        password = self.browser.find_element_by_id('id_password')
-        username.send_keys(self.user.username)
-        password.send_keys(USER_PASSWORD)
+
+        self.user = UserFactory(is_staff=True, is_superuser=True)
+        self._login()
+
+    def _login(self):
+        """ Log into Django admin. """
+        self.browser.get(self._build_url(reverse('admin:login')))
+        self.browser.find_element_by_id('id_username').send_keys(self.user.username)
+        self.browser.find_element_by_id('id_password').send_keys(USER_PASSWORD)
         self.browser.find_element_by_css_selector('input[type=submit]').click()
+        self._wait_for_page_load('dashboard')
+
+    def _wait_for_add_edit_page_to_load(self):
+        self._wait_for_page_load('change-form')
+
+    def _wait_for_excluded_course_runs_page_to_load(self):
+        self._wait_for_page_load('change-program-excluded-course-runs-form')
+
+    def _navigate_to_edit_page(self):
+        url = self._build_url(reverse(self.edit_view_name, args=(self.program.id,)))
+        self.browser.get(url)
+        self._wait_for_add_edit_page_to_load()
+
+    def _select_option(self, select_id, option_value):
+        select = Select(self.browser.find_element_by_id(select_id))
+        select.select_by_value(option_value)
+
+    def _submit_program_form(self):
+        self.browser.find_element_by_css_selector('input[type=submit][name=_save]').click()
+        self._wait_for_excluded_course_runs_page_to_load()
+
+    def assert_form_fields_present(self):
+        """ Asserts the correct fields are rendered on the form. """
+        # Check the model fields
+        actual = []
+        for element in self.browser.find_elements_by_class_name('form-row'):
+            actual += [_class for _class in element.get_attribute('class').split(' ') if _class.startswith('field-')]
+
+        expected = [
+            'field-title', 'field-subtitle', 'field-status', 'field-type', 'field-partner', 'field-banner_image',
+            'field-banner_image_url', 'field-card_image_url', 'field-marketing_slug', 'field-overview',
+            'field-credit_redemption_overview', 'field-video', 'field-weeks_to_complete',
+            'field-min_hours_effort_per_week', 'field-max_hours_effort_per_week', 'field-courses',
+            'field-order_courses_by_start_date', 'field-custom_course_runs_display', 'field-excluded_course_runs',
+            'field-authoring_organizations', 'field-credit_backing_organizations', 'field-job_outlook_items',
+            'field-expected_learning_items',
+        ]
+        self.assertEqual(actual, expected)
+
+        # Check the inline fields
+        expected = ['Program_faq-group', 'Program_individual_endorsements-group',
+                    'Program_corporate_endorsements-group']
+        actual = [element.get_attribute('id') for element in self.browser.find_elements_by_class_name('inline-group')]
+        self.assertEqual(actual, expected)
+
+    def test_program_creation(self):
+        url = self._build_url(reverse(self.create_view_name))
+        self.browser.get(url)
+        self._wait_for_add_edit_page_to_load()
+        self.assert_form_fields_present()
+
+        program = factories.ProgramFactory.build(
+            partner=Partner.objects.first(),
+            status=ProgramStatus.Unpublished,
+            type=ProgramType.objects.first()
+        )
+        self.browser.find_element_by_id('id_title').send_keys(program.title)
+        self.browser.find_element_by_id('id_subtitle').send_keys(program.subtitle)
+        self._select_option('id_status', program.status)
+        self._select_option('id_type', str(program.type.id))
+        self._select_option('id_partner', str(program.partner.id))
+        self._submit_program_form()
+
+        actual = Program.objects.latest()
+        self.assertEqual(actual.title, program.title)
+        self.assertEqual(actual.subtitle, program.subtitle)
+        self.assertEqual(actual.status, program.status)
+        self.assertEqual(actual.type, program.type)
+        self.assertEqual(actual.partner, program.partner)
+
+    def test_program_update(self):
+        self._navigate_to_edit_page()
+        self.assert_form_fields_present()
+
+        title = 'Test Program'
+        subtitle = 'This is a test.'
+
+        # Update the program
+        data = (
+            ('title', title),
+            ('subtitle', subtitle),
+        )
+
+        for field, value in data:
+            element = self.browser.find_element_by_id('id_' + field)
+            element.clear()
+            element.send_keys(value)
+
+        self._submit_program_form()
 
-        # This window size is close to the window size when running on travis
-        self.browser.set_window_size(548, 768)
-
-    def tearDown(self):
-        super(ProgramAdminFunctionalTests, self).tearDown()
-        self.browser.quit()
-
-    def test_all_fields(self):
-        # Make sure that all expected fields are present
-        classes = [css_class for field in self.browser.find_elements_by_class_name('form-row')
-                   for css_class in field.get_attribute('class').split(' ')
-                   if css_class.startswith('field-') or css_class.startswith('dynamic-')]
-        expected_classes = ['field-title', 'field-subtitle', 'field-status', 'field-type',
-                            'field-partner', 'field-banner_image', 'field-banner_image_url',
-                            'field-card_image_url', 'field-marketing_slug', 'field-overview',
-                            'field-credit_redemption_overview', 'field-video',
-                            'field-weeks_to_complete', 'field-min_hours_effort_per_week',
-                            'field-max_hours_effort_per_week', 'field-courses',
-                            'field-order_courses_by_start_date', 'field-custom_course_runs_display',
-                            'field-excluded_course_runs', 'field-authoring_organizations',
-                            'field-credit_backing_organizations', 'field-job_outlook_items',
-                            'field-expected_learning_items', 'dynamic-Program_faq',
-                            'dynamic-Program_individual_endorsements',
-                            'dynamic-Program_corporate_endorsements']
-        self.assertEqual(classes, expected_classes)
-
-    def test_sortable_select_drag_and_drop(self):
-        # Get order of select elements
-        hidden_options_text = [el.text for el in
-                               self.browser.find_elements_by_css_selector('.field-courses option')]
-        first_select_element = self.browser.find_element_by_css_selector('.field-courses .select2-selection__choice')
-
-        # Drag and drop
-        first_select_element.click()
-        ActionChains(self.browser).drag_and_drop_by_offset(first_select_element, 500, 0).perform()
-
-        # Simulate expected drag and drop
-        hidden_options_text = [hidden_options_text[1], hidden_options_text[0]]
-
-        # Get actual results of drag and drop
-        new_hidden_options_text = [el.text for el in
-                                   self.browser.find_elements_by_css_selector('.field-courses option')]
-        self.assertEqual(hidden_options_text, new_hidden_options_text)
+        # Verify the program was updated
+        self.program = Program.objects.get(pk=self.program.pk)
+        self.assertEqual(self.program.title, title)
+        self.assertEqual(self.program.subtitle, subtitle)

course_discovery/apps/course_metadata/views.py

 from django.contrib import messages
 from django.core.urlresolvers import reverse
 from django.http import HttpResponseRedirect, Http404
+from django.utils.translation import ugettext_lazy as _
 from django.views.generic import TemplateView, UpdateView
 
 from course_discovery.apps.course_metadata.forms import CourseRunSelectionForm
@@ -25,7 +26,10 @@ class CourseRunSelectionAdmin(UpdateView):
     def get_context_data(self, **kwargs):
         if self.request.user.is_authenticated() and self.request.user.is_staff:
             context = super(CourseRunSelectionAdmin, self).get_context_data(**kwargs)
-            context['program_id'] = self.object.id
+            context.update({
+                'program_id': self.object.id,
+                'title': _('Change program excluded course runs')
+            })
             return context
         raise Http404
 

course_discovery/conf/locale/en/LC_MESSAGES/django.po

@@ -7,7 +7,7 @@ msgid ""
 msgstr ""
 "Project-Id-Version: PACKAGE VERSION\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2016-10-31 11:33-0400\n"
+"POT-Creation-Date: 2016-10-31 19:34-0400\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"
@@ -200,6 +200,12 @@ msgstr ""
 msgid "Included course runs"
 msgstr ""
 
+#: apps/course_metadata/admin.py
+msgid ""
+"An error occurred while publishing the program to the marketing site. Please"
+" try again. If the error persists, please contact the Engineering Team."
+msgstr ""
+
 #: apps/course_metadata/choices.py apps/publisher/models.py
 msgid "Published"
 msgstr ""
@@ -395,6 +401,10 @@ msgstr ""
 msgid "The description of credit redemption for courses in program"
 msgstr ""
 
+#: apps/course_metadata/views.py
+msgid "Change program excluded course runs"
+msgstr ""
+
 #: apps/edx_haystack_extensions/models.py
 msgid "Function Score"
 msgstr ""
@@ -662,10 +672,6 @@ msgid "Sign Out"
 msgstr ""
 
 #: templates/metadata/admin/course_run.html
-msgid "CourseRun Selection Form"
-msgstr ""
-
-#: templates/metadata/admin/course_run.html
 msgid "Cancel"
 msgstr ""
 

course_discovery/conf/locale/en/LC_MESSAGES/djangojs.po

@@ -7,7 +7,7 @@ msgid ""
 msgstr ""
 "Project-Id-Version: PACKAGE VERSION\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2016-10-31 11:33-0400\n"
+"POT-Creation-Date: 2016-10-31 19:34-0400\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"

course_discovery/conf/locale/eo/LC_MESSAGES/django.po

@@ -7,7 +7,7 @@ msgid ""
 msgstr ""
 "Project-Id-Version: PACKAGE VERSION\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2016-10-31 11:33-0400\n"
+"POT-Creation-Date: 2016-10-31 19:34-0400\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"
@@ -241,6 +241,20 @@ msgstr "Pärtnérs Ⱡ'σяєм ιρѕυм ∂#"
 msgid "Included course runs"
 msgstr "Ìnçlüdéd çöürsé rüns Ⱡ'σяєм ιρѕυм ∂σłσя ѕιт αмєт, #"
 
+#: apps/course_metadata/admin.py
+msgid ""
+"An error occurred while publishing the program to the marketing site. Please"
+" try again. If the error persists, please contact the Engineering Team."
+msgstr ""
+"Àn érrör öççürréd whïlé püßlïshïng thé prögräm tö thé märkétïng sïté. Pléäsé"
+" trý ägäïn. Ìf thé érrör pérsïsts, pléäsé çöntäçt thé Éngïnéérïng Téäm. "
+"Ⱡ'σяєм ιρѕυм ∂σłσя ѕιт αмєт, ¢σηѕє¢тєтυя α∂ιριѕι¢ιηg єłιт, ѕє∂ ∂σ єιυѕмσ∂ "
+"тємρσя ιη¢ι∂ι∂υηт υт łαвσяє єт ∂σłσяє мαgηα αłιqυα. υт єηιм α∂ мιηιм νєηιαм,"
+" qυιѕ ησѕтяυ∂ єχєя¢ιтαтιση υłłαм¢σ łαвσяιѕ ηιѕι υт αłιqυιρ єχ єα ¢σммσ∂σ "
+"¢σηѕєqυαт. ∂υιѕ αυтє ιяυяє ∂σłσя ιη яєρяєнєη∂єяιт ιη νσłυρтαтє νєłιт єѕѕє "
+"¢ιłłυм ∂σłσяє єυ ƒυgιαт ηυłłα ραяιαтυя. єχ¢єρтєυя ѕιηт σ¢¢αє¢αт ¢υρι∂αтαт "
+"ηση ρяσι∂єηт, ѕυηт ιη ¢υłρα qυι σƒƒι¢ια ∂єѕєяυηт мσłłιт αηιм #"
+
 #: apps/course_metadata/choices.py apps/publisher/models.py
 msgid "Published"
 msgstr "Püßlïshéd Ⱡ'σяєм ιρѕυм ∂σł#"
@@ -488,6 +502,11 @@ msgstr ""
 "Thé désçrïptïön öf çrédït rédémptïön för çöürsés ïn prögräm Ⱡ'σяєм ιρѕυм "
 "∂σłσя ѕιт αмєт, ¢σηѕє¢тєтυя α#"
 
+#: apps/course_metadata/views.py
+msgid "Change program excluded course runs"
+msgstr ""
+"Çhängé prögräm éxçlüdéd çöürsé rüns Ⱡ'σяєм ιρѕυм ∂σłσя ѕιт αмєт, ¢σηѕє¢тєт#"
+
 #: apps/edx_haystack_extensions/models.py
 msgid "Function Score"
 msgstr "Fünçtïön Sçöré Ⱡ'σяєм ιρѕυм ∂σłσя ѕιт#"
@@ -783,10 +802,6 @@ msgid "Sign Out"
 msgstr "Sïgn Öüt Ⱡ'σяєм ιρѕυм ∂#"
 
 #: templates/metadata/admin/course_run.html
-msgid "CourseRun Selection Form"
-msgstr "ÇöürséRün Séléçtïön Förm Ⱡ'σяєм ιρѕυм ∂σłσя ѕιт αмєт, ¢ση#"
-
-#: templates/metadata/admin/course_run.html
 msgid "Cancel"
 msgstr "Çänçél Ⱡ'σяєм ιρѕυ#"
 

course_discovery/conf/locale/eo/LC_MESSAGES/djangojs.po

@@ -7,7 +7,7 @@ msgid ""
 msgstr ""
 "Project-Id-Version: PACKAGE VERSION\n"
 "Report-Msgid-Bugs-To: \n"
-"POT-Creation-Date: 2016-10-31 11:33-0400\n"
+"POT-Creation-Date: 2016-10-31 19:34-0400\n"
 "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
 "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
 "Language-Team: LANGUAGE <LL@li.org>\n"

course_discovery/templates/metadata/admin/course_run.html

 {% extends 'admin/base_site.html' %}
 {% load i18n %}
-{% block title %}
-{% trans "CourseRun Selection Form" %}
-{% endblock title %}
 
-{% block content %}
+{% block bodyclass %}
+  change-program-excluded-course-runs-form
+{% endblock %}
 
+{% block content %}
 <form class="form" method="post" action="">
     {% csrf_token %}
     <fieldset class="module aligned ">
@@ -30,5 +30,4 @@
         </div>
     {% endblock %}
 </form>
-
 {% endblock %}