I fear that we're going to need to support several classes of locations -- e.g., clinic locations, cities, states, etc. We don't want the list of all known villages to show up when data assistants are selecting a clinic site, right?

+1 for Jeff's proposed solution:

SET FOREIGN_KEY_CHECKS=0;
CREATE TABLE `location_type` (
 `location_type_id` int(11) NOT NULL AUTO_INCREMENT,
 `name` varchar(50) NOT NULL DEFAULT '',
 `description` varchar(50) NOT NULL DEFAULT '',
 `creator` int(11) NOT NULL DEFAULT '0',
 `date_created` datetime NOT NULL DEFAULT '0000-00-00 00:00:00',
 `retired` tinyint(1) NOT NULL DEFAULT '0',
 `retired_by` int(11) DEFAULT NULL,
 `date_retired` datetime DEFAULT NULL,
 `retire_reason` varchar(255) DEFAULT NULL,
 PRIMARY KEY (`location_type_id`),
 KEY `location_type_name` (`name`),
 KEY `user_who_created_type` (`creator`),
 KEY `user_who_retired_encounter_type` (`retired_by`),
 KEY `retired_status` (`retired`),
 CONSTRAINT `user_who_created_type` FOREIGN KEY (`creator`) REFERENCES `users` (`user_id`),
 CONSTRAINT `user_who_retired_encounter_type` FOREIGN KEY (`retired_by`) REFERENCES `users` (`user_id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8;

ALTER TABLE `location` ADD COLUMN `parent_location_id` int(11);
ALTER TABLE `location` ADD COLUMN `location_type_id` int(11);
ALTER TABLE `location` ADD KEY `parent_location_for_location` (`parent_location_id`);
ALTER TABLE `location` ADD KEY `type_of_location` (`location_type_id`);
ALTER TABLE `location` ADD CONSTRAINT `parent_location` FOREIGN KEY (`parent_location_id`) REFERENCES `location` (`location_id`);
ALTER TABLE `location` ADD CONSTRAINT `location_type` FOREIGN KEY (`location_type_id`) REFERENCES `location_type` (`location_type_id`);
SET FOREIGN_KEY_CHECKS=1;

I added a patch. Basically, it's what Jeff proposed, except types are now tags and have many-to-many mappings with locations.

• added org.openmrs.LocationTag
• org.openmrs.Location: added Set<Location> childLocations, Location parentLocation, Set<LocationTag> tags
• added/extended hbm.xml
• added db changes to sqldiff
• extended API/DAO
• refactored LocationServiceTest a bit, added LocationTag tests
• added LocationServiceTest-initialData.xml
• added new privs for LocationTag to OpenmrsConstants

Note: Two of the JUnit assertion tests fail. One concens a three-tier Location hierarchy (parent->child->childofchild).

The other test fails to load child locations from the XML dataset, claiming the number of children is 0. I've had this problem with one of my modules before: JUnit tests failed while the webapp worked fine. I think JUnit might be having problems with hierarchies of the same class. Since the webapp parts of this extension are not implemented yet, I couldn't test that, obviously.

I've run out of ideas about these two errors, maybe someone else might have an idea...

I think we also want a method:

Location.getChildLocations(boolean includeRetired);

added Location.getChildLocations(includeRetired);

Marking as Needs Review so that this is covered in one of our weekly code reviews.

Keelhaul: If you would like to join us while we look over this patch, let me know and we can schedule a day you can join us to walk through it. See http://openmrs.org/wiki/Code_Review_Schedule

Keelhaul: I can't get this to apply to the latest trunk cleanly. What are the chances you still have these changes and can do an "svn update" and attach a new patch?

Do the error occur when applying sqldiff changes, by chance? I assigned a db version to my changes manually. If the db has had any updates since 11/25, then this might be causing errors?

Do you want me to update my trunk with the changes applied, and then create a new patch off that?

I re-created the patch after an update from svn. Seems to integrate properly into an untouched trunk and compile. Prior to that, however, updating from svn broke my locally edited files (duplicate methods, svn tags e.g. "<<<<< .mine" or something like that, etc.). Had to fix them manually to resolve the conflicts.

The patch now applied quite cleanly, thanks!

FYI, those "<<< .mine" lines in there are svn specific. The proper way to resolve those conflicts is to use "Team-->edit conflicts", save the "merged" file, then do "team-->mark resolved" 'fix' the files.

When you get a chance, can you re-do the location table sql update in liquibase xml format and put it into the liquibase-update-to-latest.xml file?

Slightly modified Keelhaul's patch to save locations and execute test cases

Keelhaul: I've attached a patch that has the fixes for the test cases you mentioned. It entailed making a change to the HibernateLocationDAO.saveLocation to work like HibernateObsDAO.saveObs worked. Luckily, I had encountered the same problem when I was doing obs grouping, so I knew where to look.

The other fix was in trunk. The "parent_location_id" column was not being picked up in your dbunit xml file because the _first_ location element in there didn't have that column defined. For some reason, dbunit treats the first element as the standard for all elements in that file. Because the first element didn't have the column, the column wasn't used in any of the subsequent elements...hence your other columns not really being children of the parent. I added the column to the first with a value of [NULL] so that it would work. Update to the latest trunk as well.

Two requests:

1. Have LocationTag mimic ConceptNameTag. The properties on it should be the same, saved the same way, etc. (You don't need those default values though)
2. Clean up and shorten your test cases. I really like where you're going with the test cases. Kudos to you for it! :-) However, some of your test cases are doing too much, assert more than once, etc. You can pull a lot of tests out into really small LocationTest.java and only test, say, the addChildLocation(Location) method. Then in your LocationServiceTest you can assume that that will be working and you won't have to do so many asserts.

Something like:

LocationTest.java:
public void addChildLocation_shouldAssignParentLocation() {
  Location parentLocation = new Location();
  Location childLocation = new Location();
  parentLocation.addChildLocation(childLocation);
  assertTrue(parentLocation.equals(childLocation.getParentLocation()));
}

Strong work here, Keelhaul. Once you throw in these changes, we'll push it through an official code review and hopefully get it into trunk soon!

I'll add a liquibase element for the sql changes for you when I can...

I renamed some members and added a few methods to the LocationTag class to match those of ConceptNameTag. Renamed a few getLocationTag* methodss in the service/dao. However, ConceptService has only a 2 or 3 methods for ConceptNameTag, all others are missing.

As for the unit tests, I got rid of the asserts that have nothing to do with the operations performed by the service. No LocationTest yet, though.

Is that sufficient?

To-Do items arising from code review on Jan 26, 2009, by Darius and Ben.

• LocationService.java
  • getLocationsByTags -> split into two methods, getLocationsHavingAllTags and getLocationsHavingAnyTag
  • also add a getLocationsByTag that takes a single tag.
  • findLocationTags(String) should be renamed to getLocationTags(String), and carry that name change through to the DAO.
  • getLocationTagById(Integer) should be renamed to getLocationTag(Integer), and carry that name change through to the DAO.
• OpenmrsConstants.java
  • Remove PRIV_VIEW_LOCATION_TAGS and use PRIV_VIEW_LOCATIONS instead.
• LocationServiceImpl.java
  • populateMetadata should take a User and Date arguments so that you can just do "new Date()" once, and make use that identical timestamp for all children.
• SQL for location_tag table
  • make 'name' column unique.
• HibernateLocationDAO.java
  • in findLocationTags(String) method, the "if (search == null ..." should be pushed up to the ServiceImpl instead.
• Everywhere
  • all non-trivial methods need @should annotations in their javadoc. Ben can tell you where to get the cool eclipse plugin that will help with this.
• Location.java
  • getChildLocations(boolean)
    • javadoc should say "child.parentLocationId = this.locationId"
    • Remove the "not used often" bit.
    • if (includeRetired) ... else ... part actually only gets called one way, because it's surrounded by another if. Fix this.
  • addChildLocation(Location)
    • should check all the way up the heirarchy for loops, not just the direct parent
    • Also check to make sure this != child, i.e. we aren't marking something as its own child.
  • take out the two convenience methods for addTag(String) and addTag(String, String)
  • add unit tests for calling Location.addTag and then doing a LocationService.saveLocation, and ensure correct behavior for creating a new transient "new LocationTag()" that isn't yet persisted.
    • I propose you are not allowed to use transient tags, but feel free to disagree.
  • remove hasTag(LocationTag) (having that delegate based on just String name is wrong)
  • have isRetired() delegate directly to getRetired(). (retired won't be null)

We haven't looked at the unit tests yet, but will after these issues are addressed.

Thanks for doing this!

Do you want all PRIV_*_LOCATION_TAGS privs removed or just VIEW?

just VIEW

Ok, I think I've cleared that list. I cannot get JUnit tests to run properly anymore though, so everything is untested. =(

I agree with the no transient tags policy. Added a check when saving a location (match the name with an existing tag and overwrite, otherwise throw an exception).

Code reviewed on 23 Feb 2009.

Comments:

• (TODONOT) 298-299: remove call to getAllLocationTags(true) when passed null or empty string. Discussed - we're going to keep this for now.
• (TODO) 85: change "tag = existing" to do a remove(tag), add(tag) similar to how the concept name tag code works (see ConceptServiceImpl 1152)
• Darius brought up the issue of location hierarchy changes.
  • (TODO) Need to come up with a specific use case.

FYI, Ben is going to make the change(s) in the comments from the code review. Darius is going to get back to us on whether we need to worry about tracking changes for location hierarchy.

Mike and I discussed this and decide that the right way to do things would be to have a table like:

create table location_hierarchy_map (
   id integer primary key,
   parent_location_id not null references location,
   child_location_id not null references location,
   start_date date, -- can be null, this is when this hierarchy becomes effective
   end_date date -- can be null, this is when this hierarchy stops being effective
);

This would allow two things, both of which we've seen in practice:

• capture the fact that administrative districts change (we've seen this in both Rwanda and Peru)
• capture the fact that their can be multiple sets of location groupings. (In Peru there are 'health districts' and 'administrative districts' which don't line up exactly. A clinic is in one health district, and one administrative district.)

In practice this flexibility is not needed right now, so it probably makes sense to commit the location hierarchy patches as keelhaul has implemented them, and switch the data model around a year down the road when someone has a specific use case.

It is worth considering leaving the service methods exactly as keelhaul has defined them, but using a many-to-many mapping table instead of just the parent ref back to location, such that making a change in the future to support multiple hierarchies and history of changes is easier.

So leave the Location object as only having a single parent but add a table to store multiple parents? I don't think thats really worth it. I feel like adding a table, etc later on will be trivial. Having an unnecessary table in there for a year or so isn't really worth it.

I think it's okay to commit as-is. We can revisit later once there's a real need.

-Darius

Ok, committed to trunk in [6984] and will be included in the 1.5 release.

See ticket #1287 for adding the user interface portions of hierarchies + tags.