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

feat: include restricted runs in algolia objects #976

Merged
merged 1 commit into from
Oct 17, 2024

Conversation

pwnage101
Copy link
Contributor

@pwnage101 pwnage101 commented Oct 16, 2024

ENT-9505

Important features:

  • Restricted Runs Visible: The metadata actually used for indexing courses always comes from the "canonical restricted course" object if one exists (i.e. the related RestrictedCourseMetadata that has catalog_query=null).
  • Unicorn Course Support: A course normally is not indexed at all if it has no advertised course run. Now we index "empty" courses as long as they contain at least one restricted run (i.e. the canonical restricted variant does have an advertised run).
    • Furthermore, we do NOT associate the course with the standard set of related catalog queries. Instead we use a subset that represents only the catalog queries that explicitly allow runs in that course.

@pwnage101 pwnage101 force-pushed the pwnage101/ENT-9505 branch 4 times, most recently from 9d40ef7 to 0d00393 Compare October 17, 2024 00:26
@@ -1746,74 +1755,136 @@ def mock_replace_all_objects(products_iterable):
assert expected_call_args == self._sort_tags_in_algolia_object_list(actual_call_args)

@mock.patch('enterprise_catalog.apps.api.tasks.get_initialized_algolia_client', return_value=mock.MagicMock())
# pylint: disable=too-many-statements
Copy link
Contributor Author

Choose a reason for hiding this comment

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

github really fudged this diff view. If you look at the file directly I only added two tests:

  • test_index_algolia_restricted_runs_mixed_course
  • test_index_algolia_restricted_runs_unicorn_course

@pwnage101 pwnage101 marked this pull request as ready for review October 17, 2024 00:29
enterprise_catalog/apps/api/tasks.py Outdated Show resolved Hide resolved
# catalog queries that explicitly allow runs in those courses. We can tell that a course
# has only restricted runs simply by checking that it normally doesn't have an
# advertised run.
# pylint: disable=protected-access
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you could add an advertised_course_run_uuid() property to ContentMetadata to avoid this protected-access warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm not sure that kind of getter design is sufficiently explicit when there's potentially two options: the values from _json_metadata vs. json_metadata, especially when I expect the value to differ depending on which is queried. My preference is to keep this as written and elaborate on the code comment.

enterprise_catalog/apps/catalog/models.py Show resolved Hide resolved
@pwnage101 pwnage101 merged commit e670d28 into master Oct 17, 2024
4 checks passed
@pwnage101 pwnage101 deleted the pwnage101/ENT-9505 branch October 17, 2024 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants