-
Notifications
You must be signed in to change notification settings - Fork 44
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
ATT-47: Support for concept parameter when saving/fetching attachments. #53
base: master
Are you sure you want to change the base?
Conversation
public Obs saveImageAttachment(Visit visit, Person person, Encounter encounter, String fileCaption, | ||
MultipartFile multipartFile, String instructions) throws IOException { | ||
|
||
conceptComplex = context.getConceptComplex(ContentFamily.IMAGE); | ||
return saveImageAttachment(visit, person, encounter, null, fileCaption, multipartFile, instructions); |
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.
Why deprecate it? Just keep it and make sure it invokes the new signature as such:
conceptComplex = context.getConceptComplex(ContentFamily.IMAGE);
return saveImageAttachment(visit, person, encounter, conceptComplex, fileCaption, multipartFile, instructions);
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.
Resolved
@@ -207,7 +213,7 @@ public Obs saveImageAttachment(String imagePath, String mimeType) throws IOExcep | |||
mimeType, IOUtils.toByteArray(getClass().getClassLoader().getResourceAsStream(imagePath))); | |||
|
|||
String fileCaption = RandomStringUtils.randomAlphabetic(12); | |||
return obsSaver.saveImageAttachment(visit, patient, encounter, fileCaption, lastSavedMultipartImageFile, | |||
return obsSaver.saveImageAttachment(visit, patient, encounter, null, fileCaption, lastSavedMultipartImageFile, |
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.
Without the deprecation this change is not necessary.
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.
Resolved
* @param concept | ||
* @throws APIException if non-complex obs are mistakenly returned | ||
*/ | ||
List<Attachment> getAttachments(Patient patient, Concept concept); |
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 my main issue, the changes to AttachmentsService
are possibly much wider than thought here.
I believe this is where the existing API could be deprecated and a new one is to be introduced that caters for the new concept
argument.
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.
I believe this is where the existing API could be deprecated and a new one is to be introduced that caters for the new concept argument.
@mks-d Looks like some good time is required for this one; do you think its okay to handle this in a different/followup ticket?
public Obs saveComplexObs(Patient patient, Concept concept) throws IOException { | ||
Obs obs = new Obs(); | ||
byte[] randomData = new byte[20]; | ||
obs.setConcept(concept); | ||
obs.setObsDatetime(new Date()); | ||
obs.setPerson(patient); | ||
|
||
new Random().nextBytes(randomData); | ||
|
||
String filename = RandomStringUtils.randomAlphabetic(7) + ".ext"; | ||
MockMultipartFile multipartRandomFile = new MockMultipartFile(FilenameUtils.getBaseName(filename), filename, | ||
"application/octet-stream", randomData); | ||
obs.setComplexData( | ||
complexDataHelper.build(ValueComplex.INSTRUCTIONS_DEFAULT, multipartRandomFile.getOriginalFilename(), | ||
multipartRandomFile.getBytes(), multipartRandomFile.getContentType()).asComplexData()); | ||
|
||
return context.getObsService().saveObs(obs, null); | ||
} |
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.
Why didn't you make this parameterizable with a concept
argument?
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.
Updated
public Concept createComplexConcept(String uuid, String name, String handler, String description) { | ||
ConceptService conceptService = Context.getConceptService(); | ||
ConceptComplex conceptComplex = new ConceptComplex(); | ||
conceptComplex.setUuid(uuid); | ||
conceptComplex.setHandler(handler); | ||
ConceptName conceptName = new ConceptName(name, Locale.ENGLISH); | ||
conceptComplex.setFullySpecifiedName(conceptName); | ||
conceptComplex.setPreferredName(conceptName); | ||
conceptComplex.setConceptClass(conceptService.getConceptClassByName("Question")); | ||
conceptComplex.setDatatype(conceptService.getConceptDatatypeByUuid(ConceptDatatype.COMPLEX_UUID)); | ||
conceptComplex.addDescription(new ConceptDescription(description, Locale.ENGLISH)); | ||
|
||
return conceptService.saveConcept(conceptComplex); | ||
} |
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.
Isn't there any such concept in the standard test dataset, or one of Core's datasets?
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.
Not to my knowledge. The standard dataset has limited complex concepts.
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.
@samuelmale any chance you could look at this?
Concept concept = testHelper.createComplexConcept("f4fab86d-4a1d-4245-8aa2-19f49b5ab07a", "Random files", | ||
DefaultAttachmentHandler.class.getSimpleName(), "Random binary files"); |
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 detail but why is "files" plural?
ATT-47: Improved ComplexObsSaver and other APIs.
Ticket: https://issues.openmrs.org/browse/ATT-47