DSpace / RestContract

REST Contract for DSpace 7-8
https://wiki.lyrasis.org/display/DSDOC8x/
37 stars 48 forks source link

Embargo Item Metadata contract #175

Closed LucaGiamminonni closed 2 years ago

LucaGiamminonni commented 2 years ago

With this PR the contract for the Embargo item metadata is defined (https://github.com/DSpace/DSpace/issues/2839). Both the endpoints related to the configuration and those related to the new section of the submission form are defined.

tdonohue commented 2 years ago

@benbosman and @LucaGiamminonni : My assumption was that the existing https://github.com/DSpace/DSpace/blob/main/dspace/config/spring/api/access-conditions.xml would be adapted/updated to also support Item level access conditions (including embargo). So, when reviewing this endpoint, I was comparing it to that configuration file's structure overall.

That said, I agree with @benbosman that we likely should verify that we all have a common understanding of what the backend configuration is expected to look like. It's possible I had assumed a design that is different from what the 4Science team had anticipated.

tdonohue commented 2 years ago

@benbosman : After reviewing the code in https://github.com/DSpace/DSpace/pull/8091, I can answer your questions posed above:

submissionaccessoptions/<:option-name>: is the <:option-name> actually a submission form name, or what differentiates the options and how is it determined which option is available for your current submission?

No, the <:option-name> is the name of a new AccessConditionConfiguration object. There's a new object that's been introduced on the backend called an AccessConditionConfiguration which is a new configuration in access-conditions.xml to group together Options. So, for example, you can create different AccessConditionConfigurations for different submission configurations, allowing you to customize which Options (e.g. openAccess, lease, embarged, administrator) are available for those submission forms.

That <:option-name> can then be used in your item-submission.xml form for a new AccessConditionStep to define which options are available in that step.

I see you can set whether the item should be discoverable, but the config option seems to just indicate that it is discoverable, not that the discoverable state can be modified. Should the submissionaccessoptions use the parameter canChangeDiscoverable instead to make it easier to understand?

It appears that canChangeDiscoverable is a setting in the access-conditions.xml, defined for specific AccessConditionConfiguration objects. So, it's meant to configure whether, for this AccessConditionStep (in a specific submission form) the user is allowed to change whether the Item is "discoverable" (or not).

What kind of configuration can we expect for these options? I know that the configuration shouldn't be in the REST contract, but it' best to also discuss that before the implementation starts

Configuration updates are easier to see in the PR. See the changes to access-conditions.xml and item-submission.xml especially. The changes are fairly small, and seem to just allow you to customize the Embargo Item options available (in the AccessConditionStep for different submission forms.

@LucaGiamminonni and @abollini : Please correct my summary above if anything is incorrect.

benbosman commented 2 years ago

Thanks @tdonohue for answering my questions.

Based on your feedback, it sounds that submissionaccessoptions/<:config-name> will group the options available in a given configured AccessConditionStep. But the REST response for that AccessConditionStep is still missing in this contract.

Based on your feedback, it also sounds that the property has been renamed to canChangeDiscoverable in the code, but not in the contract

@abollini @LucaGiamminonni can the contract please be updated based on the feedback from Tim and me, it's important to have a correct contract so I can start reviewing the codebase

Micheleboychuk commented 2 years ago

Thanks @benbosman for feedback The answer for AccessConditionStep is this: https://github.com/DSpace/RestContract/pull/175/files#diff-4d5bb21f30c576d76528a9bc09f284be17a6b554102e07ada0dfc76d9d3af60fR6 in this case, it is right that it is called discoverable.

benbosman commented 2 years ago

Thanks for the updates.

It is indeed as Tim describes: There's a new section named the same as the [:config-name] as /api/config/submissionaccessoptions/[:config-name] That was the missing link.

I can't find that extra information Tim shared in the contract (but I may be overlooking it). Can you point me to the part that describes that the section:

          "itemAccessConditions": {
            "discoverable": true,
            "accessConditions": []
          }

implies you can find the potential accessConditions at /api/config/submissionaccessoptions/itemAccessConditions (the fact that you have to build the URL based on that config name)

tdonohue commented 2 years ago

@benbosman : The itemAccessConditions section you are looking for is described in this PR in the workspaceitem-data-access.md file (starting here). That shows the what the section response looks like (I verified it is correct in the implementation PR), and also how to use PATCH to add/remove/update which access conditions are selected in the workspace item.

I did just notice that workspaceitem-data-access.md doesn't have a note that this section will be named the same [:config-name] as in /api/config/submissionaccessoptions/[:config-name]. But, that appears to be the only small detail that is missing.

And perhaps either @LucaGiamminonni or @Micheleboychuk could quickly add a note at the top of that file which says:

"This section will have the same name as the configuration available off the /api/config/submissionaccessoptions/. As an example, a WorkspaceItem section here named itemAccessConditions would correspond to a configuration /api/config/submissionaccessoptions/itemAccessConditions"

Micheleboychuk commented 2 years ago

@tdonohue @benbosman restcontract updated.