search

eecs-autograder / autograder-server

GNU Lesser General Public License v3.0
8 stars 7 forks source link

Feature: Showing Only Applied Parts Of Handgrading Rubric #667

Closed developStorm closed 2 years ago

developStorm commented 2 years ago

Resolves https://github.com/eecs-autograder/autograder.io/issues/40

developStorm commented 2 years ago

This is a PoC for this feature. I wanted to check if this looks good before moving forward with implementing the instructor toggle option for this.

image

I only applied "visible" annotation/checkbox, and this is how it looks like on student's end

image
james-perretta commented 2 years ago

Ah, the reason I mentioned performance in the issue description was because I was conflating this with things that have to be computed for all groups' final submissions at once, but obviously it only happens to one at a time, so performance isn't an issue.

What you have looks good. There is still the consideration of whether it makes sense to hide checkboxes that give positive points--I think that warrants a discussion but wouldn't majorly change the implementation.

Thanks for the PR!

On Wednesday, October 19, 2022, Raymond Nook @.***> wrote:

This is a PoC for this feature. I wanted to check if this looks good before moving forward with implementing the instructor toggle option for this.

[image: image] https://user-images.githubusercontent.com/59678453/196818573-78ca4db0-d925-4956-9671-c45bc84539e2.png

[image: image] https://user-images.githubusercontent.com/59678453/196818507-36d4ea79-5586-49ca-8df5-6e024a02575b.png

— Reply to this email directly, view it on GitHub https://github.com/eecs-autograder/autograder-server/pull/667#issuecomment-1284650615, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA7IXMEAWL23SV7KWODNJA3WEB3EJANCNFSM6AAAAAARJSJCJ4 . You are receiving this because you are subscribed to this thread.Message ID: @.***>

developStorm commented 2 years ago

Thanks for clarifying! I reviewed Gradescope's practice and looks like they are not hiding checkboxes:

image

However I'm pretty sure what @ekaprits wants is to hide every unapplied rubric from student, including those with positive points ; ) so I'll probably just do that first

Or making this behavior optional is also possible. I can see if that can be done without adding too much complexity.

developStorm commented 2 years ago

Alright! I've implemented both frontend and backend part for this feature. It involves this PR and these two PRs:

https://github.com/eecs-autograder/ag-website-vue/pull/500 https://github.com/eecs-autograder/ag-client-typescript/pull/147

I've roughly tested it locally and it works without problems. Let me know what you think 😺

james-perretta commented 2 years ago

@developStorm I don't suppose you could add a few test cases for this?

james-perretta commented 2 years ago

And following up on the checkbox hiding, I'm hesitant to have the default behavior be hiding unselected checkboxes regardless of the points strategy selected. Can you double check that that's truly the behavior that you need, rather than that only being the case for "start at max and subtract"?

developStorm commented 2 years ago

@james-perretta Thanks for the review, and tests are ready now!

I double-confirmed with the professor, he intends to use this feature to minimize the rubric leakage (i.e. to prevent students share rubrics with other students from subsequent semesters). From this perspective, he believes hiding all unapplied rubrics is necessary. This is different from Gradescope's design - which is more about preventing students from the misconception that they are being docked points. I added a tooltip in settings page to emphasis the effect of this feature.

james-perretta commented 2 years ago

@developStorm Ok thanks, we can go with this for the time being. I'll update the issue with a note to expand on the implementation to allow for fine-grained control of which annotations and checkboxes to hide.

I see a couple of minor linter errors (I have yet to finish migrating my CI to github actions since Travis CI changed their free tier, sorry about that). In the interest of time so that you can release grades ASAP, I'm going to make a local copy of your branch, make the fixes and run the tests, and then make a new PR to merge.

Edit: I'm going to merge this as-is as soon as the tests finish running and let future me fix the pycodestyle warnings.

james-perretta commented 2 years ago

Tests passing locally, merging with minor pycodestyle warnings.

developStorm commented 2 years ago

Thanks @james-perretta! I didn't notice there's a linter in-place so I formatted my code with black's default rules. Maybe we should consider adding linter info to the local env setup guide?

james-perretta commented 2 years ago

@james-perretta Yes, you're right, sorry about that! I can add that along with getting CI up and running again...