Skip to content
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.

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

Skip submissions if any have been graded in next submitter view #1199

Merged

Conversation

EerikSaksi
Copy link
Contributor

Fixes #1196

Description

What?

Submitters whose older work was already graded weren't skipped when pressing the "next submitter" link in manual assesment.

Why?
We shouldn't show a user whose work has already been graded even if they have later work.

How?

Rather than checking if the most recent submission has been graded, we skip submitters based on if any of their submissions for this exercise has been graded

Fixes #1196

Testing

What type of test did you run?

  • Accessibility test using the WAVE extension.
  • Django unit tests.
  • Selenium tests.
  • Other test. (Add a description below)
  • Manual testing.

In addition to unit tests I ensured that after grading the latest assesment the next submitter link takes you correctly. I also manually added a grader to the older work of the first user and ensured that this user was skipped even if their latest submission wasn't graded.

Did you test the changes in

  • Chrome
  • Firefox
  • This pull request cannot be tested in the browser.

Think of what is affected by these changes and could become broken

Translation

Programming style

  • Did you follow our style guides?
  • Did you use Python type hinting in all functions that you added or edited? (type hints for function parameters and return values)

Have you updated the README or other relevant documentation?

  • documents inside the doc directory.
  • README.md.
  • Aplus Manual.
  • Other documentation (mention below which documentation).

Is it Done?

  • Reviewer has finished the code review
  • After the review, the developer has made changes accordingly
  • Customer/Teacher has accepted the implementation of the feature

Clean up your git commit history before submitting the pull request!

@markkuriekkinen markkuriekkinen self-requested a review June 14, 2023 17:50
@markkuriekkinen markkuriekkinen self-assigned this Jun 16, 2023
Copy link
Collaborator

@markkuriekkinen markkuriekkinen left a comment

Choose a reason for hiding this comment

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

I think the query for the latest submissions of each submitter is still not working correctly.

Remember also that a submission may have multiple submitters and those cases should also be tested. The current unit tests use too few submissions and different submitters to reveal significant bugs.

I am pushing a new commit that fixes minor issues in the current unit tests.

@@ -1428,7 +1431,19 @@ def get_url_user_id(args=''):
# an even earlier submission in another exercise doesn't matter
Copy link
Collaborator

Choose a reason for hiding this comment

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

A few lines above, user2_other_exercise_submission is actually submitted by self.user and not self.user2.

# grading in an older submission (not latest) should also mean user1 is skipped
user_earlier_submission.grader = None
user_earlier_submission.save()
user_submission.grader = self.teacher.userprofile
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment says grading in an older submission (not latest) should also mean user1 is skipped, but aren't you grading the latest submission right here?

@@ -329,6 +329,8 @@ def get(self, request: HttpRequest, *args: Any, **kwargs: Any) -> HttpResponse:
# submissions for this exercise
submissions = Submission.objects.filter(exercise=self.exercise)

submitters_with_grading = submissions.exclude(grader=None).values_list('submitters__id', flat=True)

# filter only the latest submissions
submissions = (submissions.annotate(latest=Max('submission_time'))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think the annotatecall for the latest submissions is working correctly. It doesn't seem to find the latest submission for each submitter separately.

@markkuriekkinen markkuriekkinen force-pushed the earlier-submission-proper-handling branch from 3faa9c6 to 137241d Compare June 16, 2023 18:37
@markkuriekkinen
Copy link
Collaborator

I looked at the SQL query produced by the "submissions annotate latest` statement.

(submissions.annotate(latest=Max('submission_time'))
    .filter(submission_time=F('latest'))
    .order_by('latest')))

This GROUP BY part looks suspicious since it is not grouping by submitters. See the whole query below.

GROUP BY "exercise_submission"."id"
HAVING "exercise_submission"."submission_time" = MAX("exercise_submission"."submission_time")

The whole SQL query:

SELECT "exercise_submission"."id",
       "exercise_submission"."submission_time",
       "exercise_submission"."hash",
       "exercise_submission"."exercise_id",
       "exercise_submission"."grader_id",
       "exercise_submission"."feedback",
       "exercise_submission"."assistant_feedback",
       "exercise_submission"."status",
       "exercise_submission"."grade",
       "exercise_submission"."grading_time",
       "exercise_submission"."late_penalty_applied",
       "exercise_submission"."force_exercise_points",
       "exercise_submission"."service_points",
       "exercise_submission"."service_max_points",
       "exercise_submission"."submission_data",
       "exercise_submission"."grading_data",
       "exercise_submission"."meta_data",
       MAX("exercise_submission"."submission_time") AS "latest"
  FROM "exercise_submission"
 INNER JOIN "exercise_submission_submitters"
    ON ("exercise_submission"."id" = "exercise_submission_submitters"."submission_id")
 WHERE ("exercise_submission"."exercise_id" = 61 AND "exercise_submission_submitters"."userprofile_id" = 7)
 GROUP BY "exercise_submission"."id"
HAVING "exercise_submission"."submission_time" = MAX("exercise_submission"."submission_time")
 ORDER BY "latest" ASC

I think the part for querying a single user ("exercise_submission_submitters"."userprofile_id" = 7) came from the previous submitter (prev GET query parameter):

previous_submission_time = submissions.filter(submitters__id=previous_user_id).first().submission_time

markkuriekkinen added a commit to markkuriekkinen/a-plus that referenced this pull request Jun 18, 2023
This reverts commit 0a5d7f5.

The feature for chronological sorting of the submissions in
the `NextUnassessedSubmitterView` is still not working correctly.
Thus, I revert this commit before making a new stable release.

See issues apluslms#1196 and apluslms#1110 and pull requests apluslms#1199 and apluslms#1167.
@EerikSaksi
Copy link
Contributor Author

EerikSaksi commented Jun 19, 2023

I decided to try to try reimplement it by making small changes to the original query and managed to create a query which seems to provide the correct behavior for multiple users, and seems to group users based on their profile and sort them based on their earliest submission

submitters = (UserProfile.objects
		.filter(submissions__exercise=self.exercise)
		.annotate(
				count_assessed=Count(
						'submissions__id',
						filter=(Q(submissions__grader__isnull=False)),
				),
				earliest_submission=Min('submissions__submission_time'),
		)
		.order_by('earliest_submission'))
SELECT
  "userprofile_userprofile"."id",
  "userprofile_userprofile"."user_id",
  "userprofile_userprofile"."language",
  "userprofile_userprofile"."student_id",
  "userprofile_userprofile"."organization",
  COUNT("exercise_submission_submitters"."submission_id") FILTER (
    WHERE
      "exercise_submission"."grader_id" IS NOT NULL
  ) AS "count_assessed",
  MIN("exercise_submission"."submission_time") AS "earliest_submission",
  "auth_user"."id",
  "auth_user"."password",
  "auth_user"."last_login",
  "auth_user"."is_superuser",
  "auth_user"."username",
  "auth_user"."first_name",
  "auth_user"."last_name",
  "auth_user"."email",
  "auth_user"."is_staff",
  "auth_user"."is_active",
  "auth_user"."date_joined"
FROM
  "userprofile_userprofile"
  LEFT OUTER JOIN "exercise_submission_submitters" ON (
    "userprofile_userprofile"."id" = "exercise_submission_submitters"."userprofile_id"
  )
  LEFT OUTER JOIN "exercise_submission" ON (
    "exercise_submission_submitters"."submission_id" = "exercise_submission"."id"
  )
  INNER JOIN "auth_user" ON (
    "userprofile_userprofile"."user_id" = "auth_user"."id"
  )
WHERE
  "exercise_submission"."exercise_id" = 25
GROUP BY
  "userprofile_userprofile"."id",
  "auth_user"."id"
ORDER BY
  "earliest_submission" ASC

@EerikSaksi EerikSaksi force-pushed the earlier-submission-proper-handling branch 3 times, most recently from a645825 to a87567a Compare June 20, 2023 07:42
@ihalaij1 ihalaij1 force-pushed the earlier-submission-proper-handling branch 2 times, most recently from 6e4d3df to 9fe9551 Compare May 7, 2024 06:36
@ihalaij1 ihalaij1 force-pushed the earlier-submission-proper-handling branch from 9fe9551 to 83459cf Compare May 7, 2024 06:42
@ihalaij1 ihalaij1 assigned ihalaij1 and unassigned markkuriekkinen May 7, 2024
@ihalaij1 ihalaij1 merged commit 59606cf into apluslms:master May 7, 2024
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

The next submitter link for manual assessment
3 participants