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

Fix order of annotations on feedback view and file view to be consistent #1590

Merged
merged 5 commits into from
Sep 24, 2022

Conversation

20wildmanj
Copy link
Contributor

Description

Previously, the annotations on the "viewFeedback" page were sorted by creation date of the annotation, while the annotations on the file view were sorted by line number, which meant order wasn't maintained among the two views.

Previous feedback and file view:
Screen Shot 2022-09-17 at 15 39 09
Screen Shot 2022-09-17 at 15 38 45

I added sorting to both viewFeedback.html.erb and _annotation_pane.html.erb to first sort by the filename the annotation was added to, and then to sort by the line number. The lambda function get_correct_filename was added to assessments_controller.rb since in multiple instances, we need to get the proper filename / change the filename if it's the autograded output or it's an archived file, in viewFeedback.html.erb. submissions_controller.rb also received similar code to append the proper filename to problemSummaries.

Revised annotation ordering for feedback and file view:
Screen Shot 2022-09-17 at 15 38 18
Screen Shot 2022-09-17 at 15 38 30

Motivation and Context

This issue was brought up by the 122 team, as they saw that "the order of annotations in the student feedback when they click on the score is not the same as the order when you look at the file page." There was also a TODO in viewFeedback.html.erb to make order of annotations consistent with the order of the file list.

How Has This Been Tested?

  • create / use an assessment that has multiple files (an autograded lab with additional problems is preferred)
  • as a student account, submit a solution to the assessment, make sure it gets properly autograded
  • as instructor, go to "grade submission" and click on the student submission,
  • add multiple annotations to multiple files in the student submission, and try adding some annotations in a random order with regards to line number (for example, add one annotation to the bottom of autograder output, and then add one annotation to the middle of the main submission file, then another at the bottom of the main submission file)
  • make sure to add feedback for the problem in "viewGradesheet" for the student
  • as a student, go to the assessment, and under the handin with the problem with annotations click on the score like so:
    • Screen Shot 2022-09-17 at 16 08 56
  • Verify that the order of the annotations is by file name and then by line number
  • Go to the file view, verify that the order of the annotations is the same

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting

Copy link
Contributor

@victorhuangwq victorhuangwq left a comment

Choose a reason for hiding this comment

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

Tested on nightly and works as expected. LGTM.
Left a comment on one line of the code.

Archive.get_nth_filename(@files, annotation.position)
else
@submission.filename
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious as to why do we need a duplication of this code in this controller as well?

Copy link
Contributor

@victorhuangwq victorhuangwq left a comment

Choose a reason for hiding this comment

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

LGTM!

@20wildmanj 20wildmanj merged commit 28edb0f into master Sep 24, 2022
@20wildmanj 20wildmanj deleted the joeywildman-fixannotationorder branch September 24, 2022 17:37
@damianhxy
Copy link
Member

Screen Shot 2022-09-28 at 17 26 43

There seems to be an issue when annotating archives -- but I'm not sure if this was existing behavior

@20wildmanj
Copy link
Contributor Author

Screen Shot 2022-09-28 at 17 26 43

There seems to be an issue when annotating archives -- but I'm not sure if this was existing behavior

Looks like this PR caused this, will make a hotfix, thanks for finding the bug. @michellexliu for visibility since you're requesting computing services to merge this PR into deployment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants