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

(WIP) Graded library content - Do not merge, for internal review #3

Closed
Closed
Show file tree
Hide file tree
Changes from all 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: 17 additions & 1 deletion cms/djangoapps/contentstore/views/item.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
from models.settings.course_grading import CourseGradingModel
from cms.lib.xblock.runtime import handler_url, local_resource_url
from opaque_keys.edx.keys import UsageKey, CourseKey
from opaque_keys.edx.locator import BlockUsageLocator, CourseLocator

__all__ = ['orphan_handler', 'xblock_handler', 'xblock_view_handler', 'xblock_outline_handler']

Expand Down Expand Up @@ -84,6 +85,21 @@ def usage_key_with_run(usage_key_string):
return usage_key


def _should_remove_branch(key):
"""
Determine whether or not we want the modulestore to be stripping branch
information from any keys it returns to us.
"""
branch=None
if isinstance(key, CourseLocator):
branch = key.branch
elif isinstance(key, BlockUsageLocator):
branch = key.course_key.branch
if branch is None or branch == ModuleStoreEnum.BranchName.draft:
return True # This is the default branch, so strip out branch from any locators
return False # This is not the default branch - we must leave branch information in
Copy link
Member

Choose a reason for hiding this comment

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

This feels hackier still than in #1 . Would it make sense to clean this up, ie not having get_item removing the branch ever, and fixing the higher-level code that relies on this instead? I imagine it would likely be a lot of work, but if we can leave the place cleaner after us, it will be appreciated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had to make this change because my earlier code broke the Acid XBlock in studio. I agree it's less than ideal.

About changing it in the modulestore: we would need to discuss that with upstream, probably Don. It's a small change code-wise that could have a lot of big implications throughout the codebase.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking about this some more: If we're using a new "LibraryLocator" to reference libraries, then we won't have a special branch, so this code may not be needed at all :)

I'll investigate more today.

Copy link
Member Author

Choose a reason for hiding this comment

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

@antoviaque Yep, good news: I have developed a prototype of new locators for the Content Libraries, and with that change, this code (and the other remove_branch stuff you were questioning) is no longer needed at all.

Copy link
Member

Choose a reason for hiding this comment

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

Great : ) And thank you for the other answers, it useful to read the explanations.



# pylint: disable=unused-argument
@require_http_methods(("DELETE", "GET", "PUT", "POST", "PATCH"))
@login_required
Expand Down Expand Up @@ -203,7 +219,7 @@ def xblock_view_handler(request, usage_key_string, view_name):

if 'application/json' in accept_header:
store = modulestore()
xblock = store.get_item(usage_key, remove_branch=False)
xblock = store.get_item(usage_key, remove_branch=_should_remove_branch(usage_key))
container_views = ['container_preview', 'reorderable_container_child_preview']

# wrap the generated fragment in the xmodule_editor div so that the javascript
Expand Down
93 changes: 83 additions & 10 deletions common/lib/xmodule/xmodule/library_content_module.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from collections import namedtuple
from copy import copy
import hashlib
from .mako_module import MakoModuleDescriptor
from opaque_keys.edx.locator import CourseLocator
import random
from webob import Response
Expand All @@ -10,8 +11,8 @@
from xblock.fragment import Fragment
from xmodule.modulestore.exceptions import ItemNotFoundError
from xmodule.x_module import XModule, STUDENT_VIEW
from xmodule.seq_module import SequenceDescriptor
from xmodule.studio_editable import StudioEditableModule, StudioEditableDescriptor
from .xml_module import XmlDescriptor
from pkg_resources import resource_string

# Make '_' a no-op so we can scrape strings
Expand Down Expand Up @@ -102,14 +103,14 @@ class LibraryContentFields(object):
)
filters = String(default="") # TBD
has_score = Boolean(
display_name=_("Graded"),
help=_("Is this a graded assignment"),
display_name=_("Scored"),
help=_("Set this true if this is meant to be either a graded assignment or a practice problem."),
default=False,
scope=Scope.settings,
)
weight = Integer(
display_name=_("Weight"),
help=_("If this is a graded assignment, this determines the total point value available."),
help=_("If this is scored, the total possible score will be scaled to this weight."),
default=1,
scope=Scope.settings,
)
Expand All @@ -130,8 +131,20 @@ class LibraryContentModule(LibraryContentFields, XModule, StudioEditableModule):
as children of this block, but only a subset of those children are shown to
any particular student.
"""
def selected_children(self):
Copy link
Member

Choose a reason for hiding this comment

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

Cool, you already addressed that remark from the previous PR : )

"""
Returns a set() of block_ids indicating which of the possible children
have been selected to display to the current user.

def student_view(self, context):
This reads and updates the "selected" field, which has user_state scope.

Note: self.selected and the return value contain block_ids. To get
actual BlockUsageLocators, it is necessary to use self.children,
because the block_ids alone do not specify the block type.
"""
if hasattr(self, "_selected_set"):
# Already done:
return self._selected_set
# Determine which of our children we will show:
selected = set(self.selected) if self.selected else set() # set of block_ids
valid_block_ids = set([c.block_id for c in self.children])
Expand Down Expand Up @@ -159,15 +172,26 @@ def student_view(self, context):
raise NotImplementedError("Unsupported mode.")
# Save our selections to the user state, to ensure consistency:
self.selected = list(selected)
# Cache the results
self._selected_set = selected
return selected

def _get_selected_child_blocks(self):
"""
Generator returning XBlock instances of the children selected for the
current user.
"""
selected = self.selected_children()
for child_loc in self.children:
if child_loc.block_id in selected:
yield self.runtime.get_block(child_loc)

def student_view(self, context):
fragment = Fragment()
contents = []
child_context = {} if not context else copy(context)

for child_loc in self.children:
if child_loc.block_id not in selected:
continue
child = self.runtime.get_block(child_loc)
for child in self._get_selected_child_blocks():
for displayable in child.displayable_items():
rendered_child = displayable.render(STUDENT_VIEW, child_context)
fragment.add_frag_resources(rendered_child)
Expand Down Expand Up @@ -267,13 +291,61 @@ def _get_library(self, library_key):
# Note library version is also possibly available at library.runtime.course_entry.course_key.version
return library

def get_child_descriptors(self):
"""
We override normal handling of children.
In this case, our goal is to prevent the LMS from expecting any grades
from child XBlocks. So we tell the system that we have no children.
This way, we can report a single consolidated grade.
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference in approach with the randomize module?

Copy link
Member Author

Choose a reason for hiding this comment

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

Randomize tells the system it has exactly one child (the one picked randomly). We always tell the grading system we have no children.

As you can see from the discussion on the wiki, it is up for discussion which behaviour is better.

Telling grading system we have no children:

  • Pro: can re-weight the grades of child objects consistently - may sometimes improve consistency of assessments that consist of a LibraryContent block plus other assessment questions all put in the same unit.
  • Con: "Progress" page combines all grades from children of the randomize module, potentially revealing less information to users.

Telling grading system we have [n] children:

  • Pro: Progress page shows every single score.
  • Con: Assessments that include a ContentLibrary block and other blocks may sometimes weight the ContentLibrary questions higher or lower for some students than for others.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks -- Let's see if we can get clarifications on the grading requirements from the product team to ground things in a specific use case, then out of these requirements we can finish discussing which approach for grading works best for everyone

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, for now I'm leaning toward the simpler approach suggested by Dave though. It's simpler, easier, and should support 90% of use cases at least. I've also thought of simple workarounds for the specific cases I was worried about. Updating the wiki now.

Copy link
Member

Choose a reason for hiding this comment

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

Ok - I'll check with the product team, hopefully that will be good with them. If so then good for me!


Without this, the progress tab in the LMS would show an expected grade
from each possible child, rather than the n chosen child[ren].
"""
return []

def get_score(self):
"""
Return the current user's total score and max possible score.
"""
if not self.has_score:
return None
total = 0
correct = 0
for child in self._get_selected_child_blocks():
info = child.get_score()
if info:
total += info["total"]
correct += info["score"]
correct = correct * self.weight / total
total = self.weight
return {"score": correct, "total": total}

def max_score(self):
"""
Return the current user's max possible score.
"""
return self.weight
# If we were able to intercept 'grade' events from our children, we could
# then tell the LMS runtime to update the grade and max_grade in this XBlock's
# StudentModule (currently it is always NULL). In that case, we could return
# the unweighted max score here, and the LMS runtime would do weighting for us.
# However, for now we have set always_recalculate_grades=True, so this XBlock's
# StudentModule grade/max_grade is ignored, and we must also do our own weighting.
#
#return sum(child.max_score() for child in self._get_selected_child_blocks())
Copy link
Member

Choose a reason for hiding this comment

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

What would need to be done to intercept the grades from children? Ie, is it a small or big change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I tried implementing it but it seems like a big change, and it definitely has performance implications. Normally when a 'grade' event is emitted for some XBlock, the REST API handler will instantiate that XBlock, update its cached grade, and return a success result. We would need to instantiate that XBlock and all ancestors, and ask each ancestor if it wants to be informed about the child's 'grade' event.

Copy link
Member Author

Choose a reason for hiding this comment

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

Or, we could code that behaviour into the LMS runtime's event handling specifically for grade events of children of LibraryContent blocks, but that feels a little hacky.



@XBlock.wants('user')
class LibraryContentDescriptor(LibraryContentFields, SequenceDescriptor, StudioEditableDescriptor):
class LibraryContentDescriptor(LibraryContentFields, MakoModuleDescriptor, XmlDescriptor, StudioEditableDescriptor):
"""
Descriptor class for LibraryContentModule XBlock.
"""
mako_template = 'widgets/metadata-edit.html'
module_class = LibraryContentModule
always_recalculate_grades = True # when children publish 'grade' events, the LMS runtime updates the
# grade/max_grade of the child's StudentModule, but this block's StudentModule
# isn't updated. This forces the grading system to ignore the cached grades in
# this block's StudentModule and instead call get_score() and max_score().

@XBlock.handler
def refresh_children(self, request, _):
Expand Down Expand Up @@ -332,6 +404,7 @@ def refresh_children(self, request, _):
def has_dynamic_children(self):
"""
Inform the runtime that our children vary per-user.
See get_child_descriptors() above
"""
return True

Expand Down
2 changes: 1 addition & 1 deletion lms/djangoapps/courseware/grades.py
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ def create_module(descriptor):
# TODO: We need the request to pass into here. If we could forego that, our arguments
# would be simpler
with manual_transaction():
field_data_cache = FieldDataCache([descriptor], course.id, student)
field_data_cache = FieldDataCache.cache_for_descriptor_descendents(course.id, student, descriptor, depth=None)
return get_module_for_descriptor(student, request, descriptor, field_data_cache, course.id)

for module_descriptor in yield_dynamic_descriptor_descendents(section_descriptor, create_module):
Expand Down
4 changes: 4 additions & 0 deletions lms/djangoapps/courseware/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ class StudentModule(models.Model):
MODULE_TYPES = (('problem', 'problem'),
('video', 'video'),
('html', 'html'),
('course', 'course'),
('chapter', 'Section'),
('sequential', 'Subsection'),
('library_content', 'Library Content'),
)
## These three are the key for the object
module_type = models.CharField(max_length=32, choices=MODULE_TYPES, default='problem', db_index=True)
Expand Down