-
Notifications
You must be signed in to change notification settings - Fork 169
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
base: master
Are you sure you want to change the base?
Conversation
…tation, added vocabulary version and createdBy/createdDate
…eration, moved DB definition for the annotations feature into a single SQL file
Permitting ATLAS users to create Concept Set Annotations
…d search data JSON to a human friendly format in annotations tab
annotationDetailsDTO.setSearchData(newAnnotationData.getSearchData()); | ||
conceptSetAnnotation.setAnnotationDetails(mapper.writeValueAsString(annotationDetailsDTO)); | ||
} catch (JsonProcessingException e) { | ||
throw new RuntimeException(e); |
There was a problem hiding this comment.
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){ |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add name here.
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>() { | ||
{ | ||
} | ||
}; |
There was a problem hiding this comment.
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?
import org.apache.commons.collections4.CollectionUtils; | ||
import org.apache.commons.lang3.StringUtils; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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
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. |
Reverting back entity names in @entity according to the codebase policy
Thanks for addressing those requested changes. I'll pull it down and take it for a spin. |
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