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.

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

EA-186 - Loosen providermanagement to an aware_of dependency #230

Merged
merged 17 commits into from
May 30, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -3,23 +3,22 @@
import org.apache.commons.lang3.StringUtils;
import org.openmrs.Person;
import org.openmrs.PersonName;
import org.openmrs.Provider;
import org.openmrs.Role;
import org.openmrs.User;
import org.openmrs.api.APIException;
import org.openmrs.api.PersonService;
import org.openmrs.api.ProviderService;
import org.openmrs.api.UserService;
import org.openmrs.module.emrapi.EmrApiConstants;
import org.openmrs.module.emrapi.account.provider.ProviderServiceFacade;
import org.openmrs.module.emrapi.domainwrapper.DomainWrapper;
import org.openmrs.module.emrapi.utils.GeneralUtils;
import org.openmrs.module.providermanagement.Provider;
import org.openmrs.module.providermanagement.ProviderRole;
import org.openmrs.module.providermanagement.api.ProviderManagementService;
import org.openmrs.util.OpenmrsConstants;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;

import java.lang.reflect.Method;
import java.util.Collection;
import java.util.Date;
import java.util.HashSet;
import java.util.Iterator;
Expand Down Expand Up @@ -55,9 +54,8 @@ public class AccountDomainWrapper implements DomainWrapper {
@Autowired
protected ProviderService providerService;

@Qualifier("providerManagementService")
@Autowired
protected ProviderManagementService providerManagementService;
protected ProviderServiceFacade providerServiceFacade;

@Autowired(required = false)
protected ProviderIdentifierGenerator providerIdentifierGenerator;
Expand All @@ -67,12 +65,11 @@ public AccountDomainWrapper() {

@Deprecated // use DomainWrapperFactory instead
public AccountDomainWrapper(Person person, AccountService accountService, UserService userService, ProviderService providerService,
ProviderManagementService providerManagementService, PersonService personService,
ProviderIdentifierGenerator providerIdentifierGenerator) {
ProviderServiceFacade providerServiceFacade, PersonService personService, ProviderIdentifierGenerator providerIdentifierGenerator) {
this.accountService = accountService;
this.userService = userService;
this.providerService = providerService;
this.providerManagementService = providerManagementService;
this.providerServiceFacade = providerServiceFacade;
this.personService = personService;
this.providerIdentifierGenerator = providerIdentifierGenerator;

Expand All @@ -95,8 +92,8 @@ public void setPersonService(PersonService personService) {
this.personService = personService;
}

public void setProviderManagementService(ProviderManagementService providerManagementService) {
this.providerManagementService = providerManagementService;
public void setProviderServiceFacade(ProviderServiceFacade providerServiceFacade) {
this.providerServiceFacade = providerServiceFacade;
}

public void setProviderIdentifierGenerator(ProviderIdentifierGenerator providerIdentifierGenerator) {
Expand Down Expand Up @@ -125,21 +122,23 @@ public Provider getProvider() {
return provider;
}

public void setProviderRole(ProviderRole providerRole) {

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a necessary backwards-incompatibility on the get/set providerRole methods by broadening to Object as ProviderRole is not available when the providermanagement module is not installed.

public void setProviderRole(Object providerRole) {
if (providerRole != null) {
initializeProviderIfNecessary();
this.provider.setProviderRole(providerRole);
providerServiceFacade.setProviderRole(this.provider, providerRole);
} else {
// this prevents us from creating a new provider if we are only setting the provider role to null
if (this.provider != null) {
provider.setProviderRole(null);
providerServiceFacade.setProviderRole(this.provider, null);
}
}
}

public ProviderRole getProviderRole() {
return this.provider != null ? this.provider.getProviderRole() : null;
public Object getProviderRole() {
if (this.provider == null) {
return null;
}
return providerServiceFacade.getProviderRole(this.provider);
}

public void setGivenName(String givenName) {
Expand Down Expand Up @@ -388,7 +387,7 @@ private void initializeUserIfNecessary() {

private void initializeProviderIfNecessary() {
if (provider == null) {
provider = new Provider();
provider = providerServiceFacade.newProvider();
provider.setPerson(person);
}
}
Expand Down Expand Up @@ -417,16 +416,14 @@ else if (users.size() > 1)
}

private Provider getProviderByPerson(Person person) {
List<Provider> providers = providerManagementService.getProvidersByPerson(person, false);

if (providers != null && providers.size() > 0) {
Collection<? extends Provider> providers = providerServiceFacade.getProvidersByPerson(person);
if (providers != null && !providers.isEmpty()) {
if (providers.size() == 1) {
return providers.get(0);
return providers.iterator().next();
} else {
throw new APIException("Multiple provider/provider roles per person not supported");
}
}

return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import org.openmrs.module.emrapi.EmrApiConstants;
import org.openmrs.module.emrapi.EmrApiProperties;
import org.openmrs.module.emrapi.domainwrapper.DomainWrapperFactory;
import org.openmrs.module.providermanagement.api.ProviderManagementService;
import org.springframework.transaction.annotation.Transactional;

import java.util.ArrayList;
Expand All @@ -30,8 +29,6 @@ public class AccountServiceImpl extends BaseOpenmrsService implements AccountSer

private ProviderService providerService;

private ProviderManagementService providerManagementService;

private DomainWrapperFactory domainWrapperFactory;

private EmrApiProperties emrApiProperties;
Expand All @@ -50,13 +47,6 @@ public void setProviderService(ProviderService providerService) {
this.providerService = providerService;
}

/**
* @param providerManagementService
*/
public void setProviderManagementService(ProviderManagementService providerManagementService) {
this.providerManagementService = providerManagementService;
}

/**
* @param personService the personService to set
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import org.openmrs.api.PasswordException;
import org.openmrs.api.UserService;
import org.openmrs.messagesource.MessageSourceService;
import org.openmrs.module.providermanagement.api.ProviderManagementService;
import org.openmrs.util.OpenmrsUtil;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.beans.factory.annotation.Qualifier;
Expand All @@ -24,10 +23,6 @@ public class AccountValidator implements Validator {
@Qualifier("userService")
private UserService userService;

@Autowired
@Qualifier("providerManagementService")
private ProviderManagementService providerManagementService;

public static final String USERNAME_MIN_LENGTH = "2";
public static final String USERNAME_MAX_LENGTH = "50";

Expand All @@ -42,10 +37,6 @@ public void setUserService(UserService userService) {
this.userService = userService;
}

public void setProviderManagementService(ProviderManagementService providerManagementService) {
this.providerManagementService = providerManagementService;
}

/**
* @see org.springframework.validation.Validator#supports(java.lang.Class)
*/
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package org.openmrs.module.emrapi.account;

import org.openmrs.module.providermanagement.Provider;
import org.openmrs.Provider;
Copy link
Member Author

Choose a reason for hiding this comment

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

This is a backwards-incompatible change to the ProviderIdentifierGenerator interface, though it now supports a broader, not narrower object, so should not pose any problems.


/**
* Implementers can provide implementations of this interface if they wish
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package org.openmrs.module.emrapi.account.provider;

import org.openmrs.OpenmrsMetadata;
import org.openmrs.Person;
import org.openmrs.Provider;
import org.openmrs.annotation.OpenmrsProfile;
import org.openmrs.api.ProviderService;
import org.springframework.beans.factory.annotation.Autowired;

import java.util.Collection;

/**
* Provides an abstraction over Provider services
*/
@OpenmrsProfile(modules = {"!providermanagement"})
Copy link
Member Author

Choose a reason for hiding this comment

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

Provides an implementation of provider-related methods that are used when providermanagement is not installed. This would be where we'd implement logic to support setting roles on the core Provider object as Concept role, but that can be added in a separate PR if desired since that was not pre-existing functionality here.

public class CoreProviderService implements ProviderServiceFacade {

private final ProviderService providerService;

@Autowired
public CoreProviderService(ProviderService providerService) {
this.providerService = providerService;
}

@Override
public Provider newProvider() {
return new Provider();
}

@Override
public Collection<Provider> getProvidersByPerson(Person person) {
return providerService.getProvidersByPerson(person, false);
}

@Override
public Provider saveProvider(Provider provider) {
return providerService.saveProvider(provider);
}

@Override
public Object getProviderRole(Provider provider) {
return null;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this also throw the same exception as setProviderRole?

Copy link
Member Author

Choose a reason for hiding this comment

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

The way I was seeing it, provider role is always null if the providermanagement module is not running. So why throw an exception? The setter is a different situation - the user is trying to set something and the module is unable to accommodate.

}

@Override
public void setProviderRole(Provider provider, Object providerRole) {
throw new IllegalStateException("Unable to set the provider role, the provider management module is not installed");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package org.openmrs.module.emrapi.account.provider;
Copy link
Member Author

Choose a reason for hiding this comment

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

Provides an implementation of provider-related methods that are used when providermanagement is installed. This allows us to keep any references to providermanagement hidden behind this conditionally loaded bean. The setProviderRole method is the way it is as the method signature changed from ProviderRole to Object, and this allows supporting a wide range of possible values including bind parameters in controller requests.


import org.openmrs.Person;
import org.openmrs.Provider;
import org.openmrs.annotation.OpenmrsProfile;
import org.openmrs.api.ProviderService;
import org.openmrs.module.providermanagement.ProviderRole;
import org.openmrs.module.providermanagement.api.ProviderManagementService;
import org.springframework.beans.factory.annotation.Autowired;

import java.util.Collection;

/**
* Provides an abstraction over Provider services
*/
@OpenmrsProfile(modules = {"providermanagement:*"})
public class ProviderManagementProviderService implements ProviderServiceFacade {

private final ProviderService providerService;

private final ProviderManagementService providerManagementService;

@Autowired
public ProviderManagementProviderService(ProviderService providerService, ProviderManagementService providerManagementService) {
this.providerService = providerService;
this.providerManagementService = providerManagementService;
}

@Override
public Provider newProvider() {
return new org.openmrs.module.providermanagement.Provider();
}

@Override
public Collection<org.openmrs.module.providermanagement.Provider> getProvidersByPerson(Person person) {
return providerManagementService.getProvidersByPerson(person, false);
}

@Override
public Provider saveProvider(Provider provider) {
return providerService.saveProvider(provider);
}

@Override
public Object getProviderRole(Provider provider) {
if (provider instanceof org.openmrs.module.providermanagement.Provider) {
return ((org.openmrs.module.providermanagement.Provider)provider).getProviderRole();
}
return null;
}

@Override
public void setProviderRole(Provider provider, Object providerRole) {
if (provider instanceof org.openmrs.module.providermanagement.Provider) {
org.openmrs.module.providermanagement.Provider p = (org.openmrs.module.providermanagement.Provider) provider;
if (providerRole == null) {
p.setProviderRole(null);
}
else {
ProviderRole role = null;
if (providerRole instanceof ProviderRole) {
role = (ProviderRole) providerRole;
}
else if (providerRole instanceof String) {
role = getProviderRole((String) providerRole);
}
else if (providerRole instanceof Integer) {
role = providerManagementService.getProviderRole((Integer) providerRole);
}
else if (providerRole instanceof String[]) {
String[] roles = (String[]) providerRole;
if (roles.length > 1) {
throw new IllegalArgumentException("Only one provider role may be provided");
}
role = getProviderRole(roles[0]);
}
if (role == null) {
throw new IllegalArgumentException("Unable to set provider role from " + providerRole);
}
((org.openmrs.module.providermanagement.Provider) provider).setProviderRole(role);
}
}
}

private ProviderRole getProviderRole(String ref) {
ProviderRole role = providerManagementService.getProviderRoleByUuid(ref);
if (role == null) {
try {
Integer roleId = Integer.parseInt(ref);
role = providerManagementService.getProviderRole(roleId);
}
catch (Exception ignored) {
}
}
return role;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package org.openmrs.module.emrapi.account.provider;

import org.openmrs.OpenmrsMetadata;
import org.openmrs.Person;
import org.openmrs.Provider;

import java.util.Collection;

/**
* Provides an abstraction over Provider services
*/
public interface ProviderServiceFacade {

Provider newProvider();

Collection<? extends Provider> getProvidersByPerson(Person person);

Provider saveProvider(Provider provider);

Object getProviderRole(Provider provider);

void setProviderRole(Provider provider, Object providerRole);
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,7 @@
import java.util.List;
import java.util.Map;

import static java.util.Collections.EMPTY_LIST;
import static java.util.Collections.reverseOrder;
import static java.util.Collections.sort;
import static java.util.Collections.*;
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Strange, I feel like my version of IntelliJ started doing wildcard imports again as well after I turned it off years ago.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

import static org.apache.commons.collections.CollectionUtils.find;
import static org.apache.commons.collections.CollectionUtils.select;

Expand Down
1 change: 0 additions & 1 deletion api/src/main/resources/moduleApplicationContext.xml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
<property name="userService" ref="userService"/>
<property name="personService" ref="personService"/>
<property name="providerService" ref="providerService"/>
<property name="providerManagementService" ref="providerManagementService"/>
<property name="domainWrapperFactory" ref="domainWrapperFactory"/>
<property name="emrApiProperties" ref="emrApiProperties"/>
</bean>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
/**
*
*/
public class EmrApiActivatorComponentTest extends BaseModuleContextSensitiveTest {
public class EmrApiActivatorComponentTest extends EmrApiContextSensitiveTest {

@Autowired
@Qualifier("adminService")
Expand Down
Loading
Loading