Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: relegate optgroup calculation to superclass #4540

Merged
merged 4 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 36 additions & 15 deletions course_discovery/apps/course_metadata/tests/test_widgets.py
Original file line number Diff line number Diff line change
@@ -1,19 +1,40 @@
import ddt
import itertools

from bs4 import BeautifulSoup
from django.test import TestCase
from django.urls import reverse

from course_discovery.apps.api.tests.mixins import SiteMixin
from course_discovery.apps.core.tests.factories import USER_PASSWORD, UserFactory
from course_discovery.apps.course_metadata.tests.factories import CourseFactory, ProgramFactory


from course_discovery.apps.course_metadata.widgets import SortedModelSelect2Multiple
class SortedModelSelect2MultipleTests(SiteMixin, TestCase):
def setUp(self):
super().setUp()
self.user = UserFactory(is_staff=True, is_superuser=True)
self.client.login(username=self.user.username, password=USER_PASSWORD)

def test_program_ordered_m2m(self):
"""
Verify that program page sorted m2m fields render in order. The sorted
m2m field chosen for the test is the courses field
"""

@ddt.ddt
class SortedModelSelect2MultipleTests(TestCase):
@ddt.data(
(['1', '2'], [1, 2]),
(['2', '1'], [2, 1]),
(['3'], [3,]),
)
@ddt.unpack
def test_optgroups_are_sorted(self, value, result_order):
choices = ((1, 'one'), (2, 'two'), (3, 'three'))
widget = SortedModelSelect2Multiple(url='requiredurl', choices=choices)
result = widget.optgroups('test', value)
assert result_order == [x[1][0]['value'] for x in result]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is hard to do a simple optgroups test like this now since the method calls the super method, which expects things to be set up much more properly. e.g requiring self.choices to have a queryset attribute, having the url kwarg of the widget being an actual reversible url etc.

I have added a new test in place, which renders the program admin form, and ensures that the courses are rendered in the order in which they were assigned. I am unsure if this test is more appropriate in test_admin.py now.

for courses in itertools.permutations(
[
CourseFactory(title="Blade Runner 2049"),
CourseFactory(title="History of Western Literature"),
CourseFactory(title="Urdu Poetry")
],
2
):
program = ProgramFactory(courses=courses)
response = self.client.get(reverse('admin:course_metadata_program_change', args=(program.id,)))
response_content = BeautifulSoup(response.content)
options = response_content.find('select', {'name': 'courses'}).find_all('option')
assert len(options) == len(courses)
for idx, opt in enumerate(options):
assert 'selected' in opt.attrs
assert opt.get_text().endswith(courses[idx].title)
assert opt.attrs['value'] == str(courses[idx].id)
13 changes: 2 additions & 11 deletions course_discovery/apps/course_metadata/widgets.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
from itertools import chain

from dal import autocomplete


Expand All @@ -8,16 +6,9 @@ def optgroups(self, name, value, attrs=None):
"""
Return a sorted list of optgroups for this widget.

This is a simplified version of Django's version. The big difference is that we keep the results sorted and
only support one main group (because that's all we need right now).
This is a simplified version of Django's version. The big difference is that we keep the results sorted.
"""
selected = []
for index, (option_value, option_label) in enumerate(chain(self.choices)):
is_selected = str(option_value) in value
if is_selected:
subgroup = [self.create_option(name, option_value, option_label, is_selected, index, attrs=attrs)]
item = (None, subgroup, index)
selected.append(item)
selected = super().optgroups(name, value, attrs)

ordered = []
for value_id in value:
Expand Down
Loading