-
Notifications
You must be signed in to change notification settings - Fork 2
Add publication and versioning #5
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: main
Are you sure you want to change the base?
Conversation
|
Hi @cpfaff , do you want to check out this branch maybe? |
|
maybe it would be better to try and merge this PR before adding other parts for the REST API to it. |
|
can we merge this? @jochenklar |
jochenklar
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.
Hey, I finally got some time to look at the plugin, sorry for the long wait. I guess it works (almost), but I have some remarks regarding the architecture.
rdmo_zenodo/exports/metadata.py
Outdated
| from rdmo.projects.exports import Export | ||
|
|
||
|
|
||
| class ZenodoMetadataExport(Export): |
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.
The name is a bit misleading since this is not an export plugin, or at least it is not sopposed to be used independently, right? I also think it should not inherit Export. Maybe ZenodoMetadataBuilder, since this is how it is used. It could also be a mixin for the export plugins or part of the base class, depends on why exaclty this pattern was chosen.
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 it is more like a mixin and does not need Export, it is only using these self.get_text and self.get_values methods. I think the base class was getting too big that's why I placed it in a new one.
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've now refactored this Metadata stuff into a metadata package and some builder classes. These should have no external dependencies besides the settings, the project or snapshot related things are provided by the caller of these builders in the get_post_data method.
It has some duplication with all these field names but maybe its cleaner and clearer now?
rdmo_zenodo/exports/metadata.py
Outdated
| 'metadata': self._filter_empty_values(metadata) | ||
| } | ||
|
|
||
| def _filter_empty_values(self, metadata: Dict[str, Any]) -> Dict[str, Any]: |
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 think this is more readable than:
a = settings.A
if a:
metadata['a'] = a
but ok...
rdmo_zenodo/exports/utils.py
Outdated
| @@ -0,0 +1,83 @@ | |||
| from django.http import HttpResponse | |||
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 are some missing newlines between the methods in this file.
rdmo_zenodo/exports/utils.py
Outdated
|
|
||
| def render_project_views(project, snapshot, attachments_format, view=None): | ||
|
|
||
| if view is None: |
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.
Was this for debugging, if yes, it should be removed.
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 it seems I need the view for the plugin to work. Maybe this should be a template whithin the plugin?
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 that the user can customize with a custom template? Maybe the view should be selected in the initial form as well by the user to make it easier to customize?
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.
Ive added a selection for the view and export_format to the form.
rdmo_zenodo/exports/publish.py
Outdated
|
|
||
| record_versions_url = self.validate_record_id_from_project_value_at_zenodo() | ||
| # TODO, currently the authentication can get stuck when trying out the dataset export | ||
| # first and this one afterwards, a 403 needs to be handled in the Export class. |
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 still have one problem with the authentication sometimes. When I first export the dataset and afterwards try to publish the snapshot then I get a 403 on the post call (and the Oauth Error template with None as message) . Could this be because of the different callback urls that are set for each export provider and that the token is then invalid? I can only get out of it by closing the browser.
Should these plugins not just have a single authentication and callback uri? Or does it need to handled in the https://github.com/rdmorganiser/rdmo/blob/79917de8dfdb8be0c988bc72f1352b307ce03cb1/rdmo/services/providers.py#L62 ?
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.
Maybe this issue zenodo/zenodo#2168 is related somehow?
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.
Now I'm getting another authentication error ERROR: callback error: b'{"error": "invalid_grant"}' (400) after clicking "Authorize application" on the https://sandbox.zenodo.org/oauth/authorize website. Maybe it's similar to the issue reported in zenodo/zenodo-rdm#878.
After clicking around in the debug tools,and browsing back and forth, I now get another error WARNING: post error: b'{"status": 400, "message": "Invalid value e."}' (400). It seems that I had a mistake in my metadata after all and was sending e.g. 'languages': [{'id': 'e'}] ).
After fixing that the request works again.
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 still have one problem with the authentication sometimes. When I first export the dataset and afterwards try to publish the snapshot then I get a
403on the post call (and the Oauth Error template with None as message) . Could this be because of the different callback urls that are set for each export provider and that the token is then invalid? I can only get out of it by closing the browser. Should these plugins not just have a single authentication and callback uri? Or does it need to handled in the https://github.com/rdmorganiser/rdmo/blob/79917de8dfdb8be0c988bc72f1352b307ce03cb1/rdmo/services/providers.py#L62 ?
I've added a post_with_retry method that pops the access_token and re-tries the self.post(...) after it failed with an "OAuth error". The user just needs to re-authenticate but it seems to work.
I think an extra callback post_failure should be added to https://github.com/rdmorganiser/rdmo/blob/79917de8dfdb8be0c988bc72f1352b307ce03cb1/rdmo/services/providers.py#L67 in parallel to post_success so that it can be implemented and adapted by a plugin. Or was there a problem with storing/retreiving the access token?
|
There is still a problem. For some reason, the stored zenodoid is one lower than the actual id. |
|
Also, I am not sure creating the attribute in the Plugin is a good idea. In any case the URI prefix should not be hardcoded. |
This is not a problem but actually a feature. After initially creating a record and uploading a file, Zenodo returns a From https://inveniordm.docs.cern.ch/operate/customize/dois/#parent-or-concept-dois:
So that concept id is used when publishing another rdmo snapshot to the previously created record and thereby creating a new version in that record. |
* Collects metadata for the project snapshot * Creates snapshot questions based document * Creates zenodo deposit, adds metadata, adds the file and publishes the deposit.
Refactored the Zenodo Export Provider to improve code quality, reduce duplication, and enhance error handling.
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
Signed-off-by: David Wallace <[email protected]>
d6acdf2 to
627f8f5
Compare
Signed-off-by: David Wallace <[email protected]>
yes, Ive made it configurable. I've also tested against the sandbox https://inveniordm.web.cern.ch/ instance and made it work with a configurable setting for the Now in the readme: 'zenodo_url': 'https://zenodo.org', # optional, default https://zenodo.org , or your own InvenioRDM instance url
'zenodo_auth_scope': 'deposit:write', # optional, default 'deposit:write' or 'user:email' for InvenioRDM
'publish_record_id_attribute_prefix': 'https://rdmorganiser.github.io/terms', # optional, default is shown here
'publish_record_id_attribute_key': 'project/metadata/publication/zenodo/concept_record_id', # optional, default is shown here Now I've rebased and it's ready I think! |
Signed-off-by: David Wallace <[email protected]>
jochenklar
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.
Sorry, we need to talk about this again. The current classes in metadata prevent reuse and customization.
|
|
||
|
|
||
| @dataclass | ||
| class ZenodoMetadata: |
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 model can be kept, maybe add a function get_metadata instead of get_post_data
I went further down the metadata rabbit hole and tried to implement the complete schemas for the payload (incl |
| identifier: str | None = None | ||
|
|
||
| @attrs.define | ||
| class InvenioMetadataV6: |
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 is the Invenio metadata
project.valuewith a configurable attribute (eg.https://rdmorganiser.github.io/terms/project/metadata/publication/zenodo/concept_record_id)publish_record_id_attribute_prefixandpublish_record_id_attribute_keyzenodo_url(the instance url),zenodo_auth_scope(need to set the scope touser:email)refactoring
metadata,forms.pyand methods intoutils.py