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

Streaming output frontend #1622

Merged
merged 19 commits into from
Nov 29, 2022
Merged

Streaming output frontend #1622

merged 19 commits into from
Nov 29, 2022

Conversation

michellexliu
Copy link
Contributor

@michellexliu michellexliu commented Oct 13, 2022

Description

Updates the feedback interface to show more details about a submission:

  • Shows the score breakdown floating on the side
  • Shows the autograder status (right now is hard-coded as "Completed" since we only show feedback when the autograder is finished running, will eventually have states for "Queued"/"In-Progress")
  • Only show "Graded by" if it was graded by someone

Problem with autograder score:
image

Problem without autograder score:
image

Scrolled to bottom of page:
image

Side panel with lots of problem categories:
image

Motivation and Context

  • Clean up visuals of interface
  • Allow students to see autograder score breakdown in an easier way
  • Set up interface to handle newly introduced feedback states for partial output

How Has This Been Tested?

  • Hello lab autograded feedback
  • Twosum lab autograded feedback
  • Broken feedback (last line of feedback is not formatted in the proper {"scores": {...}} format) -> displays output, but doesn't have a score breakdown
  • Manual feedback
  • Long autograder output
  • Released remarks --> remarks load at bottom of page
  • Unreleased remarks --> remarks section doesn't appear
  • Semantic Feedback --> loads semantic feedback page instead of regular feedback page
  • Side panel with lots of problem categories

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have run rubocop for style check. If you haven't, run overcommit --install && overcommit --sign to use pre-commit hook for linting
  • My change requires a change to the documentation, which is located at Autolab Docs
  • I have updated the documentation accordingly, included in this PR

@michellexliu michellexliu changed the title (wip) Streaming output frontend Streaming output frontend Oct 13, 2022
@michellexliu michellexliu marked this pull request as ready for review October 13, 2022 19:37
@victorhuangwq victorhuangwq marked this pull request as draft October 23, 2022 18:58
@victorhuangwq victorhuangwq marked this pull request as ready for review November 15, 2022 00:37
@damianhxy
Copy link
Member

Looks like the changes here would conflict with #1643 -- I modified the Graded By to include Autograder and Error Handler. If you prefer to hide the Graded By, I can remove the changes from my PR.

@victorhuangwq
Copy link
Contributor

Seeing the bigger "Correctness" at the top really tripped me up.

image

I understand it's leftover from
image

But since we are already showing all the autograded scores, do we need to repeat that?

@victorhuangwq
Copy link
Contributor

Visually, the remarks and all makes sense.

@victorhuangwq
Copy link
Contributor

My Remarks does not have a rounded border like the one you have in your screenshot

image

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 locally and it works. Left some comments, but I will approve it because it's functionally OK.

app/assets/javascripts/feedback.js Show resolved Hide resolved
@@ -575,7 +575,7 @@ def viewFeedback
redirect_to(action: "index") && return
end
@jsonFeedback = parseFeedback(@score.feedback)
@scoreHash = parseScore(@score.feedback) unless @jsonFeedback.nil?
Copy link
Contributor

Choose a reason for hiding this comment

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

may. I know why is the nil check removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with it there, it wasn't possible to get any of the score info from parseScore unless a json config file was specified for semantic; there's only one small part in parseScore that relies on having @jsonFeedback set, so I just added a nil check there instead so the rest of the function could run

@@ -594,7 +594,7 @@ def parseScore(feedback)

score_hash = JSON.parse(feedback)
score_hash = score_hash["scores"]
if @jsonFeedback.key?("_scores_order") == false
if @jsonFeedback && @jsonFeedback.key?("_scores_order") == false
Copy link
Contributor

Choose a reason for hiding this comment

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

NIT: You could perhaps use ruby's safe navigator:
https://mitrev.net/ruby/2015/11/13/the-operator-in-ruby/

Copy link
Member

Choose a reason for hiding this comment

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

Note: if @jsonFeedback is nil then @jsonFeedback&.key?("_scores_order") would be nil, which is not the same as false

Copy link
Contributor

Choose a reason for hiding this comment

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

Currently it also relies on the behavior of nil ANDed, which is also not very ideal.

@@ -868,6 +868,7 @@ def valid_asmt_tar(tar_extract)
tar_extract.each do |entry|
pathname = entry.full_name
next if pathname.start_with? "."
next if pathname.start_with? "PaxHeader"
Copy link
Contributor

Choose a reason for hiding this comment

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

what does PaxHeaders stand for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a file that gets added on mac... none of the autograders I tar'ed directly on mac would run without it, so I added it in for now to run, but I think we could look for a better way of handling something like this in the future

Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a comment on it? so that next time people who look at this code would know

app/views/assessments/viewFeedback.html.erb Outdated Show resolved Hide resolved
@michellexliu
Copy link
Contributor Author

michellexliu commented Nov 28, 2022

Seeing the bigger "Correctness" at the top really tripped me up.

image

I understand it's leftover from image

But since we are already showing all the autograded scores, do we need to repeat that?

hmmm yeah I at one point did consider just removing it, but I decided to leave it in because for non-autograded problems, there's no other way to see the score for that problem on the page. Would you suggest just hiding it if it's autograded and showing it otherwise? Or updating the styling to make it less prominent? @victorhuangwq

@victorhuangwq
Copy link
Contributor

I'm wondering if we could hide it for autograded problems, and show it for non-autograded problems

@michellexliu
Copy link
Contributor Author

I'm wondering if we could hide it for autograded problems, and show it for non-autograded problems

Done! @victorhuangwq

@michellexliu michellexliu merged commit 486e0ae into master Nov 29, 2022
@michellexliu michellexliu deleted the streaming-output-frontend branch November 29, 2022 13:36
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.

3 participants