Skip to content

Implement Track Changes - #620

Closed
jrbl wants to merge 4 commits into
openedx:masterfrom
Stanford-Online:edx-west/trackchanges
Closed

Implement Track Changes#620
jrbl wants to merge 4 commits into
openedx:masterfrom
Stanford-Online:edx-west/trackchanges

Conversation

@jrbl

@jrbl jrbl commented Aug 22, 2014

Copy link
Copy Markdown
Contributor

@stephensanchez please. I'll be on vacation next week, but @caesar2164 and @stvstnfrd will be around and can handle our end of the PR process.

  • Creates "TrackChanges" django model, which ties a (submission_uuid,
    scorer_id) pair to a set of edits suggested by peers for a particular
    submission.
  • Adds machinery for on-the-fly installation of the New York Times's ICE
    library for collaborative editing, allowing peer assessors to create
    marked-up versions of submissions, which can then be returned to the
    author as feedback.
  • Adds machinery for displaying suggested edits to the creator of a
    submission with their scores and other feedback.
  • Configurable within Studio on a per-Assessment basis by including a
    URL to the ICE library as part of the assessment definition.
  • For additional details, see:
    https://docs.google.com/document/d/18PITJu133sFpDl_B2GCbo2eb0GODcwVmVsktwPQfVSg/edit?usp=sharing

Co-authored-by: Giulio Gratta giulio@giuliogratta.com
Co-authored-by: Steven Burch stv@stanford.edu
Co-authored-by: Joe Blaylock jrbl@jrbl.org

@sefk

sefk commented Aug 22, 2014

Copy link
Copy Markdown

Peeking in on the AUTHORS file here, @jrbl is already in there but @stvstnfrd and @caesar2164 are not. Maybe one or both should be added in this PR?

@stvstnfrd

Copy link
Copy Markdown
Contributor

AUTHORS updated at 52b0d8b

@sarina

sarina commented Aug 23, 2014

Copy link
Copy Markdown
Contributor

Also this needs a rebase - appears to have a merge conflict w/ master

@caesar2164

Copy link
Copy Markdown

@sarina & @stephensanchez - this is now rebased and squashed and all pretty ready for review!

(tag @jrbl, @stvstnfrd & @sefk)

@caesar2164
caesar2164 force-pushed the edx-west/trackchanges branch from 12424fa to a72bd13 Compare August 25, 2014 14:58
@stephensanchez

Copy link
Copy Markdown

Thanks @caesar2164 ! I'm going to ping @explorerleslie and @Lyla-Fischer to get a product review of the feature before we go through the full code review.

It may be worthwhile to get some readthedocs dev documentation around this feature, to better understand what steps are required to get this working on the platform.

@sarina

sarina commented Aug 25, 2014

Copy link
Copy Markdown
Contributor

@caesar2164 while you're waiting for Product review I recommend that you investigate and solve your test failures, as well as make sure your test coverage is as close to 100% as possible.

@stephensanchez

Copy link
Copy Markdown

@andy-armstrong just FYI on a new ORA2 feature in the pipeline.

@caesar2164
caesar2164 force-pushed the edx-west/trackchanges branch 2 times, most recently from 7786117 to 26efae8 Compare August 25, 2014 17:49
* Creates "TrackChanges" django model, which ties a (submission_uuid,
  scorer_id) pair to a set of edits suggested by peers for a particular
  submission.
* Adds machinery for on-the-fly installation of the New York Times's ICE
  library for collaborative editing, allowing peer assessors to create
  marked-up versions of submissions, which can then be returned to the
  author as feedback.
* Adds machinery for displaying suggested edits to the creator of a
  submission with their scores and other feedback.
* Configurable within Studio on a per-Assessment basis by including a
  URL to the ICE library as part of the assessment definition.
* For additional details, see:
  https://docs.google.com/document/d/18PITJu133sFpDl_B2GCbo2eb0GODcwVmVsktwPQfVSg/edit?usp=sharing

Co-authored-by: Giulio Gratta <giulio@giuliogratta.com>
Co-authored-by: Steven Burch <stv@stanford.edu>
Co-authored-by: Joe Blaylock <jrbl@jrbl.org>

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

If this is the case, I think you should add a unique_together constraint so this invariant is enforced at the database level.

@stephensanchez

Copy link
Copy Markdown

Done reviewing for a first pass, once some of these changes are completed I'll try standing up a VM with the branch and see if anything stands out.

One additional comment, via the email chain associated with this, is that we'll need a way to put this all behind a feature flag.

@stephensanchez

Copy link
Copy Markdown

I don't have a silver-bullet suggestion for this, but the track changes logic throughout the mixins and xblock is a bit inter-woven and it'd be nice if it were more cleanly separated. The solution may be another mixin, that declares the additional fields and logic that is found throughout, but I imagine whatever the refactoring will be beneficial to putting track changes behind a feature flag as well.

@sarina

sarina commented Sep 16, 2014

Copy link
Copy Markdown
Contributor

Review ticket: https://openedx.atlassian.net/browse/OSPR-31 Please add any supporting information to this ticket. JIRA is a place for product owners to prioritize feature reviews. Supporting information that is good to add there is anything that can help Product understand the context for the PR - supporting documentation, edx-code email threads, timeline information, partner information, etc.

@stephensanchez

Copy link
Copy Markdown

@stvstnfrd @caesar2164 I just want to check in and make sure I did not miss anything; we're still looking to update the CSS to SASS, and put the functionality behind a feature flag? I don't mean to rush anything, I just want to make sure I'm not being a neglectful reviewer.

@sarina sarina added product review PR requires product review before merging and removed waiting on Product labels Sep 17, 2014
* Use self.runtime.platform_settings['ORA2_TRACKCHANGES_URL'] to find
  location of change tracking javascript library.
* TrackChanges is fully disabled if this variable is unset.
* TrackChanges may be enabled on a per-problem basis using a convenient
  checkbox on the Studio settings panel for the peer assessment.
* Documentation updates
* Bleach changes as they are marshaled for display, eliminating unsafe
  markup.
@sarina

sarina commented Oct 15, 2014

Copy link
Copy Markdown
Contributor

Hi everyone: just going through old PRs. Is this PR still under active development? If not, would it be appropriate to close it? It can always be reopened (or a new PR submitted) at a later date.

@sarina sarina added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed product review PR requires product review before merging labels Oct 24, 2014
@sarina

sarina commented Oct 24, 2014

Copy link
Copy Markdown
Contributor

Haven't received a response on this PR so I'm going to close it for now - it can always be reopened when it is ready to be reviewed.

@sarina sarina closed this Oct 24, 2014
@caesar2164
caesar2164 deleted the edx-west/trackchanges branch October 18, 2017 20:28
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.

8 participants