Skip to content

feat: basic get/post endpoint for v2 xblocks. TNL-10873 - #32600

Merged
kenclary merged 1 commit into
masterfrom
kenclary/TNL-10873
Jul 20, 2023
Merged

feat: basic get/post endpoint for v2 xblocks. TNL-10873#32600
kenclary merged 1 commit into
masterfrom
kenclary/TNL-10873

Conversation

@kenclary

@kenclary kenclary commented Jun 29, 2023

Copy link
Copy Markdown
Contributor

feat: basic get/post endpoint for v2 xblocks. TNL-10873

@kenclary
kenclary force-pushed the kenclary/TNL-10873 branch 5 times, most recently from 2ea4170 to e00cda1 Compare June 30, 2023 17:30
@kenclary
kenclary force-pushed the kenclary/TNL-10873 branch 25 times, most recently from 5b4b53f to 350a379 Compare July 10, 2023 19:30
@kenclary
kenclary force-pushed the kenclary/TNL-10873 branch 25 times, most recently from 52b2122 to ee47664 Compare July 18, 2023 16:44

@bradenmacdonald bradenmacdonald left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, looks good! I haven't tested this manually but I'm happy with the code.

Comment thread openedx/core/djangoapps/xblock/rest_api/views.py Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will it be an issue for the editors if some newlines go inserted here? This test set the data to '<p>test</p>' but we get the data back as '\n<p>test</p>\n'. I'm assuming that's fine but just want to confirm.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

no, in fact I don't think editors can have content without the newlines; it's just an artifact of what I wrote in the test.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I guess we should rename this function to just serialize_block_for_blockstore since we're already passing in a blockstore block here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it is called in a bunch of other places.

@connorhaugh connorhaugh left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just some comments on comments for clarity. Code looks ok.

Comment on lines 179 to 185

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
View to get/edit the field values of an XBlock as JSON (in the v2 runtime)
This class mimics the functionality of xblock_handler in block.py (for v1 xblocks), but for v2 xblocks.
However, it only implements the exact subset of functionality needed to support the v2 editors (from
the frontend-lib-content-components project). As such, it only supports GET and POST, and only the
POSTing of data/metadata fields.
"""
View for the field values of an XBlock, returned as JSON.
This class mimics the functionality of the v1 xblock_handler in block.py.
However, it only implements the exact subset of functionality needed to support xblock editing, for a single xblock which has no children blocks. As such, it only supports GET and POST.
"""

Comment thread openedx/core/djangoapps/xblock/rest_api/views.py Outdated
@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot

Copy link
Copy Markdown
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

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.

4 participants