Pass DOM element, not jQuery object to XBlock initialisation.

[smarnach]

The function initializeXBlock() expects a DOM element, and is passed one in most cases. However, when adding a new XBlock component in Studio, the function is passed a jQuery object, which ends up being forwarded to the actual initialisation function of the XBlock. This is inconsistent, and led to buggy behaviour in one of our XBlocks. This bug has been around for at least a year.

This fix tries to make the type of element passed on to XBlock implementations more consistent.

I assumed that this change would be backwards compatible, since currently XBlock initialisation routines need to be able to handle either a jQuery object or a DOM element. However, some XBlock initialisers are only ever called via the code path that wrongly uses a jQuery objects, and some of these XBlocks depend on the wrong behaviour. I fixed the problems exposed by the edx-platform test suite, but there still might be external XBlocks that will break with this patch applied.

JIRA tickets: YONK-268

Sandbox URL: LMS Studio

Partner information: For a Solutions partner project

Reviewers

• Code: @Kelketek
• Platform: @andy-armstrong

[openedx-webhooks]

Thanks for the pull request, @smarnach! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

To automatically create an OSPR issue for this pull request, just visit this link: https://openedx-webhooks.herokuapp.com/github/process_pr?number=11433&repo=edx%2Fedx-platform

[Kelketek]

@smarnach Can a test be added for this?

[smarnach]

@Kelketek I can't see a way to test this that would be reasonable to implement in the context of this change. We aren't testing that calling contracts are adhered to for the JavaScript code anywhere else.

[Kelketek]

@smarnach The fix does address the problem for DnD, but the test failures for bok_choy appear to be genuine. So that means there are a few pieces of the codebase relying on the inconsistent behavior which will need to be updated.

[smarnach]

@Kelketek I don't think the test failures are caused by this PR. They all seem completely unrelated. Most of them are related to the Courseware search, and I fail to see how this can be affected by a change to code that is exclusively used in Studio. Other failures are related to tinyMCE and CodeMirror.

I don't quite understand what Jenkins does. The latest Jenkins run lists 78 commits, the last of which is mine. I've seen this behaviour a couple of times before, but I never understood what is causing this.

[Kelketek]

@smarnach I've attempted this test with and without your patch:

paver test_bokchoy -t studio/test_studio_container.py:EditContainerTest.test_edit_raw_html

It passes before the patch, and after. I suspect these failures are, in fact, real.

[smarnach]

@Kelketek You are right about the test failures. The lettuce feature specification make it easy to reproduce the bugs manually to debug them, and I've fixed a few. The easiest way to see what failures are remaining is to wait for another full Jenkins run, so the feedback loop on this is rather slow, and in the end this might turn out to be too deep a rabbit hole to fix along the way.

Moreover, my impression that this change is backwards compatible might be wrong. I assumed that XBlocks currently have to deal with being called either with a DOM object or with a jQuery object anyway, since both of those invocation happen depending on circumstances. However, some constructors are only called in Studio, and only via the code path that wrongly uses the jQuery object, e.g. the modal dialogues that pop up when clicking "Edit" on a component. And some of these constructors depend on being called in the wrong way.

This means that this change might actually break XBlocks outside of edx-platform.

[Kelketek]

@smarnach This is possible. It may require contacting the mailing list to let people know about the change. I do think this change should be made, however, since we really shouldn't be varying what type of JS Object is passed to the block on initialization.

[Kelketek]

The alternative would be to just modify our XBlock to detect which is used or just autoconvert. Perhaps we should ask someone from edX what they'd prefer. Maybe someone from T&L?

[smarnach]

@Kelketek I got all tests passing at last, and learned a few JS debugging tricks along the way. The resulting changes are rather simple though. Could you please take another look?

[Kelketek]

👍

[smarnach]

@andy-armstrong Would you be able to review this PR?

[andy-armstrong]

Great catch @smarnach. It looks like I introduced this bug almost two years ago:

https://github.com/edx/edx-platform/pull/2539

I'll review the PR now.

[andy-armstrong]

@smarnach The change looks great to me. I find it disconcerting that you didn't need to change any tests to get this to work which indicates how poor our test coverage is in this area. Can you add some simple tests to xblock_spec.js to ensure that this doesn't regress again in the future.

[smarnach]

@andy-armstrong Thanks for the review, that was quick! I'm not quite sure how I would add a test for this – I'm not really familiar with that code, and it's rather complex. I'm also not sure whether a unit test is the right way to ensure that calling contracts are followed.

I would somehow have to spy on XBlock.initializeBlock() when calling XBlockView.render() and make sure it gets called with a DOM element. How would I do that? I don't have access to XBlock.initializeBlock() in the spec, since the files xblock.js and xblock_spec.js specify different versions of the XBlock runtime in the requirements xblock/cms.runtime.v1 vs. xblock/runtime.v1.

[andy-armstrong]

@smarnach I looked into this and I agree that it would be hard to test given how complex the function is. 👍 as it is.

[smarnach]

@andy-armstrong Thanks for the quick response!

As mentioned in the description, there is a chance that this change might break some XBlocks external to the edx-platform repo. How should we deal with this problem? We could risk merging it, and I would test some of the external XBlocks carefully on stage; I've got an idea what kind of problems I need to watch out for. If I find some problem, we revert it in the release branch. If I miss any issue, we can only cause rather limited problems on Studio, and not on the LMS. What do you think?

CC @nasthagiri (next week's Release Master)

[andy-armstrong]

@smarnach I agree that the risk is low, in that it can only affect Studio rendering of XBlocks. I think we should go ahead and merge this PR and deal with any fallout as it happens. You could also email the edx-code mailing list just to give folks a heads up that this might affect their blocks.

[bradenmacdonald]

This line has caused a regression: https://openedx.atlassian.net/browse/TNL-4161

What was the rationale for this specific change?

[smarnach]

@bradenmacdonald The rationale was that all XBlocks inside edx-platform I looked at required the element attribute to be a jQuery object. This was the only way I managed to get it working at all.

Thinking about it, it seems inconsitent to pass a DOM element to the initialisation function and use a jQuery object here, so this line should probably be reverted. However, this will require a large number of changes in other places, but it's probably what we need to do.