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.

Sign up for GitHub

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

Jump to bottom

Added --active option for reindex_courses #34078

Merged
merged 2 commits into from
Jan 23, 2024
Merged

Added --active option for reindex_courses #34078

merged 2 commits into from
Jan 23, 2024

Conversation

rijuma
Copy link
Member

@rijuma rijuma commented Jan 19, 2024

Description

This adds a new parameter for the reindex_courses command that filters out non-active courses when selecting the course batch.

Testing instructions

While on the edx-platform container shell, run:

./manage.py cms reindex_course --warning --active

To reindex all active courses. The output should look like:

(some init logging)
...
[timestamp] WARNING 967 [root] [user None] [ip None] reindex_course.py:126 - Selected # potentially active courses over a total of #.
[timestamp] WARNING 967 [root] [user None] [ip None] reindex_course.py:133 - Reindexing # courses...
...
[timestamp] WARNING 967 [root] [user None] [ip None] reindex_course.py:144 - #/# reindexed in ## seconds.

Deadline

None

@rijuma rijuma force-pushed the rijuma/add-active-courses-option-to-reindex_courses branch 2 times, most recently from 326f37f to c577ac1 Compare January 19, 2024 14:35
@rijuma rijuma marked this pull request as ready for review January 19, 2024 15:01
@rijuma rijuma force-pushed the rijuma/add-active-courses-option-to-reindex_courses branch 2 times, most recently from 316c4ff to 2dd9fe8 Compare January 19, 2024 15:36
@rijuma rijuma force-pushed the rijuma/add-active-courses-option-to-reindex_courses branch 10 times, most recently from 92abf7c to 63a5e70 Compare January 22, 2024 16:21
ashultz0
ashultz0 reviewed Jan 22, 2024
View reviewed changes
cms/djangoapps/contentstore/management/commands/tests/test_reindex_courses.py Outdated
@@ -116,3 +124,13 @@ def test_given_all_key_prompts_and_reindexes_all_courses_cancelled(self):

patched_yes_no.assert_called_once_with(ReindexCommand.CONFIRMATION_PROMPT, default='no')
patched_index.assert_not_called()

def test_given_active_key_prompt(self):
""" Test that reindexes all courses when --active key is given """
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably test that it indexes only some of the courses by making one of the courses inactive

although below it only sets up first course so is that what the test is doing and the comment is wrong?

Copy link
Member Author

@rijuma rijuma Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I forgot to update the test. It actually reindex only the courses that are active, this is, courses that have a starting date and no end date or the end date is in the future. I'll update the comment.

@rijuma rijuma force-pushed the rijuma/add-active-courses-option-to-reindex_courses branch from 63a5e70 to 498df7c Compare January 22, 2024 19:54
@rijuma rijuma force-pushed the rijuma/add-active-courses-option-to-reindex_courses branch from 498df7c to df29da8 Compare January 22, 2024 19:57
@rijuma rijuma force-pushed the rijuma/add-active-courses-option-to-reindex_courses branch from df29da8 to a6dd0fe Compare January 22, 2024 19:58
@rijuma rijuma merged commit e00b79c into master Jan 23, 2024
64 checks passed
@rijuma rijuma deleted the rijuma/add-active-courses-option-to-reindex_courses branch January 23, 2024 00:09
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashultz0 ashultz0 ashultz0 approved these changes

@germanolleunlp germanolleunlp germanolleunlp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rijuma @edx-pipeline-bot @germanolleunlp @ashultz0