BAH-273 #27

ningosi · 2018-04-16T10:47:34Z

Intersection of lucene and hibernate search

…n screen)

…e as oppossed to activator

… BAH-273

…o `ANYWHERE`

…ogramAttribute

mks-d · 2018-04-17T09:54:27Z

bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/Activator.java


 public class Activator extends BaseModuleActivator {

 	private Log log = LogFactory.getLog(this.getClass());

 	@Override
 	public void started() {
+		AdministrationService service = Context.getAdministrationService();


Please checkout this file from upstream/master, I suspect it should not have been changed.

Checked upstream and corrected

mks-d · 2018-04-17T09:58:36Z

...-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/BahmniPatientServiceImplIT.java

+public class BahmniPatientServiceImplIT extends BaseIntegrationTest {
+
+    @Autowired
+    BahmniPatientService bahmniPatientService;


Make this private (or make all members public).

mks-d · 2018-04-17T09:58:41Z

...-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/BahmniPatientServiceImplIT.java

+    BahmniPatientService bahmniPatientService;
+
+    @Autowired
+    LocationService locationService;


Make this private (or make all members public).

mks-d · 2018-04-17T09:59:59Z

...-api/src/test/java/org/bahmni/module/bahmnicore/service/impl/BahmniPatientServiceImplIT.java

+    private SessionFactory sessionFactory;
+
+    private HttpServletRequest mockedRequest = Mockito.mock(HttpServletRequest.class);
+    private HttpServletResponse mockedRsponse = Mockito.mock(HttpServletResponse.class);


I would keep those two declared in setUp() since they are used only once.

Thanks. Declared those in the setup()

mks-d · 2018-04-17T10:06:12Z

...re-api/src/main/java/org/bahmni/module/bahmnicore/service/impl/BahmniPatientServiceImpl.java

+    @Override
+    public List<PatientResponse> luceneHibernateSearch(PatientSearchParameters searchParameters) {
+        List<PatientResponse> luceneHibernateResponse = new ArrayList<>();
+        if(searchParameters.getIdentifier() != null || searchParameters.getName() != null || searchParameters.getCustomAttribute() != null){


You will probably want to use StringUtils.isEmpty() here. Or what's the intended behaviour if all the arguments are empty strings?

Yes you are right. Strings will be always empty if they contain nothing.

If all arguments are empty strings, then we don't search for the patient objects

If none of the first 3 parameters are missing and others like program attributes and address are provided, it will get to hibernate search and if nothing is supplied at all, then no search will be called

mks-d · 2018-04-17T14:28:09Z

bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/BahmniPatientService.java

@@ -18,4 +18,6 @@
    public List<Patient> get(String partialIdentifier, boolean shouldMatchExactPatientId);

    public List<RelationshipType> getByAIsToB(String aIsToB);
+    List<PatientResponse> luceneHibernateSearch(PatientSearchParameters searchParameters);


We need a very detailed Javadoc header for this one given the apparently confusing name.

Provided java doc for the method in the service class

mks-d · 2018-04-18T09:51:00Z

...re-api/src/test/java/org/bahmni/module/bahmnicore/dao/impl/BahmniPatientDaoImplLuceneIT.java

        assertEquals(1, patients.size());
        PatientResponse patient = patients.get(0);
        assertTrue(patient.getIdentifier().contains("200006"));
        assertTrue(patient.getExtraIdentifiers().contains("200006"));
    }

-}
+    @Test
+    public void shouldSearchByName() {


Surely there were already tests validating that one could search by name, could you explain here what this is in regards to existing tests?

Or perhaps are you re-writing existing tests, but this time going through the Lucene branch of the code? I guess my remarks hold for the next few test methods as well.

Yes. This is through the lucene branch of the code as it is

…AH-273

djazayeri

I made a bunch of comments. It's possible that some of my complaints are already things we've agreed to live with for now (since I wasn't part of the earlier discussions)

djazayeri · 2018-05-11T21:58:02Z

bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/PatientDaoImpl.java

-        Map<Object, Object> programAttributes = Context.getService(BahmniProgramWorkflowService.class).getPatientProgramAttributeByAttributeName(patientIds, programAttributeFieldName);
+        Set<Patient> patients = new HashSet<>();
+
+        if(identifier != null && !identifier.isEmpty()){


prefer to use org.apache.commons.lang3.StringUtils.isNotEmpty to combine both these checks.

djazayeri · 2018-05-11T22:00:09Z

bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/PatientDaoImpl.java

-        Map<Object, Object> programAttributes = Context.getService(BahmniProgramWorkflowService.class).getPatientProgramAttributeByAttributeName(patientIds, programAttributeFieldName);
+        Set<Patient> patients = new HashSet<>();
+
+        if(identifier != null && !identifier.isEmpty()){


this whole block can be refactored so as not to duplicate the code blocks about calling name and customAttribute search.
Also, you're making the same patientService.getPatients(String, boolean, int, int) call all three of identifier, name, and customAttribute searching. Don't we need to somehow distinguish between what field we're searching for?

djazayeri · 2018-05-11T22:04:08Z

bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/dao/impl/PatientDaoImpl.java

-                        return patientResponse;
-                    }else
-                        return null;
+        Map<Object, Object> programAttributes = Context.getService(BahmniProgramWorkflowService.class).getPatientProgramAttributeByAttributeName(patientIds, programAttributeFieldName);


should autowire this service in the constructor instead of doing Context.getService

djazayeri · 2018-05-11T22:07:56Z

bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/model/BahmniAddress.java

 import java.util.LinkedHashMap;

+@Indexed


Is this going to do anything here? This is not a hibernate managed class.

djazayeri · 2018-05-11T22:10:10Z

bahmnicore-api/src/main/java/org/bahmni/module/bahmnicore/service/BahmniPatientService.java

+     * @param searchParameters
+     * @return List of PatientResponse
+     */
+    List<PatientResponse> luceneHibernateSearch(PatientSearchParameters searchParameters);


Why does this method name need to say this. It's possible that the under-the-hood implementation does in fact use lucene and then hibernate, but is that actually part of the interface contract? Isn't this just "searchBy" or something like that?

djazayeri · 2018-05-11T22:25:17Z

bahmnicore-omod/src/main/resources/liquibase.xml

+    <changeSet id="201802211437" author="ningosi">
+        <preConditions onFail="MARK_RAN">
+            <sqlCheck expectedResult="1">
+                SELECT COUNT(*) FROM global_property where property = 'patientIdentifierSearch.matchMode';


if we're going to have a precondition here I'd have it also check that property_value != 'ANYWHERE'

djazayeri · 2018-05-11T22:27:18Z

bahmnicore-omod/src/main/resources/liquibase.xml

+                SELECT COUNT(*) FROM global_property where property = 'patientIdentifierSearch.matchMode';
+            </sqlCheck>
+        </preConditions>
+        <comment>Specify how patient identifiers are matched while searching for a patient</comment>


This comment is not helpful since it provides no additional info that can't be inferred from "patientIdentifierSearch.matchMode". Instead say here why we're making the change, e.g. "change from default OpenMRS patient identifier matching behavior to search anywhere"

djazayeri · 2018-05-11T22:28:13Z

bahmnicore-omod/src/main/resources/liquibase.xml

+                SELECT COUNT(*) FROM global_property where property = 'patientSearch.matchMode';
+            </sqlCheck>
+        </preConditions>
+        <comment>Specify how patient names are matched while searching for a patient</comment>


Same as above comment. Say why we're doing this, e.g. changing away from some default? (I thought this was already the default name search in OpenMRS)

djazayeri · 2018-05-11T22:28:32Z

bahmnicore-omod/src/main/resources/liquibase.xml

+                SELECT COUNT(*) FROM global_property where property = 'person.attributeSearchMatchMode';
+            </sqlCheck>
+        </preConditions>
+        <comment>Specify how person attributes are matched while searching for a patients</comment>


same as above comment

djazayeri · 2018-05-11T22:36:43Z

pom.xml

@@ -24,7 +24,7 @@

    <properties>
        <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
-        <openMRSVersion>2.1.1</openMRSVersion>
+        <openMRSVersion>2.1.3-SNAPSHOT</openMRSVersion>


Looks like 2.1.3 was already released, so this shouldn't point to a snapshot: https://github.com/openmrs/openmrs-core/releases/tag/2.1.3

…ving proerty_value ANYWHERE

ningosi added 18 commits February 14, 2018 17:37

Adjusted the openmrs dependacy to master version

077b026

Added correct dependancy of 2.1.3-SNAPSHOT

b368e4d

Initial work on BAH-273 lucerne search for patients(registration app)

727a353

BAH-273: Using OpenMRS Core Lucene API in PatientDaoImpl (registratio…

f028cd1

…n screen)

Revert BahmniPatientDaoImplLuceneIT.java

83ccc4c

Set global properties patientIdentifierSearch.matchMode in liquibas…

bfe7cdf

…e as oppossed to activator

Autowired the service instead of using openmrs context to call it

c2805cc

Merge branch 'BAH-273' of https://github.com/ningosi/bahmni-core into…

964089b

… BAH-273

Setting the gp value since openmrs already creates the property

3e344c9

Autowire service class in the current class constructure

8d7d20c

Setting patientSearch.matchMode and person.attributeSearchMatchMode t…

0cac042

…o `ANYWHERE`

Include all the search possible parameters in one lucene method

ac2de0a

Added unit tests to verify the lucene search functionality

ac0eda1

Standard naming of tests nethods after review

dfeb59a

Adding combination of lucene and hibernate query search

67d3db7

Added hibernate search annotations on the BahmniAddress and PatientPr…

0cc1d8a

…ogramAttribute

Added combination of the 2 searches

bf119fe

Add tests to confirm intersection of lucene and hibernateyy

5e56b8d

mks-d reviewed Apr 18, 2018

View reviewed changes

ningosi added 2 commits April 25, 2018 11:40

Address PR review initial comments

6c6e16d

Merge branch 'master' of https://github.com/Bahmni/bahmni-core into B…

724d06a

…AH-273

djazayeri reviewed May 11, 2018

View reviewed changes

ningosi added 5 commits May 14, 2018 14:49

Change the platform version to 2.1.3 release version

d45ab53

Add a filetr to the preconditions to only return raws that are NOT ha…

0b9563e

…ving proerty_value ANYWHERE

Clean up blank lines and organize imports

62c1f5a

Removing @indexed from non hibernate class and reorgarnize imports

b1ab8fa

Autowiring classes in the constructors

6798949

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BAH-273 #27

BAH-273 #27

ningosi commented Apr 16, 2018

mks-d Apr 17, 2018

ningosi Apr 25, 2018

mks-d Apr 17, 2018

ningosi Apr 25, 2018

mks-d Apr 17, 2018

ningosi Apr 25, 2018

mks-d Apr 17, 2018

ningosi Apr 25, 2018

mks-d Apr 17, 2018

ningosi Apr 25, 2018

ningosi Apr 25, 2018

ningosi Apr 25, 2018

mks-d Apr 17, 2018

ningosi Apr 25, 2018

mks-d Apr 18, 2018

mks-d Apr 18, 2018

ningosi Apr 25, 2018

djazayeri left a comment

djazayeri May 11, 2018

djazayeri May 11, 2018

djazayeri May 11, 2018

djazayeri May 11, 2018

djazayeri May 11, 2018

djazayeri May 11, 2018

djazayeri May 11, 2018

djazayeri May 11, 2018

djazayeri May 11, 2018

djazayeri May 11, 2018

BAH-273 #27

Are you sure you want to change the base?

BAH-273 #27

Conversation

ningosi commented Apr 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djazayeri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment