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

video_module: Add deserialize logic for metadata fields. #679

Closed
wants to merge 2 commits into from

Conversation

cahrens
Copy link

@cahrens cahrens commented Aug 14, 2013

STUD-640

@cahrens
Copy link
Author

cahrens commented Aug 14, 2013

@peter-fogg @vaxXxa Please review.

@peter-fogg
Copy link
Contributor

Looks good; 👍 when tests pass.

STUD-640

Handle the case of the double-quotes appearing within the string.

We don't need to worry about the whole concatenated ID being
double-quoted, since the concatenation was done by overriding xml_module's
export method.

Use the method already fined in XmlDescriptor.
@cahrens
Copy link
Author

cahrens commented Aug 14, 2013

I modified the solution to make use of the existing deserialize method in XmlDescriptor.

@peter-fogg
Copy link
Contributor

👍 New code also looks good.

@vasylnakvasiuk
Copy link
Contributor

@cahrens, if I correct understand, in the future we will delete block of code, where we manage quoted strings, right? Cause, VideoModule shouldn't fix this problem in the future.

@cahrens
Copy link
Author

cahrens commented Aug 15, 2013

@vaxXxa No, I don't expect we will remove this code in the future. We could possibly remove the code on L299 once we are confident that all our courses do not have the double-quoting within the concatenation (that could only have been created on export during a small time frame, since we stopped double-quoting strings), but I don't think we should remove line 357. We have continued using json.dumps for exporting all non-String types, so it is possible to hit issues with other fields as well.

Peter wrote a custom xml_exporter for videoalpha (currently in master as video). So export right now goes through video_alpha's custom implementation. But with @chrisndodge ' PR (https://github.com/edx/edx-platform/pull/688), videoalpha will go back to using XmlDescriptor's serialization mechanism, which means we should deserialize in a consistent way.

@cahrens
Copy link
Author

cahrens commented Aug 15, 2013

@vaxXxa You are correct that I can stick with the classmethod instead of staticmethod. I have made that change and pushed up.

I am now going to take these changes and put them on Chris' branch so we can release this stuff together.

@vasylnakvasiuk
Copy link
Contributor

@cahrens I'm looking for #668 now.

@cahrens
Copy link
Author

cahrens commented Aug 15, 2013

This change got pulled into Chris' PR.

@cahrens cahrens closed this Aug 15, 2013
chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014
jenkins-ks pushed a commit to nttks/edx-platform that referenced this pull request Mar 30, 2016
e-kolpakov referenced this pull request in open-craft/edx-platform Jun 22, 2016
jfavellar90 pushed a commit to eduNEXT/edx-platform that referenced this pull request Apr 11, 2018
…anslation-fix

UPD: removing translating of studio options, not needed, and breaks e…
rediris pushed a commit to gymnasium/edx-platform that referenced this pull request Feb 25, 2021
Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022
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