Skip to content

Conversation

@lduarte1991
Copy link
Contributor

Background: This is a follow-up to PR #3831. According to @cpennington and @singingwolfboy there was a race condition with TinyMCE in the annotation tools which could be fixed by loading them via student_view from the XBlock API.

*Goal of PR: * This PR focuses solely on that fix. It includes both changes to the modules in xmodule as well as to the test files to verify the fix.

CMS Updates: None.

LMS Updates: Changes to the way TinyMCE loads.

Testing Notes: Everything seems to be working in the local environment but the issue always seems to be on staging.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't you still need to set the baseURL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seemed to still work without it on my version. If I'm not mistaken this gets initialized originally and Daniel had added that change before to use our version of tinymce instead of Edx's. Now that only one is loaded I figured that wasn't needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested it again and that line doesn't seem to be necessary now that there's only one TinyMCE to worry about.

Copy link
Contributor

Choose a reason for hiding this comment

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

Huh. Well, I think you probably could have worked around the issue by just removing that line, then. Nonetheless, I think this code ends up better, so I'm happy to have the rest of the pr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, haha. Well that's good to know at least. I just fixed the tests below so I guess once the tests pass it will get merged?

Copy link
Contributor

Choose a reason for hiding this comment

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

Context is usually a dictionary, rather than a list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry about that? I thought I tried {} and it had failed but I must have looked at the wrong thing? I just made a fix and squashed it with the test fixes for just this one line.

@cpennington
Copy link
Contributor

👍 with nitpicks.

@lduarte1991
Copy link
Contributor Author

@cpennington Is that a spurious failure or did I screw something up?

@singingwolfboy
Copy link
Contributor

@lduarte1991 Seems like a spurious failure to me. I'll re-run the tests.

@lduarte1991
Copy link
Contributor Author

@singingwolfboy Cool. looks good to go!

singingwolfboy added a commit that referenced this pull request May 30, 2014
Annotation Tools: Load Tinymce in student_view
@singingwolfboy singingwolfboy merged commit 30dd47a into openedx:master May 30, 2014
lduarte1991 added a commit to lduarte1991/edx-platform that referenced this pull request Jun 9, 2014
Annotation Tools: Fixing tests for student_view

Fixed typo from autocomplete

Fixed dict for tests
singingwolfboy pushed a commit that referenced this pull request Jun 11, 2014
Annotation Tools: Load Tinymce in student_view

Annotation Tools: Fixing tests for student_view

Fixed typo from autocomplete

Fixed dict for tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants