-
Notifications
You must be signed in to change notification settings - Fork 27
Add upload agreement to project submission workflow. Ref #1324 #2479
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
base: dev
Are you sure you want to change the base?
Conversation
9a9c19f to
5fe7921
Compare
5fe7921 to
ad30545
Compare
d014d94 to
58739bc
Compare
|
@bemoody please could you take a look at this if you have the opportunity? |
|
Haven't tested this yet. I think that if a project is reassigned to a different submitting author, the new author should have to fill out the form again. Could be done by deleting the agreement upon reassignment or by recording the user who signed it. I think maybe we also want to require the form to be completed before submitting, as well as before uploading files. |
367f807 to
390d15b
Compare
|
Instead of 390d15b, please just add an UploadAgreement to demo-project.json. |
390d15b to
1ae0fe3
Compare
6f89f04 to
acb9342
Compare
Done! |
physionet-django/project/forms.py
Outdated
| agreement.human_subjects_deidentified = self.cleaned_data['human_subjects_deidentified'] | ||
| agreement.accepted = True | ||
| if self.user: | ||
| agreement.accepted_by = self.user |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what this is for. How is this different than just calling super().save(commit=False) ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're right, updated to use your suggested syntax.
|
Your help string says "Each project can have one active upload agreement." But that doesn't appear to be the case. If there is one UploadAgreement per project, there should be a OneToOneField (or a ForeignKey having unique=True.) If there is one UploadAgreement per project and accepted_by, there should be a unique_together. |
bemoody
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not want to permit multiple UploadAgreements for the same author and the same project. A good way to do that would be to remove project and accepted_by, and replace them with a single foreign key linking to project.Author and having unique=True.
get_active_agreement is full of debugging code that should be unnecessary. I would get rid of this method entirely. Possibly add a method to Author that returns the author's UploadAgreement. You do not need to do project.authors.get(is_submitting=True) to obtain the submitting author, just call project.submitting_author().
Currently it looks like this will forbid editors from copyediting a project if the submitting author has not accepted the agreement. We don't want that.
Non-submitting authors should not see the message "You must accept the upload agreement" on the files page.
As discussed in #1324, we would like to add an upload agreement as part of the project submission process.
This pull request adds a new "Upload Agreement" step to the project submission workflow. The submitting author must agree to our terms and complete one or more checkboxes. It is not possible to upload files until this step is complete.
The screenshot below shows this new step in the submission process: