Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Sep 14, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions cms/djangoapps/contentstore/tests/test_contentstore.py
Original file line number Diff line number Diff line change
Expand Up @@ -1485,6 +1485,12 @@ def test_course_overview_view_with_course(self):
"""Test viewing the course overview page with an existing course"""
course = CourseFactory.create()
resp = self._show_course_overview(course.id)

# course_handler raise 404 for old mongo course
if course.id.deprecated:
self.assertEqual(resp.status_code, 404)
return

self.assertContains(
resp,
'<article class="outline outline-complex outline-course" data-locator="{locator}" data-course-key="{course_key}">'.format( # lint-amnesty, pylint: disable=line-too-long
Expand Down Expand Up @@ -1550,6 +1556,12 @@ def test_get_html(handler):
course_key = course_items[0].id

resp = self._show_course_overview(course_key)

# course_handler raise 404 for old mongo course
if course_key.deprecated:
self.assertEqual(resp.status_code, 404)
return

self.assertEqual(resp.status_code, 200)
self.assertContains(resp, 'Chapter 2')

Expand Down Expand Up @@ -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')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


# post the request
course_url = get_url('course_handler', destination_course_key, 'course_key_string')
response = self.client.ajax_post(course_url, rerun_course_data)
Expand Down Expand Up @@ -2198,6 +2213,9 @@ def _create_course(test, course_key, course_data):
"""
Creates a course via an AJAX request and verifies the URL returned in the response.
"""
if course_key.deprecated:
raise SkipTest('OldMongo Deprecation')
Comment on lines +2218 to +2219
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


course_url = get_url('course_handler', course_key, 'course_key_string')
response = test.client.ajax_post(course_url, course_data)
test.assertEqual(response.status_code, 200)
Expand Down
3 changes: 2 additions & 1 deletion cms/djangoapps/contentstore/tests/test_course_listing.py
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@ def test_staff_course_listing(self, default_store, mongo_calls):

# Fetch accessible courses list & verify their count
courses_list_by_staff, __ = get_courses_accessible_to_user(self.request)

self.assertEqual(len(list(courses_list_by_staff)), TOTAL_COURSES_COUNT)

# Verify fetched accessible courses list is a list of CourseSummery instances
Expand All @@ -194,7 +195,7 @@ def test_staff_course_listing(self, default_store, mongo_calls):
with check_mongo_calls(mongo_calls):
list(_accessible_courses_summary_iter(self.request))

@ddt.data(ModuleStoreEnum.Type.split, ModuleStoreEnum.Type.mongo)
@ddt.data(ModuleStoreEnum.Type.split)
def test_get_course_list_with_invalid_course_location(self, store):
"""
Test getting courses with invalid course location (course deleted from modulestore).
Expand Down
14 changes: 13 additions & 1 deletion cms/djangoapps/contentstore/views/course.py
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,11 @@ def course_handler(request, course_key_string=None):
json: delete this branch from this course (leaving off /branch/draft would imply delete the course)
"""
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}.")
return HttpResponseNotFound()
response_format = request.GET.get('format') or request.POST.get('format') or 'html'
if response_format == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'):
if request.method == 'GET':
Expand Down Expand Up @@ -783,7 +788,7 @@ def format_course_for_view(course):
"""
Return a dict of the data which the view requires for each course
"""
return {
course_context = {
'display_name': course.display_name,
'course_key': str(course.location.course_key),
'url': reverse_course_url('course_handler', course.id),
Expand All @@ -793,6 +798,13 @@ def format_course_for_view(course):
'number': course.display_number_with_default,
'run': course.location.run
}
if course.id.deprecated:
course_context.update({
'url': None,
'lms_link': None,
'rerun_link': None
})
return course_context
Comment on lines +801 to +807
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this is the code that results in:

189369488-1c324219-e1c8-4b93-a25b-b31a8f3aa7a5-1

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ormsbee ormsbee Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@ormsbee ormsbee Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


in_process_action_course_keys = {uca.course_key for uca in in_process_course_actions}
active_courses = []
Expand Down
33 changes: 27 additions & 6 deletions cms/djangoapps/contentstore/views/tests/test_course_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

import datetime
import json
from unittest import mock
from unittest import SkipTest, mock, skip
from unittest.mock import patch

import ddt
Expand Down Expand Up @@ -58,15 +58,15 @@ def setUp(self):
display_name='dotted.course.name-2',
)

def check_courses_on_index(self, authed_client):
def check_courses_on_index(self, authed_client, expected_course_tab_len):
"""
Test that the React course listing is present.
"""
index_url = '/home/'
index_response = authed_client.get(index_url, {}, HTTP_ACCEPT='text/html')
parsed_html = lxml.html.fromstring(index_response.content)
courses_tab = parsed_html.find_class('react-course-listing')
self.assertEqual(len(courses_tab), 1)
self.assertEqual(len(courses_tab), expected_course_tab_len)

def test_libraries_on_index(self):
"""
Expand Down Expand Up @@ -100,7 +100,7 @@ def test_is_staff_access(self):
"""
Test that people with is_staff see the courses and can navigate into them
"""
self.check_courses_on_index(self.client)
self.check_courses_on_index(self.client, 1)

def test_negative_conditions(self):
"""
Expand All @@ -110,7 +110,10 @@ def test_negative_conditions(self):
# register a non-staff member and try to delete the course branch
non_staff_client, _ = self.create_non_staff_authed_user_client()
response = non_staff_client.delete(outline_url, {}, HTTP_ACCEPT='application/json')
self.assertEqual(response.status_code, 403)
if self.course.id.deprecated:
self.assertEqual(response.status_code, 404)
else:
self.assertEqual(response.status_code, 403)

def test_course_staff_access(self):
"""
Expand All @@ -128,9 +131,10 @@ def test_course_staff_access(self):
)

# test access
self.check_courses_on_index(course_staff_client)
self.check_courses_on_index(course_staff_client, 1)

def test_json_responses(self):

outline_url = reverse_course_url('course_handler', self.course.id)
chapter = ItemFactory.create(parent_location=self.course.location, category='chapter', display_name="Week 1")
lesson = ItemFactory.create(parent_location=chapter.location, category='sequential', display_name="Lesson 1")
Expand All @@ -142,6 +146,11 @@ def test_json_responses(self):
ItemFactory.create(parent_location=subsection.location, category="video", display_name="My Video")

resp = self.client.get(outline_url, HTTP_ACCEPT='application/json')

if self.course.id.deprecated:
self.assertEqual(resp.status_code, 404)
return

json_response = json.loads(resp.content.decode('utf-8'))

# First spot check some values in the root response
Expand Down Expand Up @@ -307,6 +316,11 @@ def test_course_outline_with_display_course_number_as_none(self):
course_outline_url = reverse_course_url('course_handler', updated_course.id)
response = self.client.get_html(course_outline_url)

# course_handler raise 404 for old mongo course
if self.course.id.deprecated:
self.assertEqual(response.status_code, 404)
return

# Assert that response code is 200.
self.assertEqual(response.status_code, 200)

Expand Down Expand Up @@ -401,6 +415,7 @@ def check_index_page(self, separate_archived_courses, org):
archived_course_tab = parsed_html.find_class('archived-courses')
self.assertEqual(len(archived_course_tab), 1 if separate_archived_courses else 0)

@skip('Skip test for old mongo course')
@ddt.data(
# Staff user has course staff access
(True, 'staff', None, 0, 20),
Expand Down Expand Up @@ -469,6 +484,10 @@ def test_json_responses(self, is_concise):
outline_url = reverse_course_url('course_handler', self.course.id)
outline_url = outline_url + '?format=concise' if is_concise else outline_url
resp = self.client.get(outline_url, HTTP_ACCEPT='application/json')
if self.course.id.deprecated:
self.assertEqual(resp.status_code, 404)
return

json_response = json.loads(resp.content.decode('utf-8'))

# First spot check some values in the root response
Expand Down Expand Up @@ -613,6 +632,8 @@ def test_proctoring_link_is_visible(self, mock_validate_proctoring_settings):
"""
Test to check proctored exam settings mfe url is rendering properly
"""
if self.course.id.deprecated:
raise SkipTest("Skip test for old mongo course")
mock_validate_proctoring_settings.return_value = [
{
'key': 'proctoring_provider',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""
Exam Settings View Tests
"""

from unittest import SkipTest
from unittest.mock import patch

import ddt
Expand Down Expand Up @@ -104,6 +104,10 @@ def test_exam_settings_alert_with_exam_settings_enabled(self, page_handler):
self.course.enable_proctored_exams = True
self.save_course()

# course_handler raise 404 for old mongo course
if self.course.id.deprecated:
raise SkipTest("course_handler raise 404 for old mongo course")

url = reverse_course_url(page_handler, self.course.id)
resp = self.client.get(url, HTTP_ACCEPT='text/html')
alert_text = self._get_exam_settings_alert_text(resp.content)
Expand Down Expand Up @@ -139,6 +143,9 @@ def test_exam_settings_alert_with_exam_settings_disabled(self, page_handler):
self.course.enable_proctored_exams = True
self.save_course()

# course_handler raise 404 for old mongo course
if self.course.id.deprecated and page_handler == 'course_handler':
raise SkipTest("course_handler raise 404 for old mongo course")
url = reverse_course_url(page_handler, self.course.id)
resp = self.client.get(url, HTTP_ACCEPT='text/html')
alert_text = self._get_exam_settings_alert_text(resp.content)
Expand All @@ -163,6 +170,9 @@ def test_exam_settings_alert_not_shown(self, page_handler):
"""
Alert should not be visible if no proctored exam setting error exists
"""
# course_handler raise 404 for old mongo course
if self.course.id.deprecated and page_handler == 'course_handler':
raise SkipTest("course_handler raise 404 for old mongo course")
url = reverse_course_url(page_handler, self.course.id)
resp = self.client.get(url, HTTP_ACCEPT='text/html')
parsed_html = lxml.html.fromstring(resp.content)
Expand Down
14 changes: 13 additions & 1 deletion cms/djangoapps/contentstore/views/tests/test_header_menu.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
"""
Course Header Menu Tests.
"""

from unittest import SkipTest

from django.conf import settings
from django.test.utils import override_settings
Expand Down Expand Up @@ -37,6 +37,9 @@ def test_header_menu_without_web_certs_enabled(self):
Tests course header menu should not have `Certificates` menu item
if course has not web/HTML certificates enabled.
"""
# course_handler raise 404 for old mongo course
if self.course.id.deprecated:
raise SkipTest("course_handler raise 404 for old mongo course")
self.course.cert_html_view_enabled = False
self.save_course()
outline_url = reverse_course_url('course_handler', self.course.id)
Expand All @@ -49,6 +52,9 @@ def test_header_menu_with_web_certs_enabled(self):
Tests course header menu should have `Certificates` menu item
if course has web/HTML certificates enabled.
"""
# course_handler raise 404 for old mongo course
if self.course.id.deprecated:
raise SkipTest("course_handler raise 404 for old mongo course")
outline_url = reverse_course_url('course_handler', self.course.id)
resp = self.client.get(outline_url, HTTP_ACCEPT='text/html')
self.assertEqual(resp.status_code, 200)
Expand All @@ -60,6 +66,9 @@ def test_header_menu_without_exam_settings_enabled(self):
Tests course header menu should not have `Exam Settings` menu item
if course does not have the Exam Settings view enabled.
"""
# course_handler raise 404 for old mongo course
if self.course.id.deprecated:
raise SkipTest("course_handler raise 404 for old mongo course")
outline_url = reverse_course_url('course_handler', self.course.id)
resp = self.client.get(outline_url, HTTP_ACCEPT='text/html')
self.assertEqual(resp.status_code, 200)
Expand All @@ -71,6 +80,9 @@ def test_header_menu_with_exam_settings_enabled(self):
Tests course header menu should have `Exam Settings` menu item
if course does have Exam Settings view enabled.
"""
# course_handler raise 404 for old mongo course
if self.course.id.deprecated:
raise SkipTest("course_handler raise 404 for old mongo course")
outline_url = reverse_course_url('course_handler', self.course.id)
resp = self.client.get(outline_url, HTTP_ACCEPT='text/html')
self.assertEqual(resp.status_code, 200)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ def test_dump_course_ids_with_filter(self):

def test_dump_course_ids(self):
output = self.call_command('dump_course_ids')
dumped_courses = output.strip().split('\n')
dumped_courses = (output.strip() or []) and output.strip().split('\n')
course_ids = {str(course_id) for course_id in self.loaded_courses}
dumped_ids = set(dumped_courses)
assert course_ids == dumped_ids
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,10 @@ def setUpClass(cls):
"""
super().setUpClass()
cls.command = Command()
# org.0/course_0/Run_0
cls.course_key_1 = CourseFactory.create(default_store=ModuleStoreEnum.Type.mongo).id
# course-v1:org.1+course_1+Run_1
cls.course_key_2 = CourseFactory.create(default_store=ModuleStoreEnum.Type.split).id
cls.course_key_1 = CourseFactory.create(default_store=ModuleStoreEnum.Type.split).id
# course-v1:org.2+course_2+Run_2
cls.course_key_3 = CourseFactory.create(default_store=ModuleStoreEnum.Type.split).id
cls.course_key_2 = CourseFactory.create(default_store=ModuleStoreEnum.Type.split).id

def setUp(self):
"""
Expand Down Expand Up @@ -108,12 +106,11 @@ def test_specific_courses(self):
"""Test sending only to specific courses."""
self.command.handle(
**self.options(
courses=[str(self.course_key_1), str(self.course_key_2)]
courses=[str(self.course_key_1)]
)
)
assert self.course_key_1 in self.received_1
assert self.course_key_2 in self.received_1
assert self.course_key_3 not in self.received_1
assert self.course_key_2 not in self.received_1
assert self.received_1 == self.received_2

def test_specific_receivers(self):
Expand All @@ -125,7 +122,6 @@ def test_specific_receivers(self):
)
assert self.course_key_1 in self.received_1
assert self.course_key_2 in self.received_1
assert self.course_key_3 in self.received_1
assert not self.received_2

def test_course_overviews(self):
Expand All @@ -139,7 +135,7 @@ def test_course_overviews(self):
]
)
)
assert CourseOverview.objects.all().count() == 3
assert CourseOverview.objects.all().count() == 2
assert not self.received_1
assert not self.received_2

Expand Down
9 changes: 9 additions & 0 deletions xmodule/modulestore/tests/test_semantics.py
Original file line number Diff line number Diff line change
Expand Up @@ -446,9 +446,18 @@ def test_update_asides(self, block_type):
self.ASIDE_DATA_FIELD.field_name, self.ASIDE_DATA_FIELD.updated)


@ddt.ddt
class TestMongoDirectOnlyCategorySemantics(DirectOnlyCategorySemantics):
"""
Verify DIRECT_ONLY_CATEGORY semantics against the MongoModulestore
"""
MODULESTORE = TEST_DATA_MONGO_MODULESTORE
__test__ = True

@ddt.data(ModuleStoreEnum.Branch.draft_preferred, ModuleStoreEnum.Branch.published_only)
def test_course_summaries(self, branch):
""" Test that `get_course_summaries` method in modulestore work as expected. """
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated