Skip to content

Use score summaries in data download #43

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

Merged
efischer19 merged 1 commit into from
Mar 8, 2016
Merged

Use score summaries in data download #43

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 15 additions & 15 deletions submissions/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -447,27 +447,27 @@ def get_all_course_submission_information(course_id, item_type, read_replica=Tru
if read_replica:
submission_qs = _use_read_replica(submission_qs)

query = submission_qs.select_related('student_item').prefetch_related('score_set').filter(
query = submission_qs.select_related('student_item__scoresummary__latest__submission').filter(

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

This select_related means that, for each submission in our query, we are fetching:

  • its student_item, which can be tied to a
  • scoresummary, which can be tied to a
  • score (latest), which can be tied to a
  • submission

The upshot is that, for each submission, we can ask the question If we tried to get the score for this submission's student_item, would the score reported be associated with this submission? If it is, the score is included in our data download. If it is not, no score is returned.

This is to handle a situation like this one:

  • student makes a submission
  • student gets a poor score on that submission
  • student makes a new submission
  • student gets a better score on that submission

The returned values in this case would be both submissions, but no score would be reported for the first one. Is this desirable? Or would it be better to return both scores even though one of them doesn't count? We have elected to go this route for now, since that seems to be how the api is set up for get_score. The third option (that I really don't like) is to include only the relevant score for each submission's student_item, but then in the scenario above you'd end up with a row that seems to indicate the first submission got the second score.

Worth noting: ORA imposes a limit of 1 submission per student item (excluding delete state requests, but that data was already being excluded), so this is irrelevant in the context of ORA data downloads. It's more a question of how edx-submissions wants to handle this theoretical case for other consumers in the future.

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

As you stated, it really is a question of who needs the data download and what they want included:

  1. Do they want all scores with additional data (like a latest flag) to help them filter the data in the download appropriately? Or,
  2. Does someone just want the latest (i.e. relevant) scores with no other historical scores?

One possibility is to go with the latest, since it is simpler, and add an option later if and when it is needed. Can we find out if this aligns with the original requirement at a minimum?

@ormsbee @scottrish Thoughts on this?

Depending on which way we go, I'll have some additional minor requests on naming and commenting to make this more clear.

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

If we stick with latest score, the comment for this method should mention "latest score" each time it currently reads "score". The function comment should also mention that an empty score will be sent for submissions with no score.

@ormsbee ormsbee Mar 9, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

My guess is that most people will only want the latest relevant score with no historical data for something like a download, so that should be the default behavior. But I think there are valid reasons for wanting every submission and its corresponding score or scores in the future.

student_item__course_id=course_id,
student_item__item_type=item_type,
).iterator()

for submission in query:
student_item = submission.student_item
if submission.score_set.count() > 0:
for score in submission.score_set.all():
yield (
StudentItemSerializer(student_item).data,
SubmissionSerializer(submission).data,
ScoreSerializer(score).data
)
else:
# Make sure we return submission information even if there isn't a score associated with it.
yield (
StudentItemSerializer(student_item).data,
SubmissionSerializer(submission).data,
{}
)
serialized_score = {}
if hasattr(student_item, 'scoresummary'):
latest_score = student_item.scoresummary.latest

# We only include the score for a given submission if it is not a reset score and it "counts", that is,
# if it is the latest score on the score summary tracking the submission's student_item. This matches the
# behavior of the API's get_score method.

@robrap robrap Mar 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 @dianakhuang Does the following comment sound more clear to you?

# Only include the score if it is not a reset score (is_hidden), and if the current submission matches 
# the latest score's submission.  This matches the behavior of the API's get_score method.

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

It's better, but I'm still looking at if the current submission matches the latest score's submission. Is there some way we can mention the student_item in there as well? Something like if the current submission corresponds to the latest score for this student_item?

@robrap robrap Mar 9, 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'd like something that makes all the lines of code more clear, including the last test comparing the submission.uuid's.

Maybe:

...if the current submission is the same as the student_item's latest score's submission.

@efischer19 efischer19 Mar 9, 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 like that one, I'll implement it when I update this code later. Thanks!

if (not latest_score.is_hidden()) and latest_score.submission.uuid == submission.uuid:
serialized_score = ScoreSerializer(latest_score).data
yield (
StudentItemSerializer(student_item).data,
SubmissionSerializer(submission).data,
serialized_score
)


def get_top_submissions(course_id, item_id, item_type, number_of_top_scores, use_cache=True, read_replica=True):
Expand Down
6 changes: 4 additions & 2 deletions submissions/tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,15 +127,17 @@ def test_get_course_submissions(self):

self.assertDictEqual(SECOND_STUDENT_ITEM, submissions_and_scores[1][0])
self._assert_submission(submissions_and_scores[1][1], submission3['answer'], student_item2.pk, 1)
self._assert_score(submissions_and_scores[1][2], 3, 4)
# submission4 also pertains to this student item and got its score later, so no score will be reported here
self.assertEqual(submissions_and_scores[1][2], {})

self.assertDictEqual(STUDENT_ITEM, submissions_and_scores[2][0])
self._assert_submission(submissions_and_scores[2][1], submission2['answer'], student_item1.pk, 2)
self._assert_score(submissions_and_scores[2][2], 2, 4)

self.assertDictEqual(STUDENT_ITEM, submissions_and_scores[3][0])
self._assert_submission(submissions_and_scores[3][1], submission1['answer'], student_item1.pk, 1)
self._assert_score(submissions_and_scores[3][2], 1, 4)
# submission2 also pertains to this student item and got its score later, so no score will be reported here
self.assertEqual(submissions_and_scores[3][2], {})

def test_get_submission(self):
# Test base case that we can create a submission and get it back
Expand Down