-
Notifications
You must be signed in to change notification settings - Fork 14
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
Forward auto-grading settings to DL assignments #6641
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,6 +42,7 @@ | |
import uuid | ||
from datetime import datetime, timedelta | ||
|
||
from marshmallow import Schema, validate | ||
from pyramid.view import view_config, view_defaults | ||
from webargs import fields | ||
|
||
|
@@ -85,12 +86,30 @@ def deep_linking_launch(context, request): | |
return {} | ||
|
||
|
||
class _AutoGradingConfigSchema(Schema): | ||
grading_type = fields.Str( | ||
required=True, validate=validate.OneOf(["all_or_nothing", "scaled"]) | ||
) | ||
activity_calculation = fields.Str( | ||
required=True, validate=validate.OneOf(["cumulative", "separate"]) | ||
) | ||
|
||
required_annotations = fields.Int(required=True, validate=validate.Range(min=0)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. required_annotations always required |
||
required_replies = fields.Int( | ||
required=False, allow_none=True, validate=validate.Range(min=0) | ||
) | ||
|
||
|
||
class DeepLinkingFieldsRequestSchema(JSONPyramidRequestSchema): | ||
content_item_return_url = fields.Str(required=True) | ||
content = fields.Dict(required=True) | ||
group_set = fields.Str(required=False, allow_none=True) | ||
title = fields.Str(required=False, allow_none=True) | ||
|
||
auto_grading_config = fields.Nested( | ||
_AutoGradingConfigSchema, required=False, allow_none=True | ||
) | ||
|
||
|
||
class LTI11DeepLinkingFieldsRequestSchema(DeepLinkingFieldsRequestSchema): | ||
opaque_data_lti11 = fields.Str(required=False, allow_none=True) | ||
|
@@ -254,6 +273,10 @@ def _get_assignment_configuration(request) -> dict: | |
if title := request.parsed_params.get("title"): | ||
params["title"] = title | ||
|
||
if auto_grading_config := request.parsed_params.get("auto_grading_config"): | ||
# Custom params must be str, encode these settings as JSON | ||
params["auto_grading_config"] = json.dumps(auto_grading_config) | ||
|
||
if content["type"] == "url": | ||
params["url"] = content["url"] | ||
else: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,13 +5,72 @@ | |
import pytest | ||
from freezegun import freeze_time | ||
from h_matchers import Any | ||
from pyramid.testing import DummyRequest | ||
|
||
from lms.resources import LTILaunchResource | ||
from lms.resources._js_config import JSConfig | ||
from lms.views.lti.deep_linking import DeepLinkingFieldsViews, deep_linking_launch | ||
from lms.validation import ValidationError | ||
from lms.views.lti.deep_linking import ( | ||
DeepLinkingFieldsRequestSchema, | ||
DeepLinkingFieldsViews, | ||
deep_linking_launch, | ||
) | ||
from tests import factories | ||
|
||
|
||
class TestDeepLinkingFieldsRequestSchema: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testing just the schema here. |
||
@pytest.mark.parametrize( | ||
"payload", | ||
[ | ||
{ | ||
"content": {"type": "url", "url": "https://example.com"}, | ||
"content_item_return_url": "return_url", | ||
"auto_grading_config": { | ||
"grading_type": "scaled", | ||
"activity_calculation": "separate", | ||
"required_annotations": 10, | ||
"required_replies": 2, | ||
}, | ||
"extra_params": {}, | ||
} | ||
], | ||
) | ||
def test_valid_payloads(self, payload): | ||
request = DummyRequest( | ||
body=json.dumps(payload), | ||
headers={"Content-Type": "application/json; charset=UTF-8"}, | ||
) | ||
request.content_type = request.headers["content-type"] = "application/json" | ||
|
||
DeepLinkingFieldsRequestSchema(request).parse() | ||
|
||
@pytest.mark.parametrize( | ||
"payload", | ||
[ | ||
{ | ||
"content": {"type": "url", "url": "https://example.com"}, | ||
"content_item_return_url": "return_url", | ||
"auto_grading_config": { | ||
"grading_type": "scaled", | ||
"activity_calculation": "RANDOM", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Best type of grading |
||
"required_annotations": 10, | ||
"required_replies": 2, | ||
}, | ||
"extra_params": {}, | ||
} | ||
], | ||
) | ||
def test_invalid_payloads(self, payload): | ||
request = DummyRequest( | ||
body=json.dumps(payload), | ||
headers={"Content-Type": "application/json; charset=UTF-8"}, | ||
) | ||
request.content_type = request.headers["content-type"] = "application/json" | ||
|
||
with pytest.raises(ValidationError): | ||
DeepLinkingFieldsRequestSchema(request).parse() | ||
|
||
|
||
@pytest.mark.usefixtures("application_instance_service", "lti_h_service") | ||
class TestDeepLinkingLaunch: | ||
def test_it( | ||
|
@@ -211,9 +270,13 @@ def test_it_for_v11( | |
{"group_set": "1", "title": "title"}, | ||
{"group_set": "1", "title": "title"}, | ||
), | ||
( | ||
{"auto_grading_config": {"key": "value"}}, | ||
{"auto_grading_config": '{"key": "value"}'}, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we not validating that this fulfills I would expect these values to be invalid. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, good point but I'm afraid I don't a satisfactory answer. The method here is unaware of the types, it just passes along what it receives, and in the case of these settings it serlizes it as json. That's what the test does. I included the private name of the method in the test name (one extra The marshmallow schema is validated before we get into the the code of individual views and it gets serialized into That being said I added a very broad valid/invalid tests for the schema itself 👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Oh, I see 👍🏼 |
||
), | ||
], | ||
) | ||
def test_get_assignment_configuration( | ||
def test__get_assignment_configuration( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed this test's name, it testing |
||
self, content, expected_from_content, pyramid_request, data, expected, uuid | ||
): | ||
pyramid_request.parsed_params.update({"content": content, **data}) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to "separate"