feat: use course cache to retrieve course data #72
Conversation
alangsto
force-pushed
the
alangsto/use_course_cache
branch
from
e60d711
to
c7a0567
Compare
| _, unit_content = get_block_content(request, user_id, course_id, unit_usage_key) | ||
| _, unit_content = get_block_content(request, user_id, course_run_id, unit_usage_key) | ||
|
|
||
| course_data = get_cache_course_data(course_id, ['skill_names', 'title']) |
There was a problem hiding this comment.
I am concerned about adding another potential blocking call to discovery to retrieve this data. If we don't include skill names, we would only need to call one cache (course run cache), not two. Skill names are only associated with courses, not course runs, which is why we need to call both caches right now.
alangsto
force-pushed
the
alangsto/use_course_cache
branch
2 times, most recently
from
f82293a
to
bc76158
Compare
alangsto
force-pushed
the
alangsto/use_course_cache
branch
from
bc76158
to
72f7c07
Compare
| course on edX. The discovery service is the source of truth for mappings between course run ID and course IDs, and | ||
| string manipulation cannot be relied on as an accurate way to map between the two forms of ID. | ||
|
|
||
| The learning-assistant CourseChatView NEED LINK accepts a course run ID as a query parameter, but parts of the python API defined |
There was a problem hiding this comment.
Do you need to add a link here?
There was a problem hiding this comment.
Yes, thanks for catching that
|
|
||
| Decision | ||
| ******** | ||
| In order to determine the mapping between a course run ID and course ID in the learning-assistant app, we will make |
There was a problem hiding this comment.
I think we should also mention the course cache to get the skill names and title.
tests/test_api.py
Outdated
| mock_get_content.return_value = (len(unit_content), unit_content) | ||
| mock_is_flag_enabled.return_value = flag_enabled | ||
| mock_cache.return_value = {'skill_names': ['skills'], 'title': ['title']} |
There was a problem hiding this comment.
I think we should assert that the title and skills appear in prompt_text somewhere. Do you think this is a good place for it?
There was a problem hiding this comment.
Yes, I can add that check!
alangsto
force-pushed
the
alangsto/use_course_cache
branch
from
72f7c07
to
95e25d8
Compare
| Decision | ||
| ******** | ||
| In order to determine the mapping between a course run ID and course ID in the learning-assistant app, we will make | ||
| use of an `existing course run cache that is defined in edx-platform`_. Similarly, to retrieve the skill names and title of a course, we will also use |
There was a problem hiding this comment.
This link isn't rendering properly because the text here and in the link definition are different (this one has "run" and the one below doesn't).
| ******** | ||
| In order to determine the mapping between a course run ID and course ID in the learning-assistant app, we will make | ||
| use of an `existing course run cache that is defined in edx-platform`_. Similarly, to retrieve the skill names and title of a course, we will also use | ||
| an `existing course cache`_. Both caches store data from the discovery API for course runs and courses, respectively. |
There was a problem hiding this comment.
It looks like this link is missing a definition below.
alangsto
force-pushed
the
alangsto/use_course_cache
branch
from
95e25d8
to
8a9f070
Compare
COSMO-22
The Xpert Platform course index does not include courses based on certain enrollment dates and other discovery fields, like
is_marketable. This means that Xpert is only available to a more limited set of courses.To increase the number of courses in which Xpert is available, we can opt to not rely on the Xpert Platform backend to inject the course title and course skills into the prompt template, and instead use an already-defined cache in the LMS to retrieve that same data.