EDIT dl all only students, name as answer author

Changes to the  "Download All" attachments button in the "Participation"
tab of an assignment.

Instructor submitted answers were being included as part of this.
However, it seems that the "Participation" tab only lists student
answers. It's probably better to be consistent with the "Participation"
tab and also only include student answer attachments. As such, I've
modified the API so that it's downloading student answer attachments
only.

I didn't know that instructors can submit answers on a student's behalf.
This caused an interesting issue where the file is linked to the
instructor but the answer is linked to the student. Since the file name
comes from the file user, the file gets the instructor's name.  This is
different from the UI, where the answer user is used. It makes more
sense to use the answer user like the UI, so I've switched it to use the
answer user in the filename.

Tests were updated. Interestingly, the tests actually assumes usage of
the answer user, so we didn't have to do much there.

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/<course>/assignments/<assignment>/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/<course>/assignments/<assignment>/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')

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;
     }
 ]);

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()
+