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.

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

Extend 'metadatablocks/{block_id}' endpoint JSON output #9091

Closed
wants to merge 2 commits into from

Conversation

JR-1991
Copy link
Contributor

@JR-1991 JR-1991 commented Oct 21, 2022

What this PR does / why we need it:

This PR extends the API endpoint /api/metadatablocks/{block_id} to include the following fields:

  • multiple: Whether or not multiple instances of this field can exist
  • isControlledVocabulary: Whether or not this field has a controlled vocabulary. Important to derive the typeClass later on
  • controlledVocabularyValues - If a vocabulary is given, includes all possible values.

With this extension it is possible to build compatible Dataverse JSON files for upload and download from any installation without having access to the TSV files. EasyDataverse would greatly benefit from this and allow users to "connect" to a Dataverse installation without the need of generating specific code.

Which issue(s) this PR closes:

Closes #8944

Special notes for your reviewer:

None

Suggestions on how to test this:

Add or extend a fixture to test if the new fields are given upon execution.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:

No.

Is there a release notes update needed for this change?:

The API endpoint /api/metadatablocks/{block_id} has been extended to include the following fields:

  • multiple: Whether or not multiple instances of this field can exist
  • isControlledVocabulary: Whether or not this field has a controlled vocabulary. Important to derive the typeClass later on
  • controlledVocabularyValues - If a vocabulary is given, includes all possible values.

Additional documentation:

None

@coveralls
Copy link

Coverage Status

Coverage increased (+0.001%) to 19.976% when pulling 6e37e0e on JR-1991:develop into cbc0e52 on IQSS:develop.

@mreekie mreekie added the bk2211 label Nov 1, 2022
Copy link
Member

@qqmyers qqmyers left a comment

Choose a reason for hiding this comment

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

Thanks! This should be useful for api users. Seems straight forward. The one technical comment that I have is that sending all of the controlledvocab values with every call is not very scalable, versus having a separate api call for that, but I'm happy to push that to 'future work' - we don't have any internal vocabs that are so large this is going to be a big issue.

One more general comment - it would be good to check the docs to see if there is somewhere the format here is described, i.e. in the api section, and to update that appropriately. With that, this seems OK to move forward.

@JR-1991
Copy link
Contributor Author

JR-1991 commented Nov 4, 2022

Thanks @qqmyers for the feedback! I see including the vocab values might not scale well for big sets. Would it be an option to get these only when specifically requested by a "complete" endpoint? Having all vocab values where the fields are would be very helpful and could reduce additional code to get them back together.

Thought of something like:

/api/metadatablocks/{id}/complete

where the corresponding endpoint would be implemented like so

@Path("{identifier}/complete")
@GET
public Response getCompleteBlock( @PathParam("identifier") String idtf ) {
    MetadataBlock b = findMetadataBlock(idtf);

    return (b != null) ? ok(json(b, complete=true)) : notFound("Can't find metadata block '" + idtf + "'");
}

This would require the additional argument complete to the json-method to check upon inclusion of the vocab. Keeping the argument default at false would persist previous functionality without any changes to other methods using the json-method.

Regarding the documentation, I will revise it and add changes to accommodate for the additions from this PR 🙂

@qqmyers
Copy link
Member

qqmyers commented Nov 4, 2022

@JR-1991 - adding /complete seems reasonable to me. It's possible that someone will have a preference for a QueryParam but I'd suggest just going ahead - easy enough to change later if that's requested.

@poikilotherm poikilotherm marked this pull request as draft November 17, 2022 13:43
@poikilotherm
Copy link
Contributor

poikilotherm commented Nov 17, 2022

@JR-1991 forgive me being encroaching and converting this to a draft. Doing this to reflect the nature of this PR not being ready for valuation and being sent into the funnel. Trying to play with a better view of what is happening with community contributions and how to get them merged faster. See also https://github.com/orgs/IQSS/projects/36/views/1

@pdurbin
Copy link
Member

pdurbin commented Nov 18, 2022

@JR-1991 I haven't had a chance to add a review yet but once we figure out the code, we'll want to add docs, of course. And a release note snippet. We'll help when the time comes. Tests would be nice too but aren't absolutely required! 😄

@pdurbin pdurbin added the Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) label Nov 29, 2022
@pdurbin
Copy link
Member

pdurbin commented Nov 29, 2022

The code change is small and looks good. Should look closer at feedback from @qqmyers. Needs docs and a release note. Gave it a 33 as a size.

p.s. Tests are failing. Investigate.

@JR-1991
Copy link
Contributor Author

JR-1991 commented Nov 30, 2022

Please excuse my late reply, been on vacation until now. First of all, thanks a lot for your review. @pdurbin going to add release notes and docs asap. Shall I also commit the proposed changes from my previous comment?

@pdurbin pdurbin self-assigned this Dec 2, 2022
@pdurbin
Copy link
Member

pdurbin commented Dec 2, 2022

@JR-1991 can you please take a look at 9e9edd3 ?

I think it's more what we want though I'm not sure about non-English values. There's a method called cvv.getLocaleStrValue(language) we could try, perhaps.

Also, it's somewhat inconvenient when PRs come in with the branch name "develop". If you don't mind, it would be nice to have a fresh PR with a new name (8944-metadatablocks-api or something).

@qqmyers I'm not too worries about performance. It seems fast enough.

@JR-1991 I'm happy to chat about all this! It could be a zoom call or https://chat.dataverse.org . Thanks for the pull request! It's great stuff!

@mreekie
Copy link

mreekie commented Dec 6, 2022

Daily discussion

  • asked dev to make a new PR based on feedback.
  • waiting on dev response

@mreekie
Copy link

mreekie commented Dec 7, 2022

Daily:

  • Phil heard back from Jan that Jan will try to get back to this today.

@pdurbin
Copy link
Member

pdurbin commented Dec 8, 2022

Closing in favor of #9213.

@pdurbin pdurbin closed this Dec 8, 2022
@pdurbin pdurbin removed bk2211 Size: 30 A percentage of a sprint. 21 hours. (formerly size:33) labels Dec 8, 2022
@pdurbin pdurbin removed their assignment Feb 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

Extend 'metadatablocks' endpoint to include the 'multiple' field
6 participants