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

Handle VitalSource assignments in the creation endpoint schema #3327

Merged
merged 1 commit into from
Nov 9, 2021

Conversation

marcospri
Copy link
Member

@marcospri marcospri commented Nov 3, 2021

Testing

Comment on lines +99 to +101
bookID = fields.Str(required=False, allow_none=True)
cfi = fields.Str(required=False, allow_none=True)
"""VitalSource IDs"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the bookID and cfi fields must be present if type is "vitalsource" and must not be present otherwise. We'd need to add that to the @validates_schema() method below I think. Let's make sure we have good unit tests for this as it'd be easy to mess up.

It's getting to the point where I feel we have three separate schemas here (one for URLs, one for files, and one for VitalSource) and we should have a meta-schema that tries to validate the request using each of the sub-schemas in turn. I'm not exactly sure how we'd implement that though.

Copy link
Member Author

@marcospri marcospri Nov 4, 2021

Choose a reason for hiding this comment

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

I don't think a few conditionals here are to bad, at least for now where different types don't interact with each other.

and must not be present otherwise

Not sure about that one I think if the type says "vitalsource" and we have enough information we should accept it even it also sends something in the URL, same for the other types. I'd be liberal in what you accept here.

lms/views/api/assignments.py Outdated Show resolved Hide resolved
@marcospri marcospri force-pushed the vitalsource-assignment-creation-fix branch from eab0902 to 518a2d0 Compare November 4, 2021 11:08
Comment on lines +116 to +117
content = data["content"]
type_ = data["content"]["type"]
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'd say this makes the rest easier to read, not sure.

@marcospri marcospri force-pushed the vitalsource-assignment-creation-fix branch from 518a2d0 to 1b61b9c Compare November 4, 2021 11:22
def test_it_doesnt_raise_valid__assignment(
self, json_request, assignment_fixture_name, request
):
assignment_body = request.getfixturevalue(assignment_fixture_name)
Copy link
Member Author

Choose a reason for hiding this comment

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

Went for this style on the happy path ones

@marcospri marcospri requested a review from seanh November 4, 2021 11:50
@marcospri marcospri changed the title Handle VitalSource assignments in the creation endpoint. Handle VitalSource assignments in the creation endpoint schema Nov 4, 2021
@marcospri marcospri merged commit a0ab291 into master Nov 9, 2021
@marcospri marcospri deleted the vitalsource-assignment-creation-fix branch November 9, 2021 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants