Skip to content

Prevent options being a string rather than JSON#4146

Merged
bjester merged 1 commit intolearningequality:hotfixesfrom
rtibbles:no_string_json
Jun 16, 2023
Merged

Prevent options being a string rather than JSON#4146
bjester merged 1 commit intolearningequality:hotfixesfrom
rtibbles:no_string_json

Conversation

@rtibbles
Copy link
Copy Markdown
Member

@rtibbles rtibbles commented Jun 14, 2023

Summary

Description of the change(s) you made

  • Removes an unnecessary json.dumps during publish that was double wrapping the JSON

Manual verification steps performed

  1. Wrote a regression test.

Fixes #4141

@rtibbles rtibbles requested a review from bjester June 14, 2023 22:13
@bjester
Copy link
Copy Markdown
Member

bjester commented Jun 15, 2023

Looking at the blame history, seems this was a json.dumps for a while? At least since #1896

@bjester
Copy link
Copy Markdown
Member

bjester commented Jun 16, 2023

@rtibbles
Copy link
Copy Markdown
Member Author

When I updated the Kolibri models for use in both kolibri_public and kolibri_content, I also used the Kolibri JSONField implementation (which saves to a text field under the hood): https://github.com/learningequality/kolibri/blob/develop/kolibri/core/content/base_models.py#L133

This was to ensure proper serialization to the API endpoints, but I missed the json.dumps that had previously been used when it had been set as a TextField rather than the JSONField (which handles the json.dumps internally) - so it was getting double json.dumpsed.

@bjester
Copy link
Copy Markdown
Member

bjester commented Jun 16, 2023

Okay that makes sense. Lets make sure that we also QA channel import just in case

@bjester bjester merged commit c79e2e1 into learningequality:hotfixes Jun 16, 2023
@rtibbles rtibbles deleted the no_string_json branch June 16, 2023 16:10
@thanksameeelian
Copy link
Copy Markdown

so it was getting double json.dumpsed.

Jason.Jason.Jason.Jason.Jason.Jason.Jason.Jason.Jason.Jason.mp4

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.

3 participants