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

groupshared feedback re-release fix #1667

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

franswel
Copy link
Contributor

Groupshared users could not re-release student feedback due to shutil.copy functionality used by release_feedback. Shutil.copy tries to chmod the destination file which does not work in groupshared scenarios where files have different owners but same groups. Therefore groupshared users cannot copy a new generated feedback with the same name over the old one owned by another user. In the fix the file gets copied in a temporary location and is moved over the old file.

Tests were not implemented as I believe it is not possible to test interactions requiring multiple users automatically. To replicate the issue, you need two users with groupshared enabled that release feedback for the same student's assignment.

@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch AaltoSciComp/nbgrader/multi-instructor-feedback

@rkdarst
Copy link
Contributor

rkdarst commented Jan 11, 2023

This is a follow up of #1000, and is live at our site for a while now. It's also nice that it makes feedback releasing an atomic operation, so I think this should be "good to go". Existing tests should cover the actual functionality, and like @franswel said testing the multi-user interaction is probably too hard.

@Gehock Gehock force-pushed the multi-instructor-feedback branch 2 times, most recently from 0fc9858 to b995709 Compare March 29, 2023 13:13
@Gehock
Copy link

Gehock commented Oct 2, 2024

Rebased on main. Like Richard mentioned, this has been running in our in our production environment without issues for a while now.

Copy link
Contributor

@brichet brichet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @franswel for the fix and @Gehock for reviving this PR.
It looks good to me.

@brichet brichet merged commit be97e17 into jupyter:main Oct 4, 2024
25 of 26 checks passed
@Gehock Gehock deleted the multi-instructor-feedback branch October 7, 2024 09:58
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.

4 participants