Remove circuit djangoapp from LMS #10324

stvstnfrd · 2015-10-23T00:53:59Z

This revives the work started in https://github.com/edx/edx-platform/pull/7180 .

These endpoints (edit_circuit and save_circuit) had already been
commented out of urls.py, so these views were disabled.

During manual testing of the previous PR, it was found that the wiki integration had been broken for months. At this point, it's probably been broken and unused for at least a year.

openedx-webhooks · 2015-10-23T00:54:03Z

Thanks for the pull request, @stvstnfrd! I've created OSPR-898 to keep track of it in JIRA. JIRA is a place for product owners to prioritize feature reviews by the engineering development teams.

Feel free to add as much of the following information to the ticket:

supporting documentation
edx-code email threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will still be done via the GitHub pull request interface. As a reminder, our process documentation is here.

stvstnfrd · 2015-10-23T00:55:05Z

common/lib/xmodule/xmodule/js/src/capa/schematic.js

The only references to this function are in the two wiki templates, removed in this commit.

grep -r 'add_schematic_handler' /edx/app/edxapp/edx-platform/

stvstnfrd · 2015-10-23T03:38:49Z

common/static/js/capa/schematicinput.js

These (global!) variables are are only used in the removed code from common/static/js/vendor/CodeMirror/addons/edx_markdown.js.

stvstnfrd · 2015-10-23T03:49:28Z

CC from #7180: @sarina @pmitros @adampalay

stvstnfrd · 2015-10-23T04:59:38Z

Something to consider..

The wiki schematic creation is broken and should be removed as no one plans to work on it.
I can't confirm the behavior of viewing an already existing wiki schematic, since I can't create one, nor are we using this feature anywhere in production at Stanford. Can someone at edX determine if/how you're using this?

If you are, I suspect we'll want to back out part of the second commit.

sarina · 2015-10-26T21:26:51Z

@stvstnfrd I think the failing lettuce test was due to some code that's since been reverted. Can you rebase?

sarina · 2015-10-26T21:28:33Z

@adampalay In the last PR you said

EDIT: it looks like the wiki feature has been used as recently as December

Could you let me know where it's been used? I can take that data and test how this PR affects it.

These endpoints (`edit_circuit` and `save_circuit`) had already been commented out of `urls.py`, so these views were disabled.

for hanging indent

stvstnfrd · 2015-10-26T22:53:08Z

@sarina Rebase complete.

sarina · 2015-10-27T00:32:32Z

@stvstnfrd This has been approved from a product perspective, and the code looks good to me, but before merging I'd like to get some more info from Adam and try testing things out.

stvstnfrd · 2015-10-27T05:34:10Z

Sounds good @sarina , give a holler when you need me.

adampalay · 2015-10-27T13:57:20Z

@sarina , I found that information by running the following query:

select * from wiki_articlerevision
where content LIKE "%circuit-schematic%"

sarina · 2015-10-27T14:06:58Z

@adampalay - who can I ask to run that query for me? I don't have permissions to do so. I would like to find courses where I could find examples (better yet, urls to the wiki posts, otherwise I'm not sure how I would find the instances)

adampalay · 2015-10-27T14:12:44Z

@sarina , I didn't realize you didn't have permissions. I'm able to run that query, but I don't know how to get urls to the wiki posts from it.

However, since the query that I ran gives examples of how to write up a circuit schematic in the wiki, I can forward you an example from the DB. You can also probably peruse old runs of 6.002x. Let me know what would be most useful.

sarina · 2015-10-27T14:16:38Z

An example from the DB would probably work - I can see how it renders on
the current release branch, then compare that to how it renders on this
branch.

On Tue, Oct 27, 2015 at 10:12 AM, Adam notifications@github.com wrote:

@sarina https://github.com/sarina , I didn't realize you didn't have
permissions. I'm able to run that query, but I don't know how to get urls
to the wiki posts from it.

However, since the query that I ran gives examples of how to write up a
circuit schematic in the wiki, I can forward you an example from the DB.
You can also probably peruse old runs of 6.002x. Let me know what would be
most useful.

—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/10324#issuecomment-151508548.

adampalay · 2015-10-27T14:19:32Z

sent an example in a PM

pmitros · 2015-10-27T14:20:27Z

I have not read the PR, but if we do end up removing this, please do make sure it doesn't migrate old data away from the database.

sarina · 2015-10-27T14:29:20Z

@pmitros can you let us know what data is at risk, so we can verify?

pmitros · 2015-10-27T14:32:00Z

@sarina Any circuits in the database. We are removing lms/djangoapps/circuit/models.py. I don't know whether that table contains data, and if so, whether this might migrate the data away.

stvstnfrd · 2015-10-27T17:02:59Z

You should confirm on your end, but per @adampalay in the previous PR..

The circuit_servercircuit table is empty in our db.

sarina · 2015-10-28T13:50:20Z

@stvstnfrd So, my testing shows the wiki circuit integration works on master. Here's a schematic example to try out: https://gist.github.com/sarina/e7b5f37e3c6f3b3adbc4

sarina · 2015-10-28T13:53:12Z

So, it seems to make sense to remove code that's been commented out forever, but the wiki integration is still working and apparently at least some of our users are making use of it. This PR (obviously) breaks the integration:

Master

This pr

pmitros · 2015-10-28T14:25:01Z

If it is working, I would definitely not remove. It is a high-value feature in circuits courses, and provides a framework for embedding other types of rich content into the wiki. The implementation and user experience was very elegant at the time it was built (although my impression is it has slipped a fair bit).

sarina · 2015-10-28T14:41:50Z

I'm actually going to talk with this with Leslie. This circuit simulator has been embedded in a total of 8 wiki pages, with the last bit of use being October 2013.

We have a ton of code to support and keep working. This untested, rarely utilized code does not seem to be broadly useful. It may be better to continue to support adding in embedded images, and people can add screenshots of the LMS circuit tool if they have questions.

@stvstnfrd I will discuss this with Leslie next week; no action needed from you just now.

pmitros · 2015-10-28T15:33:15Z

@explorerleslie It's up to Leslie. However, this does a lot more than allow embedded images. You can make things resembling an interactive textbook, with in-line simulations. It's not a lot of code, it's pretty independent code, and it doesn't cause a lot of problems. I remember figuring out how to do this took a lot of time. An alternative might be to remove it when and if it causes problems, or to expand it if we ever get back to doing this kind of innovation in teaching-and-learning.

explorerleslie · 2015-10-28T20:37:26Z

@pmitros it is actually actively broken. See @stvstnfrd 's first comment on the PR. I've been involved in the product review of this OSPR, and I agree with the change -- it would be different if it wasn't broken, but it is, and I don't have resources to fix it.

sarina · 2015-10-28T21:02:05Z

@explorerleslie From my testing (see screenshots in my comment above), the wiki integration does actually work. However, it's been used to embed about 15 circuits in about 8 individual pages, with the last editing being in October 2013.

Additionally, we clearly can't support this feature well. It WAS actually broken a few months ago, and apparently something changed to make it work again, and we don't know what broke or fix it.

If you could make a decision taking this additional information under consideration, I'd appreciate it.

explorerleslie · 2015-10-28T21:08:12Z

@sarina thanks for the additional context. if the last time any course used it was October 2013, and we don't have time or effort to support it, then I still think it should be removed.

sarina · 2015-10-29T15:15:04Z

lms/djangoapps/circuit/models.py

Deleting this file won't cause data issues, because the endpoints that would allow someone to create circuits & save them have never been enabled.

Remove circuit djangoapp from LMS

stvstnfrd · 2015-10-29T22:14:21Z

Thanks for all the follow-up work to see this through @sarina @explorerleslie !

`module-js` and `module-descriptor-js` are old JavaScript group indicators, left over from when we managed XModule assets via Django Pipeline. We would like to get rid of them in order to make it easier to build XModule JS without using Python. There is one single usage of `module-js` in the entire platform (the rest have been replaced with Webpack references, which is the less-outdated way of managing XModule assets :). The lone `module-js` reference was added in 2013 [1] so that circuit diagrams would display in the course wiki. However, the ability to render circuits in the wiki was removed in 2015 [2], so it is safe to remove the reference. There is also one single usage of `module-descriptor-js`. It's in the legacy bulk email editor, which hackily cribs from the old HtmlBlock editor. Fortunately, we are able to simply replace the Django Pipeline reference with the equivalent XModule JS Webpack bundle. (Note: The old email editor is currently still supported, but is currently being replaced by frontend-app-communications, so this hack will be gone eventually). Finally, this commit also sneaks in one styling fix: it adds the HtmlBlockEditor CSS back to the aforementioned legacy bulk email page. The missing CSS was causing a read-only 1-line codemirror editor to appear below the HTML editor [3]. This bug was introduced during the original XModule SCSS decoupling [4], which removed builtin block CSS from the LMS-wide bundle, thus removing the HTML editor CSS from the bulk email page. We imagine that nobody noticed because the bug only exists in master (not Palm) and frontend-app-communications seems to be globally enabled on edx.org. As a simple fix, we add the new CSS link to the legacy bulk email page, and it renders fine again [5]. References: 1. openedx@3fc59b3 2. openedx#10324 3. Before fix: https://github.com/openedx/edx-platform/assets/3628148/25fc41b2-403d-4339-8c49-0b04664dfa02 4. openedx#32018 5. After fix: https://github.com/openedx/edx-platform/assets/3628148/9a5d74f1-cc83-4ebe-8f0c-ee270f7721b8 Part of: openedx#32481

`module-js` and `module-descriptor-js` are old JavaScript group indicators, left over from when we managed XModule assets via Django Pipeline. We would like to get rid of them in order to make it easier to build XModule JS without using Python. There is one single usage of `module-js` in the entire platform (the rest have been replaced with Webpack references, which is the less-outdated way of managing XModule assets :). The lone `module-js` reference was added in 2013 [1] so that circuit diagrams would display in the course wiki. However, the ability to render circuits in the wiki was removed in 2015 [2], so it is safe to remove the reference. There is also one single usage of `module-descriptor-js`. It's in the legacy bulk email editor, which hackily cribs from the old HtmlBlock editor. Fortunately, we are able to simply replace the Django Pipeline reference with the equivalent XModule JS Webpack bundle. (Note: The old email editor is currently still supported, but is currently being replaced by frontend-app-communications, so this hack will be gone eventually). Finally, this commit also sneaks in one styling fix: it adds the HtmlBlockEditor CSS back to the aforementioned legacy bulk email page. The missing CSS was causing a read-only 1-line codemirror editor to appear below the HTML editor [3]. This bug was introduced during the original XModule SCSS decoupling [4], which removed builtin block CSS from the LMS-wide bundle, thus removing the HTML editor CSS from the bulk email page. We imagine that nobody noticed because the bug only exists in master (not Palm) and frontend-app-communications seems to be globally enabled on edx.org. As a simple fix, we add the new CSS link to the legacy bulk email page, and it renders fine again [5]. References: 1. 3fc59b3 2. #10324 3. Before fix: https://github.com/openedx/edx-platform/assets/3628148/25fc41b2-403d-4339-8c49-0b04664dfa02 4. #32018 5. After fix: https://github.com/openedx/edx-platform/assets/3628148/9a5d74f1-cc83-4ebe-8f0c-ee270f7721b8 Part of: #32481

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U needs triage labels Oct 23, 2015

stvstnfrd reviewed Oct 23, 2015
View reviewed changes

stvstnfrd force-pushed the debt/circuit branch from 67582dd to 3b29498 Compare October 23, 2015 02:18

stvstnfrd reviewed Oct 23, 2015
View reviewed changes

stvstnfrd force-pushed the debt/circuit branch from 3b29498 to 5afe36c Compare October 23, 2015 03:43

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Oct 26, 2015

stvstnfrd added 3 commits October 26, 2015 15:50

Remove circuit djangoapp from LMS

d77f31d

These endpoints (`edit_circuit` and `save_circuit`) had already been commented out of `urls.py`, so these views were disabled.

Remove circuit-schematic integration from wiki

a8d30c5

Fix PEP8: E126 continuation line over-indented

c2fe448

for hanging indent

stvstnfrd force-pushed the debt/circuit branch from 5afe36c to c2fe448 Compare October 26, 2015 22:52

openedx-webhooks added awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 27, 2015

openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed awaiting prioritization labels Oct 28, 2015

openedx-webhooks added product review PR requires product review before merging and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Oct 28, 2015

sarina reviewed Oct 29, 2015
View reviewed changes

sarina added a commit that referenced this pull request Oct 29, 2015

Merge pull request #10324 from stvstnfrd/debt/circuit

2206b8a

Remove circuit djangoapp from LMS

sarina merged commit 2206b8a into openedx:master Oct 29, 2015

stvstnfrd deleted the debt/circuit branch October 29, 2015 22:14

stvstnfrd mentioned this pull request Nov 6, 2015

Badging: Core Refactor #10498

Merged

kdmccormick mentioned this pull request Jul 6, 2023

[1/3] refactor: remove XModule JS from Django Pipeline #32530

Merged

Remove circuit djangoapp from LMS #10324

Remove circuit djangoapp from LMS #10324

Uh oh!

Conversation

stvstnfrd commented Oct 23, 2015

Uh oh!

openedx-webhooks commented Oct 23, 2015

Uh oh!

stvstnfrd Oct 23, 2015

Choose a reason for hiding this comment

Uh oh!

stvstnfrd Oct 23, 2015

Choose a reason for hiding this comment

Uh oh!

stvstnfrd commented Oct 23, 2015

Uh oh!

stvstnfrd commented Oct 23, 2015

Uh oh!

sarina commented Oct 26, 2015

Uh oh!

sarina commented Oct 26, 2015

Uh oh!

stvstnfrd commented Oct 26, 2015

Uh oh!

sarina commented Oct 27, 2015

Uh oh!

stvstnfrd commented Oct 27, 2015

Uh oh!

adampalay commented Oct 27, 2015

Uh oh!

sarina commented Oct 27, 2015

Uh oh!

adampalay commented Oct 27, 2015

Uh oh!

sarina commented Oct 27, 2015

Uh oh!

adampalay commented Oct 27, 2015

Uh oh!

pmitros commented Oct 27, 2015

Uh oh!

sarina commented Oct 27, 2015

Uh oh!

pmitros commented Oct 27, 2015

Uh oh!

stvstnfrd commented Oct 27, 2015

Uh oh!

sarina commented Oct 28, 2015

Uh oh!

sarina commented Oct 28, 2015

Uh oh!

pmitros commented Oct 28, 2015

Uh oh!

sarina commented Oct 28, 2015

Uh oh!

pmitros commented Oct 28, 2015

Uh oh!

explorerleslie commented Oct 28, 2015

Uh oh!

sarina commented Oct 28, 2015

Uh oh!

explorerleslie commented Oct 28, 2015

Uh oh!

sarina Oct 29, 2015

Choose a reason for hiding this comment

Uh oh!

stvstnfrd commented Oct 29, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants