Implement Track Changes #620

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
8 participants
@jrbl
Contributor

jrbl commented Aug 22, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@sefk

sefk Aug 22, 2014

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?

sefk commented Aug 22, 2014

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

This comment has been minimized.

Show comment
Hide comment
@stvstnfrd

stvstnfrd Aug 22, 2014

Member

AUTHORS updated at 52b0d8b

Member

stvstnfrd commented Aug 22, 2014

AUTHORS updated at 52b0d8b

@sarina

This comment has been minimized.

Show comment
Hide comment
@sarina

sarina Aug 23, 2014

Contributor

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

Contributor

sarina commented Aug 23, 2014

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

@caesar2164

This comment has been minimized.

Show comment
Hide comment
@caesar2164

caesar2164 Aug 25, 2014

Member

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

(tag @jrbl, @stvstnfrd & @sefk)

Member

caesar2164 commented Aug 25, 2014

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

(tag @jrbl, @stvstnfrd & @sefk)

@stephensanchez

This comment has been minimized.

Show comment
Hide comment
@stephensanchez

stephensanchez Aug 25, 2014

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.

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

This comment has been minimized.

Show comment
Hide comment
@sarina

sarina Aug 25, 2014

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.

Contributor

sarina commented Aug 25, 2014

@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

This comment has been minimized.

Show comment
Hide comment
@stephensanchez

stephensanchez Aug 25, 2014

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

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

Track Changes initial implementation
* 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>
+
+class TrackChanges(models.Model):
+ """Store copies of student submission texts with inline editing marks."""
+ # A (owner_submission_uuid, scorer_id) pair uniquely specifies a piece of edited content

This comment has been minimized.

@wedaly

wedaly Sep 5, 2014

Contributor

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

@wedaly

wedaly Sep 5, 2014

Contributor

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

openassessment/xblock/grade_mixin.py
+ track_changes = TrackChanges.objects.filter(scorer_id = assessment['scorer_id'],
+ owner_submission_uuid = assessment['submission_uuid'])
+ if track_changes:
+ assessment['track_changes'] = track_changes.get().edited_content

This comment has been minimized.

@wedaly

wedaly Sep 5, 2014

Contributor

Without a database constraint, I think this can fail if multiple records match the query.

@wedaly

wedaly Sep 5, 2014

Contributor

Without a database constraint, I think this can fail if multiple records match the query.

+ $('<span class="peer' + peerNumber + '">Peer ' + peerNumber + "'s Edits</span>")
+ .appendTo(gradingTitleHeader).hide();
+ $('<option value="peer' + peerNumber + '">Peer ' + peerNumber + "'s Edits</option>")
+ .appendTo(peerEditSelect);

This comment has been minimized.

@wedaly

wedaly Sep 5, 2014

Contributor

These strings need to be internationalized.

@wedaly

wedaly Sep 5, 2014

Contributor

These strings need to be internationalized.

This comment has been minimized.

@jrbl

jrbl Sep 22, 2014

Contributor

@caesar2164 Could you please do this?

@jrbl

jrbl Sep 22, 2014

Contributor

@caesar2164 Could you please do this?

+ {% with peer_num=forloop.counter %}
+ {% if assessment.track_changes %}
+ <div class="submission__answer__display__content submission__answer__display__content__edited" id="submission__answer__display__content__edited__peer{{ peer_num }}" data-peer-num="{{ peer_num }}">
+ {% autoescape off %}{{ assessment.track_changes }}{% endautoescape %}

This comment has been minimized.

@wedaly

wedaly Sep 5, 2014

Contributor

I'm concerned about a potential injection attack here. If I'm understanding this correctly, the markup gets sent from the browser to the server. If so, we can't trust that the markup is safe.

@jbau what do you think?

@wedaly

wedaly Sep 5, 2014

Contributor

I'm concerned about a potential injection attack here. If I'm understanding this correctly, the markup gets sent from the browser to the server. If so, we can't trust that the markup is safe.

@jbau what do you think?

This comment has been minimized.

@jbau

jbau Sep 5, 2014

you're probably right. maybe we (@jrbl @caesar2164) want to experiment with running the track changes output through bleach first. I think it should preserve formatting, but eliminate possible <script> tags

i'll discuss with them

@jbau

jbau Sep 5, 2014

you're probably right. maybe we (@jrbl @caesar2164) want to experiment with running the track changes output through bleach first. I think it should preserve formatting, but eliminate possible <script> tags

i'll discuss with them

@@ -99,6 +100,20 @@ OpenAssessment.BaseView.prototype = {
},
+ /**
+ Enable TrackChanges
+ **/
+ enableTrackChangesView: function () {

This comment has been minimized.

@stephensanchez

stephensanchez Sep 10, 2014

I don't think you need these two functions here. In peer, and other JS files, you should be able to do this.trackChangesView.enableTrackChanges();

@stephensanchez

stephensanchez Sep 10, 2014

I don't think you need these two functions here. In peer, and other JS files, you should be able to do this.trackChangesView.enableTrackChanges();

This comment has been minimized.

@@ -0,0 +1,104 @@
+(function (window) {

This comment has been minimized.

@stephensanchez

stephensanchez Sep 10, 2014

Why is this all wrapped in a function(window) ?

@stephensanchez

stephensanchez Sep 10, 2014

Why is this all wrapped in a function(window) ?

This comment has been minimized.

@stvstnfrd

stvstnfrd Sep 11, 2014

Member

This is an IIFE [1]. It leverages JavaScript's function-based scoping to
provide an isolated local scope. This ensures that no local variable
declarations are implicitly added to the global namespace. It also
allows us to enable strict mode for the entire file/module without
applying these strict requirements to nearby code.

In browser applications, the window object represents the global
namespace; it is the default value for this. By passing the window
object as a parameter to the IIFE, the code is more modular and explicit
by ensuring that accessing any global property is done explicitly
through the window object. This behavior is enforced with a linter.

Dereferencing properties from the global object makes code:

  • more performant by preventing namespace resolution from traversing the
    scope tree upon access of each property
  • more obvious by removing implicit globals properties

[1] http://en.wikipedia.org/wiki/Immediately-invoked_function_expression

@stvstnfrd

stvstnfrd Sep 11, 2014

Member

This is an IIFE [1]. It leverages JavaScript's function-based scoping to
provide an isolated local scope. This ensures that no local variable
declarations are implicitly added to the global namespace. It also
allows us to enable strict mode for the entire file/module without
applying these strict requirements to nearby code.

In browser applications, the window object represents the global
namespace; it is the default value for this. By passing the window
object as a parameter to the IIFE, the code is more modular and explicit
by ensuring that accessing any global property is done explicitly
through the window object. This behavior is enforced with a linter.

Dereferencing properties from the global object makes code:

  • more performant by preventing namespace resolution from traversing the
    scope tree upon access of each property
  • more obvious by removing implicit globals properties

[1] http://en.wikipedia.org/wiki/Immediately-invoked_function_expression

This comment has been minimized.

+ TrackChangesView.prototype.enableTrackChanges = function enableTrackChanges() {
+ var tracker;
+ var $ = window.jQuery;
+ var ice = window.ice;

This comment has been minimized.

@stephensanchez

stephensanchez Sep 10, 2014

If you take a look at oa_shared.js, it'd be nice to follow a pattern here that we do for i18n, and other JS libraries that may or may not be available in the platform. This way, if we get to this javascript, it will not fail in an obscure way if ice is not available.

@stephensanchez

stephensanchez Sep 10, 2014

If you take a look at oa_shared.js, it'd be nice to follow a pattern here that we do for i18n, and other JS libraries that may or may not be available in the platform. This way, if we get to this javascript, it will not fail in an obscure way if ice is not available.

This comment has been minimized.

@stvstnfrd

stvstnfrd Sep 11, 2014

Member

This is actually a different use case.

What's happening in oa_shared.js is that we're stubbing out methods
that may not have been provided by the runtime environment.

On the other hand, here in oa_trackchanges.js, the ice object must
exist. It will exist regardless of the runtime environment in which this
method is executed, since we're including it explicitly and not
relying on the runtime to provide it for us (as is the case with i18n,
etc.).

In the off case that it was not present, the code should
fail here. It will not fail in an obscure way; we'll receive a
ReferenceError: ice is not defined (or browser equivalent).

@stvstnfrd

stvstnfrd Sep 11, 2014

Member

This is actually a different use case.

What's happening in oa_shared.js is that we're stubbing out methods
that may not have been provided by the runtime environment.

On the other hand, here in oa_trackchanges.js, the ice object must
exist. It will exist regardless of the runtime environment in which this
method is executed, since we're including it explicitly and not
relying on the runtime to provide it for us (as is the case with i18n,
etc.).

In the off case that it was not present, the code should
fail here. It will not fail in an obscure way; we'll receive a
ReferenceError: ice is not defined (or browser equivalent).

This comment has been minimized.

@stephensanchez

stephensanchez Sep 12, 2014

Thanks for the clarification! I'm wondering what'd happen if someone misconfigures this, or if it points to a URL for the JS source and gets a 404 (maybe temporarily). Will it fail somewhat gracefully?

@stephensanchez

stephensanchez Sep 12, 2014

Thanks for the clarification! I'm wondering what'd happen if someone misconfigures this, or if it points to a URL for the JS source and gets a 404 (maybe temporarily). Will it fail somewhat gracefully?

This comment has been minimized.

@stvstnfrd

stvstnfrd Sep 15, 2014

Member

If the ICE library cannot be loaded, we'll see a ReferenceError: ice is not defined message. This is what we want; the code should fail hard
if a required library is not added.

@stvstnfrd

stvstnfrd Sep 15, 2014

Member

If the ICE library cannot be loaded, we'll see a ReferenceError: ice is not defined message. This is what we want; the code should fail hard
if a required library is not added.

This comment has been minimized.

@stephensanchez

stephensanchez Sep 16, 2014

Sounds good, thanks

@stephensanchez

stephensanchez Sep 16, 2014

Sounds good, thanks

+ due="2015-12-21T22:22-7:00"
+ must_grade="2"
+ must_be_graded_by="2"
+ track_changes="http://d5j76nbt6pruj.cloudfront.net/ice-master.min.js"/>

This comment has been minimized.

@stephensanchez

stephensanchez Sep 10, 2014

I think this line would have to be removed before commit.

@stephensanchez

stephensanchez Sep 10, 2014

I think this line would have to be removed before commit.

This comment has been minimized.

@stvstnfrd

stvstnfrd Sep 12, 2014

Member

Removed at 252b5c2

@stvstnfrd

stvstnfrd Sep 12, 2014

Member

Removed at 252b5c2

- "<assessment name=\"peer-assessment\" start=\"2014-02-27T09:46:28\" due=\"2014-03-01T00:00:00\" must_grade=\"5\" must_be_graded_by=\"3\" />",
- "<assessment name=\"self-assessment\" start=\"2014-04-01T00:00:00\" due=\"2014-06-01T00:00:00\" />",
+ "<assessment name=\"peer-assessment\" start=\"2014-02-27T09:46:28\" due=\"2014-03-01T00:00:00\" must_grade=\"5\" must_be_graded_by=\"3\" track_changes=\"\" />",
+ "<assessment name=\"self-assessment\" start=\"2014-04-01T00:00:00\" due=\"2014-06-01T00:00:00\" track_changes=\"\" />",

This comment has been minimized.

@stephensanchez

stephensanchez Sep 10, 2014

Why does self assessment have track_changes?

@stephensanchez

stephensanchez Sep 10, 2014

Why does self assessment have track_changes?

This comment has been minimized.

@stvstnfrd

stvstnfrd Sep 15, 2014

Member

"Peer-assessments" and "self-assessments" are both, at their core,
"assessments". Since the track_changes field is associated with the
assessment model, the field is present on any assessment and included in
its serialization (defaults to the empty string).

Naturally, self-assessments don't need track_changes, but we include
it here because of how the tests check for equality (tests expect all fields
to be included in the serialization).

@stvstnfrd

stvstnfrd Sep 15, 2014

Member

"Peer-assessments" and "self-assessments" are both, at their core,
"assessments". Since the track_changes field is associated with the
assessment model, the field is present on any assessment and included in
its serialization (defaults to the empty string).

Naturally, self-assessments don't need track_changes, but we include
it here because of how the tests check for equality (tests expect all fields
to be included in the serialization).

This comment has been minimized.

@@ -269,6 +275,13 @@ def student_view(self, context=None):
frag = Fragment(template.render(context))
frag.add_css(load("static/css/openassessment.css"))
frag.add_javascript(load("static/js/openassessment-lms.min.js"))
+
+ track_changes_fragments = set([x['track_changes'] for x in ui_models if x.get('track_changes', None)])

This comment has been minimized.

@stephensanchez

stephensanchez Sep 10, 2014

I'm unclear as to what's being done here. We're iterating over all the assessment models and if any single one has track_changes enabled, we set the JS url for the HTML fragment based on that setting? I think there should be a single boolean value for whether or not we track changes, and another value for the entire block for the URL. This way you don't make a typo on one module, and break the feature across the board.

@stephensanchez

stephensanchez Sep 10, 2014

I'm unclear as to what's being done here. We're iterating over all the assessment models and if any single one has track_changes enabled, we set the JS url for the HTML fragment based on that setting? I think there should be a single boolean value for whether or not we track changes, and another value for the entire block for the URL. This way you don't make a typo on one module, and break the feature across the board.

@@ -0,0 +1,33 @@
+.submission__answer__display__content__peeredit__select {

This comment has been minimized.

@stephensanchez

stephensanchez Sep 10, 2014

We've been using SASS consistently for the project, could you create SASS instead of CSS?

@stephensanchez

stephensanchez Sep 10, 2014

We've been using SASS consistently for the project, could you create SASS instead of CSS?

This comment has been minimized.

@caesar2164

caesar2164 Sep 12, 2014

Member

SASS is useful when there's a lot of nesting or there are variables to reuse. In this case this file is so short that it has no real need for SASS...

@caesar2164

caesar2164 Sep 12, 2014

Member

SASS is useful when there's a lot of nesting or there are variables to reuse. In this case this file is so short that it has no real need for SASS...

This comment has been minimized.

@stephensanchez

stephensanchez Sep 12, 2014

I understand that point of view. I had thought it'd be good to be consistent by keeping all our styles in a set of SASS files, that way, when a new developer comes to look at the ORA project, they won't have to find another CSS file as well.

The SASS is compiled into a single CSS file as well, which is nice, so we don't have to worry about loading multiple resources.

@stephensanchez

stephensanchez Sep 12, 2014

I understand that point of view. I had thought it'd be good to be consistent by keeping all our styles in a set of SASS files, that way, when a new developer comes to look at the ORA project, they won't have to find another CSS file as well.

The SASS is compiled into a single CSS file as well, which is nice, so we don't have to worry about loading multiple resources.

@@ -271,6 +276,14 @@ def create_assessment(
scored_at
)
+ if track_changes_edits:

This comment has been minimized.

@stephensanchez

stephensanchez Sep 10, 2014

I would like there to be a track changes API file for this work, that is responsible for all CRUD operations and business logic to the track changes model. This would keep any track changes logic from existing in the peer assessment API, and would avoid model queries in the XBlock mixin (grade_mixin.py, I'll add a comment there)

Theoretically, you could create a context manager for track_changes that could be used in the peer assessment mixin (or eventually, other mixins) that can:

  1. check if track_changes is enabled,
  2. invoke the peer assessment call,
  3. create the TrackChanges model associated,
  4. wrap all of this in a single DB transaction.

Also, this will make the simple interactions with creating and reading TrackChanges operations easily testable separate from the APIs and mixins.

@stephensanchez

stephensanchez Sep 10, 2014

I would like there to be a track changes API file for this work, that is responsible for all CRUD operations and business logic to the track changes model. This would keep any track changes logic from existing in the peer assessment API, and would avoid model queries in the XBlock mixin (grade_mixin.py, I'll add a comment there)

Theoretically, you could create a context manager for track_changes that could be used in the peer assessment mixin (or eventually, other mixins) that can:

  1. check if track_changes is enabled,
  2. invoke the peer assessment call,
  3. create the TrackChanges model associated,
  4. wrap all of this in a single DB transaction.

Also, this will make the simple interactions with creating and reading TrackChanges operations easily testable separate from the APIs and mixins.

@@ -350,4 +351,10 @@ def _assessment_grade_context(self, assessment):
option_label_key = (part['criterion']['name'], part['option']['name'])
part['option']['label'] = option_labels.get(option_label_key, part['option']['name'])
+ assessment['track_changes'] = None

This comment has been minimized.

@stephensanchez

stephensanchez Sep 10, 2014

As mentioned above in peer.py, this entire operation would ideally be in an api
track_changes.get_changes(scorer_id)

@stephensanchez

stephensanchez Sep 10, 2014

As mentioned above in peer.py, this entire operation would ideally be in an api
track_changes.get_changes(scorer_id)

@stephensanchez

This comment has been minimized.

Show comment
Hide comment
@stephensanchez

stephensanchez Sep 10, 2014

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.

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

This comment has been minimized.

Show comment
Hide comment
@stephensanchez

stephensanchez Sep 10, 2014

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.

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

This comment has been minimized.

Show comment
Hide comment
@sarina

sarina Sep 16, 2014

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.

Contributor

sarina commented Sep 16, 2014

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

This comment has been minimized.

Show comment
Hide comment
@stephensanchez

stephensanchez Sep 16, 2014

@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.

@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.

Toggle TrackChanges with platform-wide setting
* 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.
@stvstnfrd

This comment has been minimized.

Show comment
Hide comment
@stvstnfrd

stvstnfrd Sep 22, 2014

Member

Indentation is off here. Do you mind updating to follow the style guide's convention?

assessment['track_changes'] = bleach.clean(
    track_changes.get().edited_content,
    tags=['p', 'span'],
    attributes=['*', 'class', 'data-userid', 'data-cid', 'data-username', 'data-time'],
)

While you're in there, can you use a consistent quote character (choose single or double) and include a trailing comma in the argument list?

Indentation is off here. Do you mind updating to follow the style guide's convention?

assessment['track_changes'] = bleach.clean(
    track_changes.get().edited_content,
    tags=['p', 'span'],
    attributes=['*', 'class', 'data-userid', 'data-cid', 'data-username', 'data-time'],
)

While you're in there, can you use a consistent quote character (choose single or double) and include a trailing comma in the argument list?

@sarina

This comment has been minimized.

Show comment
Hide comment
@sarina

sarina Oct 15, 2014

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.

Contributor

sarina commented Oct 15, 2014

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

This comment has been minimized.

Show comment
Hide comment
@sarina

sarina Oct 24, 2014

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.

Contributor

sarina commented Oct 24, 2014

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 Stanford-Online:edx-west/trackchanges branch Oct 18, 2017

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