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

Closes #2554: Generalize Suggestion box to support controlled vocabulary #2555

Conversation

dkorbel
Copy link

@dkorbel dkorbel commented Sep 23, 2024

Extended functionality for the Suggestion input to support dictionaries (with a controlled vocabulary).
Adds optional validator to verify is value coming from controlled vocabulary or not.

Issue: #2554

@dkorbel dkorbel force-pushed the dkorbel-2554-suggestion-box-support-for-controlled-vocabulary-values branch from 357ae52 to be984a2 Compare September 23, 2024 06:11
@dkorbel dkorbel self-assigned this Sep 23, 2024
@dkorbel dkorbel requested review from sniewczas, diasf and madryk and removed request for diasf and madryk September 23, 2024 06:16
@dkorbel dkorbel requested review from sniewczas and removed request for sniewczas September 23, 2024 16:05
@dkorbel dkorbel force-pushed the dkorbel-2554-suggestion-box-support-for-controlled-vocabulary-values branch 2 times, most recently from 73247e2 to 99ec619 Compare September 23, 2024 16:20
@dkorbel dkorbel requested review from diasf and removed request for madryk September 24, 2024 07:26
@dkorbel dkorbel force-pushed the dkorbel-2554-suggestion-box-support-for-controlled-vocabulary-values branch from 99ec619 to f3f5931 Compare September 24, 2024 11:44
Copy link

@diasf diasf left a comment

Choose a reason for hiding this comment

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

In general looks fine, just address the comment in the README, please.

README.md Outdated
Starting integration tests dependencies:

```bash
./mvnw docker:start -Dtest.solr.port=8984 -pl dataverse-webapp
Copy link

Choose a reason for hiding this comment

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

It might be a bit misleading to list those commands in that order, as you don't need to start/stop the dependencies when you want to execute all the tests or a single test on the command line. The verify command will start the dependencies on it's own.
The start/stop commands are only useful when you want to execute some tests through the IDE during development.

Maybe a section with something similar to this:

Integration test dependencies can be started manually in order to execute integration tests through the IDE:

./mvnw docker:start -Dtest.solr.port=8984 -pl dataverse-webapp

Once started, all the integration tests can be run through the IDE. When finished, containers can be stopped with:

./mvnw docker:stop -pl dataverse-webapp

Copy link
Author

Choose a reason for hiding this comment

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

Improved

.findFirst()
.map(entry -> {
Map<String, String> filterValues = new HashMap<>();
// Assuming getFieldValue() returns an Optional or similar
Copy link

Choose a reason for hiding this comment

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

Is this a leftover after refactoring or something?

Copy link
Author

Choose a reason for hiding this comment

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

Removed

Map<String, String> filterValues = new HashMap<>();
// Assuming getFieldValue() returns an Optional or similar
filterValues.put(entry.getValue(), entry.getKey());
return filterValues;
Copy link

Choose a reason for hiding this comment

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

To me the name getFilterValue seems a bit misleading, but tbh not sure what would be a better one.
The main issue is that the contract between isDependentOnSiblings, getAllowedFilters and this method is not very obvious.
For instance, I wonder if it wouldn't be clearer to have getAllowedFilters return a list of FilterConfig objects. Those filter objects would tell how a value for that filter can be retrieved, either from dataset fields or from db filter configuration.

But I feel that would require a bigger refactoring

Copy link
Author

Choose a reason for hiding this comment

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

I've added improved comment.

…ary values

Co-authored-by: Sylwester Niewczas <[email protected]>
Co-authored-by: Krzysztof Mądry <[email protected]>
Co-authored-by: Filipe Dias Lewandowski
Signed-off-by: Daniel Korbel <[email protected]>
@dkorbel dkorbel force-pushed the dkorbel-2554-suggestion-box-support-for-controlled-vocabulary-values branch from f3f5931 to 8b1ade7 Compare September 25, 2024 05:11
@diasf diasf merged commit 2eb0ad6 into develop Sep 25, 2024
1 check passed
@diasf diasf deleted the dkorbel-2554-suggestion-box-support-for-controlled-vocabulary-values branch September 25, 2024 07:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants