Skip to content
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.

Sign up for GitHub

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

[#252] Support for 'addresshierarchy' domain. #254

Merged
merged 11 commits into from
Nov 8, 2023
Merged

[#252] Support for 'addresshierarchy' domain. #254

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e81a3f8
[#252]: Support explicit addresshierarchy domain.
Ruhanga Oct 27, 2023
aec83be
[#252]: Full Address Cache to be initialized always
Ruhanga Nov 1, 2023
8b3dbd3
[#252]: Addressing PR reviews
Ruhanga Nov 2, 2023
27ce839
[#252]: addresshierarchy domain to load conditionally + added docs
Ruhanga Nov 3, 2023
9a61b09
Update README.md
mks-d Nov 6, 2023
aec02f4
Augmented addresshierarchy.md
mks-d Nov 7, 2023
7281385
Update addresshierarchy.md
mks-d Nov 7, 2023
c9464a8
[#252]: Improve AH domain docs
Ruhanga Nov 7, 2023
0938baa
[#252]: Fix Tests
Ruhanga Nov 8, 2023
a0ccc37
[#252]: Iniz to be aware of addresshierarchy
Ruhanga Nov 8, 2023
a21f9ef
Update addresshierarchy.md
mks-d Nov 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/src/main/java/org/openmrs/module/initializer/Domain.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ public enum Domain {
PROVIDER_ROLES,
LOCATIONS,
LOCATION_TAG_MAPS,
ADDRESS_HIERARCHY,
Ruhanga marked this conversation as resolved.
Show resolved Hide resolved
BAHMNI_FORMS,
OCL,
CONCEPTS,
Expand Down
45 changes: 45 additions & 0 deletions api/src/main/java/org/openmrs/module/initializer/api/loaders/AddressHierarchyLoader.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package org.openmrs.module.initializer.api.loaders;

import java.nio.file.Paths;
import java.util.List;

import org.openmrs.api.context.Context;
import org.openmrs.module.addresshierarchy.config.AddressConfigurationLoader;
import org.openmrs.module.addresshierarchy.service.AddressHierarchyService;
import org.openmrs.module.initializer.Domain;
import org.openmrs.module.initializer.api.ConfigDirUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.stereotype.Component;

@Component
public class AddressHierarchyLoader extends BaseLoader {

protected final Logger log = LoggerFactory.getLogger(getClass());

@Override
protected Domain getDomain() {
return Domain.ADDRESS_HIERARCHY;
}

@Override
public ConfigDirUtil getDirUtil() {
return new ConfigDirUtil(iniz.getConfigDirPath(), iniz.getChecksumsDirPath(), getDomainName(), true);
}

@Override
public void loadUnsafe(List<String> wildcardExclusions, boolean doThrow) throws Exception {

try {
AddressConfigurationLoader.loadAddressConfiguration(Paths.get(iniz.getConfigDirPath()), Paths.get(iniz.getChecksumsDirPath()), getDomain().toString());
Context.getService(AddressHierarchyService.class).initI18nCache();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mogoodrich this is always gona initialize the i18n cache at startup from here. initializeFullAddressCache() is implicitly called by AddressConfigurationLoader.loadAddressConfiguration(...)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've also tweaked this to ensure the initializeFullAddressCache is always called not only when checksums are changed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would seem like Initializer should stick to loading in AH entries, and not try to take responsibllity over the full address cache or the i18n cache. I'd be more inclined to make changes to the AddressConfigurationLoader.loadAddressConfiguration method to ensure these caches are initialized whenever it is called. Basically at the bottom of AddressConfigurationLoader.loadAddressConfiguration:

1 If the AH entries are changed and reloaded, it should reload these caches
2.If the AH entries are not changed and reloaded, but the caches have not been initialized yet, reload these caches

@mogoodrich does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, @mseaton that is my preference as well, that Iniz sticks to loading metadata, and not being responsible for setting up in-memory caches.

I believe the original loadAddressConfiguration method already did #1: reloads the cache if the AH entries are reloaded. #2 could be added as well, but is not really necessary if the if we keep the method that refreshes the cache in the contextRefreshed method of the Address Hierarchy activator.

@Ruhanga @mks-d I assume the intent of Iniz is primarily to do metadata loading, not to do overall module start-up tasks? For instance, if for some reason Iniz was uninstalled from an implementation, I'd still expect a module to start up properly, as long as it's metadata was already loaded?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the intent of Iniz is primarily to do metadata loading, not to do overall module start-up tasks?

Yes, I think that's explicitly the purpose here.

Copy link
Member Author

@Ruhanga Ruhanga Nov 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instance, if for some reason Iniz was uninstalled from an implementation, I'd still expect a module to start up properly, as long as it's metadata was already loaded?

@mogoodrich, @ibacher, this would cause some malfunctioning already(as is the case) since Iniz does load the i18n message properties necessary for proper cache initialization from the now proxy addresshierarchy domain (in the context of Iniz).

}
catch (Exception e) {
log.error(e.getMessage());
if (doThrow) {
log.error("The loading of the '" + getDomainName() + "' configuration was aborted.", e);
throw new RuntimeException(e);
}
}
}
}
5 changes: 5 additions & 0 deletions api/src/test/java/org/openmrs/module/initializer/DomainBaseModuleContextSensitiveTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ public InitializerService getService() {
*/
public DomainBaseModuleContextSensitiveTest() {
super();
{
Module mod = new Module("", "exti18n", "", "", "", "1.0.0");
mod.setFile(new File(""));
ModuleFactory.getStartedModulesMap().put(mod.getModuleId(), mod);
}
{
Module mod = new Module("", "fhir2", "", "", "", "1.6.0");
mod.setFile(new File(""));
Expand Down
24 changes: 16 additions & 8 deletions .../AddressHierarchyMessagesLoadingTest.java → ...ddressHierarchyLoaderIntegrationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.module.initializer;
package org.openmrs.module.initializer.api;

import java.io.File;
import java.io.FileReader;
Expand All @@ -23,7 +23,6 @@
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.openmrs.GlobalProperty;
import org.openmrs.PersonAddress;
Expand All @@ -38,13 +37,18 @@
import org.openmrs.module.addresshierarchy.config.AddressConfigurationLoader;
import org.openmrs.module.addresshierarchy.service.AddressHierarchyService;
import org.openmrs.module.exti18n.ExtI18nConstants;
import org.openmrs.module.exti18n.api.AddressHierarchyI18nCache;
import org.openmrs.module.initializer.DomainBaseModuleContextSensitiveTest;
import org.openmrs.module.initializer.InitializerConfig;
import org.openmrs.module.initializer.InitializerConstants;
import org.openmrs.module.initializer.InitializerMessageSource;
import org.openmrs.module.initializer.api.InitializerService;
import org.openmrs.module.initializer.api.loaders.AddressHierarchyLoader;
import org.openmrs.test.Verifies;
import org.openmrs.util.OpenmrsConstants;
import org.springframework.beans.factory.annotation.Autowired;

@Ignore
public class AddressHierarchyMessagesLoadingTest extends DomainBaseModuleContextSensitiveTest {
public class AddressHierarchyLoaderIntegrationTest extends DomainBaseModuleContextSensitiveTest {

protected static final String MODULES_TO_LOAD = "org/openmrs/module/addresshierarchy/include/"
+ ExtI18nConstants.MODULE_ARTIFACT_ID + ".omod";
Expand All @@ -54,14 +58,17 @@ public class AddressHierarchyMessagesLoadingTest extends DomainBaseModuleContext
@Autowired
protected InitializerConfig cfg;

@Autowired
private AddressHierarchyLoader loader;

@Before
public void setup() {
// Disabling AH full caching otherwise loading takes too long
Context.getAdministrationService().saveGlobalProperty(new GlobalProperty(
AddressHierarchyConstants.GLOBAL_PROP_INITIALIZE_ADDRESS_HIERARCHY_CACHE_ON_STARTUP, "false"));

Context.getAdministrationService()
.saveGlobalProperty(new GlobalProperty(OpenmrsConstants.GLOBAL_PROPERTY_LOCALE_ALLOWED_LIST, "en, km_KH"));
Context.getAdministrationService().saveGlobalProperty(
new GlobalProperty(OpenmrsConstants.GLOBAL_PROPERTY_LOCALE_ALLOWED_LIST, "en_GB, km_KH"));
ibacher marked this conversation as resolved.
Show resolved Hide resolved

Context.getAdministrationService()
.saveGlobalProperty(new GlobalProperty(ExtI18nConstants.GLOBAL_PROP_REV_I18N_SUPPORT, "true"));
Expand All @@ -84,10 +91,9 @@ public void tearDown() {
public void refreshCache_shouldLoadAddressHierarchyMessages() throws IOException {

// Replay
loader.load();
AddressConfigurationLoader.loadAddressConfiguration();

AddressHierarchyService ahs = Context.getService(AddressHierarchyService.class);
ahs.initI18nCache();
InitializerService iniz = Context.getService(InitializerService.class);

File csvFile = new File(
Expand All @@ -106,6 +112,8 @@ public void refreshCache_shouldLoadAddressHierarchyMessages() throws IOException
address.setCountyDistrict("ច្បារមន");
address.setAddress1("សុព័រទេព");

AddressHierarchyI18nCache ahI18nCache = Context.getRegisteredComponent(ExtI18nConstants.COMPONENT_AH_REVI18N,
AddressHierarchyI18nCache.class);
// Looking for possible villages based on an address provided in km_KH
AddressHierarchyLevel villageLevel = ahs.getAddressHierarchyLevelByAddressField(AddressField.CITY_VILLAGE);
List<AddressHierarchyEntry> villageEntries = ahs.getPossibleAddressHierarchyEntries(address, villageLevel);
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@
<serializationVersion>0.2.14</serializationVersion>

<!-- For tests -->
<addresshierarchyVersion>2.11.0</addresshierarchyVersion>
<addresshierarchyVersion>3.0.0-SNAPSHOT</addresshierarchyVersion>
<exti18nVersion>1.0.0</exti18nVersion>
<appframeworkVersion>2.14.0</appframeworkVersion>
<providermanagementVersion>2.13.0</providermanagementVersion>
Expand Down
Loading