TRUNK-4144 #546
TRUNK-4144 #546
Conversation
|
Can you also add some unit tests? |
| * @param conceptSource | ||
| * @param withAnyOfTheseTypes | ||
| * @param includeRetired | ||
| * @since 1.10.x |
|
No units tests? See https://wiki.openmrs.org/display/docs/Generate+Test+Case+Plugin |
| /** | ||
| * @see org.openmrs.api.ConceptService#getDrugByMapping(String, ConceptSource, Collection, boolean) | ||
| */ | ||
| public Drug getDrugByMapping(String code, ConceptSource conceptSource, Collection<ConceptMapType> withAnyOfTheseTypesOrOrderOfPreference, boolean includeRetired) throws DAOException; |
There was a problem hiding this comment.
I think you should typically have one DAO method that returns multiple and then let the code in the ConceptServiceImpl filter out and return a single drug
There was a problem hiding this comment.
Having a single DAO method will avoid code duplication in both methods
*Include only one method in the dao *Filter the other method within the serviceimpl
| //get only the ones with these types by looping through the collection of conceptMapTypes | ||
| for(ConceptMapType conceptMapType : withAnyOfTheseTypes) { | ||
| if(conceptMapType != null) { | ||
| searchCriteria.add(Restrictions.eq("conceptMapType", conceptMapType)); |
There was a problem hiding this comment.
I don't think this is correct, can you please add unit tests to show that this code is correct? I.e. you can find a drug given a code, source and map types
| return null; | ||
| } | ||
|
|
||
| private Criteria createSearchDrugByMappingCriteria(String code, ConceptSource conceptSource, Collection<ConceptMapType> withAnyOfTheseTypesOrOrderOfPreference, boolean includeRetired) { |
There was a problem hiding this comment.
The withAnyOfTheseTypesOrOrderOfPreference parameter is never used in this method, probably you should remove it
| searchCriteria.createAlias("drugReferenceMaps", "map"); | ||
| // join to the conceptReferenceTerm table | ||
| searchCriteria.createAlias("map.conceptReferenceTerm", "term"); | ||
| // match the source code to the passed code |
There was a problem hiding this comment.
you need to check that code is not null
| @@ -1193,12 +1197,6 @@ public void changeConceptFromBooleanToCoded_shouldConvertTheDatatypeOfABooleanCo | |||
| /** | |||
| * @see {@link ConceptService#getConcept(Integer)} | |||
| */ | |||
| @Test(expected = APIException.class) | |||
There was a problem hiding this comment.
Why did you remove this test, i will add it back
|
Merged at 08c9bc3 |
Initial work done on TRUNK-4144