[OldMongo FC-0004] Disable editing of Old Mongo courses in Studio #30933

UvgenGen · 2022-09-01T14:14:41Z

Description

This removes user-facing Studio edit support for Old Mongo courses (courses that have a CourseKey of the format {org}/{course}/{run}). This does not affect our normal courses, which have CourseKeys starting with "course-v1:". After this commit:

Old Mongo courses will continue to appear on the Studio course listing page, but are not clickable.
Any attempt to directly access an Old Mongo course in Studio via URL fail with a 404 error.
Course certificates will still be available for Old Mongo courses
Old Mongo courses will continue to be returned by CourseOverviews and get_course_summaries() calls.

We decided against removing Old Mongo courses from the listing entirely because that would require very expensive CourseOverviews query to filter them out. Making that query more efficient would involve a database migration to add appropriate indexing, which is something else that we are looking to avoid. In general, we want to avoid changing how CourseOverviews work, in order to minimize risk.

This is part of the Old Mongo Modulestore deprecation effort: openedx/public-engineering#62

openedx-webhooks · 2022-09-01T14:14:45Z

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:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

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.

natabene · 2022-09-07T02:48:11Z

@UvgenGen Thank you for the contribution. @ormsbee I hear you will be reviewing, please ping TNL team when ready with your review.

ormsbee

First pass review done. Mostly asking to reduce the scope of the changes to preserve some of the metadata being returned and to avoid potential performance issues around CourseOverview.

ormsbee · 2022-09-07T20:48:33Z

openedx/core/djangoapps/content/course_overviews/models.py

+        deprecated_course_ids = [id for id in course_overviews.values_list('id', flat=True) if id.deprecated]
+        course_overviews = course_overviews.exclude(id__in=deprecated_course_ids)


This operation will be very expensive on 2U environments that have hundreds or thousands of Old Mongo courses. Is it possible to mirror the LMS approach on the learner dashboard, where the courses still appear, but the links no longer work? That would mean leaving get_all_courses the way it is for now.

I've updated this like in the LMS for the learner dashboard:

ormsbee · 2022-09-08T02:59:47Z

cms/djangoapps/contentstore/views/course.py

+                course_key = CourseKey.from_string(course_key_string)
+                # raise 404 if old mogo course
+                if course_key.deprecated:
+                    raise Http404


A few small notes/suggestions:

Since we're doing a blanket 404 for all Old Mongo courses, we can do the conditional at the top, at the beginning of the try block.

It's probably worth logging what the course_key is, in case we want to track down what courses people are still trying to access for some reason.

Nit: Just for consistency with what's already there, please use HttpResopnseNotFound() instead of Http404.

So maybe something like:

try: if course_key_string: course_key = CourseKey.from_string(course_key_string) if course_key.deprecated: logging.error(f"User {request.user.id} tried to access Studio for Old Mongo course {course_key}.") raise HttpResponseNotFound() # or maybe HttpResponseBadRequest?

applied your recommendation

ormsbee · 2022-09-08T03:03:20Z

xmodule/modulestore/mixed.py

@@ -295,6 +295,11 @@ def get_course_summaries(self, **kwargs):
                    log.warning(
                        "Modulestore %s have duplicate courses %s; skipping from result.", store, course_id
                    )
+                # Skip course if it's from old mongo modulestore.
+                elif course_id.deprecated:


Let's continue to return the course summary for now. We should be able to grab that information from the root CourseBlock anyhow.

lms/djangoapps/course_api/tests/test_views.py

ormsbee · 2022-09-09T16:38:15Z

Please let me know if (or when) this is ready for a second review.

UvgenGen · 2022-09-12T08:25:33Z

@ormsbee Yes, this PR is ready for a second review. All comments updated

ormsbee

Question about course summaries, UI tweak request, and test commenting.

ormsbee · 2022-09-12T13:30:29Z

cms/djangoapps/contentstore/tests/test_contentstore.py

@@ -1879,6 +1891,9 @@ def post_rerun_request(
        rerun_course_data.update(destination_course_data)
        destination_course_key = _get_course_id(self.store, destination_course_data)

+        if destination_course_key.deprecated:
+            raise SkipTest('OldMongo Deprecation')


Do we ever run a test here with the destination course key as an Old Mongo course? I didn't think our system ever supported re-running an Old Mongo course as another Old Mongo course, and if we have any tests that do that, we should probably remove them.

Could you please add a comment explaining how/why this happens?

ormsbee · 2022-09-12T13:30:42Z

cms/djangoapps/contentstore/tests/test_contentstore.py

+    if course_key.deprecated:
+        raise SkipTest('OldMongo Deprecation')


Could you please add a comment explaining how/why this happens?

Skips is added due to the return of HttpResponseNotFound in course_handler for old mongo courses. Comment added.
Skips will be removed in future PRs.

ormsbee · 2022-09-12T13:38:25Z

cms/djangoapps/contentstore/views/course.py

+        if course.id.deprecated:
+            course_context.update({
+                'url': None,
+                'lms_link': None,
+                'rerun_link': None
+            })
+        return course_context


I assume this is the code that results in:

That text for the last course still looks clickable. Is that because it's still making the <a> tag, but with a blank href? If so, can we not make the tag when there's no link?

FYI @jmakowski1123, @JAAkana, @jristau1984 on how this is presented to the user.

@ormsbee I may not be completely up to speed on what the end goal is for course authors, but if I'm reading this thread correctly, the Old Mongo courses are archived and only displaying in the Archived Courses tab as a means to mitigate cost, as opposed to disappearing completely. If that's the case, is the (what looks like?) a url to rerun the course also confusing for the author? Or can the course actually be rerun? Can authors access the archived content? If the answer to all of the above is no, then displaying all that text as grayed out is a better UX option I think?

In general, archived courses can be viewed in Studio. I think that the Old Mongo course is the last one here (with no "Re-run Course" or "View Live" link). Old Mongo courses will not be re-runnable or viewable after this merges. The fact that they show up in this list at all is to mitigate cost and risk.

I updated CourseOrLibraryListing and now OldMongo courses renders without <a> tag:

@UvgenGen: Thank you for the update.

@jmakowski1123, @jristau1984, @JAAkana: Unless there are objections by tomorrow, I'm planning to merge this PR with the UI pictured above (where Old Mongo courses display like the third course in the above screenshot). We can adjust the UI later if it's important to do so (e.g. add some kind of extra messaging?). If you believe this approach will impose too much of a burden on support teams and needs to be addressed immediately, please let me know.

Again, the reason we're doing it this way is to make the smallest change possible to disable Studio access to these courses. Filtering out all Old Mongo courses at the database layer is a potentially very expensive operation (because the database tables are not properly indexed for that), and we're trying to avoid doing database migrations during this work to minimize risk. Cosmetic changes like the above where links are removed are simpler and have a much smaller chance of causing regressions elsewhere.

So @jristau1984 checked with Jon Fay:

Jonathan Fay [10:18 AM]
I think it’s ok for now. This page will get a redesign at some point.

Still waiting until TNL is fully staffed tomorrow before merge.

ormsbee · 2022-09-12T13:40:59Z

xmodule/modulestore/tests/test_semantics.py

+        with self.store.branch_setting(branch_setting=branch):
+            course_summaries = self.store.get_course_summaries()
+            # get_course_summaries skip old mongo courses
+            assert len(course_summaries) == 0


The course summaries should still be generated for Old Mongo courses, shouldn't they? This was the basic metadata like display_name?

Oh yes you are right, course summaries should still be generated for Old Mongo courses. I'll update it.

ormsbee · 2022-09-13T14:25:22Z

@UvgenGen: Paranoid question, but this doesn't affect the display of content libraries at all, does it?

UvgenGen · 2022-09-14T08:22:33Z

@UvgenGen: Paranoid question, but this doesn't affect the display of content libraries at all, does it?

@ormsbee This may affect the display of content libraries if the library reference in the context is None. But libraries should always have a link. Otherwise, it will simply not be active as for the course.
Library with the link:

openedx-webhooks · 2022-09-14T14:53:37Z

@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.

edx-pipeline-bot · 2022-09-14T16:17:37Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

edx-pipeline-bot · 2022-09-14T17:20:13Z

EdX Release Notice: This PR has been deployed to the production environment.

0x29a · 2023-11-10T21:56:50Z

Hi @ormsbee and @UvgenGen, is it in theory possible to update Old Mongo course so it's editable after this change?

Update: it seems I can use the management command that has been removed here.

ormsbee · 2023-11-11T18:43:36Z

@0x29a: Yeah, you can make a re-run of it using the code you cited, but the original would not be editable.

feat: Do not display Old Mongo courses in Studio

e41400b

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 1, 2022

UvgenGen force-pushed the sagirov/EDXOLDMNG-62 branch 2 times, most recently from 772b234 to 01d27ee Compare September 2, 2022 09:14

test: update tests

0c6272a

UvgenGen force-pushed the sagirov/EDXOLDMNG-62 branch from 01d27ee to 0c6272a Compare September 2, 2022 11:11

GlugovGrGlib mentioned this pull request Sep 6, 2022

[WIP] EDXOLDMNG-62 Do not display Old Mongo courses in Studio raccoongang/edx-platform#2449

Closed

UvgenGen mentioned this pull request Sep 7, 2022

[OldMongo FC-0004] Remove code related to Old -> Split migration + re-running Old Mongo courses as Split #30955

Merged

GlugovGrGlib mentioned this pull request Sep 7, 2022

[WIP] EDXOLDMNG Remove code related to Old -> Split migration + re-running Old Mongo courses as Split raccoongang/edx-platform#2453

Closed

ormsbee self-assigned this Sep 7, 2022

UvgenGen changed the title ~~[DEPR] Do not display Old Mongo courses in Studio~~ [Fc-0004] Do not display Old Mongo courses in Studio Sep 7, 2022

UvgenGen changed the title ~~[Fc-0004] Do not display Old Mongo courses in Studio~~ [OldMongo FC-0004] Do not display Old Mongo courses in Studio Sep 7, 2022

ormsbee requested changes Sep 8, 2022

View reviewed changes

feat: code review

8f354ab

ormsbee assigned UvgenGen and unassigned ormsbee Sep 11, 2022

ormsbee requested changes Sep 12, 2022

View reviewed changes

feat: update tests and CourseOrLibraryListing

de0a878

ormsbee approved these changes Sep 14, 2022

View reviewed changes

ormsbee merged commit b429e55 into openedx:master Sep 14, 2022

ormsbee changed the title ~~[OldMongo FC-0004] Do not display Old Mongo courses in Studio~~ [OldMongo FC-0004] Disable editing of Old Mongo courses in Studio Sep 14, 2022

ormsbee mentioned this pull request Sep 7, 2023

[DEPR]: DraftModuleStore (Old Mongo Modulestore) openedx/public-engineering#62

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OldMongo FC-0004] Disable editing of Old Mongo courses in Studio #30933

[OldMongo FC-0004] Disable editing of Old Mongo courses in Studio #30933

UvgenGen commented Sep 1, 2022 •

edited by ormsbee

Loading

openedx-webhooks commented Sep 1, 2022

natabene commented Sep 7, 2022

ormsbee left a comment

ormsbee Sep 7, 2022

UvgenGen Sep 9, 2022

ormsbee Sep 8, 2022

UvgenGen Sep 9, 2022

ormsbee Sep 8, 2022

ormsbee commented Sep 9, 2022

UvgenGen commented Sep 12, 2022

ormsbee left a comment

ormsbee Sep 12, 2022

ormsbee Sep 12, 2022

UvgenGen Sep 13, 2022

ormsbee Sep 12, 2022

jmakowski1123 Sep 12, 2022

ormsbee Sep 13, 2022 •

edited

Loading

UvgenGen Sep 13, 2022

ormsbee Sep 13, 2022 •

edited

Loading

ormsbee Sep 13, 2022

ormsbee Sep 12, 2022

UvgenGen Sep 13, 2022

UvgenGen Sep 13, 2022

ormsbee commented Sep 13, 2022

UvgenGen commented Sep 14, 2022 •

edited

Loading

openedx-webhooks commented Sep 14, 2022

edx-pipeline-bot commented Sep 14, 2022

edx-pipeline-bot commented Sep 14, 2022

0x29a commented Nov 10, 2023 •

edited

Loading

ormsbee commented Nov 11, 2023

		deprecated_course_ids = [id for id in course_overviews.values_list('id', flat=True) if id.deprecated]
		course_overviews = course_overviews.exclude(id__in=deprecated_course_ids)

		if course_key.deprecated:
		raise SkipTest('OldMongo Deprecation')

[OldMongo FC-0004] Disable editing of Old Mongo courses in Studio #30933

[OldMongo FC-0004] Disable editing of Old Mongo courses in Studio #30933

Conversation

UvgenGen commented Sep 1, 2022 • edited by ormsbee Loading

Description

openedx-webhooks commented Sep 1, 2022

natabene commented Sep 7, 2022

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee commented Sep 9, 2022

UvgenGen commented Sep 12, 2022

ormsbee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee Sep 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ormsbee commented Sep 13, 2022

UvgenGen commented Sep 14, 2022 • edited Loading

openedx-webhooks commented Sep 14, 2022

edx-pipeline-bot commented Sep 14, 2022

edx-pipeline-bot commented Sep 14, 2022

0x29a commented Nov 10, 2023 • edited Loading

ormsbee commented Nov 11, 2023

UvgenGen commented Sep 1, 2022 •

edited by ormsbee

Loading

ormsbee Sep 13, 2022 •

edited

Loading

ormsbee Sep 13, 2022 •

edited

Loading

UvgenGen commented Sep 14, 2022 •

edited

Loading

0x29a commented Nov 10, 2023 •

edited

Loading