-
Notifications
You must be signed in to change notification settings - Fork 7
Comment section for code review #256
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #256 +/- ##
==========================================
+ Coverage 86.08% 86.51% +0.42%
==========================================
Files 19 19
Lines 891 949 +58
Branches 77 84 +7
==========================================
+ Hits 767 821 +54
- Misses 105 106 +1
- Partials 19 22 +3
Continue to review full report at Codecov.
|
interface/scoring.py
Outdated
| for comment in submission.comments.all(): | ||
| if ( | ||
| comment.user | ||
| in submission.assignment.course.teaching_assistants.all() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Could you save this variable before the outer for? and then do only
if comment.user in teaching_assistants:
| <div class="col-lg-2"></div> | ||
| <div class="col-lg-9"> | ||
| <pre> | ||
| /^--^\ /^--^\ /^--^\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. .
|\_---_/|
/ o_o \
| U |
\ ._I_. /
`-_____-'
| ) | ||
|
|
||
| review_message = "+10.0: Hacker" | ||
| response = client.post( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Could you also put a test where the student comments with something like? 🙏
+10.0: I really did a nice job!
interface/views.py
Outdated
| } | ||
|
|
||
| if ( | ||
| file.getvalue() != "The file is missing!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Could you save that value to don't having to call the same method twice?
like
file_value = file.getValue()
and then you can do
if file_value not in ["The file is missing!", "The archive is missing!"]:
Jokeswar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall 👍
interface/views.py
Outdated
|
|
||
| tree = tree_view(request, submission) | ||
|
|
||
| if filename == "code": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might introduce the issue of actually having a file named code in the archive. A solution could be having two paths in our urlpatterns:
"submission/<int:pk>/code/",
"submission/<int:pk>/code/<path:filename>/",
Those two should point to different views. One will handle the if case and the other the else case. It will also make it clearer where you are in your logic (empty page / page with code).
| comment = form.save(commit=False) | ||
| comment.submission = submission | ||
| comment.user = request.user | ||
| comment.path = filename | ||
| comment.line = request.POST.get("line", "") | ||
| comment.save() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you can add hidden inputs in your form so those can be filled in by form.save().
| all_tas = submission.assignment.course.teaching_assistants.all() | ||
|
|
||
| if request.user not in all_tas and request.user is not submission.user: | ||
| return HttpResponse(status=403) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Q: Could you also add a test for this? 👍
gmuraru
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Description
- This feature allows the ta to review and grade the homework directly on the code. They can leave multiple comments on every line (which will be highlighted) and change the score. The student can reply back.
Fixes #165
Testing
Checklist