Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Copy integrity #1755

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Copy integrity #1755

wants to merge 5 commits into from

Conversation

tuncbkose
Copy link
Contributor

  • 11da69c: Adds error messages for when fetch/fetch_feedback fails
  • 783e0d2: Wraps copying function into try/except/finally to ensure that in the event of an error, already copied files have correct permissions
  • 8d8d898: Wraps copying function into try/except to ensure that even if not all files were copied, timestamp.txt is still created and feedback workflow doesn't break later
    • Right now, the student receives a "Submission Error" and no indication that something is submitted. However, files are still copied to the exchange folder.
    • I think it is fine to not prevent any copying (i.e. delete already-copied files if an error is encountered at any point), because if a student sees "Submission Error", they will likely try submitting again, hopefully after the problem is fixed.
    • The error message user sees could be clearer.

@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch AaltoSciComp/nbgrader/copy-2

@brichet
Copy link
Contributor

brichet commented Jun 12, 2023

Thanks @tuncbkose.

  • 11da69c: Adds error messages for when fetch/fetch_feedback fails

Can you add tests on it please ?

For the others commit do you have a workflow how to reproduce the error ?
Also, if it is possible, it would be wonderful to have tests on it.

@tuncbkose
Copy link
Contributor Author

All of these came into play when files had wrong permissions (which was a concern for us, in case something might go wrong in mounting network drives). They are easily reproducible if you remove read rights from a notebook and try to submit it, for example.

I am not sure if I can do a similar thing for a test (or rather if it is a good idea), so I'll try to come up with something else. Let me know if you have any suggestions.

@tuncbkose
Copy link
Contributor Author

I am slowly working on adding tests, but the following might be worth noting:

Currently, pressing the "fetch feedback" button when feedback is not available does nothing from the users' perspective. In my experience, this has led students to wonder if the button was working properly.

In the current state of the PR, pressing the button when feedback is not available will create an error dialogue with "Fetch Failed: feedback not fetched" and the traceback. Even though the traceback at the end includes "Assignment not found at ...", it may be unreasonable to expect students to infer from this that feedback is not available.

This change in my opinion is pretty neutral, but it might be worth changing the default error message to something like "Feedback not available or something went wrong".

@tuncbkose tuncbkose force-pushed the copy-2 branch 2 times, most recently from 2ca5a29 to af5fe32 Compare July 12, 2023 11:31
@tuncbkose
Copy link
Contributor Author

Is there a way to set some nbgrader configuration for a single test? Only 783e0d2 remains untested, but it applies only when CourseDirectory.groupshared == true.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants