Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #11921 +/- ##
============================================
- Coverage 21.67% 21.45% -0.23%
- Complexity 9491 9615 +124
============================================
Files 267 269 +2
Lines 36273 36995 +722
Branches 474 547 +73
============================================
+ Hits 7863 7937 +74
- Misses 27940 28515 +575
- Partials 470 543 +73
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Chriun
left a comment
There was a problem hiding this comment.
Looks good from what I tested. Code may need a refactor and additional features (such as viewing annotations from other graders) in the future.
As of now, though, image annotation and student viewing of the graded annotated images works. Others should most likely test as well.
jeffrey-cordero
left a comment
There was a problem hiding this comment.
Overall, the image annotation features look very robust. I could only find an issue with being unable to view/download annotations on the student-view, which I'll look into after a VM reset, but otherwise, the annotation components worked well. Having a native save button within the modal itself, and clicking on submissions_processed automatically opening up the modal would be nice, but these can be future enhancements.
lavalleeale
left a comment
There was a problem hiding this comment.
Added code comments but functionality works well for me
Co-authored-by: Alex Lavallee <alex@lavallee.one>
Co-authored-by: Alex Lavallee <alex@lavallee.one>
jeffrey-cordero
left a comment
There was a problem hiding this comment.
The work so far to implement image annotation looks solid, laying great groundwork for further optimizations and new features.
williamschen23
left a comment
There was a problem hiding this comment.
Code looks decent and functionality works as intended
|
|
||
| $annotation_path = FileUtils::joinPaths($this->core->getConfig()->getCoursePath(), 'annotations', $gradeable_id, $id, $active_version); | ||
| $annotation_jsons = []; | ||
| $md5_path = md5($path); |
There was a problem hiding this comment.
md5 probably provides enough entropy here given that the input data is not easily controlled by users, but we could consider a better hash function if desired.
| } | ||
| } | ||
| else { | ||
| $anon_path = $this->getPath($file_path, $gradeable_id, false); |
There was a problem hiding this comment.
getPath() doesn't do any form of path sanitization when the third argument is false. I believe that means that this code allows an arbitrary read if a carefully constructed $_POST['file_path'] argument is provided. It would be better to use FileUtils::joinPaths() in getPath() at a minimum, but it would be even better to just get rid of any sort of logic where we pass paths from the client side to the server side.
Even if these security vulnerabilities are fixed now, passing paths around unnecessarily opens the door to developers unintentionally creating new security vulnerabilities in the future.
There was a problem hiding this comment.
I'm trying to understand exactly how to prevent this pass from client side to server side. Should I just be passing a file name to the post request and building the rest of the filepath? Should I not be not passing full filepaths to the client from the viewi at all, and if so does that mean I should also just be just passing a filename there? My understanding of the filepath specifics isn't the greatest because I tried to use the same functions/logic as the previous pdf annotator.
There was a problem hiding this comment.
The previous PDF annotator is unfortunately something we've had security issues with in the past, so it's best to not rely on it too much. In general, I think passing the most minimal part needed to uniquely identify a file is the best way to approach that. Is that just the filename, or a part of the filename?
| $display_file_url = $this->core->buildCourseUrl(['display_file']); | ||
|
|
||
| $localcss = []; | ||
| $localcss[] = $this->core->getOutput()->timestampResource(FileUtils::joinPaths('image', 'image_annotation.css'), 'css'); |
There was a problem hiding this comment.
You could use addInternalCss() here instead.
There was a problem hiding this comment.
I couldn't figure out how to make addInternalCss work here, is there a performance reason for switching them out?
| $localjs = []; | ||
|
|
||
| if ($jquery) { | ||
| $localjs[] = $this->core->getOutput()->timestampResource(FileUtils::joinPaths('jquery', 'jquery.min.js'), 'vendor'); |
There was a problem hiding this comment.
You could use addVendorJs() here.
| import * as luxon from 'luxon'; | ||
| import * as Twig from 'twig'; | ||
| import * as markerjs3 from '@markerjs/markerjs3'; | ||
| import * as markerjsUI from '@markerjs/markerjs-ui'; |
There was a problem hiding this comment.
How big are these libraries?
There was a problem hiding this comment.
Unless I'm checking this wrong they should be 23 and 27 kBs respectively.
There was a problem hiding this comment.
Are these libraries loaded for every page, or only the pages where annotation is needed?
Why is this Change Important & Necessary?
PDF Annotation was recently removed, and so as a replacement we agreed to instead support image annotation.
What is the New Behavior?
What steps should a reviewer take to reproduce or test the bug or new feature?
Bulk upload to a gradable on the new branch, and assign the result to a student. Go to their submisson, and open up an image. This page should now show, and editing the image should render the changes. These changes should persist across instances. Downloading should download the image with the annotations built on top.
The student can now see their annotated image from the gradable at the bottom of the page, which is the same as with pdf annotation:
Automated Testing & Documentation
This feature will likely need submitty.org documentation.
Other information
A future PR/issue should store this annotated image somewhere without having to build to see the annotated image.