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

Forward auto-grading settings to DL assignments #6641

Merged
merged 1 commit into from
Sep 9, 2024
Merged
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
23 changes: 23 additions & 0 deletions lms/views/lti/deep_linking.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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(
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed this to "separate"

required=True, validate=validate.OneOf(["cumulative", "separate"])
)

required_annotations = fields.Int(required=True, validate=validate.Range(min=0))
Copy link
Member Author

Choose a reason for hiding this comment

The 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)
Expand Down Expand Up @@ -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:
Expand Down
67 changes: 65 additions & 2 deletions tests/unit/lms/views/lti/deep_linking_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Member Author

Choose a reason for hiding this comment

The 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",
Copy link
Member Author

Choose a reason for hiding this comment

The 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(
Expand Down Expand Up @@ -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"}'},
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we not validating that this fulfills _AutoGradingConfig definition? 🤔

I would expect these values to be invalid.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 request.parsed_params, if that not valid the view is never called.

That being said I added a very broad valid/invalid tests for the schema itself 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

The marshmallow schema is validated before we get into the the code of individual views and it gets serialized into request.parsed_params, if that not valid the view is never called.

Oh, I see 👍🏼

),
],
)
def test_get_assignment_configuration(
def test__get_assignment_configuration(
Copy link
Member Author

@marcospri marcospri Sep 4, 2024

Choose a reason for hiding this comment

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

Fixed this test's name, it testing _get_assignment_configuration not get_assignment_configuration

self, content, expected_from_content, pyramid_request, data, expected, uuid
):
pyramid_request.parsed_params.update({"content": content, **data})
Expand Down
Loading