Copy integrity

nbgrader/exchange/default/exchange.py

@@ -97,31 +97,36 @@ def do_copy(self, src, dest, log=None):
             self.log.error("Directory size is too big")
             raise RuntimeError(f"Directory size is too big. Size is {dir_size}, maximum size is {1000 * max_dir_size}")
 
-        shutil.copytree(src, dest,
-                        ignore=ignore_patterns(exclude=self.coursedir.ignore,
-                                               include=self.coursedir.include,
-                                               max_file_size=self.coursedir.max_file_size,
-                                               log=self.log))
-        # copytree copies access mode too - so we must add go+rw back to it if
-        # we are in groupshared.
-        if self.coursedir.groupshared:
-            for dirname, _, filenames in os.walk(dest):
-                # dirs become ug+rwx
-                st_mode = os.stat(dirname).st_mode
-                if st_mode & 0o2770 != 0o2770:
-                    try:
-                        os.chmod(dirname, (st_mode|0o2770) & 0o2777)
-                    except PermissionError:
-                        self.log.warning("Could not update permissions of %s to make it groupshared", dirname)
-
-                for filename in filenames:
-                    filename = os.path.join(dirname, filename)
-                    st_mode = os.stat(filename).st_mode
-                    if st_mode & 0o660 != 0o660:
+        try:
+            shutil.copytree(src, dest,
+                            ignore=ignore_patterns(exclude=self.coursedir.ignore,
+                                                   include=self.coursedir.include,
+                                                   max_file_size=self.coursedir.max_file_size,
+                                                   log=self.log))
+        except OSError as err:
+            raise err
+        # Set permissions for copied files, even if some failed to copy
+        finally:
+            # copytree copies access mode too - so we must add go+rw back to it if
+            # we are in groupshared.
+            if self.coursedir.groupshared:
+                for dirname, _, filenames in os.walk(dest):
+                    # dirs become ug+rwx
+                    st_mode = os.stat(dirname).st_mode
+                    if st_mode & 0o2770 != 0o2770:
                         try:
-                            os.chmod(filename, (st_mode|0o660) & 0o777)
+                            os.chmod(dirname, (st_mode|0o2770) & 0o2777)
                         except PermissionError:
-                            self.log.warning("Could not update permissions of %s to make it groupshared", filename)
+                            self.log.warning("Could not update permissions of %s to make it groupshared", dirname)
+
+                    for filename in filenames:
+                        filename = os.path.join(dirname, filename)
+                        st_mode = os.stat(filename).st_mode
+                        if st_mode & 0o660 != 0o660:
+                            try:
+                                os.chmod(filename, (st_mode|0o660) & 0o777)
+                            except PermissionError:
+                                self.log.warning("Could not update permissions of %s to make it groupshared", filename)
 
     def start(self):
         if sys.platform == 'win32':

nbgrader/exchange/default/submit.py

@@ -131,15 +131,23 @@ def copy_files(self):
         self.log.info("Source: {}".format(self.src_path))
         self.log.info("Destination: {}".format(dest_path))
 
+        errors = []
+
         # copy to the real location
         self.check_filename_diff()
-        self.do_copy(self.src_path, dest_path)
-        with open(os.path.join(dest_path, "timestamp.txt"), "w") as fh:
-            fh.write(self.timestamp)
-        self.set_perms(
-            dest_path,
-            fileperms=(S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH),
-            dirperms=(S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH))
+        try:
+            self.do_copy(self.src_path, dest_path)
+        except OSError as err:
+            errors.append(err)
+
+        # Create timestamp even if copying is incomplete to not break later functions
+        if os.path.isdir(dest_path):
+            with open(os.path.join(dest_path, "timestamp.txt"), "w") as fh:
+                fh.write(self.timestamp)
+            self.set_perms(
+                dest_path,
+                fileperms=(S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH),
+                dirperms=(S_IRUSR | S_IWUSR | S_IXUSR | S_IRGRP | S_IXGRP | S_IROTH | S_IXOTH))
 
         # Make this 0777=ugo=rwx so the instructor can delete later. Hidden from other users by the timestamp.
         os.chmod(
@@ -150,10 +158,16 @@ def copy_files(self):
         # also copy to the cache
         if not os.path.isdir(self.cache_path):
             os.makedirs(self.cache_path)
-        self.do_copy(self.src_path, cache_path)
+        try:
+            self.do_copy(self.src_path, cache_path)
+        except OSError as err:
+            errors.append(err)  # probably duplicates from above
         with open(os.path.join(cache_path, "timestamp.txt"), "w") as fh:
             fh.write(self.timestamp)
 
         self.log.info("Submitted as: {} {} {}".format(
             self.coursedir.course_id, self.coursedir.assignment_id, str(self.timestamp)
         ))
+
+        if errors:
+            raise OSError(errors)

nbgrader/nbextensions/assignment_list/assignment_list.js

@@ -381,6 +381,25 @@ define([
         return container;
     };
 
+    Assignment.prototype.fetch_error = function (data) {
+        var body = $('<div/>').attr('id', 'fetch-message');
+
+        body.append(
+            $('<div/>').append(
+                $('<p/>').text('Assignment not fetched:')
+            )
+        );
+        body.append(
+            $('<pre/>').text(data.value)
+        );
+
+        dialog.modal({
+            title: "Fetch failed",
+            body: body,
+            buttons: { OK: { class : "btn-primary" } }
+        });
+    };
+
     Assignment.prototype.submit_error = function (data) {
         var body = $('<div/>').attr('id', 'submission-message');
 
@@ -400,6 +419,25 @@ define([
         });
     };
 
+    Assignment.prototype.fetchfeedback_error = function (data) {
+        var body = $('<div/>').attr('id', 'fetchfeedback-message');
+
+        body.append(
+            $('<div/>').append(
+                $('<p/>').text('Feedback not fetched:')
+            )
+        );
+        body.append(
+            $('<pre/>').text(data.value)
+        );
+
+        dialog.modal({
+            title: "Fetch Feedback failed",
+            body: body,
+            buttons: { OK: { class : "btn-primary" } }
+        });
+    };
+
     Assignment.prototype.make_button = function () {
         var that = this;
         var container = $('<span/>').addClass('item_status col-sm-4');
@@ -417,7 +455,15 @@ define([
                     },
                     type : "POST",
                     dataType : "json",
-                    success : $.proxy(that.on_refresh, that),
+                    success : function (data, status, xhr) {
+                        if (!data.success) {
+                            that.fetch_error(data);
+                            button.text('Fetch');
+                            button.removeAttr('disabled');
+                        } else {
+                            that.on_refresh(data, status, xhr);
+                        }
+                    },
                     error : function (xhr, status, error) {
                         container.empty().text("Error fetching assignment.");
                         utils.log_ajax_error(xhr, status, error);
@@ -479,7 +525,15 @@ define([
                     },
                     type : "POST",
                     dataType : "json",
-                    success : $.proxy(that.on_refresh, that),
+                    success : function (data, status, xhr) {
+                        if (!data.success) {
+                            that.fetchfeedback_error(data);
+                            button.text('Fetch Feedback');
+                            button.removeAttr('disabled');
+                        } else {
+                            that.on_refresh(data, status, xhr);
+                        }
+                    },
                     error : function (xhr, status, error) {
                         container.empty().text("Error fetching feedback.");
                         utils.log_ajax_error(xhr, status, error);

nbgrader/server_extensions/assignment_list/handlers.py

@@ -313,8 +313,11 @@ def post(self, action):
         if action == 'fetch':
             assignment_id = data['assignment_id']
             course_id = data['course_id']
-            self.manager.fetch_assignment(course_id, assignment_id)
-            self.finish(json.dumps(self.manager.list_assignments(course_id=course_id)))
+            output = self.manager.fetch_assignment(course_id, assignment_id)
+            if output["success"]:
+                self.finish(json.dumps(self.manager.list_assignments(course_id=course_id)))
+            else:
+                self.finish(json.dumps(output))
         elif action == 'submit':
             assignment_id = data['assignment_id']
             course_id = data['course_id']
@@ -326,8 +329,11 @@ def post(self, action):
         elif action == 'fetch_feedback':
             assignment_id = data['assignment_id']
             course_id = data['course_id']
-            self.manager.fetch_feedback(course_id, assignment_id)
-            self.finish(json.dumps(self.manager.list_assignments(course_id=course_id)))
+            output = self.manager.fetch_feedback(course_id, assignment_id)
+            if output["success"]:
+                self.finish(json.dumps(self.manager.list_assignments(course_id=course_id)))
+            else:
+                self.finish(json.dumps(output))
 
 
 class CourseListHandler(BaseAssignmentHandler):

nbgrader/tests/apps/test_nbgrader_submit.py

@@ -248,4 +248,16 @@ def test_submit_max_dir_size(self, exchange, cache, course_dir):
         self._make_file(join("ps1", "large_file"), contents="x" * 2001)
         with pytest.raises(RuntimeError):
             self._submit("ps1", exchange, cache,
-                        flags=['--CourseDirectory.max_dir_size=3'])
+                        flags=['--CourseDirectory.max_dir_size=3'])
+
+    def test_ensure_timestamp(self, exchange, cache, course_dir):
+        # Ensure timestamp is created even if something goes wrong in submitting
+        # If timestamp is not created, feedback workflow can be broken
+        # see: https://github.com/jupyter/nbgrader/pull/1755
+        self._release_and_fetch("ps1", exchange, cache, course_dir)
+        os.chmod("ps1/p1.ipynb", 0o244)
+        with pytest.raises(OSError):
+            self._submit("ps1", exchange, cache)
+        os.chmod("ps1/p1.ipynb", 0o644)
+        dirname = os.listdir(join(exchange, "abc101", "inbound"))[0]
+        assert os.path.isfile(join(exchange, "abc101", "inbound", dirname, "timestamp.txt"))