[OldMongo FC-0004] Remove support for children in Old Mongo #31134
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @UvgenGen! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
UvgenGen
force-pushed
the
sagirov/EDXOLDMNG-146
branch
4 times, most recently
from
fd96327
to
7caebe7
Compare
UvgenGen
force-pushed
the
sagirov/EDXOLDMNG-146
branch
from
7caebe7
to
036a49a
Compare
UvgenGen
changed the title
There was a problem hiding this comment.
Okay, there is a whole lot of stuff here, and this review is not complete. But I wanted to give some feedback reasonably soon so you could start on the next iteration of this work.
First off, congratulations. Even just getting a passing build of a 5.5K PR like this takes a lot of work to pull off.
I did not anticipate the extent to which you had to skip tests in order to get this building. I know I made the decision that it was okay to disable tests that had never worked in Split, but I didn't realize so much core functionality like access control was included in these tests, and relying on the fact that the two modulestores more were mostly compatible in behavior.
I do think that many of these tests will need to be fixed as opposed to skipped entirely, and we're going to need to triage that. This will likely increase the scope of the project some more (FYI @e0d).
So my first request is to please make a separate pull request that has all the straightforward, non-controversial test conversions. These are the test files where:
- No tests are marked "skip".
- There are no PR comments/questions about logic changes.
- You're removing Old Mongo from ddt'd tests or just changing the one modulestore being used.
You can do that before digging into any of the questions I put on this PR review. If there are borderline cases where you're unsure, don't include them in the new PR just yet. I just want to separate out the really safe test changes to reduce some of the noise here–and to merge in the simple stuff as quickly as possible before there's more drift from master.
Once that's merged and this is rebased, I want to take a look at the broad areas of what gaps in testing we're going to see–access control, forums, drafts, instructor tasks, etc.
Thank you.
| @@ -76,6 +76,7 @@ def _create_certificate(self, enrollment_mode): | |||
| ) | |||
|
|
|||
|
|
|||
| @unittest.skip("OldMongo Deprecation") | |||
There was a problem hiding this comment.
Shouldn't the certificates tests still run the same as before? Does this PR break certs functionality for Old Mongo somehow?
There was a problem hiding this comment.
Problem in test_certificate_available_date test in case where past_date is False.
Looks like _check_message checker not correct, this checker expect one message ('Your grade and certificate will be ready after') for both cases but message with is_past flag False should be 'Congratulations! Your certificate is ready.'
I'll fix it
| with mock.patch('xmodule.modulestore.mongo.base.MongoKeyValueStore', mock.Mock(side_effect=Exception)): | ||
| with mock.patch( | ||
| 'xmodule.modulestore.split_mongo.caching_descriptor_system.SplitMongoKVS', mock.Mock(side_effect=Exception) | ||
| ): |
There was a problem hiding this comment.
We should also keep testing that course listings don't break with Old Mongo courses (they'll still appear on the Learner Dashboard, even if they're not clickable).
There was a problem hiding this comment.
We can't create the old mongo test course because update_item is deprecated and raised NotImplementedError
| self.block = self.store.create_child( | ||
| self.learner2.id, subsection.location, 'html', 'new_component' | ||
| ) |
There was a problem hiding this comment.
Why was it necessary to delete this?
There was a problem hiding this comment.
returned with changed name
cms/djangoapps/contentstore/management/commands/tests/test_cleanup_assets.py
Show resolved
Hide resolved
cms/djangoapps/contentstore/management/commands/tests/test_create_course.py
Show resolved
Hide resolved
| @@ -1884,6 +1884,7 @@ def test_no_due_dates(self): | |||
| assert status.HTTP_200_OK == response_status_code | |||
| assert [] == response_course_runs[0]['due_dates'] | |||
|
|
|||
| @skip("OldMongo Deprecation") | |||
There was a problem hiding this comment.
Okay, due dates are important. We'll probably have to dig into these.
| @@ -272,6 +273,7 @@ def test_certificates_with_user_filter(self): | |||
| response = self.client.get(url) | |||
| self.assertContains(response, "userFilter: 'student@example.com'") | |||
|
|
|||
| @skip("OldMongo Deprecation") | |||
There was a problem hiding this comment.
problem due to escaping course id in response: course\\u002Dv1:org.0+course_0+Run_0 instead of course-v1:org.0+course_0+Run_0
| CourseOverview.get_from_id(course.id) # Ensure course in cached in CourseOverviews | ||
| assert CourseOverview.objects.filter(id=course.id).exists() | ||
| assert CourseOverview.course_exists(course.id) | ||
|
|
||
| @ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo) |
There was a problem hiding this comment.
CourseOverview generation is one of the small handful of things that we will need to keep testing on both Modulestores, because CourseOverviews get regenerated whenever new attributes are added, and that process relies on being able to query the root CourseBlock of the courses in question.
There was a problem hiding this comment.
We can't create the old mongo test course because update_item is deprecated and raised NotImplementedError
There was a problem hiding this comment.
Can we mock the data at the Mongo layer–i.e. exercise Old Mongo read logic on the root CourseBlock by mocking what it gets back from the database? CourseOverview compatibility is one of the things that has to keep working, or it will cause operational errors the next time someone adds something to CourseOverviews, which may be months from now.
There was a problem hiding this comment.
We investigate possibility to mock draft course data, but in witch case CourseOverview can be regenerated if we removed all create/update methods for draft modulestore?
| course.mobile_available = False | ||
| with self.store.branch_setting(ModuleStoreEnum.Branch.draft_preferred): | ||
| self.store.update_item(course, ModuleStoreEnum.UserID.test) | ||
| # Create a course where mobile_available is True. |
There was a problem hiding this comment.
Like the note above, CourseOverviews here need to continue testing both modulestores.
There was a problem hiding this comment.
We can't create the old mongo test course because update_item is deprecated and raised NotImplementedError.
| self.course_3 = CourseFactory.create(default_store=ModuleStoreEnum.Type.mongo) | ||
| self.course_1 = CourseFactory.create() | ||
| self.course_2 = CourseFactory.create() | ||
| self.course_3 = CourseFactory.create() |
There was a problem hiding this comment.
FWIW, this change is fine to me. CourseOverview tests that test both modulestores should stay testing both modulestores, but if there's on like this that just assumes a modulestore, we might as well switch that one over to split by default (as you do here).
|
Merged #31224. Please rebase this PR. |
UvgenGen
force-pushed
the
sagirov/EDXOLDMNG-146
branch
6 times, most recently
from
8072604
to
56e0c27
Compare
UvgenGen
force-pushed
the
sagirov/EDXOLDMNG-146
branch
4 times, most recently
from
24b44b1
to
e20eaa8
Compare
UvgenGen
force-pushed
the
sagirov/EDXOLDMNG-146
branch
2 times, most recently
from
ca68c81
to
3fd7194
Compare
|
@Agrendalath: I think it's more a question of whether that 581f07c should have been necessary–i.e. why did the calls to |
|
@ormsbee, I ran
(('video.html',
{'autoadvance_enabled': False,
'branding_info': None,
'bumper_metadata': 'null',
'cdn_eval': False,
'cdn_exp_group': None,
'display_name': 'A Name',
'download_video_link': 'example.mp4',
'handout': None,
'hide_downloads': False,
'id': 'i4x-org_0-course_0-video-A_Name',
'is_embed': False,
'license': None,
'metadata': '{"autoAdvance": false, "autohideHtml5": false, "autoplay": '
'false, "captionDataDir": null, "completionEnabled": false, '
'"completionPercentage": 0.95, "duration": null, "end": 3610.0, '
'"generalSpeed": 1.0, "lmsRootURL": "http://localhost:8000", '
'"poster": null, "prioritizeHls": false, '
'"publishCompletionUrl": '
'"i4x://org.0/course_0/video/A_Name/publish_completion", '
'"recordedYoutubeIsAvailable": true, "savedVideoPosition": 0.0, '
'"saveStateEnabled": true, "saveStateUrl": '
'"i4x://org.0/course_0/video/A_Name/xmodule_handler/save_user_state", '
'"showCaptions": "true", "sources": ["example.mp4", '
'"example.webm"], "speed": null, "start": 3603.0, "streams": '
'"1.00:3_yD_cEKoCk", "transcriptAvailableTranslationsUrl": '
'"i4x://org.0/course_0/video/A_Name/transcriptavailable_translations", '
'"transcriptLanguage": "en", "transcriptLanguages": {"en": '
'"English"}, "transcriptTranslationUrl": '
'"i4x://org.0/course_0/video/A_Name/transcripttranslation/__lang__", '
'"ytApiUrl": "https://www.youtube.com/iframe_api", '
'"ytMetadataEndpoint": "", "ytTestTimeout": 1500}',
'poster': 'null',
'public_video_url': None,
'track': None,
'transcript_download_format': 'srt',
'transcript_download_formats_list': [{'display_name': 'SubRip (.srt) file',
'value': 'srt'},
{'display_name': 'Text (.txt) file',
'value': 'txt'}]}),
{})
(('video.html',
{'autoadvance_enabled': False,
'branding_info': None,
'bumper_metadata': 'null',
'cdn_eval': False,
'cdn_exp_group': None,
'display_name': 'A Name',
'download_video_link': 'example.mp4',
'handout': None,
'hide_downloads': False,
'id': 'i4x-org_0-course_0-video-A_Name',
'is_embed': False,
'license': None,
'metadata': '{"autoAdvance": false, "autohideHtml5": false, "autoplay": '
'false, "captionDataDir": null, "completionEnabled": false, '
'"completionPercentage": 0.95, "duration": null, "end": 3610.0, '
'"generalSpeed": 1.0, "lmsRootURL": "http://localhost:8000", '
'"poster": null, "prioritizeHls": false, '
'"publishCompletionUrl": '
'"/courses/org.0/course_0/Run_0/xblock/i4x:;_;_org.0;_course_0;_video;_A_Name/handler/publish_completion", '
'"recordedYoutubeIsAvailable": true, "savedVideoPosition": 0.0, '
'"saveStateEnabled": true, "saveStateUrl": '
'"/courses/org.0/course_0/Run_0/xblock/i4x:;_;_org.0;_course_0;_video;_A_Name/handler/xmodule_handler/save_user_state", '
'"showCaptions": "true", "sources": ["example.mp4", '
'"example.webm"], "speed": null, "start": 3603.0, "streams": '
'"1.00:3_yD_cEKoCk", "transcriptAvailableTranslationsUrl": '
'"/courses/org.0/course_0/Run_0/xblock/i4x:;_;_org.0;_course_0;_video;_A_Name/handler/transcript/available_translations", '
'"transcriptLanguage": "en", "transcriptLanguages": {"en": '
'"English"}, "transcriptTranslationUrl": '
'"/courses/org.0/course_0/Run_0/xblock/i4x:;_;_org.0;_course_0;_video;_A_Name/handler/transcript/translation/__lang__", '
'"ytApiUrl": "https://www.youtube.com/iframe_api", '
'"ytMetadataEndpoint": "", "ytTestTimeout": 1500}',
'poster': 'null',
'public_video_url': None,
'track': None,
'transcript_download_format': 'srt',
'transcript_download_formats_list': [{'display_name': 'SubRip (.srt) file',
'value': 'srt'},
{'display_name': 'Text (.txt) file',
'value': 'txt'}]}),
{})Current version (from this PR): (('video.html',
{'autoadvance_enabled': False,
'branding_info': None,
'bumper_metadata': 'null',
'cdn_eval': False,
'cdn_exp_group': None,
'display_name': 'A Name',
'download_video_link': 'example.mp4',
'handout': None,
'hide_downloads': False,
'id': 'A_Name',
'is_embed': False,
'license': None,
'metadata': '{"autoAdvance": false, "autohideHtml5": false, "autoplay": '
'false, "captionDataDir": null, "completionEnabled": false, '
'"completionPercentage": 0.95, "duration": null, "end": 3610.0, '
'"generalSpeed": 1.0, "lmsRootURL": "http://localhost:8000", '
'"poster": null, "prioritizeHls": false, '
'"publishCompletionUrl": '
'"/courses/course-v1:org.0+course_0+Run_0/xblock/block-v1:org.0+course_0+Run_0+type@video+block@A_Name/handler/publish_completion", '
'"recordedYoutubeIsAvailable": true, "savedVideoPosition": 0.0, '
'"saveStateEnabled": true, "saveStateUrl": '
'"/courses/course-v1:org.0+course_0+Run_0/xblock/block-v1:org.0+course_0+Run_0+type@video+block@A_Name/handler/xmodule_handler/save_user_state", '
'"showCaptions": "true", "sources": ["example.mp4", '
'"example.webm"], "speed": null, "start": 3603.0, "streams": '
'"1.00:3_yD_cEKoCk", "transcriptAvailableTranslationsUrl": '
'"/courses/course-v1:org.0+course_0+Run_0/xblock/block-v1:org.0+course_0+Run_0+type@video+block@A_Name/handler/transcript/available_translations", '
'"transcriptLanguage": "en", "transcriptLanguages": {"en": '
'"English"}, "transcriptTranslationUrl": '
'"/courses/course-v1:org.0+course_0+Run_0/xblock/block-v1:org.0+course_0+Run_0+type@video+block@A_Name/handler/transcript/translation/__lang__", '
'"ytApiUrl": "https://www.youtube.com/iframe_api", '
'"ytMetadataEndpoint": "", "ytTestTimeout": 1500}',
'poster': 'null',
'public_video_url': None,
'track': None,
'transcript_download_format': 'srt',
'transcript_download_formats_list': [{'display_name': 'SubRip (.srt) file',
'value': 'srt'},
{'display_name': 'Text (.txt) file',
'value': 'txt'}]}),
{})Version from before 581f07c: (('video.html',
{'autoadvance_enabled': False,
'branding_info': None,
'bumper_metadata': 'null',
'cdn_eval': False,
'cdn_exp_group': None,
'display_name': 'A Name',
'download_video_link': 'example.mp4',
'handout': None,
'hide_downloads': False,
'id': 'A_Name',
'is_embed': False,
'license': None,
'metadata': '{"autoAdvance": false, "autohideHtml5": false, "autoplay": '
'false, "captionDataDir": null, "completionEnabled": false, '
'"completionPercentage": 0.95, "duration": null, "end": 3610.0, '
'"generalSpeed": 1.0, "lmsRootURL": "http://localhost:8000", '
'"poster": null, "prioritizeHls": false, '
'"publishCompletionUrl": '
'"/courses/course-v1:org.0+course_0+Run_0/xblock/block-v1:org.0+course_0+Run_0+type@video+block@A_Name/handler/publish_completion", '
'"recordedYoutubeIsAvailable": true, "savedVideoPosition": 0.0, '
'"saveStateEnabled": true, "saveStateUrl": '
'"/courses/course-v1:org.0+course_0+Run_0/xblock/block-v1:org.0+course_0+Run_0+type@video+block@A_Name/handler/xmodule_handler/save_user_state", '
'"showCaptions": "true", "sources": ["example.mp4", '
'"example.webm"], "speed": null, "start": 3603.0, "streams": '
'"1.00:3_yD_cEKoCk", "transcriptAvailableTranslationsUrl": '
'"/courses/course-v1:org.0+course_0+Run_0/xblock/block-v1:org.0+course_0+Run_0+type@video+block@A_Name/handler/transcript/available_translations", '
'"transcriptLanguage": "en", "transcriptLanguages": {"en": '
'"English"}, "transcriptTranslationUrl": '
'"/courses/course-v1:org.0+course_0+Run_0/xblock/block-v1:org.0+course_0+Run_0+type@video+block@A_Name/handler/transcript/translation/__lang__", '
'"ytApiUrl": "https://www.youtube.com/iframe_api", '
'"ytMetadataEndpoint": "", "ytTestTimeout": 1500}',
'poster': 'null',
'public_video_url': None,
'track': None,
'transcript_download_format': 'srt',
'transcript_download_formats_list': [{'display_name': 'SubRip (.srt) file',
'value': 'srt'},
{'display_name': 'Text (.txt) file',
'value': 'txt'}]}),
{})
<div class="aside-block"
data-success-class="fa fa-check-square-o fa-lg pass"
data-failure-class="fa fa-times fa-lg fail"
data-error-class="fa fa-exclamation-triangle fa-lg error"
data-unknown-class="fa fa-question-circle fa-lg unknown"
data-local-resource-url="/static/xblock/resources/acid/public/test_data.json"
>
<button>Acid Aside for block-v1:org.0+course_0+Run_0+type@video+block@A_Name</button>
<div>
<p>JS init function run:
<span class="js-init-run">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</p>
<p>Resource Url Test:
<span class="local-resource-test">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</p>
<table class='storage-tests'>
<tr>
<th>Scope</th>
<th>Server-side<br>handler_url<br>returned</th>
<th>Server-side<br>handler_url<br>succeeded</th>
<th>Client-side<br>handler_url<br>returned</th>
<th>Client-side<br>handler_url<br>succeeded</th>
</tr>
<tr class="scope-storage-test scope-user_state "
data-handler-url="/courses/course-v1:org.0+course_0+Run_0/xblock/aside-usage-v2:block-v1$:org.0+course_0+Run_0+type@video+block@A_Name::acid_aside/handler/check_storage/SUFFIX1465?QUERY=1465&SCOPE=user_state"
data-scope="user_state"
data-value="1465"
>
<td>user_state</td>
<td>
<span class="server-storage-test-returned">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="server-storage-test-succeeded">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="client-storage-test-returned">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="client-storage-test-succeeded">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
</tr>
<tr class="scope-storage-test scope-preferences alt"
data-handler-url="/courses/course-v1:org.0+course_0+Run_0/xblock/aside-usage-v2:block-v1$:org.0+course_0+Run_0+type@video+block@A_Name::acid_aside/handler/check_storage/SUFFIX7096?QUERY=7096&SCOPE=preferences"
data-scope="preferences"
data-value="7096"
>
<td>preferences</td>
<td>
<span class="server-storage-test-returned">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="server-storage-test-succeeded">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="client-storage-test-returned">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="client-storage-test-succeeded">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
</tr>
<tr class="scope-storage-test scope-user_info "
data-handler-url="/courses/course-v1:org.0+course_0+Run_0/xblock/aside-usage-v2:block-v1$:org.0+course_0+Run_0+type@video+block@A_Name::acid_aside/handler/check_storage/SUFFIX4376?QUERY=4376&SCOPE=user_info"
data-scope="user_info"
data-value="4376"
>
<td>user_info</td>
<td>
<span class="server-storage-test-returned">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="server-storage-test-succeeded">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="client-storage-test-returned">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="client-storage-test-succeeded">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
</tr>
<tr class="scope-storage-test scope-user_state_summary alt"
data-handler-url="/courses/course-v1:org.0+course_0+Run_0/xblock/aside-usage-v2:block-v1$:org.0+course_0+Run_0+type@video+block@A_Name::acid_aside/handler/check_storage/SUFFIX2238?QUERY=2238&SCOPE=user_state_summary"
data-scope="user_state_summary"
data-value="2238"
>
<td>user_state_summary</td>
<td>
<span class="server-storage-test-returned">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="server-storage-test-succeeded">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="client-storage-test-returned">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
<td>
<span class="client-storage-test-succeeded">
<i class="fa fa-question-circle fa-lg unknown"></i>
</span>
</td>
</tr>
</table>
<p>Sees Acid DOM: <span class="acid-dom"><i class="fa fa-question-circle fa-lg unknown"></i></span></p>
</div>
</div>It seems that #31472 changed the URLs in this context, but it didn't trigger any test failures. Therefore, the behavior change of |
|
@ormsbee there are several production issues in-flight right now, I will have to wait to merge this until the impact from those issues dies down a bit. |
UvgenGen
force-pushed
the
sagirov/EDXOLDMNG-146
branch
from
581f07c
to
35f6bec
Compare
|
@ormsbee PR is rebased but I have an error with JS tests. Looks like this pipeline needs to be restarted |
UvgenGen
force-pushed
the
sagirov/EDXOLDMNG-146
branch
from
35f6bec
to
822ebb4
Compare
| @@ -117,7 +116,7 @@ def mocked_sign(self, *args, **kwargs): | |||
| self.addCleanup(patcher.stop) | |||
|
|
|||
| def test_lti_constructor(self): | |||
| generated_content = self.block.render(STUDENT_VIEW).content | |||
| generated_content = self.block.student_view(None).content | |||
There was a problem hiding this comment.
This is the one thing that's holding up this PR for me at this point. Why does self.block.render(STUDENT_VIEW).content no longer work?
There was a problem hiding this comment.
Student view content has been updated (html was added in content), it is fully described here: #31134 (comment)
That's why I use a similar method
There was a problem hiding this comment.
Okay, I realize I'm probably being very dense here, but why was html added in the content?
My basic concern is that we've somehow changed the behavior of the render method, which doesn't seem like it should be necessary when removing Old Mongo functionality. If this PR does change the render method behavior, why does it do that? If this PR does not change the render method behavior, why is this change to the test necessary?
I get that HTML was added to the content, but I don't understand why HTML was added to the content.
Thank you.
There was a problem hiding this comment.
@UvgenGen: Do you have time to chat about this in the next couple of days? I think maybe I'm just not understanding it properly. Please ping me on Slack when you have a moment.
UvgenGen
force-pushed
the
sagirov/EDXOLDMNG-146
branch
from
822ebb4
to
0919907
Compare
UvgenGen
force-pushed
the
sagirov/EDXOLDMNG-146
branch
2 times, most recently
from
bab0bdf
to
95c1596
Compare
UvgenGen
force-pushed
the
sagirov/EDXOLDMNG-146
branch
from
95c1596
to
ede39e1
Compare
|
@UvgenGen 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
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. |
This commit leaves behind just enough Old Mongo (DraftModulestore)
functionality to allow read-only access to static assets and the
root CourseBlock. It removes:
It also removes or converts tests for this functionality.
The ability to read from the root CourseBlock was maintained for
backwards compatibility, since top-level course settings are often
stored here, and this is used by various parts of the codebase,
like displaying dashboards and re-building CourseOverview models.
Any attempt to read the contents of a course by getting the
CourseBlock's children will return an empty list (i.e. it will look
empty).
This commit does not delete content on MongoDB or run any sort of
data migration or cleanup.
Useful information to include:
openedx/public-engineering#80