search

github-education-resources / classroom

GitHub Classroom automates repository creation and access control, making it easy for teachers to distribute starter code and collect assignments on GitHub.
https://classroom.github.com
1.34k stars 567 forks source link

Improved Grading and Commenting Platform via PRs #1825

Open DavidGriswoldTeacher opened 5 years ago

DavidGriswoldTeacher commented 5 years ago

Feature request :sparkles:

The Problem

Right now, the described best practice for commenting and reviewing student submissions is on a commit by commit basis.

However, if students make several commits over he course of the assignment (a good practice!) then it can be hard to find individual changes in the commit view to comment on them.

The solution

@alexch Proposes a workflow at the education forum that uses Pull Requests to provide a Platform to provide comments on ALL changes made by a student, no matter how many commits covered by the changes. It consists of three major steps:

  1. Create a branch called "template" from "master" at the time of assignment creation or just before.
  2. Add one small change to the "master" branch so that the branches are not identical. (I add a date line to the readme at the bottom)
  3. Create a pull request from Template <-- Master with a title like "PR used for teacher comments, do not merge."

As students make commits to master, their changes will appear in the PR, and comments can be added by the teachers and students over time or after submission. This is a much better interface for communication than the commit based comment threads.

The Requested Feature

GitHub Classroom should automate this process. This would require four changes.

DavidGriswoldTeacher commented 5 years ago

I will work on this feature IF there is any confirmation that it might someday be accepted into the repo. I would like to improve my ruby on rails skills anyway, now that I have a working dev environment on my windows machine thanks to #1818

If it will not be accepted I will instead work on a script to do it for myself.

Siddharth6 commented 5 years ago

I will work on this feature IF there is any confirmation that it might someday be accepted into the repo. thankyou.

faloi commented 5 years ago

Hi @DavidGriswoldTeacher!

I wrote a small CLI app on Ruby that does something very similar to what is being discussed here, but designed to be executed after an assignment deadline. Basically, it creates two branches: one pointing to the "template" commit and another one pointing to the last commit of the student.

Doc and part of the code is in Spanish, but you can reach me if you are interested on using/contributing to it. The repo is https://github.com/faloi/yanapiri.

DavidGriswoldTeacher commented 5 years ago

Cool! I will look at it. How does it keep track of what commit was the template commit? Do you just pass it in the original template repository? Or search backward for the last commit by a specific user (the teacher)? Does it break if you make changes to the template after some students have created assignments?

I played with this for a few hours but didn't really get anywhere. I finally figured out that I need to have classroom wait until the source code import is complete before it can do make he branches and so forth, but that is done asynchronously and I'm not sure the best path forward there. Still working it out, but I think I'm going to need to leverage webhooks to do that. Alternatively, I could do something kind of like your app but built in that simply stores the final template commit ID/hash in the database for each assignment repo, then adds a button/link to the assignment view screen to create the comment PR(s), if they do not yet exist. When a teacher is ready to grade they could click that link, wait a minute, then be ready to go.

On Sat, May 11, 2019, 19:25 Federico Aloi notifications@github.com wrote:

Hi @DavidGriswoldTeacher https://github.com/DavidGriswoldTeacher!

I wrote a small CLI app on Ruby that does something very similar to what is being discussed here, but designed to be executed after an assignment deadline. Basically, it creates two branches: one pointing to the "template" commit and another one pointing to the last commit of the student.

Doc and part of the code is in Spanish, but you can reach me if you are interested on using/contributing to it. The repo is https://github.com/faloi/yanapiri.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/education/classroom/issues/1825#issuecomment-491548386, or mute the thread https://github.com/notifications/unsubscribe-auth/ALWSINVM2DO7O4BATCNTMYTPU5BVJANCNFSM4HMH43BQ .

faloi commented 5 years ago

You have to manually provide the SHA of the template commit. I thought about automatically retrieving it from the template repository but it's not that easy: we are a lot of teachers using the same exercises, so the template repo could change at any moment. Perhaps I will work on that in a near future.

DavidGriswoldTeacher commented 5 years ago

No, I think your system is fine. The advantage of rolling this feature into classroom itself is we could simply add the ID to the database at assignment copying, so it's easy to keep track of without any fragile assumptions.

The more I think about it, the more I like the idea of making the comment PR process be user initiated at grading time (or any time after copying) so I don't need to worry about webhooks etc. Plus then they don't have to do it if they don't want to. And if the student hasn't committed anything, then that could be noted on the assignment screen.

Edge case: if an assignment has no starter code there is nothing to PR against, but it would still be nice to be able to do comments via the same interface. Should classroom commit a default Readme in that case to create the PR against, perhaps with the same information as appears on the default empty repository screen?

On Mon, May 13, 2019, 08:38 Federico Aloi notifications@github.com wrote:

You have to manually provide the SHA of the template commit. I thought about automatically retrieving it from the template repository but it's not that easy: we are a lot of teachers using the same exercises, so the template repo could change at any moment. Perhaps I will work on that in a near future.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/education/classroom/issues/1825?email_source=notifications&email_token=ALWSINURZEVXRFI3UYXFJ7LPVFHJRA5CNFSM4HMH43B2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVIBHTA#issuecomment-491787212, or mute the thread https://github.com/notifications/unsubscribe-auth/ALWSINRR3LXLFG4BD47LDL3PVFHJRANCNFSM4HMH43BQ .

faloi commented 5 years ago

The advantage of rolling this feature into classroom itself is we could simply add the ID to the database at assignment copying, so it's easy to keep track of without any fragile assumptions.

There are a lot of advantages of including this into Classroom. I did it by myself because I needed it quickly.

The more I think about it, the more I like the idea of making the comment PR process be user initiated at grading time (or any time after copying) so I don't need to worry about webhooks etc.

Sounds good. Perhaps it could be weird for unexperienced students to have "something" (the PR) that they shouldn't touch, but it can be explained in class.

And if the student hasn't committed anything, then that could be noted on the assignment screen.

I chose to create an issue in that case, because the PR would be empty - not a problem if a minor change is introduced in the README, as you suggested.

Should classroom commit a default Readme in that case to create the PR against, perhaps with the same information as appears on the default empty repository screen?

Sounds good. Something like Repository created by GitHub Classroom. could do the trick.

d12 commented 5 years ago

:wave: @DavidGriswoldTeacher

Thanks for your feature suggestion and offer to help out! Open source contributions are always welcome, but thanks for checking in with us re: will this get merged.

I think the idea is great, we've discussed something similar before. Your implementation makes sense to me, let me know if you get stuck at all along the way. I'd be glad to help :)