@mksd that problem is happing because of the way modules are started and spring application context initialized during tests. But all should be well at runtime. For the sake of making your tests pass, just put that bean in this file https://github.com/mekomsolutions/openmrs-module-addresshierarchy/blob/tmp/api/src/test/resources/TestingApplicationContext.xml
Yeah sorry, I forgot to tell you. For the sake of this troubleshooting, please skip tests, I’ll fix that later. I know that a test starts failing when we touch the way this bean is being wired.
This is not working at runtime, all the things I said above are relevant logs and comments about runtime.
Can you also change this https://github.com/mekomsolutions/openmrs-module-addresshierarchy/blob/tmp/api/src/main/java/org/openmrs/module/addresshierarchy/service/AddressHierarchyServiceImpl.java#L1078 to this? Context.getRegisteredComponents(I18nCache.class).get(0)
Not sure if I understand this.
If another module is dependent on AH, then AH will be loaded first. So the interface definition that the other module relies on will always exist and not throw ClassNotFoundException. Same for BahmniCore being dependency to another module. I am not suggesting that you make Ext I18N module to depend on Bahmni. The interface for i18 needs to be in AH module. Bahmni depends on AH, Exti18n depends on AH - no conflict. If you are saying that the order of loading of Ext118 and BahmiCore can cause the issue, then do a runtime discovery. It can be referenced runtime using calling “Context.getRegisteredComponent(I18Interface.class)” - it does not need to be a class level variable at all.
Also not sure why the transform() function is a problem at the controller level. Maybe I am not getting the context. But idea is simple - the core service call fetches me the entries - that should not change. (I make no assumotion that any codified address entries) - and then I just apply a transformation, which again is applied if a transformer is provided or not. The Transformer figures out if it should apply any transformation or not - if not just return the original. The base of things do not change, only surfacial transformation does.
If we need to take action within deeper internal service layers, well then I would tend to believe that its a smell that the design in not quite right.
Btw, we should really meetup early next week to get a better sense of your contexts and requirements in Bahmni context.
My understanding is that exti18n will not depend on addresshierarchy - rather, addresshierarchy will be awareof exti18n. We want addresshierarchy to utilize exti18n when it is present, and not to use it when it is not.
I think adding
@OpenmrsProfile only isn’t enough for a bean to get picked up, you would probably need to add
@Component too. Don’t you need to specify the openmrs version too?
@dkayiwa / @wyclif - is someone able to actually try this out and see if the functionality works? We can speculate here all we want, but as far as I can tell so far, we have documented support for a feature, but we have no working examples that we can point anyone to. It should be straightforward to fire up a new SDK, install addresshierarchy and exti18n and try this out. Does anyone have cycles to do that?
I got it working thanks to @dkayiwa’s support! There’s a caveat regarding unit tests tho. I’ll put everything together and report back about the inner details on Monday or so. And I’ll be able to finally do this PR for ADDR-108.
Great news! Keep me posted…
So… It works and here are a few things to know as per my latest understanding of things:
- Segregate and isolate all imports from the module being aware of into one bean implementation class.
- That bean is entirely defined for Spring through its
@OpenmrsProfileannotation. There should be (1) no entry in the application context XML file and (2) no
@Componentannotation. Those would trigger Spring to wire that bean ahead of the OpenMRS-specific conditions leading to runtime trouble. All this implies that the bean won’t have a known bean ID, meaning that…
- This bean can only be
@Autowired, and even presumably
@Autowired(required = false)depending on the conditions.
All the above can lead to a slight headache on the context-sensitive tests side of things. Hence those two new topics on which I would be eager to get input:
@mogoodrich basically this all means that the PR for ADDR-108 is being delayed…
Thanks @mksd! If there’s a short-term fix to work around test issues while we figure out the “right” way to go about it, feel free to submit.
Take care, Mark
Also @mksd and @mogoodrich - remember, any addition to what the core OpenmrsProfile annotation can do, or what base unit test cases are available will lead to us needing to increase the minimum supported OpenMRS version in the 1.9 and beyond release lines. This may not be desirable and should be evaluated.
@mksd i do not seem to understand what the blocker is. I thought all tests are now passing and the required runtime functionality works as expected.
The tests are not passing, no.
Because by setting the entry in TestingApplicationContext.xml as you suggested, we would autowire the same implementation of
I18nCache for all tests. But certain tests cover the case when Ext I18N is present, and other tests cover the cases when Ext I18N is missing.
Knowing this I am assessing whether I could push for changes that would allow the runtime configuration to be handled seamlessly within context-sensitive tests as well.
Unfortunately as @mseaton just hinted, this might be too costly for their team. AH would need to require Core 1.9.12.
I also understand that there are workarounds, but I am keen on getting this done as neatly as possible because we want to advocate for a similar (yet less intrusive) effort to be made within Bahmni Core as well. This is why I’m wishing for an adequate handling of the modules startup within unit tests, and for a wider scenario coverage for
Are your changes committed for us to take a look at the failing tests?
Not to sidetrack us too much, but, for what it’s worth, I don’t know much of the details of the exti18n module, but if we think it’s stable and going to be a core part of our internationalization strategy going forward, we could consider just making address hierarchy fully depend on exti18n (and add it to the Ref App and the PIH EMR distro).
@dkayiwa, not really just right now as I have been testing multiple things locally. Thanks for offering to look tho. I know that there are workarounds, and that I can achieve to have a working version using a one or another.
But I’m testing the waters here:
- Would it be possible to enrich the test framework? (this thread).
- Is it sensible to extend
@OpenmrsProfile's behaviour? (that other thread).
- If both the above can be considered, could I use both for our real world case: AH aware of Ext I18N?
- Is that acceptable for the maintainers of AH?
- Could that pave the way for a very similar approach in Bahmni Core?
@mogoodrich, I’m glad to read that you guys would consider adopting Ext I18N as you said. However let’s keep in mind that Ext I18N is a workaround in itself. I would wish for higher versions of the Core (3.0?) that would not necessitate it anymore, In that light it makes sense to keep it as an optional artifact, with everything running smoothly when it’s not there.
@mksd regardless of what the AH maintainers decide to go for, it will still be awesome to enrich the test framework and also extend the @OpenmrsProfile’s behavior.
Thanks @dkayiwa, that’s supportive!
After months of running this on our own CI, we are now in a position to PR a (yet much smaller) change, but similar in nature, to Bahmni Core…
…since we finally have a development environment with OpenMRS Core 2.1.1 (bringing
@OpenmrsProfile loading beans based on modules not started)!
Bahmni Core API would depend on Ext I18N 1.0.0. However Bahmni Core would only be aware of Ext I18N and only one single bean’s implementation does in fact refer to Ext I18N, the one bean that is loaded if the module is present (see here on our fork).
This technique has worked well with AH and is part of its release 2.11.0. And as I said it has worked well with Bahmni Core as well since we have been running a full English-Khmer version of Bahmni on our servers for months.
I need to squash my commits and bring all this to master and I shall have a PR ready by tomorrow. Giving here the heads up for someone to be on the fence for reviewing it.