BAH-3223 - Appointment module logs errors at startup if atom feed mod… #129
Conversation
…ule is not installed
|
|
| * Registers AOP advice when the context starts up. | ||
| * This is done instead of declaring advice in config.xml in order to allow it to be conditionally loaded | ||
| */ | ||
| public class AtomFeedAdviceProvider implements ApplicationContextAware { |
There was a problem hiding this comment.
How is this wired up? through component scan?
Also should the advices be wired up conditionally, since the intention is to work without the atomfeed module? should @conditional annotation be used?
There was a problem hiding this comment.
@angshu - most likely this needs to have a @component annotation added to it. In my initial testing I was going to see if just implementing the interface was enough, but it's likely it needs to be a registered component. Because this is in the atom-feed maven sub-module, which is itself already conditionally loaded, I assumed that this didn't also need conditional loading associated with it.
There was a problem hiding this comment.
Is there a preferred approach to implementing such conditional loading? I was hoping this had been resolved so that i learn from it
| @@ -1,5 +1,6 @@ | |||
| package org.openmrs.module.appointments.web.controller; | |||
There was a problem hiding this comment.
The changes in this file are not for this PR, but pending in another PR (see BAH-3222). I need these in place to successfully build and test.
| @@ -31,6 +32,7 @@ | |||
|
|
|||
| @RunWith(PowerMockRunner.class) | |||
| @PrepareForTest(Context.class) | |||
| @PowerMockIgnore( {"javax.*", "org.apache.*", "org.slf4j.*"} ) | |||
There was a problem hiding this comment.
This is just an incidental fix- this was throwing stack traces in the logs during the build, and adding this ignore line fixes that.
| @@ -27,18 +30,34 @@ public class AppointmentsActivator extends BaseModuleActivator { | |||
| private Log log = LogFactory.getLog(this.getClass()); | |||
|
|
|||
There was a problem hiding this comment.
I moved away from the original approach of using ApplicationContextAware, in favor of a more generally understood approach of using the module activator and iterating over registered components. I made this generalized as it is not specific to advice, but could be utilized for other reasons if needed.
One of the main reasons I moved to this approach is to make an attempt to also clean up / remove advice if the module is stopped like is done with standard advice in the ModuleFactory. This is just an additional precaution, though OpenMRS doesn't really function properly anyway (or I wouldn't trust it to) if modules are started/stopped via the legacy admin interface, and this isn't really a huge concern.
|
|
||
| private static final Log log = LogFactory.getLog(AtomFeedAdviceActivatorComponent.class); | ||
|
|
||
| private final AppointmentServiceDefinitionAdvice appointmentServiceDefinitionAdvice; |
There was a problem hiding this comment.
I needed to use these properties and two different constructors just so that I could properly mock in unit tests.
…ule is not installed