adding display_coursenumber instance to search index #873
Conversation
There was a problem hiding this comment.
Thanks @estherjsuh. Would it be possible to write tests for this fix? We can work on that next week during our 1:1s.
| @@ -553,6 +553,7 @@ class CourseAboutSearchIndexer(object): | |||
| AboutInfo("title", AboutInfo.ANALYSE | AboutInfo.PROPERTY, AboutInfo.FROM_ABOUT_INFO), | |||
| AboutInfo("university", AboutInfo.ANALYSE | AboutInfo.PROPERTY, AboutInfo.FROM_ABOUT_INFO), | |||
| AboutInfo("number", AboutInfo.ANALYSE | AboutInfo.PROPERTY, AboutInfo.FROM_COURSE_PROPERTY), | |||
| AboutInfo("display_coursenumber", AboutInfo.ANALYSE | AboutInfo.PROPERTY, AboutInfo.FROM_COURSE_PROPERTY), | |||
There was a problem hiding this comment.
Thanks @estherjsuh! Would it be possible to add a test for this new field?
If you're curious on tests on Juniper, you can follow this guide on how we use tox to run tests.
It seems like a good test case to add into the TestCoursewareSearchIndexer._test_course_about_store_index test case.
Once this fix is tested and merged, we should actually contribute it to the edX upstream repository. There's no reason to keep just on Tahoe. I've added a GitHub issue so we do it later (#874).
There was a problem hiding this comment.
@OmarIthawi That's a great idea! I think is a great opportunity for knowledge transfer next week. 💪
This is a companion PR of appsembler/edx-theme-codebase#147
Fixed the backed logic to be able to index
display_coursenumberon elasticsearch, and then being able to process the json response from elasticsearch response.