fix: use a single 'provider_type' key for storing discussion provider type in course - #36039
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @xitij2000! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
mphilbrick211
added
the
needs reviewer assigned
0x29a
left a comment
0x29a
left a comment
There was a problem hiding this comment.
👍
- I tested this: tried cloning a course with different discussion settings and ensured that they're preserved.
- I read through the code.
cbddd4e to
227be76
Compare
7059328 to
956f6c2
Compare
956f6c2 to
5f31c69
Compare
|
I think this makes sense to me, but I don't think I can provide an approving review. @xitij2000 is there an edx-platform CC within OpenCraft who can provide an approving review for your PR? |
openedx-webhooks
added
the
core contributor
Sorry for missing this comment. I'll check if @Agrendalath, @farhaanbukhsh or @navinkarkera can have a look. |
623c7f6 to
ddaac3d
Compare
|
@xitij2000 I wasn't able to get the testing instructions to work right. I tried to clone the course from cms.djangoapps.contentstore.views.course import create_new_course, get_course_and_check_access, rerun_course
from django.contrib.auth import get_user_model
from opaque_keys.edx.keys import CourseKey
user = get_user_model().objects.get(username="edx")
source_course_key = CourseKey.from_string("course-v1:lib101+cs101+2020")
destination_course_key = CourseKey.from_string(course-v1:lib101_clone+cs101+2020)
source_course_run = get_course_and_check_access(source_course_key, user)
fields = {
'display_name': source_course_run.display_name,
}
destination_course_run_key = rerun_course(
user, source_course_key, destination_course_key.org, destination_course_key.course,
destination_course_key.run, fields, background=False,
)
destination_course_run = get_course_and_check_access(destination_course_run_key, user)In the original course, I went to studio -> page and resources -> discussion and Hide the discussion tab but when I clone the Hide the discussion tab toggle is off. Am I getting this wrong? |
farhaanbukhsh
left a comment
farhaanbukhsh
left a comment
There was a problem hiding this comment.
Can you elaborate on the testing instructions please.
| if ( | ||
| ENABLE_NEW_STRUCTURE_DISCUSSIONS.is_enabled() | ||
| and not course.discussions_settings['provider_type'] == Provider.OPEN_EDX | ||
| and not course.discussions_settings['provider'] == Provider.OPEN_EDX |
There was a problem hiding this comment.
Nit: it would make sense to use .get() here, in case the course does not have this attribute.
… type in course Both 'provider' and 'provider_type' have been used for storing the discussion provider type in course 'discussions_settings' field, there are some places in the code checking for 'provider' and others checking for 'provider_type', in some cases this can cause a bug where it doesn't detect the correct provider which causes discussion settings not being copied correctly when a course is cloned. This change prioritises the `provider_type` setting over `provider` and reads `provider` only as a fallback. The `provider` setting is now made read-only just for backwards-compatibility, to avoid confusion.
ddaac3d to
4e97104
Compare
…rovider type in course
…ssion provider type in course
There was a problem hiding this comment.
👍
Thank you @xitij2000 for adding the comments explaining this decision.
- ✅ I tested this using tutor and clone course code
- ✅ I read through the code
- ❌ I checked for accessibility issues
- ✅ Includes documentation
openedx-webhooks
removed
the
needs reviewer assigned
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
|
2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production. |
|
2U Release Notice: This PR has been deployed to the edX production environment. |
1 similar comment
|
2U Release Notice: This PR has been deployed to the edX production environment. |
8 participants
Description
Both 'provider' and 'provider_type' have been used for storing the discussion provider type in course 'discussions_settings' field, there are some places in the code checking for 'provider' and others checking for 'provider_type', in some cases this can cause a bug where it doesn't detect the correct provider which causes discussion settings not being copied correctly when a course is cloned.
This change prioritises the
provider_typesetting overproviderand readsprovideronly as a fallback. Theprovidersetting is now made read-only just for backwards-compatibility, to avoid confusion.Testing instructions
studio.local.openedx.io:8001/api/v1/course_runs/clone/Deadline
None