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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,31 @@ Dataverse is a trademark of President and Fellows of Harvard College and is regi
[chat.dataverse.org]: http://chat.dataverse.org
[Dataverse Community Meeting]: https://dataverse.org/events
[open source]: LICENSE.md


# Running integration tests

All tests:

```bash
./mvnw verify
```

Single test:

```bash
./mvnw verify -Dit.test=UserNotificationRepositoryIT -pl dataverse-persistence
```


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

```bash
./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:

```bash
./mvnw docker:stop -pl dataverse-webapp
```
Original file line number Diff line number Diff line change
Expand Up @@ -3108,4 +3108,5 @@ geobox.invalid.longitude=The longitude must be a number between -180 and 180.
geobox.invalid.latitude=The latitude must be a number between -90 and 90.
geobox.invalid.latitude.relation=The north latitude must be greater or equal to the south latitude.

validation.nonunique=The field must have a unique value.
validation.nonunique=The field must have a unique value.
validation.value.not.allowed.in.controlled.vocabulary=Value "{0}" is not allowed. Please select one from the list.
Original file line number Diff line number Diff line change
Expand Up @@ -3059,4 +3059,5 @@ geobox.invalid.longitude=D\u0142ugo\u015B\u0107 geograficzna musi zawiera\u0107
geobox.invalid.latitude=Szeroko\u015B\u0107 geograficzna musi zawiera\u0107 si\u0119 mi\u0119dzy -90 a 90.
geobox.invalid.latitude.relation=Szeroko\u015B\u0107 p\u00F3\u0142nocna musi by\u0107 nie mniejsza od po\u0142udniowej.

validation.nonunique=Pole musi mie\u0107 tylko jedn\u0105 warto\u015B\u0107.
validation.nonunique=Pole musi mie\u0107 tylko jedn\u0105 warto\u015B\u0107.
validation.value.not.allowed.in.controlled.vocabulary=Warto\u015B\u0107 "{0}" jest niedozwolona. Wybierz jedn\u0105 z listy.
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,16 @@ public List<ControlledVocabularyValue> findByDatasetFieldTypeId(Long dsftId) {
return query.getResultList();

}
public List<ControlledVocabularyValue> findByDatasetFieldTypeNameAndValueLike(String datasetFieldTypeName, String suggestionSourceFieldValue, int queryLimit) {

String queryString = "select DISTINCT v from ControlledVocabularyValue as v " +
"where UPPER(v.strValue) LIKE CONCAT('%', UPPER(:suggestionSourceFieldValue), '%') " +
"and v.datasetFieldType.id = (select d.id from DatasetFieldType as d where d.name = :datasetFieldTypeName)";
TypedQuery<ControlledVocabularyValue> query = em.createQuery(queryString, ControlledVocabularyValue.class);
query.setParameter("suggestionSourceFieldValue", suggestionSourceFieldValue);
query.setParameter("datasetFieldTypeName", datasetFieldTypeName);
query.setMaxResults(queryLimit);
query.setHint("eclipselink.QUERY_RESULTS_CACHE", "TRUE");
return query.getResultList();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,11 @@ public List<Suggestion> createSuggestions(DatasetField datasetField, String quer
Map<String, String> suggestionFilteredFields = new HashMap<>();

if (!datasetFieldTypeToSuggestionFilterMapping.isEmpty()) {

suggestionFilteredFields = findFilterValues(datasetField, datasetFieldTypeToSuggestionFilterMapping);
if(suggestionHandler.isDependentOnSiblings()) {
suggestionFilteredFields = findFilterValuesInSiblings(datasetField, datasetFieldTypeToSuggestionFilterMapping);
} else {
suggestionFilteredFields = getFilterValue(datasetField, datasetFieldTypeToSuggestionFilterMapping);
}

if (suggestionFilteredFields.isEmpty()){
return new ArrayList<>();
Expand All @@ -133,7 +136,15 @@ public List<Suggestion> createSuggestions(DatasetField datasetField, String quer

// -------------------- PRIVATE --------------------

private Map<String, String> findFilterValues(DatasetField datasetField, Map<String, String> datasetFieldTypeToSuggestionFilterMapping) {
/**
* In this scenario we are using indirectly value from datasetFieldTypeToSuggestionFilterMapping
* which comes from datasetField configuration.
* This value is used indirectly to filter out suggestions.
* This value is the name of other siblings datasetField in datasetField group,
* that shares common parent. Method loops over all siblings to find out
* the one that have corresponding name, then its value is used to filter out suggestions.
*/
private Map<String, String> findFilterValuesInSiblings(DatasetField datasetField, Map<String, String> datasetFieldTypeToSuggestionFilterMapping) {
Map<String, String> filteredValues = datasetField.getDatasetFieldParent()
.getOrElseThrow(() -> new NullPointerException("datasetfield with type: " + datasetField.getTypeName() + " didn't have any parent required to generate suggestions"))
.getDatasetFieldsChildren().stream()
Expand All @@ -146,4 +157,22 @@ private Map<String, String> findFilterValues(DatasetField datasetField, Map<Stri
return filteredValues.containsValue(StringUtils.EMPTY) ? new HashMap<>() : filteredValues;
}

/**
* In this scenario we are using directly value from datasetFieldTypeToSuggestionFilterMapping
* which comes from datasetField configuration.
* This value is used directly to filter out suggestions.
*/
private Map<String, String> getFilterValue(DatasetField datasetField, Map<String, String> datasetFieldTypeToSuggestionFilterMapping) {
return datasetFieldTypeToSuggestionFilterMapping
.entrySet()
.stream()
.filter(entry -> entry.getKey().equalsIgnoreCase(datasetField.getTypeName()))
.findFirst()
.map(entry -> {
Map<String, String> filterValues = new HashMap<>();
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.

})
.orElseGet(HashMap::new);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package edu.harvard.iq.dataverse.dataset.metadata.inputRenderer.suggestion;

import com.google.common.collect.Lists;
import edu.harvard.iq.dataverse.ControlledVocabularyValueServiceBean;
import edu.harvard.iq.dataverse.dataset.metadata.inputRenderer.Suggestion;

import javax.ejb.Stateless;
import javax.inject.Inject;
import java.util.List;
import java.util.Map;

import static java.util.stream.Collectors.toList;

/**
* This suggestion handler provides suggestions based on controlled vocabulary values.
*/
@Stateless
public class ControlledVocabularySuggestionHandler implements SuggestionHandler {

public static final String CONTROLLED_VOCABULARY_NAME_COLUMN = "controlledVocabularyName";
private ControlledVocabularyValueServiceBean controlledVocabularyValueServiceBean;

// -------------------- CONSTRUCTORS --------------------

@Deprecated
public ControlledVocabularySuggestionHandler() {
}

@Inject
public ControlledVocabularySuggestionHandler(ControlledVocabularyValueServiceBean controlledVocabularyValueServiceBean) {
this.controlledVocabularyValueServiceBean = controlledVocabularyValueServiceBean;
}

// -------------------- LOGIC --------------------

/**
* {@inheritDoc}
* <p>
* This implementation always returns class name.
*/
@Override
public String getName() {
return getClass().getSimpleName();
}


/**
* This is not dependent on siblings input values. All values will be taken
* to build matching suggestions list.
*/
@Override
public boolean isDependentOnSiblings() {
return false;
}

/**
* {@inheritDoc}
* <p>
* This implementation filers out base on controlled vocabulary (input) name,
* a.k.a. dictionary name
*/
@Override
public List<String> getAllowedFilters() {
return Lists.newArrayList(CONTROLLED_VOCABULARY_NAME_COLUMN);
}

@Override
public List<Suggestion> generateSuggestions(Map<String, String> filters, String suggestionSourceFieldValue) {
return controlledVocabularyValueServiceBean
.findByDatasetFieldTypeNameAndValueLike(filters.get(CONTROLLED_VOCABULARY_NAME_COLUMN), suggestionSourceFieldValue, 10)
.stream().map(vocabulary -> new Suggestion(vocabulary.getStrValue(), vocabulary.getIdentifier()))
.collect(toList());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,17 @@ public String getName() {
return getClass().getSimpleName();
}

/**
* This suggestion is dependent on sibling input value.
* Only values that match pointed out sibling input value will be taken
* to create suggestion.
* @see this#getAllowedFilters()
*/
@Override
public boolean isDependentOnSiblings() {
return !getAllowedFilters().isEmpty();
}

/**
* {@inheritDoc}
* <p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,15 @@ public String getName() {
return getClass().getSimpleName();
}

/**
* This suggestion is not dependent on siblings.
* All values will be taken to mach the suggestion string.
*/
@Override
public boolean isDependentOnSiblings() {
return !getAllowedFilters().isEmpty();
}

/**
* {@inheritDoc}
* <p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,10 @@
import com.google.common.collect.Lists;
import edu.harvard.iq.dataverse.dataset.metadata.inputRenderer.Suggestion;
import edu.harvard.iq.dataverse.persistence.dataset.suggestion.GrantSuggestion;
import edu.harvard.iq.dataverse.persistence.dataset.suggestion.GrantSuggestions;

import javax.ejb.Stateless;
import javax.inject.Inject;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

Expand Down Expand Up @@ -42,6 +40,17 @@ public String getName() {
return getClass().getSimpleName();
}

/**
* This suggestion is dependent on sibling input value.
* Only values that match pointed out sibling input value will be taken
* to create suggestion.
* @see this#getAllowedFilters()
*/
@Override
public boolean isDependentOnSiblings() {
return !getAllowedFilters().isEmpty();
}

/**
* {@inheritDoc}
* <p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,17 @@ public String getName() {
return getClass().getSimpleName();
}

/**
* This suggestion is dependent on sibling input value.
* Only values that match pointed out sibling input value will be taken
* to create suggestion.
* @see this#getAllowedFilters()
*/
@Override
public boolean isDependentOnSiblings() {
return !getAllowedFilters().isEmpty();
}

/**
* {@inheritDoc}
* <p>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,20 @@ public interface SuggestionHandler {
*/
String getName();

/**
* @return true if this handler is dependent on siblings input values.
sniewczas marked this conversation as resolved.
Show resolved Hide resolved
* If it is dependant it will use other input values
* to filter out while generating suggestions.
* Sibling means the group of inputs that shares common parent input.
* F.e.: when input grant is parent and have children as follow: grantAgency,
* grantRor, grantAcronym it means that whenever one of its children will have value
* it will be taken to filter out values during generating suggestions.
* Of course its need to be configured via <code>getAllowedFilters() method</code>
*
* @see this#getAllowedFilters()
*/
boolean isDependentOnSiblings();

/**
* Returns name of filters that this handler understands
* and can handle.
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package edu.harvard.iq.dataverse.validation.field.validators;

import edu.harvard.iq.dataverse.ControlledVocabularyValueServiceBean;
import edu.harvard.iq.dataverse.common.BundleUtil;
import edu.harvard.iq.dataverse.persistence.dataset.ControlledVocabularyValue;
import edu.harvard.iq.dataverse.persistence.dataset.ValidatableField;
import edu.harvard.iq.dataverse.validation.field.FieldValidationResult;
import org.omnifaces.cdi.Eager;

import javax.enterprise.context.ApplicationScoped;
import javax.inject.Inject;
import java.util.List;
import java.util.Map;

/**
* Validator for fields that should only contain values from a controlled vocabulary.
* The vocabulary is passed in through the parameters.
*/
@Eager
@ApplicationScoped
public class ControlledVocabularyValidator extends MultiValueValidatorBase {

@Inject
private ControlledVocabularyValueServiceBean vocabularyDao;

// -------------------- LOGIC --------------------

@Override
public String getName() {
return "controlled_vocabulary_validator";
}

@Override
public FieldValidationResult validateValue
(String value,
ValidatableField field,
Map<String, Object> params,
Map<String, ? extends List<? extends ValidatableField>> fieldIndex) {

List<ControlledVocabularyValue> matchesFromDb =
vocabularyDao.findByDatasetFieldTypeNameAndValueLike(
field.getDatasetFieldType().getName(), value, 1
);
if(matchesFromDb.size() == 1 && matchesFromDb.get(0).getStrValue().equalsIgnoreCase(value)) {
return FieldValidationResult.ok();
} else {
return FieldValidationResult.invalid(
field,
BundleUtil.getStringFromBundle("validation.value.not.allowed.in.controlled.vocabulary", value)
);
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,6 @@
* @see edu.harvard.iq.dataverse.persistence.dataset.ValidatableField
* @see edu.harvard.iq.dataverse.persistence.dataset.DatasetField
* @see edu.harvard.iq.dataverse.persistence.dataset.DatasetFieldType#getValidation()
*
* @author Filipe Dias Lewandowski, Krzysztof Mądry, Daniel Korbel, Sylwester Niewczas
*/
@Eager
@ApplicationScoped
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@
* @see edu.harvard.iq.dataverse.persistence.dataset.ValidatableField
* @see edu.harvard.iq.dataverse.persistence.dataset.DatasetField
* @see edu.harvard.iq.dataverse.persistence.dataset.DatasetFieldType#getValidation()
*
* @author Filipe Dias Lewandowski, Krzysztof Mądry, Daniel Korbel, Sylwester Niewczas
*/
@Eager
@ApplicationScoped
Expand Down
Loading
Loading