mitodl / edx-platform

The Open edX platform, the software that powers edX!
http://open.edx.org/
GNU Affero General Public License v3.0
6 stars 1 forks source link

"+New Unit" Feature Within Other Units in Studio Broken #230

Closed pdpinch closed 2 years ago

pdpinch commented 3 years ago

There's a new way to add a unit to a course: via a button that is shown on any Unit page in Studio. Clicking that button should add a new unit, but right now, nothing happens. You can see/test it here: https://studio-mitx-qa-next.mitx.mit.edu/container/block-v1:MITx+mkd.upgradeF2020+2020_Fall+type@vertical+block@9e6c89c774e64ceb9dea89ffcd391a85

And here is a gif from Edge.edx.org of the feature working: addUnitDoesn'tWork https://studio.edge.edx.org/container/block-v1:MITx+mkd.1r+2020_Summer+type@vertical+block@188a829764584922bd088ad3df912aff

pdpinch commented 3 years ago

Questions:

pdpinch commented 3 years ago

FYI @briangrossman

briangrossman commented 3 years ago

FYI: This failed on xPRO RC

asadiqbal08 commented 3 years ago

1- Getting 403 forbidden error while hitting https://studio.edge.edx.org/container/block-v1:MITx+mkd.1r+2020_Summer+type@vertical+block@188a829764584922bd088ad3df912aff

2- Trying to login RC studio (https://studio-rc.xpro.mit.edu/) to verify this issue but getting a No oauth2 provider error. Can anyone fix for my user ?

3- Which mitodl openedx (https://github.com/mitodl/edx-platform) release / branch should I checkout for fixing or generating this issue on DevStack ?

pdpinch commented 3 years ago
  1. You can work around this by logging into xpro first, I think. But No oauth2 provider error is definitely a bug that needs to be addressed.

  2. Please test master first. If you can't reproduce on master, then test on Koa, which is our target for xPRO and for Residential MITx.

asadiqbal08 commented 3 years ago

@pdpinch thank you for a quick reply. The .gif attached above is pointing to working state of studio unit. Can anyone add up the failure state in the issue description ? I just mean, It would be helpful for me to understand how actually the studio seemed broken there ?

asadiqbal08 commented 3 years ago

@pdpinch I tried to produce this bug over these branches and seems working fine. Please see the attached video what actually I am doing and let me know is still I have missed to understand the bug ?

Branches:

Findings

Video Link

briangrossman commented 3 years ago

@asadiqbal08

I tested this again on studio-rc here: https://studio-rc.xpro.mit.edu/course/course-v1:MITx+TestPoll+TestPoll

Unfortunately, the behavior isn't consistent. I made a video in which it fails once, but succeeds the rest of the times. I have been unable to find a full-reproducible test case, but I am able to get it to fail: https://www.dropbox.com/s/n5vvmpxd0cd3ui6/NewUnitButton.mov?dl=0

asadiqbal08 commented 3 years ago

@briangrossman I again give it a try and yes behavior isn't consistent and now on every attempt it seems working correctly. I just mean to know what is the priority of this random occurring bug ?

briangrossman commented 3 years ago

@asadiqbal08 I'm not able to reproduce it today... did you do anything to fix it? Or is it just a random event?

As Peter clarified in our standup, this can be lower priority.

pdpinch commented 3 years ago

We're getting reports from users that this is a reproducible issue on Residential MITx. It happens reliably for me at, for example, https://studio-mitx-qa.mitx.mit.edu/container/block-v1:MITx+mkd.2021_1+2021_Spring+type@vertical+block@b513c7e8d71c4f108e05ce04ff6ca243

On page load I get this console error:

Uncaught SyntaxError: expected expression, got '<'

When clicking the "+New Unit" button I get this error in the console:

Uncaught TypeError: text is undefined

​ I think it would be useful to try again to reproduce this in devstack with a combination of the mitx/koa branch of edx-platform with koa branch of the mitx-theme.

asadiqbal08 commented 3 years ago

@pdpinch Today, I configured the platform over mitx/koa branch and applied the koa theme from mitx-theme but sorry to say I did not find any issue with studio units. I tried many times with some different testing methods but it is not occurred locally. Can you point out anything else I need to look around to produce this issue locally ?

pdpinch commented 3 years ago

@asadiqbal08 a couple of thoughts:

  1. Have you looked at the behavior on the QA server at https://studio-mitx-qa.mitx.mit.edu/container/block-v1:MITx+mkd.2021_1+2021_Spring+type@vertical+block@b513c7e8d71c4f108e05ce04ff6ca243 ? There's a clear console error produced there. Does that help you identify the cause?

  2. Is it possible that the issue is related to course content? Would it help you to reproduce the error to export the course from the QA server and load it into your devstack?

asadiqbal08 commented 3 years ago

@peter Today I import a course from mitx-qa instance with the help of @HamzaIbnFarooq and tested it. It works well locally again without any crash. I noticed the behavior over https://studio-mitx-qa.mitx.mit.edu/container/block-v1:MITx+mkd.2021_1+2021_Spring+type@vertical+block@b513c7e8d71c4f108e05ce04ff6ca243 and strangely adding any unit over any course is not working there but it is really hard to get idea from these messages in console. So it does not seems me related to course content.

image

One more thing, we compare the system-feedback.undersocre from mitx-qa and local devstack instance and did not find any difference in code there too.

HamzaIbnFarooq commented 3 years ago

Today, after discussing with @asadiqbal08 I tried to reproduce the issue on my devstack, but the feature seems to be working fairly for me.

pdpinch commented 3 years ago

I'm going to put this on hold and we'll see if it still exists in Lilac.

pdpinch commented 3 years ago

This issue still exists in our Lilac QA deployment. Mark's JavaScript debugging still seems relevant: https://github.mit.edu/mitx/Summer-2021/issues/1#issuecomment-54404

Tested in a new course at https://studio-mitx-qa-next.mitx.mit.edu/course/course-v1:TestX+101+2014_T1

asadiqbal08 commented 3 years ago

@pdpinch the issue is not produceable over lilac.master, what I did yet in order to produce it.

Question# Can you please tell me is there any other instance where we getting this error ?

What can I assume, may be the javascript files are not packed well during deployment over mitx-qa-next instance. Locally, we do not have pipeline_enabled so not compressed_js code executed.

asadiqbal08 commented 3 years ago

Today, I noticed few differences in local and mitx-qa environment.

I noticed over https://studio-mitx-qa-next.mitx.mit.edu/ is that, inside the HTML of page, I am seeing that system-feedback.underscore is adding up there as

<script type="text/javascript" charset="utf-8" async="" data-requirecontext="_" data-requiremodule="common/templates/components/system-feedback.underscore" src="https://d2qn4lwm63grp6.cloudfront.net/static/studio/common/templates/components/system-feedback.underscore.js"></script>

while, I am sure, as this is .underscore file so the type should be text/template instead of text/javascript , I noticed, requireJs module is adding up .js extension at end of the .underscore file that is not happening on local dev instance.

Not sure but that can be a reason for browser throwing syntax error while loading system-feedback.underscore.js file.

Need to know, is there any different configuration for requireJs for underscore files ?

asadiqbal08 commented 3 years ago

Ok after digging into further today. Here are some in-depth findings that may be the reasons for this bug on https://studio-mitx-qa-next.mitx.mit.edu/

Currently, In our case, as mentioned above comment, The extension .js is appended to .underscore file and when browser load its content then throw a syntax error. Let talk about XHR restrictions for !text plugin.

From the documentation:

XHR restrictions The text plugin works by using XMLHttpRequest (XHR) to fetch the text for the resources it handles.

However, XHR calls have some restrictions, due to browser/web security policies:

Many browsers do not allow file:// access to just any file. You are better off serving the application from a local web server than using local file:// URLs. You will likely run into trouble otherwise.

There are restrictions for using XHR to access files on another web domain. While CORS can help enable the server for cross-domain access, doing so must be done with care (in particular if you also host an API from that domain), and not all browsers support CORS.

So if the text plugin determines that the request for the resource is on another domain, it will try to access a ".js" version of the resource by using a script tag. Script tag GET requests are allowed across domains. The .js version of the resource should just be a script with a define() call in it that returns a string for the module value.

Example: if the resource is 'text!example.html' and that resolves to a path on another web domain, the text plugin will do a script tag load for 'example.html.js'.

So the text plugin gives a hint to the solution: It's possible to configure the plugin in a way that it always fetches remote resources via XHR without appending the .js.

In case of MITx, it loading assets from cloudfront and that is different domain than MITx. e.g. https://d2qn4lwm63grp6.cloudfront.net/static/studio/common/templates/components/system-feedback.underscore.js

Further Reading.

As per Docs:

The baseUrl can be a URL on a different domain as the page that will load require.js. RequireJS script loading works across domains. The only restriction is on text content loaded by text! plugins: those paths should be on the same domain as the page, at least during development. The optimization tool will inline text! plugin resources so after using the optimization tool, you can use resources that reference text! plugin resources from another domain.

One option is to test the deployment without CDN and for this someone, probably devops, can disable it.

@pdpinch @HamzaIbnFarooq @blarghmatey FYI.

pdpinch commented 3 years ago

Ok. I think @shaidar has disabled the CDN for testing purposes and the "New Unit" button is working now! (although some image assets appear to be missing).

So, how does this work on edx.org? They seem to be using a CDN for images, but not for JS?

asadiqbal08 commented 3 years ago

@pdpinch kindly see the attached screenshot, At-least for studio, Edx is not using CDN for images as well.

Screen Shot 2021-07-07 at 12 25 45 PM
pdpinch commented 3 years ago

We think we can mimic the edX approach using Fast.ly as the CDN instead of Cloudfront. Since this is a significant change though, we're going to try it in the new edX deployment for MITx Online first. If it goes well we will make a similar change to Residential.

pdpinch commented 3 years ago

@briangrossman did we ever test this on mitxonline?

briangrossman commented 3 years ago

@briangrossman did we ever test this on mitxonline?

@pdpinch : Yup! I did a couple quick tests and it worked fine!

pdpinch commented 3 years ago

@blarghmatey a small issue, leading to a big question -- do you plan to use Fastly for the next releases of Residential MITx and MIT xPRO?

blarghmatey commented 3 years ago

Yes, the intent is to bring the residential and xPro deployments in line with the architecture that we set up for MITx Online, which includes using Fastly in front of them.

shaidar commented 2 years ago

@pdpinch This seems to be working on residential. Can someone double check it and close this issue?

pdpinch commented 2 years ago

Yes, confirmed on https://github.mit.edu/mitx/spring-2022/issues/9