[CST-5347] Researcher profile rest contract #180

LucaGiamminonni · 2022-02-22T10:30:14Z

These changes concern the introduction of a rest repository for researcher profiles. It is part of the migration issue of ORCID from cris (DSpace/DSpace#8157).

The researcher profile, modeled as an item of a specific type (such as Person), is associated with a specific ePerson. An ePerson can only have one profile.

tdonohue · 2022-03-02T23:25:06Z

@LucaGiamminonni : If possible, could you write up a high level summary of how a Profile is planned to be implemented? I tried to review this PR today, but I came away with a number of high level questions:

How is a Profile different from a Person Entity (e.g. https://demo7.dspace.org/entities/person/b1b2c768-bda1-448a-a073-fc541e8b24d9)? Currently, we already have the basics of a Researcher Profile represented in the Person Entity. It's unclear to me how this new endpoint is related to the existing Person Entity, and why we cannot use Person Entities to represent a user's Profile.
How would this Profile be represented in the database layer or Java API layer? Is the plan for it to be a separate table, or is it just a different way of representing the relationship between an EPerson object and a Person Entity?
I'd also prefer not to put this under /api/cris/, as I think that'd imply it's a separate object type specific to CRIS systems. It likely should be alongside the existing EPerson or Item endpoints. We might consider whether this would be more appropriate under /api/eperson/profiles or similar...but I might be able to provide a better suggestion once I understand the design.

Overall, I'm not against the general idea of having a Researcher Profile. But, I need to better understand the proposed high-level design here before I can accurately review this REST Contract. Currently, I see a large amount of overlap with Person Entities in this REST Contract -- and I'm wanting to better understand how these two are related.

LucaGiamminonni · 2022-03-03T10:12:38Z

Hi @tdonohue,

in cris the researcher profile is nothing more than a Person item (or an item with the configured type) which is linked to an eperson through authority with the cris.owner metadata. This specific endpoint allows you to do specific operations related to the researcher profile without using the more generic ItemRestRepository. The profile id coincides with the eperson owner's id (the search is done using the cris.owner metadata).
the profile model does not require additional tables, it is modeled with an item anyway.
yes I agree that we can change the path with /api/eperson/profiles or something similar

tdonohue

@LucaGiamminonni : Thanks for your answers to my questions. I've given this a more thorough review now based on the assumptions that this /profile endpoint is really just an easier way to determine which Person Entity (Item) is linked up with a given EPerson. Overall, the idea here is good, but I have a lot of suggested changes, and a few questions inline as well.

If necessary, we also could find time to discuss the design here in a future DevMtg. Just let me know if you feel that'd be appropriate.

endpoints.md

profiles.md

LucaGiamminonni · 2022-03-16T16:08:24Z

Hi @tdonohue, thanks for your feedbacks. I have updated the contract, let me know if it is ok for you now. Thanks again.

benbosman

Thanks for the new feature contract.

I've reviewed this contract, and added some change requests inline

benbosman · 2022-03-24T10:42:01Z

profiles.md

+
+Attributes:
+* id: the UUID of the EPerson who owns the profile. This also corresponds to the ID of the profile.
+* visible: indicates whether the profile is public (visible: true) or private (otherwise). If private only 


Can the visible property be dropped?

Permissions are handled by item resource policies, and having a boolean can make it behave strangely and hard to keep in sync (especially if the resource policies make it neither public or private, but a permission in between (e.g. staff group read)

I agree with @benbosman 's feedback here. I did also want to add that we do have a concept of a "discoverable" or "non-discoverable" Item as well which is also similar, but that's about whether the Item is indexed or not.

So, I agree this "visible" attribute is a bit confusing after looking at it again. It should likely either be replaced by "discoverable / non-discoverable" or resource policies...we shouldn't be building a different way to access restrict a profile.

The visible attribute indicates only whether the profile (ie the item that models it) is visible or not to anonymous users. Internally, the visibility of the item is already managed with the resource policies, and in fact this value is true if the Anonymous group has the permissions to read on the anonymous user and false if it does not. A patch in which the visibility is set to true therefore means to add the read policy to the anonymous group while setting the value to false means to remove it.

The posibility to change the profile visibility using this attribute is not strictly necessary for orcid, so if you are not convinced we can also not bring it to dspace. As you prefer.

@LucaGiamminonni : I'd like to hear @benbosman's thoughts on this as well, but one approach here could be to make it clearer what the "visible" property does. If it's just a quick way to add/remove an Anonymous READ resource policy, then we should say that in the description in this REST Contract. Currently, it implies it does more than that, as it says setting it to "false" makes it Administrator Only (But that's not entirely true, as removing the Anonymous READ policy just means it's no longer available anonymously, but other policies might specify a different permission level than Admin-only)

So, in my opinion...

Either, we need to fix this REST Contract description to say something like
visible: This property provides a quick way to add/remove the Anonymous READ policy. When set to "true", then the Anonymous READ Policy is added (and the profile becomes accessible to anonymous users). When set to "false", then the Anonymous READ policy is removed (and the profile is no longer accessible anonymously).

Or, we should just remove it entirely and require that you add/remove policies on the Item using the /api/authz/resourcepolicies endpoint.

I would prefer to remove it entirely, but I think the alternative from @tdonohue can work, assuming it's renamed to public and it only adjusts the Anonymous READ policy (while leaving the other policies unchanged)

I can change the description of the visible attribute but I prefer to keep visible instead of public, even cause "public" is a reserved word.

@LucaGiamminonni it's good you removed the wording which claims to know who has permissions if not visible.
If we'd have to keep this, I would recommend to further clarify this since this is a very atypical property. This can be done by stating the permissions are unknown if visible=false. It may be accessible to many users, or admins only, or also the EPerson itself (if they're granted READ permissions), …. The only thing that's known is whether there's an Anonymous READ policy without start/end date

benbosman · 2022-03-24T10:45:32Z

profiles.md

+ --data ${dspace-url}/api/core/items/cec6ee1b-7730-44da-a224-a7c5af63f821
+```
+
+## Hide/unhide a profile


Similar as mentioned above, permissions are handled by resource policies

benbosman · 2022-03-24T10:55:30Z

profiles.md

+  "name": null,
+  "handle": "123456789/511",
+  "metadata": {
+    "dspace.object.owner": [{


Will it be required to restrict edits to this field from all endpoints?
Wouldn't it also be better to use the UUID in the value, as the value can otherwise deviate from the EPerson name (if the latter is updated)

No for us it is not necessary to restrict the edit on this metadata. Regarding the value, it contains the name of the eperson and the authority contains the uuid of the eperson itself.

My questions here are primarily:

how will it handle issues with dspace.object.owner not being valid

how will it handle a change to the eperson's name, will this metadata value be updated as well. I see duplication of names which is hard to keep in sync

The dspace.object.owner can be set manually only with the administrative edit; this metadata can therefore be corrupted as can all the others. We don't think specific management is necessary.

I don't think it's a relevant problem (it is a common situation for all relations via authority). If we really want to manage a possible update we can modify the logic that changes the name of the eperson to also update a possible connected profile.

@LucaGiamminonni what I'm mainly pointing out here is that you're duplicating the name, and I don't see any reason for it. Unless you have a reason to duplicate the name, I would recommend to use the UUID in the value, which ensures there's no risk in the name being outdated

@benbosman the eperson name must be on the metadata because it could be shown. There cannot be the id which instead must be used to set the authority.

tdonohue

@LucaGiamminonni : Overall, this is looking better to me now. I do have a few more minor requests inline below. I think the contract itself is making sense though, and the remaining feedback that I have is all very minor in nature.

profiles.md

tdonohue

@LucaGiamminonni : I re-reviewed this after submitting my review of DSpace/DSpace#8232 (review) Now that I've seen the code, I think this Contract mostly makes sense. However, I do have a few inline comments below that I've added based on what I saw in the code itself.

endpoints.md

profiles.md

benbosman

I've updated the inline comments above with feedback on the last iteration

…ced 409 with 422

LucaGiamminonni · 2022-04-22T08:58:27Z

I changed the contract for:

move the position of the new endpoint in the endpoints.md
replace status 409 with 422
improve the description of the patch on visibility
I will also modify the rest implementation to change the behavior in case of conflict (returning 422 and not 409).

tdonohue

👍 Thanks @LucaGiamminonni ! All my prior feedback has been addressed. This looks good to me. However, obviously I think we should wait to merge this until we complete the review of the REST API PR, just in case minor changes there affect the contract.

abollini · 2022-05-05T14:39:40Z

Documentation about the ORCID functionalities is available in the DSpace-CRIS technical documentation pp 76-89 of pdf https://github.com/4Science/DSpace/releases/download/dspace-cris-2022.01.01/dspace-cris-2022.01.01.pdf
it can moved to the dspace wiki once the contribution is finalized.
It is almost accurate also for the porting with the exception of the features related to nested metadata that cannot be ported to DSpace as this concept is not yet supported in DSpace

benbosman

@LucaGiamminonni I've copied the feedback which was not handled yet

The remainder looks ok

benbosman · 2022-05-17T07:55:15Z

profiles.md

+
+Attributes:
+* id: the UUID of the EPerson who owns the profile. This also corresponds to the ID of the profile.
+* visible: this property provides a quick way to add/remove the Anonymous READ policy. When set to "true", then the Anonymous READ Policy is added (and the profile becomes accessible to anonymous users). When set to "false", then the Anonymous READ policy is removed (and the profile is no longer accessible anonymously).


It seems my previous feedback is no longer associated with this line, and not very visible anymore

I would recommend to further clarify this since this is a very atypical property. This can be done by stating the permissions are unknown if visible=false. It may be accessible to many users, or admins only, or also the EPerson itself (if they're granted READ permissions), …. The only thing that's known is whether there's an Anonymous READ policy without start/end date

@benbosman : To resolve this, maybe we add a bit of clarification on the end of the existing description?

So, for example, right after:
When set to "false", then the Anonymous READ policy is removed (and the profile is no longer accessible anonymously).
Maybe we clarify it with:
Keep in mind, this setting only impacts the Anonymous READ policy. So, if other custom READ policies exist on the profile, those policies will take effect when "visible=false".

tdonohue · 2022-06-09T15:04:16Z

@LucaGiamminonni : Could you add the one minor change to this REST Contract that was suggested above? #180 (comment)

This is ready to merge except for that minor change

LucaGiamminonni · 2022-06-09T15:19:52Z

Hi @tdonohue, I made the minor change.

tdonohue · 2022-06-09T15:31:04Z

Thanks @LucaGiamminonni ! This now looks good. I gave approval previously & with this change, @benbosman 's final feedback above has been addressed.

Merging this immediately. If we find other small changes are needed they can be done in a separate PR.

[CST-5347] Researcher profile rest contract

348c5f6

tdonohue assigned LucaGiamminonni Feb 22, 2022

tdonohue added this to the 7.3 milestone Feb 22, 2022

tdonohue self-requested a review February 24, 2022 15:49

tdonohue requested changes Mar 3, 2022

View reviewed changes

[CST-5347] Improved researcher profile rest contract

b5ac4ac

[CST-5347] Replaced cris with eperson on profile's url

80e9e51

tdonohue self-requested a review March 17, 2022 15:38

benbosman requested changes Mar 24, 2022

View reviewed changes

tdonohue reviewed Mar 25, 2022

View reviewed changes

profiles.md Outdated Show resolved Hide resolved

profiles.md Show resolved Hide resolved

profiles.md Show resolved Hide resolved

profiles.md Outdated Show resolved Hide resolved

[CST-5347] Improved profiles rest contract

d458def

tdonohue requested review from tdonohue and benbosman March 31, 2022 14:11

[CST-5347] Improved profile's visible attribute description

5c96e9c

LucaGiamminonni mentioned this pull request Apr 4, 2022

Researcher profile REST repository DSpace/DSpace#8232

Merged

5 tasks

tdonohue requested changes Apr 14, 2022

View reviewed changes

endpoints.md Outdated Show resolved Hide resolved

profiles.md Outdated Show resolved Hide resolved

profiles.md Outdated Show resolved Hide resolved

profiles.md Show resolved Hide resolved

benbosman requested changes Apr 21, 2022

View reviewed changes

LucaGiamminonni added 2 commits April 22, 2022 10:22

[CST-5347] Improved profile's visible attribute description and repla…

817da2b

…ced 409 with 422

[CST-5347] Replaced 409 with 422

2a79284

LucaGiamminonni requested review from tdonohue and benbosman April 22, 2022 13:56

LucaGiamminonni mentioned this pull request Apr 28, 2022

Update ORCID synchronization preferences #190

Merged

tdonohue approved these changes May 4, 2022

View reviewed changes

tdonohue added the high priority label May 5, 2022

benbosman requested changes May 17, 2022

View reviewed changes

[CST-5347] Minor improvement

92b0a72

tdonohue merged commit 95b2bfe into DSpace:main Jun 9, 2022

abollini mentioned this pull request Jan 25, 2023

Supervision order REST endpoints DSpace/DSpace#8621

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CST-5347] Researcher profile rest contract #180

[CST-5347] Researcher profile rest contract #180

LucaGiamminonni commented Feb 22, 2022

tdonohue commented Mar 2, 2022

LucaGiamminonni commented Mar 3, 2022

tdonohue left a comment •

edited

Loading

LucaGiamminonni commented Mar 16, 2022

benbosman left a comment

benbosman Mar 24, 2022

tdonohue Mar 24, 2022

LucaGiamminonni Mar 25, 2022

tdonohue Mar 25, 2022

benbosman Mar 31, 2022

LucaGiamminonni Apr 1, 2022

benbosman Apr 21, 2022

benbosman Mar 24, 2022

benbosman Mar 24, 2022

LucaGiamminonni Mar 25, 2022

benbosman Mar 31, 2022

LucaGiamminonni Apr 1, 2022

benbosman Apr 21, 2022

LucaGiamminonni Apr 21, 2022

tdonohue left a comment

tdonohue left a comment •

edited

Loading

benbosman left a comment

LucaGiamminonni commented Apr 22, 2022

tdonohue left a comment

abollini commented May 5, 2022

benbosman left a comment

benbosman May 17, 2022

tdonohue May 18, 2022

tdonohue commented Jun 9, 2022

LucaGiamminonni commented Jun 9, 2022

tdonohue commented Jun 9, 2022

[CST-5347] Researcher profile rest contract #180

[CST-5347] Researcher profile rest contract #180

Conversation

LucaGiamminonni commented Feb 22, 2022

tdonohue commented Mar 2, 2022

LucaGiamminonni commented Mar 3, 2022

tdonohue left a comment • edited Loading

Choose a reason for hiding this comment

LucaGiamminonni commented Mar 16, 2022

benbosman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdonohue left a comment

Choose a reason for hiding this comment

tdonohue left a comment • edited Loading

Choose a reason for hiding this comment

benbosman left a comment

Choose a reason for hiding this comment

LucaGiamminonni commented Apr 22, 2022

tdonohue left a comment

Choose a reason for hiding this comment

abollini commented May 5, 2022

benbosman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdonohue commented Jun 9, 2022

LucaGiamminonni commented Jun 9, 2022

tdonohue commented Jun 9, 2022

tdonohue left a comment •

edited

Loading

tdonohue left a comment •

edited

Loading