diff --git a/compair/api/assignment_attachment.py b/compair/api/assignment_attachment.py index 51bc209d3..80b0e791b 100644 --- a/compair/api/assignment_attachment.py +++ b/compair/api/assignment_attachment.py @@ -23,9 +23,13 @@ # differs completely from that used by file.py, I've had to split it out. -# given an assignment, download all attachments in that assignment. -# /api/courses//assignments//attachments/download -class DownloadAllAttachmentsAPI(Resource): +# given an assignment, download all student attachments in that assignment. +# This is restricted to only student answers to match the behaviour of the +# "Participation" tab in the UI, where it only lists students. +# /api/courses//assignments//attachments/download_students +class DownloadAllStudentAttachmentsAPI(Resource): + DELIM = ' - ' + @login_required def get(self, course_uuid, assignment_uuid): # course unused, but we need to call it to check if it's a valid course @@ -38,13 +42,21 @@ def get(self, course_uuid, assignment_uuid): message="Sorry, your system role does not allow downloading all attachments") # grab answers so we can see how many has files - answers = self.getAnswersByAssignment(assignment) + answers = self.getStudentAnswersByAssignment(assignment) fileIds = [] + fileAuthors = {} for answer in answers: if not answer.file_id: continue # answer has an attachment fileIds.append(answer.file_id) + # the user who uploaded the file can be different from the answer + # author (e.g. instructor can upload on behalf of student), so + # we need to use the answer author instead of file uploader + author = answer.user_fullname + if answer.user_student_number: + author += self.DELIM + answer.user_student_number + fileAuthors[answer.file_id] = author if not fileIds: return {'msg': 'Assignment has no attachments'} @@ -64,13 +76,9 @@ def get(self, course_uuid, assignment_uuid): current_app.config['ATTACHMENT_UPLOAD_FOLDER'], srcFile.name ) - # set filename to 'full name - student number - uuid.ext' - # omit student number or extension if not exist - delim = ' - ' - srcFileName = srcFile.user.fullname - if srcFile.user.student_number: - srcFileName += delim + srcFile.user.student_number - srcFileName += delim + srcFile.name + # filename should be 'full name - student number - uuid.ext' + # student number is omitted if user doesn't have one + srcFileName = fileAuthors[srcFile.id] + self.DELIM + srcFile.name #current_app.logger.debug("writing " + srcFileName) zipFile.write(srcFilePath, srcFileName) #current_app.logger.debug("Writing zip file") @@ -79,7 +87,7 @@ def get(self, course_uuid, assignment_uuid): # this really should be abstracted out into the Answer model, but I wasn't # able to get the join with UserCourse to work inside the Answer model. - def getAnswersByAssignment(self, assignment): + def getStudentAnswersByAssignment(self, assignment): return Answer.query \ .outerjoin(UserCourse, and_( Answer.user_id == UserCourse.user_id, @@ -91,7 +99,10 @@ def getAnswersByAssignment(self, assignment): Answer.practice == False, Answer.draft == False, or_( - and_(UserCourse.course_role != CourseRole.dropped, Answer.user_id != None), + and_( + UserCourse.course_role == CourseRole.student, + Answer.user_id != None + ), Answer.group_id != None ) )) \ @@ -102,4 +113,4 @@ def getFilesByIds(self, fileIds): filter(File.id.in_(fileIds)).all() -api.add_resource(DownloadAllAttachmentsAPI, '/download') +api.add_resource(DownloadAllStudentAttachmentsAPI, '/download_students') diff --git a/compair/static/modules/gradebook/gradebook-module.js b/compair/static/modules/gradebook/gradebook-module.js index 7dcdf2813..e1fd9d1ea 100644 --- a/compair/static/modules/gradebook/gradebook-module.js +++ b/compair/static/modules/gradebook/gradebook-module.js @@ -31,7 +31,7 @@ module.factory( ['$resource', function($resource) { - var ret = $resource('/api/courses/:courseId/assignments/:assignmentId/attachments/download'); + var ret = $resource('/api/courses/:courseId/assignments/:assignmentId/attachments/download_students'); return ret; } ]); diff --git a/compair/tests/api/test_assignment_attachment.py b/compair/tests/api/test_assignment_attachment.py index 3c9d117a8..230bc25ea 100644 --- a/compair/tests/api/test_assignment_attachment.py +++ b/compair/tests/api/test_assignment_attachment.py @@ -37,14 +37,24 @@ def _getUrl(self, courseUuid=None, assignmentUuid=None): if assignmentUuid is None: assignmentUuid = self.fixtures.assignment.uuid return "/api/courses/" + courseUuid + "/assignments/" + assignmentUuid \ - + "/attachments/download" + + "/attachments/download_students" # we need to create actual files - def _createAttachmentsInAssignment(self, assignment): + # isFileUploadedByInstructor simulates when instructors submit answers on + # behalf of students, the file would be owned by the instructor instead of + # the student + def _createAttachmentsInAssignment( + self, + assignment, + isFileUploadedByInstructor=False + ): uploadDir = current_app.config['ATTACHMENT_UPLOAD_FOLDER'] for answer in assignment.answers: - attachFile = self.fixtures.add_file(answer.user) + uploader = answer.user + if isFileUploadedByInstructor: + uploader = self.fixtures.instructor + attachFile = self.fixtures.add_file(uploader) attachFilename = attachFile.uuid + '.png' attachFile.name = attachFilename attachFile.uuid = attachFile.uuid @@ -58,35 +68,7 @@ def _createAttachmentsInAssignment(self, assignment): db.session.add(answer) db.session.commit() - def test_download_all_attachments_block_unauthorized_users(self): - # test login required - rv = self.client.get(self._getUrl()) - self.assert401(rv) - - # test unauthorized user - with self.login(self.fixtures.unauthorized_instructor.username): - rv = self.client.get(self._getUrl()) - self.assert403(rv) - - def test_download_all_attachments_require_valid_course_and_assignment(self): - with self.login(self.fixtures.instructor.username): - # invalid course - url = self._getUrl("invalidUuid") - rv = self.client.get(url) - self.assert404(rv) - # invalid assignment - url = self._getUrl(None, "invalidUuid") - rv = self.client.get(url) - self.assert404(rv) - - def test_download_all_attachments_return_msg_if_no_attachments(self): - with self.login(self.fixtures.instructor.username): - rv = self.client.get(self._getUrl()) - self.assert200(rv) - self.assertEqual('Assignment has no attachments', rv.json['msg']) - - def test_download_all_attachments(self): - self._createAttachmentsInAssignment(self.fixtures.assignment) + def _downloadAndCheckFiles(self): with self.login(self.fixtures.instructor.username): rv = self.client.get(self._getUrl()) self.assert200(rv) @@ -110,8 +92,8 @@ def test_download_all_attachments(self): # we don't include inactive, draft, or practice answers if not answer.active or answer.draft or answer.practice: continue - # we don't include dropped students - if answer.user in self.fixtures.dropped_students: + # we only want current active students + if answer.user not in self.fixtures.students: continue expectedAttachment = '{} - {} - {}'.format( answer.user.fullname, @@ -120,3 +102,38 @@ def test_download_all_attachments(self): ) self.assertTrue(expectedAttachment in actualAttachments) + def test_download_all_attachments_block_unauthorized_users(self): + # test login required + rv = self.client.get(self._getUrl()) + self.assert401(rv) + + # test unauthorized user + with self.login(self.fixtures.unauthorized_instructor.username): + rv = self.client.get(self._getUrl()) + self.assert403(rv) + + def test_download_all_attachments_require_valid_course_and_assignment(self): + with self.login(self.fixtures.instructor.username): + # invalid course + url = self._getUrl("invalidUuid") + rv = self.client.get(url) + self.assert404(rv) + # invalid assignment + url = self._getUrl(None, "invalidUuid") + rv = self.client.get(url) + self.assert404(rv) + + def test_download_all_attachments_return_msg_if_no_attachments(self): + with self.login(self.fixtures.instructor.username): + rv = self.client.get(self._getUrl()) + self.assert200(rv) + self.assertEqual('Assignment has no attachments', rv.json['msg']) + + def test_download_all_attachments(self): + self._createAttachmentsInAssignment(self.fixtures.assignment) + self._downloadAndCheckFiles() + + def test_files_named_for_answer_author_not_uploader(self): + self._createAttachmentsInAssignment(self.fixtures.assignment, True) + self._downloadAndCheckFiles() +