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

Added new static facet to show metadata blocks with values #8793

Merged

Conversation

abujeda
Copy link
Contributor

@abujeda abujeda commented Jun 10, 2022

What this PR does / why we need it:
New configurable facet to improve dataset discoverability.
This change creates a new facet called Dataset Feature. This facet will be populated with the name of the metadata blocks for which a value has been set in one of their fields.

By default, the UI will not show this new facet. It needs to be configured to show up in the search side bar.
It is configured by dataverse and the changes is inherited. The configuration tells the UI what metadata blocks we want to show. See screenshots in the issue #8536

Which issue(s) this PR closes:
Closes #8536

Special notes for your reviewer:

Suggestions on how to test this:
Read comments from Gustavo on new requirements for the API behaviour.

This new facet needs to be configured for it to appear in the search side panel.
For example, to display Geospatial Metadata in the root dataverse (and its children), use:

First, set the metadata block facet root to true:
curl http://localhost:8080/api/dataverses/root/metadatablockfacets/isRoot -X POST -d 'true' -H "X-Dataverse-key: API_KEY" -H "Content-type: application/json"

Second, set the blocks:
curl http://localhost:8080/api/dataverses/root/metadatablockfacets -X POST -d '["geospatial"]' -H "X-Dataverse-key: API_KEY" -H "Content-type: application/json"

To display nothing in a child dataverse when the parent has been configured to show one:
curl http://localhost:8080/api/dataverses/<dataverseId>/metadatablockfacets -X POST -d '[]' -H "X-Dataverse-key: API_KEY" -H "Content-type: application/json"

The payload is an array with names of the metadata blocks that you want to display or empty.

To disable this feature:
curl http://localhost:8080/api/dataverses/root/metadatablockfacets -X DELETE -H "X-Dataverse-key: API_KEY"
curl http://localhost:8080/api/dataverses/root/metadatablockfacets/isRoot -X POST -d 'false' -H "X-Dataverse-key: API_KEY" -H "Content-type: application/json"

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No interface change. It just add a new facet called by default "Dataset Feature"

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

Additional documentation:
New documentation added to the guide in this PR

return metadataBlockFacetRoot;
}

public void setMetadataBlockFacetRoot(boolean metadataBlockFacetRoot) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be set to false when you create a new Dataverse? Also for the root dataverse do we need to set it to true - since it doesn't have an owner? And it seems as if the only way to get a child dataverse to not inherit facets from its parent would be to give it its own facets via the api, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be false by default. Because it is a boolean primitive, the default value is false. I was following the same pattern as with metadataBlockRoot and facetRoot.

As for the root dataverse, technically, there is no need to set it to true because of the way getMetadataBlockFacetRoot was coded (checking for the owner). But if metadataBlockRoot and others are set to true in the root dataverse, then I guess it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sekmiller are you happy with the explanation?
Do you want me to change anything regarding metadataBlockFacetRoot?

Copy link
Contributor

Choose a reason for hiding this comment

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

@adaybujeda - yes, sorry I was away for a few days. I just want to do another pass through the code, but I will probably send it along to QA shortly.

Copy link
Contributor

@sekmiller sekmiller left a comment

Choose a reason for hiding this comment

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

Looks good. thanks for the contribution.

@sekmiller
Copy link
Contributor

@adaybujeda Please update this branch with the latest from dev. Thanks!

@abujeda abujeda force-pushed the 8535-new-static-facet-show-metadata-blocks branch from e28098a to 9ce0e5d Compare July 20, 2022 13:19
@abujeda
Copy link
Contributor Author

abujeda commented Jul 20, 2022

I have rebased from the latest from development. Ready to go 🚀

Thanks @sekmiller

@sekmiller sekmiller removed their assignment Jul 20, 2022
@coveralls
Copy link

coveralls commented Jul 20, 2022

Coverage Status

Coverage increased (+0.3%) to 20.048% when pulling 34f64cd on adaybujeda:8535-new-static-facet-show-metadata-blocks into 2c67e22 on IQSS:develop.

@kcondon kcondon self-assigned this Jul 22, 2022
@kcondon
Copy link
Contributor

kcondon commented Jul 22, 2022

@adaybujeda Hi, I'm getting a flyway error when attempting to deploy this. Can you refresh from develop once more?

OK, it looks like the issue is that the flyway migration script you'd originally checked in has a v5.10.1 prefix in the name, named V5.10.1.4__8536-metadata-block-facet.sql, and since we are now on v5.11 and there is already a script applied and merged using prefix 5.11.0.1, likely yours would need to be 5.11.0.2, unless for some reason another script gets merged ahead of you.

@abujeda abujeda force-pushed the 8535-new-static-facet-show-metadata-blocks branch from 9ce0e5d to 37f7b46 Compare July 22, 2022 22:18
@abujeda
Copy link
Contributor Author

abujeda commented Jul 22, 2022

Thanks @kcondon.

I have rebased from develop and renamed the DB update script to V5.11.0.2__8536-metadata-block-facet.sql

I hope it deploys now.

@kcondon
Copy link
Contributor

kcondon commented Jul 25, 2022

@adaybujeda Thanks, it deploys now. Would you mind providing an example metadata_blocks.json file? I see the example values sentence and I recognize that mirrors the sentence from the preexisting facets api but it would be useful to have actual example files (for both) but you are only responsible for your api :)

Issues found:

  1. Please add an example metadata_block.json file to the doc.
  2. Attempting to add metadata facet to root dv with non root admin fails with permissions error but still enables feature and adds facet to root.
  3. There doesn't seem to be a way to disable the feature, once enabled? Using -X DELETE fails with HTTP 405 and setting a blank metadatablock entry fails with invalid block name.

@abujeda
Copy link
Contributor Author

abujeda commented Jul 25, 2022

Thanks @kcondon. Looking into the issues.

@abujeda
Copy link
Contributor Author

abujeda commented Jul 26, 2022

1 . Please add an example metadata_block.json file to the doc.

I have update the documentation to add the json files with the examples. I have updated the references to the files to be links to download the files. I saw several examples that looked similar. I have updated the facets API too.

Screenshot 2022-07-26 at 12 42 28

2 . Attempting to add metadata facet to root dv with non root admin fails with permissions error but still enables feature and adds facet to root.

I have added an explicit command to update the block facets. The problem before is that I was making an update to the dataverse object in the controller and the JPA library was automatically updating he DB regardless of the command errors.
With the new commands, the updates will only be done after the user verification process.

There doesn't seem to be a way to disable the feature, once enabled? Using -X DELETE fails with HTTP 405 and setting a blank metadatablock entry fails with invalid block name.

Yes, I missed a delete function to remove this feature from a dataverse. This is now implemented. I have updated the PR description with more info and updated the docs

@kcondon This is ready for review again

@abujeda abujeda force-pushed the 8535-new-static-facet-show-metadata-blocks branch 2 times, most recently from e464fbb to f88ae8f Compare July 26, 2022 12:04

@Override
public Dataverse execute(CommandContext ctxt) throws CommandException {
editedDv.setMetadataBlockFacetRoot(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

will this leave "orphan" records on the DataverseMetadataBlockFacet table?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really.
The next line: editedDv.setMetadataBlockFacets(Collections.emptyList()); will clear the records in the DataverseMetadataBlockFacet table.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, I just looked at Dataverse.java and I see the cascade remove and orphan removal = true. Thanks!

@kcondon kcondon assigned scolapasta and unassigned kcondon Jul 26, 2022
@kcondon
Copy link
Contributor

kcondon commented Jul 26, 2022

@adaybujeda Thanks for working on this. I have tested it and the issues I've reported are addressed. Passing back to review as Gustavo wanted to take a look.

@scolapasta scolapasta added HDC: 2 Harvard Data Commons Obj. 2 HDC Harvard Data Commons labels Aug 1, 2022
@kcondon kcondon self-assigned this Aug 18, 2022
@kcondon
Copy link
Contributor

kcondon commented Aug 18, 2022

Hi @adaybujeda thanks for all the examples in how to test. I am running into a minor issue with the first one: curl http://localhost:8080/api/dataverses/root/metadatablockfacets/isRoot -X POST -d 'true' -H "X-Dataverse-key: "
{"status":"ERROR","code":415,"message":"HTTP 415 Unsupported Media Type"}

@abujeda
Copy link
Contributor Author

abujeda commented Aug 18, 2022

Thanks @kcondon,

With the changes to the API I forgot to add the Content-Type to the example in the description.
The curl command should be:
curl http://localhost:8080/api/dataverses/root/metadatablockfacets/isRoot -X POST -d 'true' -H "X-Dataverse-key: API_KEY" -H "Content-type: application/json"

I have fixed the command in the description

@abujeda
Copy link
Contributor Author

abujeda commented Aug 18, 2022

I have updated the API docs with the missing Content-Type header

@kcondon
Copy link
Contributor

kcondon commented Aug 18, 2022

@adaybujeda So far, so good but when I try to disable a child collection from using metadatablock facets using the example, it complains: curl http://localhost:8080/api/dataverses/kc081822b/metadatablockfacets -X POST -d '[]' -H "X-Dataverse-key: " -H "Content-type: application/json"
{"status":"ERROR","message":"Dataverse: kc081822b must have metadata block facet root set to true"}

and the disable metadatablocks example also needs mediatype:
curl http://localhost:8080/api/dataverses/root/metadatablockfacets/isRoot -X POST -d 'false' -H "X-Dataverse-key: "
{"status":"ERROR","code":415,"message":"HTTP 415 Unsupported Media Type"}

I think maybe there are two, related features: setting isroot=true first for the child dv, allowing it to have a different set of facets, then setting the facets to blank to effectively have none?

@abujeda
Copy link
Contributor Author

abujeda commented Aug 18, 2022

This is part of the new requirements set by @scolapasta. In order to disable a child collection from using metadatablock facets from its parent, you need to first set the facet root in that collection to true.

@abujeda
Copy link
Contributor Author

abujeda commented Aug 18, 2022

Yes, sorry. Updated the description for the disable curl example. Added the Content-Type header

@kcondon
Copy link
Contributor

kcondon commented Aug 18, 2022

@adaybujeda Ok, that's fine, I was able to figure it out. The code worked as expected but maybe there is a slight issue with the explanation in the doc.

So first, before a child can set the facet list, if by definition a parent has it enabled, it must first set isroot=true, meaning it becomes the root or source for facets and does not inherit. The doc has it reversed: "When updating the root to false, it will clear any metadata block facets from the dataverse. When updating to true, it will copy the metadata block facets from the parent dataverse." Also, a minor but confusing point, under setting the facets section, you do helpfully mention setting it to an empty array to remove facets inherited by a parent but to do so you must first set isroot=true for the child collection. And a consistency/changed thing is we now refer to dataverses as collections for copyright reasons, unless it is the Dataverse software. There are references to dataverse in both the set and isroot api sections. I can check in with Gustavo to confirm but I believe that is true, though I mentally still think of them as dataverses.

Thanks for your patience and all the changes. Looking good!

@abujeda
Copy link
Contributor Author

abujeda commented Aug 18, 2022

@kcondon both statements are true.
Before a dataverse can set its own block facets, it must set its facet root to true. Regardless of the state of the parent.

When setting the facet root:
if it changes from true to false, it will clear the block facets (if any).
if it changes from false to true, it will copy the blocks from the first parent that is root (if any)
if it doesn't change, nothing is saved/copied

I will update the docs to refer to them as collections and clarify how to use the API

@kcondon
Copy link
Contributor

kcondon commented Aug 18, 2022

@adaybujeda OK, I think I get what you are saying and it mirrors the UI: when you inherit, you don't have a local copy of the list, just query the parent list, right? So setting isroot=false (I'm inheriting) clears the local facet list? When you become root, isroot=true, you stop inheriting but as a convenience in the UI and so here, I imagine, you make a local copy of the parent facet list so you can modify it?

@abujeda abujeda force-pushed the 8535-new-static-facet-show-metadata-blocks branch from 78c4080 to 34f64cd Compare August 18, 2022 16:38
@abujeda
Copy link
Contributor Author

abujeda commented Aug 18, 2022

Yes @kcondon, that is the implemented behaviour as specified by Gustavo.

I have updated the documentation:

  • Changed the references from dataverse to collection
  • Clarify that the root needs to be set to true before setting/clearing any blocks.

@kcondon
Copy link
Contributor

kcondon commented Aug 18, 2022

@adaybujeda Ok, that makes sense now. I am having trouble setting the metadatadata block facet for a child isroot=true. The command returns without error or status and doesn't change the facets and there is a server.log error:
chg_facet_err.txt Btw, I have a 1pm meeting, will check back.

@abujeda
Copy link
Contributor Author

abujeda commented Aug 18, 2022

Thanks @kcondon, could you paste the actual curl request to see If I can reproduce locally.
Not quite sure the error is related with reading the request or rendering the response.

@abujeda
Copy link
Contributor Author

abujeda commented Aug 18, 2022

It looks like the problem is with the payload of the request coming to Dataverse.
I was able to reproduce the error message by sending an invalid array payload.

Are the block names quoted inside the array?

@kcondon
Copy link
Contributor

kcondon commented Aug 18, 2022

I see, user error! Should have quotes. Apologies, the original example did have quotes.

curl http://localhost:8080/api/dataverses/kc081822b/metadatablockfacets -X POST -d '[socialscience]' -H "X-Dataverse-key: " -H "Content-type: application/json"

@kcondon
Copy link
Contributor

kcondon commented Aug 18, 2022

@adaybujeda OK, everything tested ok, thanks. I am seeing an integration test failure though. This should be the last thing before merging. If you have questions about the test, @scolapasta should be able to help. https://jenkins.dataverse.org/blue/organizations/jenkins/IQSS-Dataverse-Develop-PR/detail/PR-8793/15/tests
Screen Shot 2022-08-18 at 2 33 17 PM
Screen Shot 2022-08-18 at 2 33 36 PM

@abujeda
Copy link
Contributor Author

abujeda commented Aug 18, 2022

I am able to run this test locally and it runs successfully (I have tried 3 times).
Could this be a transient error?
Could you run it again?
@kcondon @scolapasta ^^

@kcondon
Copy link
Contributor

kcondon commented Aug 18, 2022

@adaybujeda OK, will try it again. The suite takes 42 mins to run.

@abujeda
Copy link
Contributor Author

abujeda commented Aug 18, 2022

Just for the record:

Screenshot 2022-08-18 at 20 04 38

@kcondon kcondon merged commit 6ca1c9d into IQSS:develop Aug 18, 2022
@kcondon
Copy link
Contributor

kcondon commented Aug 18, 2022

@adaybujeda Thanks for all the effort.

@mreekie mreekie added the NIH OTA: 1.3.1 3 | 1.3.1 | Support software metadata | 5 prdOwnThis is an item synched from the product planning... label Dec 15, 2022
@mreekie mreekie added pm.GREI-d-1.3.1 NIH, yr1, aim3, task1: Support software metadata pm.GREI-d-1.3.2 NIH, yr1, aim3, task2: R & D phase biomedical workflows support labels Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
HDC Harvard Data Commons HDC: 2 Harvard Data Commons Obj. 2 NIH OTA: 1.3.1 3 | 1.3.1 | Support software metadata | 5 prdOwnThis is an item synched from the product planning... pm.GREI-d-1.3.1 NIH, yr1, aim3, task1: Support software metadata pm.GREI-d-1.3.2 NIH, yr1, aim3, task2: R & D phase biomedical workflows support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request/Idea: Add new static facet to show the metadata blocks types that are populated.
7 participants