-
Notifications
You must be signed in to change notification settings - Fork 50
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
Openaire suggestions (publication claim) rest contracts #192
Conversation
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 mechanism, to me, in what you are proposing is very similar to what we already have regarding external sources and authority. I think we should bring this specification close to what is already defined for those mechanisms.
For instance, if we compare how the ID is defined here:
"id": "reciter:gf3d657-9d6d-4a87-b905-fef0f8cae26:24694772",
and how is defined in:
https://github.com/DSpace/RestContract/blob/main/external-authority-sources.md#single-entry
"id": "0000-0002-4271-0436",
Having the self HAL links as:
"https://dspace7.4science.cloud/server/api/integration/externalsources/orcid/entryValues/0000-0002-4271-0436"
I think the source could also be part of the endpoint in the model you are proposing.
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.
@frabacche or @LucaGiamminonni : I've reviewed this today compared to the implementation PR at DSpace/DSpace#8280. This contract looks good overall, but I've found a few minor mistakes compared to the implementation. See inline comments below. Once these are fixed, I'm +1 this PR.
suggestiontargets.md
Outdated
|
||
Exposed links: | ||
* target: link to the items that represent the person to whom the suggestions are proposed | ||
* suggestions: link to the suggestions entries |
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 currently don't see the suggestions
link in the implementation at DSpace/DSpace#8280
When I view the responses from this endpoint, I'm not able to find a way to trigger a "suggestions" link. Is this not yet implemented, or maybe I'm overlooking it? If it's not yet implemented, then we should remove it from here & from the JSON example in this section.
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.
My feedback here was accidentally overlooked. I don't see the suggestions
link for suggestiontargets
. I don't believe this exists yet? So, we should remove it from this line & also from the JSON example in this section.
suggestiontargets.md
Outdated
"type": "suggestiontarget", | ||
"_links": { | ||
"target": { | ||
"href": "https://dspace7.4science.cloud/server/api/core/items/gf3d657-9d6d-4a87-b905-fef0f8cae26" |
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.
Could we replace all of the "https://dspace7.4science.cloud/" URLs with "https://demo.dspace.org/"? We need to make sure that URL examples in our REST Contract are not pointing to institution/service provider specific URLs.
suggestionsources.md
Outdated
* the *total* attribute is the number of target with suggestions. It can be 0 if there are no target with suggestions | ||
|
||
Exposed links: | ||
* suggestiontargets: link to the suggestion targets entries |
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 currently don't see this suggestiontargets
link in the implementation. When I call this endpoint directly, it's also missing. Is this not yet implemented? If it's not yet implemented we should remove it from here & from the JSON example in this section
suggestions.md
Outdated
```json | ||
{ | ||
"_embedded": { | ||
"reciterSourceEntries": [ |
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 should say "suggestions" not "reciterSourceEntries", as that is what the implementation returns.
suggestions.md
Outdated
"id": "24694772", | ||
"display": "publication one", | ||
"source": "reciter", | ||
"external-source-uri": "https://dspace7.4science.cloud/server/api/integration/reciterSourcesEntry/pubmed/entryValues/24694772", |
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.
In the implementation this property is named externalSourceUri
not external-source-uri
suggestions.md
Outdated
"source": "reciter", | ||
"external-source-uri": "https://dspace7.4science.cloud/server/api/integration/reciterSourcesEntry/pubmed/entryValues/24694772", | ||
"evidences": { | ||
"acceptedRejectedEvidence": { |
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.
All of the evidences listed in this example are not currently in DSpace. The only two that I can find are these:
"AuthorNamesScorer" : {
"score" : "100.00",
"notes" : "some notes, empty or null"
},
"DateScorer" : {
"score" : "0.00",
"notes" : "some notes, empty or null"
}
Can we update this example to only list the AuthorNamesScorer and DateScorer?
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.
Evidences shown here in the contract are currently customizable in every bean of class org.dspace.app.suggestion.openaire.PublicationLoader on its property called pipeline . We chose to show all these evidences as examples. On the dspace/config/spring/api/suggestions.xml file we already set the bean called OpenairePublicationLoader with a pipeline attribue having the AuthorNameScorer and the DateScorer.
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.
@frabacche : I understand they are configurable. However, the REST Contract is supposed to provide examples that are realistic based on DSpace's current features. I feel listing a large number of evidences that don't exist in DSpace is misleading. This list should at least be updated to show the few that currently exist and then maybe have one example of how it might look if extended.
suggestions.md
Outdated
"score": "62.7", | ||
"external-source-uri": "https://dspace7.4science.cloud/server/api/integration/reciterSourcesEntry/pubmed/entryValues/24694772", | ||
"evidences": { | ||
"acceptedRejectedEvidence": { |
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.
Same as other comment. We should change this list to only include AuthorNamesScorer and DateScorer.
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.
see reply.
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 I noted in my later reply, I think we need to keep the list of "evidences" realistic, otherwise developers will be confused if they cannot find these in DSpace.
I'm OK with providing one example of a custom evidence. But, currently this REST Contract PR has a list of 9 evidences all of which do not exist in DSpace. I propose we update this to say something like:
"AuthorNamesScorer" : {
"score" : "100.00",
"notes" : "some notes, empty or null"
},
"DateScorer" : {
"score" : "0.00",
"notes" : "some notes, empty or null"
},
"customEvidence": {
"score" : "23.7",
"notes" : "some notes, empty or null"
},
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.
@frabacche : Could you update the list of evidences in this section of suggestions.md
the same way that you updated them below? This section still has the list of invalid "evidences"
For example:
"AuthorNamesScorer" : {
"score" : "100.00",
"notes" : "some notes, empty or null"
},
"DateScorer" : {
"score" : "0.00",
"notes" : "some notes, empty or null"
},
"customEvidence": {
"score" : "23.7",
"notes" : "some notes, empty or null"
},
suggestions.md
Outdated
"display": "publication one", | ||
"source": "reciter", | ||
"score": "62.7", | ||
"external-source-uri": "https://dspace7.4science.cloud/server/api/integration/reciterSourcesEntry/pubmed/entryValues/24694772", |
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.
Same as other comment. Should be named externalSourceUri
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.
@frabacche : Thanks for the updates to this REST Contract. I've re-reviewed your changes, but it appears you accidentally overlooked a few requests. I'd also appreciate it if we can provide a list of "evidences" that is realistic (see comment below). Our REST Contract should not list a lot of unimplemented features (in this case evidences). We need to ensure it is accurate based on what currently exists in DSpace.
Once these final changes are made, this PR should be ready to merge. Thanks!
suggestions.md
Outdated
"score": "62.7", | ||
"external-source-uri": "https://dspace7.4science.cloud/server/api/integration/reciterSourcesEntry/pubmed/entryValues/24694772", | ||
"evidences": { | ||
"acceptedRejectedEvidence": { |
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 I noted in my later reply, I think we need to keep the list of "evidences" realistic, otherwise developers will be confused if they cannot find these in DSpace.
I'm OK with providing one example of a custom evidence. But, currently this REST Contract PR has a list of 9 evidences all of which do not exist in DSpace. I propose we update this to say something like:
"AuthorNamesScorer" : {
"score" : "100.00",
"notes" : "some notes, empty or null"
},
"DateScorer" : {
"score" : "0.00",
"notes" : "some notes, empty or null"
},
"customEvidence": {
"score" : "23.7",
"notes" : "some notes, empty or null"
},
suggestiontargets.md
Outdated
|
||
Exposed links: | ||
* target: link to the items that represent the person to whom the suggestions are proposed | ||
* suggestions: link to the suggestions entries |
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.
My feedback here was accidentally overlooked. I don't see the suggestions
link for suggestiontargets
. I don't believe this exists yet? So, we should remove it from this line & also from the JSON example in this section.
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.
@frabacche : Thanks for the updates! There's one update you still overlooked though. This appears to be the last change that was missing. See comment below.
suggestions.md
Outdated
"score": "62.7", | ||
"external-source-uri": "https://dspace7.4science.cloud/server/api/integration/reciterSourcesEntry/pubmed/entryValues/24694772", | ||
"evidences": { | ||
"acceptedRejectedEvidence": { |
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.
@frabacche : Could you update the list of evidences in this section of suggestions.md
the same way that you updated them below? This section still has the list of invalid "evidences"
For example:
"AuthorNamesScorer" : {
"score" : "100.00",
"notes" : "some notes, empty or null"
},
"DateScorer" : {
"score" : "0.00",
"notes" : "some notes, empty or null"
},
"customEvidence": {
"score" : "23.7",
"notes" : "some notes, empty or null"
},
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.
👍 Thank you @frabacche and @LucaGiamminonni ! This looks good to me now!
Rest contract for suggestion (publication claim) rest repositories related to https://www.openaire.eu/open-call-winner-phase-1-4science
Detailed documentation about the aims of the project, the implementation and the configuration options is available at https://4science.github.io/oaire-eld/#/publication-claim