Integrate feedback distribution within nbgrader #1120

danielmaitre · 2019-06-01T06:53:24Z

This is the result of the work of @dsblank, @perllaghu, @billywardrop29 and @danielmaitre at the Edinburgh hackathon.

It includes UI integration of the feedback step, release of the feedback to students in the exchange, and fetching of this feedback by the students.

Some UI work needs to be finished but starting a PR will help check the coverage of the existing tests.

Fixes #795
Replaces #1096

can release to a single student better handling in releaseFeedback.py

Billy's Javascript Assignment link

Updated docstrings

Ian's and Doug's documentation updates

danielmaitre · 2019-06-04T09:20:51Z

There are problems with the automatic CI tests, on travis only with python nighty and on appveyor only for python 3.5/6 (not sure which one it is, the executable says 3.5, the environment 3.6), anyone has any idea how to fix this?

perllaghu · 2019-06-04T09:56:33Z

@BertR - you did some stuff with the Travis at the hackathon, got time to have a look?

dsblank · 2019-06-04T10:03:25Z

Both errors on AppVeyor and Travis look like issues in nbconvert that has a missing language_info key in something. I don't think that is related to anything we (or nbgrader) has changed.

jhamrick · 2019-06-04T19:03:13Z

Both errors on AppVeyor and Travis look like issues in nbconvert that has a missing language_info key in something. I don't think that is related to anything we (or nbgrader) has changed.

Yes, that is my suspicion as well, I have restarted the tests to see.

jhamrick

Only a few very minor changes and then I am happy to merge this!

Also, as a side note, I had a bit of confusion with the UI, which is that I collected a new assignment and then tried to generate feedback (without autograding), and it wouldn't regenerate the feedback. At first I thought this was because you might need to pass the force option but really I think the answer was that I needed to autograde first. I wonder if there is a way we can make this clearer in the UI---not something we should do in this PR but just wondering out loud so I don't forget 🙂

jhamrick · 2019-06-04T19:06:34Z

.gitignore

@@ -112,3 +112,6 @@ package-lock.json
 .goutputstream*


+fakecoursedir/


Are these changes needed for this PR? They don't seem to actually appear in the other changes.

jhamrick · 2019-06-04T19:21:40Z

nbgrader/nbextensions/assignment_list/assignment_list.js

@@ -315,14 +389,51 @@ define([

    Assignment.prototype.make_row = function () {
        var row = $('<div/>').addClass('col-md-12');
-        row.append(this.make_link());
+	var link = this.make_link();


Suggested change

var link = this.make_link();

var link = this.make_link();

nbgrader/server_extensions/formgrader/apihandlers.py

dsblank · 2019-06-05T00:28:26Z

@jhamrick @danielmaitre Final fixes can be found in danielmaitre#11

Formatting; gitignore; added @check_notebook_dir

danielmaitre · 2019-06-05T06:24:43Z

All checks have passed!

perllaghu · 2019-06-05T06:55:30Z

WOO-HOO!!!

Dang - that was some good work guys...

dsblank · 2019-06-05T08:27:20Z

I very much enjoyed working with you all! Great project to dive into most of the topics of nbgrader, and a great team! Let's do it again :)

jhamrick · 2019-06-05T18:30:02Z

Fabulous, thanks so much to all of you for working on this!

dsblank · 2019-06-05T18:32:04Z

Party!

Thanks for being our mentor through this @jhamrick ! I know how much patience it takes. Much appreciated!

jhamrick · 2019-06-05T19:15:02Z

I am more than happy to---it's really exciting seeing other want to contribute! 😄

danielmaitre · 2019-06-06T06:26:46Z

Thank you for your help on this project!
Github says I can safely delete the branch, should I?

jhamrick · 2019-06-06T08:20:28Z

Fine by me.

rkdarst · 2019-08-18T23:16:02Z

I was looking at this, wanting to take it into our use. It would be very nice to have and save us a lot of work! But first I had to figure out how it works. I was hoping that someone could confirm my observations below:

First, it seems to list the exchange inbound path (submitted assignments) to get notebook hashes, which are used to copy assignments back. But the security of the inbound path is by not allowing students to list it. Is this the case, do students have to list the inbound directory in order to fetch feedback?

Second, it also ensures that {course_directory}/feedback/ is read/execute by students and fails with an error if not. This would mean that students can list that entire directory and see all other students feedbacks. Is this the expected situation? At first I thought this could make the feedback dir read but not execute, so that students could get their feedback if they knew their exact filename (based on the hash), but not list all other assignments.

My thought, if the above is the case, chaneg it to do this: find the the hashes of the submitted notebooks from ~/.local/share/jupyter/nbgrader_cache/{course_id}/, which only the submitter would have for their own notebooks. Then, use that to get the feedback from the feedback dir, which they can access to copy known filenames from but not list.

jhamrick · 2019-08-21T20:12:53Z

I am not sure the answers to your questions off the top of my head, but can help try to answer them later this weekend 😄 in the meantime perhaps @danielmaitre @dsblank @perllaghu would know?

Sefriol · 2019-08-21T22:06:13Z

I mimicked how Feedback Exchanger does determine submission and it's feedback in our environment.

By testing this with couple of submissions, you can indeed generate the same feedback md5-hash from ~/.local/share/jupyter/nbgrader_cache/{course_id}/ as you do from inbound directory which would guide the student to right feedback form. With this method student could then copy the feedback without giving student ls rights to the inbound directory. Much preferred way of doing things.

Also, currently fetch assignment is meant for students only (according to documentation), but it accepts student ID or no ID specified as a parameter. Other student should not be allowed to look for other students submissions / feedbacks straight on, therefore this configuration should not exist in the first place. For instructors, it's not really needed anyway. (and if we use ~/.local/share/jupyter/nbgrader_cache/{course_id}/ as a hash source, student_id would not be needed in the end)

All in all, I came into same observations as Richard and how the things currently seem to be, I think change in this is a requirement before this can go live.

Side note: FetchFeedbackApp has fetch assignment examples. Probably it shouldn't ;)

- jupyter#1120 (comment)

- This should not have to be listable, then any student can get any other student's data. - jupyter#1120 (comment)

jhamrick · 2019-08-26T20:48:54Z

Let's continue discussion in #1202

dsblank and others added 30 commits May 29, 2019 17:07

Add generate and release feedback to UI

08ca3fc

first steps for releaseFeedback

165e0c0

added feedback info to Exchangelist

83f1cbf

added fetchfeedback api

5686888

Add individual generate/release feedback

1d0b329

Merge remote-tracking branch 'dsblamk/add-feedbackto-ui' into edinburgh

c91f51b

Ian's patch from PR jupyter#1003

76ec956

Bug fixes and integration

1ccce7f

Bug fixes and integration

7ae1439

Merge remote-tracking branch 'dsblamk/add-feedbackto-ui' into edinburgh

1d80d37

hooked releaseFeedback to UI

e0ebf22

hooked releaseFeedback better

ce93b06

can release to a single student better handling in releaseFeedback.py

added releasefeedback for all students at once

8830e46

Ignore feedback folder in submissions

c28937e

Force feedback to use correct template

db1238c

New bug fixes: import config, proper use of temp_attrs

deea648

fixed typo and added tests of releasefeedback api

02eabc7

Merge remote-tracking branch 'dsblamk/add-feedbackto-ui' into edinburgh

7252b70

added tests for feedback fetch and release

bebd5ee

rename releasefeedback and fetchfeedback with underscores

7dddd31

added command example and added test for release_feedback

2c66859

fixed list tests

62d1696

Billy's Javascript Assignment link

65c5019

improved test_api (still not working)

a7c8fca

Merge pull request #2 from dsblank/edinburgh

9c88530

Billy's Javascript Assignment link

Updated docstrings

61acb96

Merge pull request #3 from dsblank/edinburgh

06748f3

Updated docstrings

added info about local feedback path

bae0676

Ian's and Doug's documentation updates

183df81

Merge pull request #4 from dsblank/edinburgh

a782e1e

Ian's and Doug's documentation updates

added tests to improve coverage

8380610

jhamrick requested changes Jun 4, 2019

View reviewed changes

Formatting; gitignore; added @check_notebook_dir

0b23fdc

Merge pull request #11 from dsblank/edinburgh

307fddd

Formatting; gitignore; added @check_notebook_dir

jhamrick changed the title ~~WIP: Integrate feedback distribution within nbgrader~~ Integrate feedback distribution within nbgrader Jun 5, 2019

jhamrick approved these changes Jun 5, 2019

View reviewed changes

jhamrick merged commit 2539be8 into jupyter:master Jun 5, 2019

danielmaitre deleted the edinburgh branch June 7, 2019 14:39

rkdarst added a commit to AaltoSciComp/nbgrader that referenced this pull request Aug 23, 2019

Feedback: find notebook hashes via the cache, not the inbound dir

9b1fc0d

- jupyter#1120 (comment)

rkdarst added a commit to AaltoSciComp/nbgrader that referenced this pull request Aug 23, 2019

Feedback: don't require exchange directory to be listable

052ff5e

- This should not have to be listable, then any student can get any other student's data. - jupyter#1120 (comment)

Sefriol mentioned this pull request Aug 23, 2019

Remove student_id and change root to cache, permission check to only execute #1186

Merged

jhamrick mentioned this pull request Aug 26, 2019

Consider how to prevent access to feedback from other students #1202

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate feedback distribution within nbgrader #1120

Integrate feedback distribution within nbgrader #1120

danielmaitre commented Jun 1, 2019 •

edited by jhamrick

Loading

danielmaitre commented Jun 4, 2019

perllaghu commented Jun 4, 2019

dsblank commented Jun 4, 2019

jhamrick commented Jun 4, 2019

jhamrick left a comment

jhamrick Jun 4, 2019

dsblank Jun 5, 2019

jhamrick Jun 4, 2019

dsblank Jun 5, 2019

dsblank commented Jun 5, 2019

danielmaitre commented Jun 5, 2019

perllaghu commented Jun 5, 2019

dsblank commented Jun 5, 2019

jhamrick commented Jun 5, 2019

dsblank commented Jun 5, 2019

jhamrick commented Jun 5, 2019

danielmaitre commented Jun 6, 2019

jhamrick commented Jun 6, 2019

rkdarst commented Aug 18, 2019

jhamrick commented Aug 21, 2019

Sefriol commented Aug 21, 2019 •

edited

Loading

jhamrick commented Aug 26, 2019

		@@ -112,3 +112,6 @@ package-lock.json
		.goutputstream*


		fakecoursedir/

Integrate feedback distribution within nbgrader #1120

Integrate feedback distribution within nbgrader #1120

Conversation

danielmaitre commented Jun 1, 2019 • edited by jhamrick Loading

danielmaitre commented Jun 4, 2019

perllaghu commented Jun 4, 2019

dsblank commented Jun 4, 2019

jhamrick commented Jun 4, 2019

jhamrick left a comment

Choose a reason for hiding this comment

jhamrick Jun 4, 2019

Choose a reason for hiding this comment

dsblank Jun 5, 2019

Choose a reason for hiding this comment

jhamrick Jun 4, 2019

Choose a reason for hiding this comment

dsblank Jun 5, 2019

Choose a reason for hiding this comment

dsblank commented Jun 5, 2019

danielmaitre commented Jun 5, 2019

perllaghu commented Jun 5, 2019

dsblank commented Jun 5, 2019

jhamrick commented Jun 5, 2019

dsblank commented Jun 5, 2019

jhamrick commented Jun 5, 2019

danielmaitre commented Jun 6, 2019

jhamrick commented Jun 6, 2019

rkdarst commented Aug 18, 2019

jhamrick commented Aug 21, 2019

Sefriol commented Aug 21, 2019 • edited Loading

jhamrick commented Aug 26, 2019

danielmaitre commented Jun 1, 2019 •

edited by jhamrick

Loading

Sefriol commented Aug 21, 2019 •

edited

Loading