openedx / openedx/openedx-platform
Notes not being saved with the correct course id in CCXs
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 8.2k
- Forks
- 4.4k
- Avg merge
- 6d 18h
- Merged PRs (30d)
- 42
Description
Description
Currently, when a note is created in a CCX (this process is handled by a decorator https://github.com/open-craft/edx-platform/blob/master/lms/djangoapps/edxnotes/decorators.py#L14-L69), it is saving the note using a versioned course_id (e.g. ccx-v1:edX+DemoX.1+2014+branch@published-branch+version@651d7121f4285e6db96715e0+ccx@1) and the right behavior, based on how notes for regular courses are saved and how it worked before, should be to save it using the CCX course_id without any versions (e.g. ccx-v1:edX+DemoX.1+2014+ccx@1).
Cause of the issue
I have a vague understanding of the context for applying the change that causes the issue. However, here is the PR that contains these changes (https://github.com/openedx/edx-platform/pull/30715/files#diff-aee892f54ee20b3aae56aceefea75775fdb3b1f77421976e7eb0eb626201936eR36-R37). From what I could understand, the way the code is now obtaining the course_key causes, when calling modulestore, to obtain a course block with the parameter id being a versioned CCXCourseLocator, then notes obtain the course_id string from this parameter causing it to save it wrongly.
Potential solution
The solution that we applied in our fork, consists on putting a conditional in the notes decorator to verify if the course is a CCX and if so, remove any versions from the course_key. The solution can be viewed in the following PR https://github.com/Pearson-Advance/edx-platform/pull/117/files.
Discarded solutions
We reviewed other places to put the fix and enforce the behavior lower. Those were xmodule.modulestore.DraftVersioningModuleStore.get_course and xmodule.course_module.CourseBlock.id, however these were discarded because it may affect other functionalities of the platform. It's worth to open the discussion of reviewing if is feasible to put the fix in these places or others than the discussed before.
Migration of the data
Finally, it's worth to note that instances of the platform which have notes with the issue should perform a migration process.
@pdpinch This issue aims to continue the conversation from slack about the issue.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in lms/djangoapps/edxnotes/decorators.py, especially the decorator linked in the issue, and compare the course_key handling with the referenced upstream and fork PRs. Verify that notes created for CCXs use the unversioned CCX course_id while regular-course behavior remains unchanged; also review the issue's migration note for existing incorrectly keyed data.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100