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

Extending Concept Set Items with Annotations #2403

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

alex-odysseus
Copy link
Contributor

@alex-odysseus alex-odysseus commented Oct 29, 2024

Addressing #2318

A new Concept Set Annotation entity has been added being associated with a Concept Set to which the Concept Set Annotations relate to

When Concept Set is copied its Concept Set Annotations are preserved so that it will be known that some of the Concept Set Annotations were originated from a particular Concept Set of a specific Version

When a Concept Set deleted it is deleted together with the Concept Set Annotations

Concept Set Annotations can be deleted by the Administrators only or by those who have a specific permission assigned

annotationDetailsDTO.setSearchData(newAnnotationData.getSearchData());
conceptSetAnnotation.setAnnotationDetails(mapper.writeValueAsString(annotationDetailsDTO));
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's add a log entry

}
}
}
private ConceptSetAnnotation copyAnnotation(ConceptSetAnnotation sourceConceptSetAnnotation, int sourceConceptSetId, int targetConceptSetId){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please move it down below the public method where it is used

try {
annotationDetails = mapper.readValue(conceptSetAnnotation.getAnnotationDetails(), AnnotationDetailsDTO.class);
} catch (JsonProcessingException e) {
throw new RuntimeException(e);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A log entry should be added additionally

@PUT
@Path("/update/{id}/annotation")
@Produces(MediaType.APPLICATION_JSON)
public AnnotationDTO updateConceptSetAnnotation(@PathParam("id") final int id, AnnotationDTO annotationDTO) throws IOException {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method is obsolete at this moment, if in the future it will be necessary to add/update a comment to a specific Concept Set Annotation it might be added

@@ -0,0 +1,46 @@
CREATE SEQUENCE ${ohdsiSchema}.concept_set_annotation_sequence;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's combine one migration script

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, let's combine into 1 script and also this is targeting v2.15 so if we do create 1 migrations cript let's just make a new one with all the others combined, call it V2.15 ...

@@ -1786,7 +1791,8 @@
"includedConcepts": "Included Concepts",
"includedSourceCodes": "Included Source Codes",
"versions": "Versions",
"messages": "Messages"
"messages": "Messages",
"metadata": "Metadata"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be removed

@@ -36,7 +37,7 @@
*
* @author fdefalco
*/
@Entity(name = "ConceptSet")
@Entity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please revert this as we have a convention to name the entity, you can see this in CohortDefinition, Source, etc.

@@ -21,7 +21,7 @@
* @author Anthony Sena <https://github.com/ohdsi>
*/

@Entity(name = "ConceptSetGenerationInfo")
@Entity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above, please revert this change.

@@ -29,7 +29,7 @@
* @author fdefalco
*/

@Entity(name = "ConceptSetItem")
@Entity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as above, we name Entities.

import javax.persistence.Table;
import java.io.Serializable;

@Entity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add name here.

Comment on lines +11 to +20
private static Map<String, String> writePermissions = new HashMap<String, String>() {
{
put("conceptset:annotation:%s:delete", "Delete Concept Set Annotation with ID %s");
}
};

private static Map<String, String> readPermissions = new HashMap<String, String>() {
{
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is odd to me:

Shouldn't the writePermissions contain all permissions related to creating annotations and the read have the ones related to reading annotations?

Comment on lines +32 to +33
import org.apache.commons.collections4.CollectionUtils;
import org.apache.commons.lang3.StringUtils;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just might want to check this import (collections4 is something I'm not familiar with). is there an existing package we depend on that has the function in CollectionUtils that you are using?

Just trying to keep extra dependencies down....when we get to the 3.x line we're going to be combing through the code locating redundant dependcies (ie: libraries that support string manipulation will be de-duplicated) So if we can remove a source of additional duplication, we should try to do it here.

@@ -0,0 +1 @@
ALTER TABLE ${ohdsiSchema}.concept_set_annotation ADD concept_set_version VARCHAR;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we sure VARCHAR is the correct type here? That will allow 2GB of text

@chrisknoll
Copy link
Collaborator

Hi, Everyone. I added comments and if you could address those I will work on pulling the branch down to test.

Specifically, the migration scripts being combined and re-labeled to 2.15 is soemthing I'd like to have changed before pulling it down, because otherwise I'd have to manually roll back the stuff in the 2.14 migrations and then let them re apply from the 2.15 migration, and it gets to be a headache. Once that's done I can play-test the functionality locally and provide feedback.

@anthonysena anthonysena linked an issue Nov 5, 2024 that may be closed by this pull request
Reverting back entity names in @entity according to the codebase policy
@chrisknoll
Copy link
Collaborator

Thanks for addressing those requested changes. I'll pull it down and take it for a spin.

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.

Add annotations to Concept Set Items
4 participants