Skip to content

Clear state option #34

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
efischer19 wants to merge 4 commits into from
Closed

Clear state option #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2df28d2
Clear state option
Feb 4, 2016
f5e3409
Allow None FK on Submission model
Feb 4, 2016
d5381d4
Need to save model changes
Feb 4, 2016
1901812
Maybe making migration backwards-safe?
Feb 4, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion submissions/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -643,7 +643,7 @@ def get_latest_score_for_submission(submission_uuid, read_replica=False):
return ScoreSerializer(score).data


def reset_score(student_id, course_id, item_id):
def reset_score(student_id, course_id, item_id, clear_state=False):
"""
Reset scores for a specific student on a specific problem.

Expand All @@ -655,6 +655,7 @@ def reset_score(student_id, course_id, item_id):
student_id (unicode): The ID of the student for whom to reset scores.
course_id (unicode): The ID of the course containing the item to reset.
item_id (unicode): The ID of the item for which to reset scores.
clear_state (boolean): If True, unlink the Submission and StudentItem so the Submission cannot be accessed.

Returns:
None
Expand Down Expand Up @@ -684,6 +685,12 @@ def reset_score(student_id, course_id, item_id):
item_id=item_id,
)

if clear_state:
# sever the link between this student item and any submissions it may current have
for sub in student_item.submission_set.all():
sub.student_item = None
sub.save(update_fields=['student_item'])

@efischer19 efischer19 Feb 5, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mulby We're a little curious about how this will impact analytics.

Essentially what happens on a "reset state" request is that we leave the Submission item in the database, and when the StudentModule is deleted (https://github.com/edx/edx-platform/blob/master/lms/djangoapps/instructor/enrollment.py#L246-L253), the only possible reference to the submission (via its uuid) is lost.

However, with the addition of staff tools, now a third party (the staff user) can construct a triplet of (user/course/problem location) and use that as a key to access a supposedly orphaned submission. The way we're attempting to address the issue in this PR is to null out the key for an orphaned submission, which will then be persisted in the database.

Will this be an issue for analytics? It will make it possible for Submission data to exist that cannot be easily linked to a student. Would adding events coverage make it any better?

@efischer19 efischer19 Feb 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

An alternative approach is #35, where we mangle the key instead of deleting it. The course and student information would remain intact, but the problem identifier is reversed.

@mulby mulby Feb 8, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm not sure how valuable it is to keep the data if it can't actually be associated with the original student anymore.

Are we confident we have good event log coverage of this?

@efischer19 efischer19 Feb 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure how thorough the event coverage is, but see my latest comment on #35. We may be able to keep the original student association mostly intact.


except DatabaseError:
msg = (
u"Error occurred while reseting scores for"
Expand Down
20 changes: 20 additions & 0 deletions submissions/migrations/0003_submission_null_FK.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('submissions', '0002_auto_20151119_0913'),
]

operations = [
migrations.AlterField(
model_name='submission',
name='student_item',
field=models.ForeignKey(to='submissions.StudentItem', null=True, default=1),
preserve_default=False
),
]

@efischer19 efischer19 Feb 4, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

One potential issue I've discovered during dev testing - trying to reverse this migration after NULL values have been added results in an error, so I'll have to figure that out, at least.

django.db.utils.OperationalError: (1138, 'Invalid use of NULL value').

@efischer19 efischer19 Feb 5, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@maxrothman Here's some data from running this migration on my devstack with ~1400 rows of data, the read-replica indicates ~670,000 on production

https://gist.github.com/efischer19/b56b9d14768de28cf612

@maxrothman maxrothman Feb 8, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@efischer19 Do you feel those results are representative enough to indicate lack of an issue with ~100x rows?

@efischer19 efischer19 Feb 8, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think so. The two longest lines seem to be these, what else should we be considering when extrapolating?

DEBUG:django.db.backends:(0.114) ALTER TABLE `submissions_submission` MODIFY `student_item_id` integer NULL; args=[]
DEBUG:django.db.backends:(0.130) ALTER TABLE `submissions_submission` ADD CONSTRAINT `su_student_item_id_d3801ff833d05b1_fk_submissions_studentitem_id` FOREIGN KEY (`student_item_id`) REFERENCES `submissions_studentitem` (`id`); args=[]

There's also now a possibility of avoiding migrations entirely if the approach in #35 wins out as our best option.

2 changes: 1 addition & 1 deletion submissions/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ class Submission(models.Model):

uuid = UUIDField(version=1, db_index=True)

student_item = models.ForeignKey(StudentItem)
student_item = models.ForeignKey(StudentItem, null=True)

# Which attempt is this? Consecutive Submissions do not necessarily have
# increasing attempt_number entries -- e.g. re-scoring a buggy problem.
Expand Down