Andal.LND/edx-platform
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #26987 from edx/ormsbee/tnl-8057-malformed-structures

Browse Source 
Improved error handling in Course Outline generation.
This commit is contained in:
David Ormsbee
2021-03-15 10:11:57 -04:00
committed by GitHub
parent 8ec2699269 3a552c4fdf
commit 51811f0c5e
12 changed files with 713 additions and 126 deletions
Download Patch File Download Diff File Expand all files Collapse all files

266
openedx/core/djangoapps/content/learning_sequences/admin.py
View File

@@ -6,15 +6,112 @@ from enum import Enum
import json
from django.contrib import admin
from django.utils.html import format_html
from django.utils.html import format_html, format_html_join
from django.utils.translation import gettext_lazy as _
from opaque_keys import OpaqueKey
import attr
from .api import get_course_outline
from .models import LearningContext
from .api import get_content_errors, get_course_outline
from .models import CourseContext, CourseSectionSequence
class LearningContextAdmin(admin.ModelAdmin):
class HasErrorsListFilter(admin.SimpleListFilter):
"""
Filter to find Courses with content errors.
The default Django filter on an integer field will give a choice of values,
which isn't something we really want. We just want a filter for > 0 errors.
"""
title = _("Error Status")
parameter_name = 'status'
def lookups(self, request, model_admin):
return (
('has_errors', _('Courses with Errors')),
)
def queryset(self, request, queryset):
if self.value() == 'has_errors':
return queryset.filter(
learning_context__publish_report__num_errors__gt=0,
)
class CourseSectionSequenceInline(admin.TabularInline):
"""
Inline for showing the sequences of a course.
The queries look a bit weird because a CourseSectionSequence holds both
course-specific Sequence metadata, while much of the data we care about is
in LearningSequence (like the title) and CourseSequenceExam.
"""
model = CourseSectionSequence
verbose_name = "Sequence"
verbose_name_plural = "Sequences"
fields = (
'title',
'is_time_limited',
'is_proctored_exam',
'is_practice_exam',
'accessible_after_due',
'visible_to_staff_only',
'hide_from_toc',
)
readonly_fields = (
'title',
'is_time_limited',
'is_proctored_exam',
'is_practice_exam',
'accessible_after_due',
'visible_to_staff_only',
'hide_from_toc',
)
ordering = ['ordering']
def get_queryset(self, request):
"""
Optimization to cut an extra two requests per sequence.
We still have an N+1 issue, but given the number of sequences in a
course, this is tolerable even for large courses. It is possible to get
this down to one query if we do a lower level rendering for the
sequences later.
"""
qs = super().get_queryset(request)
qs = qs.select_related('section', 'sequence')
return qs
def title(self, cs_seq):
return cs_seq.sequence.title
title.short_description = "Title"
def accessible_after_due(self, cs_seq):
return not cs_seq.inaccessible_after_due
accessible_after_due.boolean = True
def visible_to_staff_only(self, cs_seq):
return not cs_seq.visible_to_staff_only
visible_to_staff_only.boolean = True
visible_to_staff_only.short_description = "Staff Only"
def is_time_limited(self, cs_seq):
return cs_seq.exam.is_time_limited
is_time_limited.boolean = True
is_time_limited.short_description = "Timed Exam"
def is_proctored_exam(self, cs_seq):
return cs_seq.exam.is_proctored_exam
is_proctored_exam.boolean = True
is_proctored_exam.short_description = "Proctored Exam"
def is_practice_exam(self, cs_seq):
return cs_seq.exam.is_practice_exam
is_practice_exam.boolean = True
is_practice_exam.short_description = "Practice Exam"
class CourseContextAdmin(admin.ModelAdmin):
"""
This is a read-only model admin that is meant to be useful for querying.
@@ -25,25 +122,124 @@ class LearningContextAdmin(admin.ModelAdmin):
to be written to from the Studio process.
"""
list_display = (
'context_key',
'course_key',
'title',
'published_at',
'published_version',
'modified'
'num_errors',
'num_sections',
'num_sequences',
)
list_select_related = (
'learning_context',
'learning_context__publish_report',
)
list_filter = (
HasErrorsListFilter,
'learning_context__published_at',
)
readonly_fields = (
'context_key',
'course_key',
'title',
'published_at',
'published_version',
'created',
'modified',
'outline',
)
search_fields = ['context_key', 'title']
actions = None
'course_visibility',
'self_paced',
'days_early_for_beta',
'entrance_exam_id',
def outline(self, obj):
'error_details',
'raw_outline',
)
raw_id_fields = (
'learning_context',
)
fieldsets = (
(
None,
{
'fields': (
'course_key',
'title',
'published_at',
'course_visibility',
'self_paced',
'days_early_for_beta',
'entrance_exam_id',
),
}
),
(
'Outline Data',
{
'fields': (
'num_sections', 'num_sequences', 'num_errors', 'error_details',
),
}
),
(
'Debug Details',
{
'fields': (
'published_version', 'created', 'modified', 'raw_outline'
),
'classes': ('collapse',),
}
),
)
inlines = [
CourseSectionSequenceInline,
]
search_fields = ['learning_context__context_key', 'learning_context__title']
ordering = ['-learning_context__published_at']
def get_queryset(self, request):
qs = super().get_queryset(request)
qs = qs.prefetch_related('section_sequences')
return qs
def created(self, course_context):
return course_context.learning_context.created
created.short_description = "Record Created"
def modified(self, course_context):
return course_context.learning_context.modified
modified.short_description = "Record Modified"
def course_key(self, course_context):
return course_context.learning_context.context_key
def title(self, course_context):
return course_context.learning_context.title
def published_at(self, course_context):
published_at_dt = course_context.learning_context.published_at
return published_at_dt.strftime("%B %-d, %Y, %-I:%M %p")
published_at.short_description = "Published at (UTC)"
def published_version(self, course_context):
return course_context.learning_context.published_version
def _publish_report_attr(self, course_context, attr_name):
learning_context = course_context.learning_context
if not hasattr(learning_context, 'publish_report'):
return None
return getattr(learning_context.publish_report, attr_name)
def num_errors(self, course_context):
return self._publish_report_attr(course_context, 'num_errors')
num_errors.short_description = "Errors"
def num_sections(self, course_context):
return self._publish_report_attr(course_context, 'num_sections')
num_sections.short_description = "Sections"
def num_sequences(self, course_context):
return self._publish_report_attr(course_context, 'num_sequences')
num_sequences.short_description = "Sequences"
def raw_outline(self, obj):
"""
Computed attribute that shows the outline JSON in the detail view.
"""
@@ -56,7 +252,7 @@ class LearningContextAdmin(admin.ModelAdmin):
return value.isoformat()
return value
outline_data = get_course_outline(obj.context_key)
outline_data = get_course_outline(obj.learning_context.context_key)
outline_data_dict = attr.asdict(
outline_data,
recurse=True,
@@ -65,6 +261,46 @@ class LearningContextAdmin(admin.ModelAdmin):
outline_data_json = json.dumps(outline_data_dict, indent=2, sort_keys=True)
return format_html("<pre>\n{}\n</pre>", outline_data_json)
def error_details(self, course_context):
"""
Generates the HTML for Error Details.
"""
learning_context = course_context.learning_context
if not hasattr(learning_context, 'publish_report'):
return ""
content_errors = get_content_errors(learning_context.context_key)
if not content_errors:
return format_html("<p>No errors were found.</p>")
list_items = format_html_join(
"\n",
"<li>{} <br><small>Usage Key: {}</small></li>",
(
(err_data.message, err_data.usage_key)
for err_data in content_errors
)
)
return format_html(
"""
<p>
Parts of the course content were skipped when generating the Outline
because they did not follow the standard Course → Section →
Subsection hierarchy. Course structures like this cannot be created
in Studio, but can be created by OLX import. In OLX, this hierarchy
is represented by the tags <code>{}</code> → <code>{}</code> →
<code>{}</code>.
</p>
<ol>
{}
</ol>
""",
"<course>",
"<chapter>",
"<sequential>",
list_items,
)
def has_add_permission(self, request):
"""
Disallow additions. See docstring for has_change_permission()
@@ -93,4 +329,4 @@ class LearningContextAdmin(admin.ModelAdmin):
return False
admin.site.register(LearningContext, LearningContextAdmin)
admin.site.register(CourseContext, CourseContextAdmin)

1
openedx/core/djangoapps/content/learning_sequences/api/__init__.py
View File

@@ -1,5 +1,6 @@
# lint-amnesty, pylint: disable=missing-module-docstring
from .outlines import (
get_content_errors,
get_course_keys_with_outlines,
get_course_outline,
get_user_course_outline,

86
openedx/core/djangoapps/content/learning_sequences/api/outlines.py
View File

@@ -6,7 +6,7 @@ __init__.py imports from here, and is a more stable place to import from.
import logging
from collections import defaultdict
from datetime import datetime
from typing import Optional # lint-amnesty, pylint: disable=unused-import
from typing import Optional, List # lint-amnesty, pylint: disable=unused-import
import attr # lint-amnesty, pylint: disable=unused-import
from django.contrib.auth import get_user_model
@@ -19,6 +19,7 @@ from opaque_keys.edx.locator import LibraryLocator
from opaque_keys.edx.keys import CourseKey # lint-amnesty, pylint: disable=unused-import
from ..data import (
ContentErrorData,
CourseLearningSequenceData,
CourseOutlineData,
CourseSectionData,
@@ -29,12 +30,14 @@ from ..data import (
VisibilityData,
)
from ..models import (
ContentError,
CourseSection,
CourseSectionSequence,
CourseContext,
CourseSequenceExam,
LearningContext,
LearningSequence
LearningSequence,
PublishReport,
)
from .permissions import can_see_all_content
from .processors.content_gating import ContentGatingOutlineProcessor
@@ -49,6 +52,7 @@ log = logging.getLogger(__name__)
# Public API...
__all__ = [
'get_content_errors',
'get_course_keys_with_outlines',
'get_course_outline',
'get_user_course_outline',
@@ -193,7 +197,10 @@ def _get_course_context_for_outline(course_key: CourseKey) -> CourseContext:
)
try:
course_context = (
LearningContext.objects.select_related('course_context').get(context_key=course_key).course_context
LearningContext.objects
.select_related('course_context')
.get(context_key=course_key)
.course_context
)
except LearningContext.DoesNotExist:
# Could happen if it hasn't been published.
@@ -203,6 +210,25 @@ def _get_course_context_for_outline(course_key: CourseKey) -> CourseContext:
return course_context
def get_content_errors(course_key: CourseKey) -> List[ContentErrorData]:
"""
Get ContentErrors created in the most recent publish of this Course run.
"""
try:
learning_context = (
LearningContext.objects.select_related('publish_report')
.get(context_key=course_key)
)
publish_report = learning_context.publish_report
except (LearningContext.DoesNotExist, PublishReport.DoesNotExist):
return []
return [
ContentErrorData(usage_key=error.usage_key, message=error.message)
for error in publish_report.content_errors.all().order_by('id')
]
@function_trace('learning_sequences.api.get_user_course_outline')
def get_user_course_outline(course_key: CourseKey,
user: User,
@@ -246,6 +272,13 @@ def get_user_course_outline_details(course_key: CourseKey,
def _get_user_course_outline_and_processors(course_key: CourseKey, # lint-amnesty, pylint: disable=missing-function-docstring
user: User,
at_time: datetime):
"""
Helper function that runs the outline processors.
This function returns a UserCourseOutlineData and a dict of outline
processors that have executed their data loading and returned which
sequences to remove and which to mark as inaccessible.
"""
# Record the user separately from the standard user_id that views record,
# because it's possible to ask for views as other users if you're global
# staff. Performance is going to vary based on the user we're asking the
@@ -316,10 +349,11 @@ def _get_user_course_outline_and_processors(course_key: CourseKey, # lint-amnes
@function_trace('learning_sequences.api.replace_course_outline')
def replace_course_outline(course_outline: CourseOutlineData):
def replace_course_outline(course_outline: CourseOutlineData,
content_errors: Optional[List[ContentErrorData]] = None):
"""
Replace the model data stored for the Course Outline with the contents of
course_outline (a CourseOutlineData).
course_outline (a CourseOutlineData). Record any content errors.
This isn't particularly optimized at the moment.
"""
@@ -329,17 +363,20 @@ def replace_course_outline(course_outline: CourseOutlineData):
)
set_custom_attribute('learning_sequences.api.course_id', str(course_outline.course_key))
if content_errors is None:
content_errors = []
with transaction.atomic():
# Update or create the basic CourseContext...
course_context = _update_course_context(course_outline)
# Wipe out the CourseSectionSequences join+ordering table so we can
# delete CourseSection and LearningSequence objects more easily.
# Wipe out the CourseSectionSequences join+ordering table
course_context.section_sequences.all().delete()
_update_sections(course_outline, course_context)
_update_sequences(course_outline, course_context)
_update_course_section_sequences(course_outline, course_context)
_update_publish_report(course_outline, content_errors, course_context)
def _update_course_context(course_outline: CourseOutlineData):
@@ -457,3 +494,38 @@ def _update_course_section_sequences(course_outline: CourseOutlineData, course_c
else:
# Otherwise, delete any exams associated with it
CourseSequenceExam.objects.filter(course_section_sequence=course_section_sequence).delete()
def _update_publish_report(course_outline: CourseOutlineData,
content_errors: List[ContentErrorData],
course_context: CourseContext):
"""
Record ContentErrors for this course publish. Deletes previous errors.
"""
set_custom_attribute('learning_sequences.api.num_content_errors', len(content_errors))
learning_context = course_context.learning_context
try:
# Normal path if we're updating a PublishReport
publish_report = learning_context.publish_report
publish_report.num_errors = len(content_errors)
publish_report.num_sections = len(course_outline.sections)
publish_report.num_sequences = len(course_outline.sequences)
publish_report.content_errors.all().delete()
except PublishReport.DoesNotExist:
# Case where we're creating it for the first time.
publish_report = PublishReport(
learning_context=learning_context,
num_errors=len(content_errors),
num_sections=len(course_outline.sections),
num_sequences=len(course_outline.sequences),
)
publish_report.save()
publish_report.content_errors.bulk_create([
ContentError(
publish_report=publish_report,
usage_key=error_data.usage_key,
message=error_data.message,
)
for error_data in content_errors
])

63
openedx/core/djangoapps/content/learning_sequences/api/tests/test_outlines.py
View File

@@ -23,6 +23,7 @@ from common.djangoapps.student.auth import user_has_role
from common.djangoapps.student.roles import CourseBetaTesterRole
from ...data import (
ContentErrorData,
CourseLearningSequenceData,
CourseOutlineData,
CourseSectionData,
@@ -31,6 +32,7 @@ from ...data import (
VisibilityData,
)
from ..outlines import (
get_content_errors,
get_course_outline,
get_user_course_outline,
get_user_course_outline_details,
@@ -1274,3 +1276,64 @@ class SequentialVisibilityTestCase(CacheIsolationTestCase):
assert len(user_course_outline.sequences) == 6
assert all(is_sequence_accessible),\
'Sequences should be accessible to enrolled, staff users for a public_outline course'
class ContentErrorTestCase(CacheIsolationTestCase):
"""Test error collection and reporting."""
def test_errors(self):
"""
Basic tests for writing and retriving errors.
"""
course_key = CourseKey.from_string("course-v1:OpenEdX+Outlines+Errors")
outline = CourseOutlineData(
course_key=course_key,
title="Outline Errors Test Course!",
published_at=datetime(2021, 3, 21, tzinfo=timezone.utc),
published_version="8ebece4b69dd593d82fe2020",
sections=[],
self_paced=False,
days_early_for_beta=None,
entrance_exam_id=None,
course_visibility=CourseVisibility.PRIVATE,
)
usage_key_1 = course_key.make_usage_key('sequential', 'seq1')
usage_key_2 = course_key.make_usage_key('sequential', 'seq2')
replace_course_outline(
outline,
content_errors=[
# Explicitly set to no usage key.
ContentErrorData(message="Content is Hard", usage_key=None),
# Implicitly set usage key
ContentErrorData("Simple Content Error Description"),
# Multiple copies of the same usage key
ContentErrorData(message="Seq1 is wrong", usage_key=usage_key_1),
ContentErrorData(message="Seq1 is still wrong", usage_key=usage_key_1),
# Another key
ContentErrorData(message="Seq2 is also wrong", usage_key=usage_key_2)
]
)
assert outline == get_course_outline(course_key)
# Ordering is preserved.
assert get_content_errors(course_key) == [
ContentErrorData(message="Content is Hard", usage_key=None),
ContentErrorData(message="Simple Content Error Description", usage_key=None),
ContentErrorData(message="Seq1 is wrong", usage_key=usage_key_1),
ContentErrorData(message="Seq1 is still wrong", usage_key=usage_key_1),
ContentErrorData(message="Seq2 is also wrong", usage_key=usage_key_2),
]
# Now do it again and make sure updates work as well as inserts
replace_course_outline(
outline,
content_errors=[
ContentErrorData(message="Content is Hard", usage_key=None),
]
)
assert get_content_errors(course_key) == [
ContentErrorData(message="Content is Hard", usage_key=None),
]

16
openedx/core/djangoapps/content/learning_sequences/data.py
View File

@@ -50,6 +50,22 @@ class ObjectDoesNotExist(Exception):
pass # lint-amnesty, pylint: disable=unnecessary-pass
@attr.s(frozen=True)
class ContentErrorData:
"""
A human-readable description of something wrong with the content, to ease
the debugging of content issues–especially ones where content had to be
skipped because it was somehow malformed. The messages should be
comprehensible to course teams and support staff.
Errors can refer to things that are not anywhere in the outline, such as
when things don't show up where we expect then to be and we omit them from
the outline (unknown tag types, sequences where we expect sections, etc.)
"""
message = attr.ib(type=str)
usage_key = attr.ib(type=Optional[UsageKey], default=None)
@attr.s(frozen=True)
class VisibilityData:
"""

34
openedx/core/djangoapps/content/learning_sequences/migrations/0009_contenterror_publishreport.py Normal file
View File

@@ -0,0 +1,34 @@
# Generated by Django 2.2.19 on 2021-03-12 16:31
from django.db import migrations, models
import django.db.models.deletion
import opaque_keys.edx.django.models
class Migration(migrations.Migration):
dependencies = [
('learning_sequences', '0008_add_learning_context_title_index'),
]
operations = [
migrations.CreateModel(
name='PublishReport',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('num_errors', models.PositiveIntegerField()),
('num_sections', models.PositiveIntegerField()),
('num_sequences', models.PositiveIntegerField()),
('learning_context', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='publish_report', to='learning_sequences.LearningContext')),
],
),
migrations.CreateModel(
name='ContentError',
fields=[
('id', models.BigAutoField(primary_key=True, serialize=False)),
('usage_key', opaque_keys.edx.django.models.UsageKeyField(max_length=255, null=True)),
('message', models.TextField(max_length=10000)),
('publish_report', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='content_errors', to='learning_sequences.PublishReport')),
],
),
]

28
openedx/core/djangoapps/content/learning_sequences/migrations/0010_add_publishreport_indexes.py Normal file
View File

@@ -0,0 +1,28 @@
# Generated by Django 2.2.19 on 2021-03-13 18:25
from django.db import migrations, models
class Migration(migrations.Migration):
dependencies = [
('learning_sequences', '0009_contenterror_publishreport'),
]
operations = [
migrations.AlterField(
model_name='publishreport',
name='num_errors',
field=models.PositiveIntegerField(db_index=True),
),
migrations.AlterField(
model_name='publishreport',
name='num_sections',
field=models.PositiveIntegerField(db_index=True),
),
migrations.AlterField(
model_name='publishreport',
name='num_sequences',
field=models.PositiveIntegerField(db_index=True),
),
]

21
openedx/core/djangoapps/content/learning_sequences/migrations/0011_course_meta_names.py Normal file
View File

@@ -0,0 +1,21 @@
# Generated by Django 2.2.19 on 2021-03-14 05:48
from django.db import migrations
class Migration(migrations.Migration):
dependencies = [
('learning_sequences', '0010_add_publishreport_indexes'),
]
operations = [
migrations.AlterModelOptions(
name='coursecontext',
options={'verbose_name': 'Course', 'verbose_name_plural': 'Courses'},
),
migrations.AlterModelOptions(
name='coursesectionsequence',
options={'verbose_name': 'Course Sequence', 'verbose_name_plural': 'Course Sequences'},
),
]

60
openedx/core/djangoapps/content/learning_sequences/models.py
View File

@@ -87,6 +87,13 @@ class CourseContext(TimeStampedModel):
self_paced = models.BooleanField(default=False)
entrance_exam_id = models.CharField(max_length=255, null=True)
class Meta:
verbose_name = 'Course'
verbose_name_plural = 'Courses'
def __str__(self):
return f"{self.learning_context.context_key} ({self.learning_context.title})"
class LearningSequence(TimeStampedModel):
"""
@@ -200,6 +207,11 @@ class CourseSectionSequence(CourseContentVisibilityMixin, TimeStampedModel):
unique_together = [
['course_context', 'ordering'],
]
verbose_name = 'Course Sequence'
verbose_name_plural = 'Course Sequences'
def __str__(self):
return f"{self.section.title} > {self.sequence.title}"
class CourseSequenceExam(TimeStampedModel):
@@ -212,3 +224,51 @@ class CourseSequenceExam(TimeStampedModel):
is_practice_exam = models.BooleanField(default=False)
is_proctored_enabled = models.BooleanField(default=False)
is_time_limited = models.BooleanField(default=False)
class PublishReport(models.Model):
"""
A report about the content that generated this LearningContext publish.
All these fields could be derived with aggregate SQL functions, but it would
be slower and make the admin code more complex. Since we only write at
publish time, keeping things in sync is less of a concern.
"""
learning_context = models.OneToOneField(
LearningContext, on_delete=models.CASCADE, related_name='publish_report'
)
num_errors = models.PositiveIntegerField(null=False, db_index=True)
num_sections = models.PositiveIntegerField(null=False, db_index=True)
num_sequences = models.PositiveIntegerField(null=False, db_index=True)
class ContentError(models.Model):
"""
Human readable content errors.
If something got here, it means that we were able to make _something_ (or
there would be no LearningContext at all), but something about the published
state of the content is wrong and should be flagged to course and support
teams. In many cases, this will be some malformed course structure that gets
uploaded via OLX import–a process that is more forgiving than Studio's UI.
It's a little weird to store errors in such a freeform manner like this. It
would be more efficient and flexible in terms of i18n if we were to store
error codes, and leave the message generation to the time of display. The
problem with that is that we don't know up front what the parameterization
for such errors would be. The current error messages being created are
fairly complicated and include references to multiple attributes of multiple
pieces of content with supporting breadcrumbs. Other future errors might
just be about display_name string length.
So instead of trying to model all that internally, I'm just allowing for
freeform messages. It is quite possible that at some point we will come up
with a more comprehensive taxonomy of error messages, at which point we
could do a backfill to regenerate this data in a more normalized way.
"""
id = models.BigAutoField(primary_key=True)
publish_report = models.ForeignKey(
PublishReport, on_delete=models.CASCADE, related_name='content_errors'
)
usage_key = UsageKeyField(max_length=255, null=True)
message = models.TextField(max_length=10000, blank=False, null=False)