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.

Sign up for GitHub

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

Jump to bottom

Invenio-damap: Support connection from InvenioRDM #83

Merged
merged 1 commit into from
Sep 19, 2024
Merged

Invenio-damap: Support connection from InvenioRDM #83

merged 1 commit into from
Sep 19, 2024

Conversation

rekt-hard
Copy link
Collaborator

@rekt-hard rekt-hard commented Feb 27, 2023 •
edited by SotosTsepe
Loading

Description

Feature

What does this PR do?

  • Supports incoming connections from InvenioRDM-based repositories.
  • Adds 2 new endpoints for:
    • fetching DMPs and,
    • setting DMP information based on the InvenioRDM record which is being connected.
  • Creates a new version after adding a dataset remotely.

Breaking changes

Code review focus

Mapping common maDMP standard dataset to DmpDO.

Dependencies

Checks

  • Tested with Oracle/PostgreSQL
  • Export updated
  • Documentation added
  • Tests added
  • E2e tests created
  • Successfully ran e2e tests before merge

closes fair-data-austria/invenio-damap#13, fair-data-austria/invenio-damap#7

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug C 2 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 10 Code Smells

2.5% 2.5% Coverage
0.0% 0.0% Duplication

@rekt-hard rekt-hard marked this pull request as ready for review April 20, 2023 11:21
@rekt-hard rekt-hard marked this pull request as draft April 20, 2023 11:22
@rekt-hard rekt-hard linked an issue Jan 19, 2024 that may be closed by this pull request
Support connection to Invenio RDM #48
Closed
@SotosTsepe SotosTsepe self-requested a review January 23, 2024 11:02
@rekt-hard rekt-hard force-pushed the invenio-damap branch 2 times, most recently from 0a03762 to a1ecd3b Compare February 5, 2024 12:14
@rekt-hard rekt-hard force-pushed the invenio-damap branch 3 times, most recently from f92d1d4 to 21bfb59 Compare March 29, 2024 10:23
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Passed Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
85.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

@rekt-hard rekt-hard marked this pull request as ready for review March 29, 2024 10:35
@rekt-hard
Copy link
Collaborator Author

Please make sure to squash the commits :)

@rekt-hard rekt-hard marked this pull request as draft April 15, 2024 15:45
@SotosTsepe SotosTsepe changed the title Invenio damap Invenio-damap: Support connection from InvenioRDM Jul 29, 2024
@SotosTsepe SotosTsepe marked this pull request as ready for review July 29, 2024 12:18
@SotosTsepe SotosTsepe force-pushed the invenio-damap branch 2 times, most recently from 88cda19 to 47e5e7b Compare July 30, 2024 15:19
@SotosTsepe SotosTsepe force-pushed the invenio-damap branch 4 times, most recently from f759727 to 5203c11 Compare August 8, 2024 14:10
rekt-hard
rekt-hard commented Aug 14, 2024
View reviewed changes
Copy link
Collaborator Author

@rekt-hard rekt-hard left a comment

Choose a reason for hiding this comment

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

nice work implementing the jwt authorization 🚀

What might be missing is refactoring/moving some of the logic from the resource level to the service level. Also making sure that the service can be overwritten aka an own service can be provided.
Might be appropriate to move authorization logic to the security service (also ensuring an own service can be provided).

I still have to test this locally so some other issues might come up.

src/main/java/org/damap/base/rest/invenio_damap/DMPPayload.java Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/DMPPayload.java Outdated Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/DMPPayload.java Outdated Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/InvenioDAMAPResource.java Outdated Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/InvenioDAMAPResource.java Outdated Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/InvenioDAMAPResource.java Outdated Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/InvenioDAMAPResource.java Outdated Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/InvenioDAMAPResource.java Outdated Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/InvenioDAMAPResource.java Outdated Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/InvenioDAMAPResource.java Outdated Show resolved Hide resolved
rekt-hard
rekt-hard commented Aug 30, 2024
View reviewed changes
Copy link
Collaborator Author

@rekt-hard rekt-hard left a comment

Choose a reason for hiding this comment

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

DAMAP should return a:

  • 401 if the no valid authentication schema is provided, identity could not be resolved or multiple identities resolve to different DMPs.
  • 403 if the identity can be resolved, but this identity is not allowed to perform a certain action
  • 404 if an endpoint is not found, a resource is not available (i.e. wrong DMP id when adding a dataset), or to mask a 403 if the existence of a resource should not be revealed)

rekt-hard
rekt-hard commented Sep 9, 2024
View reviewed changes
Copy link
Collaborator Author

@rekt-hard rekt-hard left a comment

Choose a reason for hiding this comment

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

Looking very good. Just some small comments.

src/main/java/org/damap/base/rest/invenio_damap/InvenioDAMAPResource.java Outdated Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/InvenioDAMAPService.java Outdated Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/InvenioDAMAPService.java Outdated Show resolved Hide resolved
src/main/java/org/damap/base/rest/invenio_damap/InvenioDAMAPService.java Outdated Show resolved Hide resolved
@rekt-hard rekt-hard force-pushed the invenio-damap branch 3 times, most recently from 5b741b6 to b628558 Compare September 16, 2024 14:08
@SotosTsepe
Copy link
Member

LGTM 🚀

@rekt-hard @SotosTsepe @ValentinFutterer
invenio-damap: enable connection from InvenioRDM
5465edd 
* Update re3data XML with root element
* Update library usages due to library migration

---------

Co-authored-by: Sotiris Tsepelakis <[email protected]>
Co-authored-by: ValentinFutterer <[email protected]>
@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
74.9% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@SotosTsepe SotosTsepe merged commit ab4f13e into next Sep 19, 2024
1 of 2 checks passed
@rekt-hard rekt-hard deleted the invenio-damap branch September 19, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ValentinFutterer ValentinFutterer ValentinFutterer left review comments

@SotosTsepe SotosTsepe SotosTsepe approved these changes

@ZenoLC ZenoLC ZenoLC approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create new version of DMP after adding dataset Support connection to Invenio RDM
4 participants
@rekt-hard @SotosTsepe @ZenoLC @ValentinFutterer