-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat: Added support for the CRediT Taxonomy for contributors #4512
base: master
Are you sure you want to change the base?
Conversation
1584586
to
22aaba0
Compare
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.
So glad to see this work being done! Thanks again @MartinPaulEve.
I am looking forward to testing out the interface. I wasn't able to do so yet because I ran into a migration error:
CommandError: Conflicting migrations detected; multiple leaf nodes in the migration graph: (0083_creditrecord_preprint_author, 0085_alter_article_jats_article_type_override_and_more in submission).
What we like to do nowadays with git conflicts or split migration graphs is rebase dev branches on master and change the dependency of the dev migration. This allows us to avoid creating duplicate merge migrations in multiple people's dev instances. Could you do that please before I go further with my review?
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.
Thanks again for this Martin.
I wrote some changes inline that I'd like to see before merging. Some of them are important to take care of regardless of how the UI works, while some of them could be informed by the conversation we have around #4519 . More details inline.
Two global comments:
- Shouldn't there be an interface for adding and removing CREDIT roles in the repository submission system, if there is a field for repository author on the role object? Or did I miss that integration?
- Shouldn't the canonical URI to each CRediT role be captured to make it more machine-readable in your plugin and other places like our own JATS? Taking a quick look at the implementation guidelines, it seems like the URLs would be useful for name authority. I think this is especially the case since the strings used contain questionable characters for downstream consumption, like the ampersand in "Writing - Review & Editing".
for credit_record in credit_records: | ||
credit_record.frozen_author = frozen_author | ||
credit_record.save() | ||
|
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.
There is an indentation problem here. This code is inline with line 564, outside the if
, meaning the else
on 576 will be a syntax error. It needs to be indented so that it is part of the if clause started on 564, right?
I think this will also need to remove roles from the frozen author that have been removed on the author, right?
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.
OK, so I wasn't sure what force_update meant in "if frozen_author and force_update", and assumed it was a unique case, but that we would always want to save the credit_records when we freeze an article. Is that not right?
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.
Yes, I agree it's a good idea to update credit records in this spot, it just needs to be inside the first if
clause / indented four spaces.
WRT to force_update
, it is defined on snapshot_authors
if you search for it (granted, it should be defined on snapshot_self
). It is "Whether or not to update existing records". So this if
block is supposed to run when there are existing frozen authors and when they need to be updated, which is a lot of the time, like in the pre-publication stage, IIRC.
With the current indentation, you are changing the code below from if / else
to for / else
, which is not what we want, because the else
will always run. We only want lines 577 etc. to run if the conditions on line 564 are not met.
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.
Martin has now fixed the indentation and syntax bugs here.
But I still think this needs to remove past CRediT roles from the FrozenAuthor during a forced update. As mentioned on 5 Dec above: "I think this will also need to remove roles from the frozen author that have been removed on the author, right?"
Currently I do not think the interface would invite users to edit credit roles on the author after submission, but still it makes sense to keep the function straightforward and idempotent because it may be a bug for other places that create these records. Think about importers, plugins, etc.
FYI @MartinPaulEve: We now have something of a plan for the submission page (see #4519 (comment)). So, we'd like to ask if you would remove the back-office UI elements from this PR. I think that's everything in |
Adding a note here to record my conversation with @MartinPaulEve about URIs and name authority: Martin is going to implement the URIs that I mentioned in my comment above, perhaps as keys in the choices, perhaps as part of a separate model. The URIs are:
More details in the ANSI/NISO spec document. |
@MartinPaulEve as mentioned off GH, I'm also linking our original feature request for CRediT roles. The requester asked for these things in their wish-list:
@StephDriver and @mauromsl and I had a brief chat about this and were hoping you could confirm for us whether these remaining things are in scope? My sense is that 2 is done for reader display on Janeway, but it's not yet in the JATS. Is that right? Were you planning to put it in the JATS or not? For 3, I think it should be in scope, because CRediT roles might not work for all journals and disciplines, and editors will want the option to avoid collecting metadata that is not useful to them, since it would be inconsistent if some articles had this information and others didn't. I think it should be on by default, but controllable at the journal level. A boolean-type My sense is that 4 is not in scope because it has to do with a different issue and involves UI design. Just let me know what you think. |
OK, so I've added CRediT to the JATS, including the work on canonical URLs (which Mauro suggested to store in the choices list, rather than in the database). 4 is definitely out of scope for now, but 3 is an interesting question. My suggestion for this is that the new interface that you build, when re-doing submission, should simply hide the ability to add CRediT information if it's disabled, because all of the display stuff I've done will NOT show anything if CRediT information is not provided. |
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.
Hi @MartinPaulEve, thanks for this progress.
I still think we need to think more about how the snapshot_self
function works with CRediT roles. What about this scenario: An author submits an article on which they are drafting author, and then, while it's in production, they submit another one to the same journal on which they are the editing author. How does Janeway not erroneously pick up the editing author role from article 2 when it's updating the frozen authors for article 1, say in typesetting or prepub? This is a harder problem to solve, because I think it might involve creating frozen authors when author metadata is first entered during submission, and not updating them when we snapshot the author at a later date, just allow editing the frozen author. That's perhaps beyond the scope of your work, but I want to raise it here so @mauromsl and @ajrbyers are aware of it, and we'll have to address it before we release 1.8.
the new interface ... should simply hide the ability to add CRediT information if it's disabled
Yeah that's what I was thinking, but how does an editor or press manager disable it? Have I missed a setting you have introduced? Any thoughts on my suggestion above: "A boolean-type core.models.Setting
should do the trick, and you can add one in src/utils/install/journal_defaults.json
." The setting should be introduced in core Janeway before you try to make your OA switchboard plugin hook into it, because otherwise you won't know how to check for whether CRediT roles are expected or not, for a given journal's metadata. Ideally, this setting, not whether CRediT records are present, should control whether this metadata is displayed on the article page. Editors will expect a single toggle on / off to control it everywhere.
so I've added CRediT to the JATS, including the work on canonical URLs (which Mauro suggested to store in the choices list, rather than in the database)
Super, though I'm wondering why we have spaces and ampersands in the DB keys? Any reason why we shouldn't use the less error-prone short slug form of each role, e.g. formal-analysis
? We can also change this after this PR is merged if you don't have time to keep making changes like this.
Again, really appreciate this contribution. As I have upcoming annual leave I am going to request changes and also request Andy's and Mauro's review so you have some other perspectives sooner rather than later.
I've checked this over and can't see how this scenario would play out as I'd agree with the other two suggestions if there is time to make them @MartinPaulEve. |
3879680
to
2b7e64d
Compare
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.
@ajrbyers asked me to review this one more time to double-check the author leaking issue, and take a look at the new setting.
I think I was mistaken before about the account leaking its roles, as pointed out above, so that's no longer an issue.
But I would still like to see changes regarding a few other things mentioned before. I've put additional comments inline to make them extra clear.
use_credit = setting_handler.get_setting( | ||
"general", "use_credit", | ||
journal=request.journal | ||
) |
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.
This isn't working yet because it needs to get the associated setting value.
for credit_record in credit_records: | ||
credit_record.frozen_author = frozen_author | ||
credit_record.save() | ||
|
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.
Martin has now fixed the indentation and syntax bugs here.
But I still think this needs to remove past CRediT roles from the FrozenAuthor during a forced update. As mentioned on 5 Dec above: "I think this will also need to remove roles from the frozen author that have been removed on the author, right?"
Currently I do not think the interface would invite users to edit credit roles on the author after submission, but still it makes sense to keep the function straightforward and idempotent because it may be a bug for other places that create these records. Think about importers, plugins, etc.
), | ||
} | ||
|
||
CREDIT_ROLE_CHOICES = [(key, values[0]) for key, values in CREDIT_ROLES.items()] |
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.
As mentioned in my last review, this still seems unnecessarily complicated to me. Couldn't we have a simpler choices set like this, to make it easier to maintain and to keep ampersands and spaces out of controlled strings in the database?
CREDIT_ROLE_CHOICES = [
("conceptualization", "Conceptualization"),
("data-curation", "Data Curation"),
("formal-analysis", "Formal Analysis"),
("funding-acquisition", "Funding Acquisition"),
...
("writing-review-editing": "Writing - Review & Editing")
]
Then to get the URI form for downstream metadata, a method:
class CreditRecord(AbstractLastModifiedModel):
def uri(self):
return f"https://credit.niso.org/contributor-roles/{self.role}/"
And for the string representation, we can get the choice label:
def __str__(self):
return self.get_role_display()
This PR adds the CRediT taxonomy to Janeway. This is a prerequisite for the OA Switchboard work.
The PR:
Not in scope, still to do as part of Janeway 1.8:
closes #3037