I am working on a ticket to extend cohort_member table with new columns start_date and end_date as mentioned and discussed in the Design Forum.
Now I have added some code from what I have known before and working on openmrs-core has been a real challenge for me. I have few questions that could help me steer in the right direction.
I still haven’t grasped the idea of working with a collection mapping. Since cohort_member is a mapped Set within the Cohort data model. How does adding two additional columns affect this relationship and how can I work around this?
The design forum mentions adding two methods to the Cohort class
Cohort.getMembers(Date asOfDate)Cohort.getMemberChanges(Date fromDate, Date toDate)
How do I implement these methods? I have implemented the hibernate methods but I am sure I am wrong in a way. I believe if point 1 is answered, I could probably get an idea of what these methods should be doing.
Apart from the hibernate methods, what other areas of the openmrs-core that need to be reworked?
Testing my changes
So far I have only been running jetty:run from the webapp directory. Since there are several modules that use the cohort_member table, for example, the reporting-compatibility module. Install all module doesn’t seem feasible likely because of the dependencies and specific version requirements.
Thanks for working on this and reaching out for help!
I have put some comments in your commits.
Is Obs.groupMembers helpful to look at?
Before getting into how to implement, do you understand what the methods are supposed to be doing from the API consumer’s perspective? If yes, put some javadoc on the methods and create some failing unit tests.
To reduce complexity, first assume that this is all, and get what you know in a working state.
Regarding pt. 2, I wasn’t part of the discussions during the design forum, so I don’t know the extent of the purpose of these methods from an API’s standpoint. Probably @burke or @darius could chime in and help us out?
Additionally, I will go through the suggestions you mentioned and reply soon. Thanks so much for the quick reply.
However, I would prefer to remove the collection completely from Cohort. For performance reasons basically - in case of big cohorts. You can still get its members through the new persistent.
Currently cohort_member is used as a mapping table between patients and cohorts they belong to, with the new columns added to it, it will cease to be just a mapping table. Therefore as @lluismf mentioned we need to introduce a new class named CohortMember that encapsulates the cohort member info, seems to me like that ticket wasn’t well curated to mention what needs to be done
@wyclif@lluismf Thanks for your replies. I assume with CohortMember class will have its own hbm and hibernate methods? I’ll be working on your suggestions today.
For the past week, I have been working on creating the new object CohortMember along with its own hibernate methods. I wrote the unit tests for the hibernate methods and came across the conflict with mapping present in Cohort.
to make all the unit tests pass. So it seems to me, removing the mapping in Cohort is one way to fix this conflict? And while we are going forward, what will be the solution that will be deemed ideal by the OpenMRS community?
@dkayiwa, since you have been slightly involved in this thread, I’d appreciate if you can look at my last 3 commits in this repo.
Error messages from the failed unit test: junit.log.txt (73.6 KB)
I commented on one of the commits, you need to add a separate primary key column i.e cohort_member_id to the cohort_member table so that cohort_id stays as the FK to the cohort table, you will also have to add a cohortMemberId field to the CohortMember class which maps to cohort_member_id. You shouldn’t be adding a new CohortMemberService but instead use the existing CohortService
@wyclif I’m preparing to create the pull request. I was going through the pull request tips. Some of the tests with respect to the Cohort methods seem to be failing. Should I proceed to create the pull request keeping this in mind?
In theory you create a pull request when you’re done, but at times if you want to have someone look at your code before you’re done it’s okay to create a pull request and continue committing and pushing to the same linked remote branch and github will automatically update the original pull request to reflect follow up commits.
@wyclif I made most of the changes suggested by you. If I’ve to commit, do I need to squash over the last commit? How do you recommend I go about it?
Also, I have few questions regarding the methods in Cohort.java.
For constructors that have only the patient ID, should I be creating new CohortMember objects using a new constructor for e.g.: new CohortMember(Patient patientId)? What will be the constructor signature for such a scenario?
And if it’s a patientId, do I create new Patient objects using the patientId? Or what if it’s only an Integer Id?
The methods addMember(Integer memberId) and removeMember(Integer memberId), do I have to change the type to CohortMember in this case or should it be Integer cohortMemberId?
Also I’m unsure how to implement the get/setMembers methods.
For getMembers(), I was working with the tests that required me to add a patient to a cohort where I added this line of code: cohort.getMembers().add(patient.getPatientId()). With this, I couldn’t save the patient to the cohort. Logically shouldn’t getMembers() return a Set<CohortMember>?
This is what I have done for getMembers and setMembers as per the Pull Request comments:
/**
* @return a set of patient ids
*/
public Set<Integer> getMembers() {
Set<Integer> patientIds = new TreeSet<Integer>();
if (members != null) {
for (CohortMember cohortMember : members) {
patientIds.add(cohortMember.getPatientId().getId());
}
}
return patientIds;
}
public void setMembers(Set<Integer> ids) {
Set<CohortMember> members = new TreeSet<CohortMember>();
for (Integer id : ids) {
CohortMember cohortMember = new CohortMember(new Patient(id), new Date());
members.add(cohortMember);
}
this.members = members;
}
The getMembers should return a Collection of CohortMembers and not ids, use Collection and not Set for the return type, the setter should also take in a Collection of CohortMembers.
You’ll need at least 2 constructors for CohortMember, the default and one that takes in a Patient and Cohort object, as for the existing ones that take in patientIds you might want to update them to convert the patientId to a Patient object by creating a new instance of Patient with new Patient(patientId)
@wyclif Thanks for your effort in helping me through. I have two more questions that I came across while fixing the unit tests:
Service method removePatientFromCohort(Cohort cohort, Patient patient)
How do I execute this method? Should I create an ancillary method getCohortMember(Cohort, Patient) to fetch the CohortMember and then remove using getMembers().remove(CohortMember)? Or is there any other way?
Hibernate method List<Cohort> getCohortsContainingPatientId(Integer patientId)
I have trouble forming the query. Previous query was:
"from Cohort c where :patientId in elements(c.memberIds) and c.voided = false order by name". Now with members collection, I’m looking for a way to fetch from the Collection using the patientId. Is that even possible?
For the first issue, my guess is that Cohort.remove(Integer) and Cohort.add(Integer) should possibly be deprecated and replaced with those that take Patient objects and then update the deprecated ones to delete to the new ones. This implies that CohortService.removePatientFromCohort(Cohort, Patient) would be update to just call Cohort.remove(Patient) and then save the Cohort.
For the second, because the hibernate mappings have changed you have to update some queries and Criteria objects in the DAO e.g HibernateCohortDAO.getCohortsContainingPatientId() needs to be updated based on the changes in the relationships, I’m not sure how familiar you are with hibernate specifically its Criteria API and HQL.
(I am replying without reading lots of the above messages in this thread, so this is not authoritative.)
I would think you do something simpler like this:
select distinct cohort from CohortMember where patient = :patient
Basically you want to select from the entity that maps between cohorts and patients, filter by the patient, and select the cohorts. (It probably has to be “distinct” in some way because a patient could be in a cohort > 1 time.)
