Skip to content

Conversation

@stvstnfrd
Copy link
Contributor

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

@openedx-webhooks
Copy link

Thanks for the pull request, @stvstnfrd! I've created OSPR-431 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was my original inspiration.

@sarina
Copy link
Contributor

sarina commented Mar 2, 2015

test this please

(rerunning tests because of odd migrations errors in the first build)

@sarina
Copy link
Contributor

sarina commented Mar 2, 2015

LGTM 👍

@pmitros could you please confirm this is correct? Git blame suggests you edited /added some of these files.

@sarina
Copy link
Contributor

sarina commented Mar 2, 2015

odd issues occurring in the test startup - I've pinged test eng about this.

@pmitros
Copy link
Contributor

pmitros commented Mar 2, 2015

I'm okay with this iff the circuit editor still works in the wiki has been manually verified. This functionality was written in days before we had a test suite, so I am not confident it has proper test coverage. I know several times there were changes which made circuits in the wiki not work or effectively not work (in the latter case, bad CSS).

The steps to confirm:

  • In the wiki, type a single line with the text "circuit-schematic:"
  • A blank circuit schematic should appear. Click on it to edit the circuit.
  • Add components, and hit save.
  • Save page

There should be an in-line circuit schematic.

If this (or prior) changes broke this, they should be rolled back. If this (and prior) changes didn't break this, it's good to kill dead code.

@sarina
Copy link
Contributor

sarina commented Mar 2, 2015

@stvstnfrd - can you please manually test as Piotr suggests and report back with results? Also I have no idea why your build is failing. Perhaps rebase later this afternoon.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed community manager review labels Mar 2, 2015
These endpoints (`edit_circuit` and `save_circuit`) had already been
commented out of `urls.py`, so these views were disabled.
@sarina
Copy link
Contributor

sarina commented Mar 16, 2015

Needs manual test confirmation as described above, as well as a clean build.

@stvstnfrd
Copy link
Contributor Author

@sarina @pmitros What do you want first, the good news or the bad news?

The good news: I don't think I broke it.
The bad news: I think someone already did.

Even running against edx/master, I have been unable to add a circuit schematic to a wiki page; the console complains about JavaScript errors involving CodeMirror. Could either of you confirm or deny this behavior on your instance?

Assuming this is broken across-the-board, how would you like to proceed?

@pmitros Is the wiki the only place which should be using this code at this point in time?

Thanks; please advise.

@sarina
Copy link
Contributor

sarina commented Mar 17, 2015

@stvstnfrd honestly I know nothing about this :( CodeMirror was updated a few months ago, and if this hasn't raised any red flags my guess would be no one is using this. But I'll let Piotr comment.

@stvstnfrd
Copy link
Contributor Author

honestly I know nothing about this

@sarina No worries. If you could, I'd appreciate a "second opinion" if you're able to test on your instance to corroborate my devstack findings

Are you actually using this feature in live courses? If you are and haven't had any reports (and we can confirm it is indeed broken), can we pretty_pretty_pretty please burn this whole thing to the ground? I have a "nuke-circuits" PR just standing by... ;)

@sarina
Copy link
Contributor

sarina commented Mar 17, 2015

maybe @adampalay knows more?

@openedx-webhooks openedx-webhooks added community manager review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Mar 17, 2015
@adampalay
Copy link
Contributor

I've actually never heard of this feature. The circuit_servercircuit table is empty in our db. It doesn't look like this has shown up in the wiki in over a year.

And yeah, the wiki feature isn't working for me either.

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

@sarina
Copy link
Contributor

sarina commented Mar 17, 2015

I'm not sure what the right thing to do here is, then. If the url endpoints were already disabled, I don't see a ton of risk to merging this. However, it seems more prudent to fix the feature before merging this in, to verify the fixed feature doesn't break on this. That said, none of us here are going to be able to take the time to do so.

@pmitros do you have any opinions/thoughts here, please

@pmitros
Copy link
Contributor

pmitros commented Mar 17, 2015

The ability to embed circuits in the wiki was actually very helpful in 6.002x. Having the feature also provides a nice architectural basis for doing similar things for other types of course content. The way this worked was pretty brilliant to -- Bridger did a great job with it. I'd be a little sad to lose the foundation to do this in the future. Re-implementng this from scratch would be a ton of work. Extending this, much less so.

On the other hand, I don't see anyone having time to fix this anytime soon.

@sarina
Copy link
Contributor

sarina commented Mar 17, 2015

I'm going to put this, then, into the product review category and ask Leslie her thoughts on this.

@openedx-webhooks openedx-webhooks added product review PR requires product review before merging and removed community manager review labels Mar 17, 2015
@openedx-webhooks openedx-webhooks added community manager review and removed product review PR requires product review before merging labels May 5, 2015
@sarina
Copy link
Contributor

sarina commented May 7, 2015

@stvstnfrd this is good to merge, but I can't re-run a test on it until it gets updated infrastructure. Would you mind rebasing?

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed community manager review labels May 7, 2015
@sarina
Copy link
Contributor

sarina commented May 7, 2015

@stvstnfrd FYI there are no merge conflicts so the rebase should be quite straightforward. Just need to pick up the updated test infrastructure.

@stvstnfrd
Copy link
Contributor Author

Thanks for following up on this @sarina .

Since I first opened this PR, I've found another block or two of code that should be removed along with this. I'll look to tack that on by end-of-week.

@sarina
Copy link
Contributor

sarina commented May 17, 2015

@stvstnfrd ok, please ping me when that's done. If you can't get to that, let's at least just get this rebased and merged.

@sarina
Copy link
Contributor

sarina commented May 26, 2015

@stvstnfrd - I'm going to close this tomorrow because it seems like you don't have time to get to it.

Alternatively, if you can rebase this atop master today, I can merge what has been done in.

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

Labels

waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants