TRUNK-6354: Add Provider Roles Service and Dao layer #5073

mherman22 · 2025-06-12T13:51:19Z

Description of what I changed

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-6354

Checklist: I completed these to help reviewers :)

My IDE is configured to follow the code style of this project.

No? Unsure? -> configure your IDE, format the code and add the changes with git add . && git commit --amend
I have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)

No? -> write tests and add them to this commit git add . && git commit --amend
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.

No? -> execute above command
All new and existing tests passed.

No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.

No? Unsure? -> execute command git pull --rebase upstream master

dkayiwa · 2025-06-12T20:58:17Z

I thought we had agreed adding these methods to the existing ProviderService? I do not see why we should create a new and separate service just for the roles.

mherman22 · 2025-06-12T21:02:09Z

I thought we had agreed adding these methods to the existing ProviderService? I do not see why we should create a new and separate service just for the roles.

Skipped my mind i guess, let me amend that

dkayiwa · 2025-06-13T09:32:52Z

api/src/main/java/org/openmrs/api/ProviderService.java

 import org.openmrs.annotation.Authorized;
 import org.openmrs.annotation.Handler;
 import org.openmrs.util.PrivilegeConstants;

+import static org.openmrs.util.PrivilegeConstants.PROVIDER_ROLE_API_PRIVILEGE;
+import static org.openmrs.util.PrivilegeConstants.PROVIDER_ROLE_API_READ_ONLY_PRIVILEGE;


Is this how we have been importing privileges in other services?

dkayiwa · 2025-06-13T09:34:38Z

api/src/main/java/org/openmrs/api/ProviderService.java

+	 * @since 2.8.0
+	 */
+	@Authorized(value = {PROVIDER_ROLE_API_PRIVILEGE, PROVIDER_ROLE_API_READ_ONLY_PRIVILEGE}, requireAll = false)
+	public List<ProviderRole> getAllProviderRoles(boolean includeRetired);


This seems to deviate from our conventions. Can we use the same privilege pattern as for the rest of the services?

dkayiwa · 2025-06-13T09:36:37Z

api/src/main/java/org/openmrs/api/ProviderRoleInUseException.java

+ * Thrown when user attempts to delete a provider role that is currently linked to a provider
+ */
+public class ProviderRoleInUseException extends Exception {
+


Do we need a dedicated class for this when we already have CannotUpdateObjectInUseException?

dkayiwa · 2025-06-13T09:37:39Z

api/src/main/java/org/openmrs/api/db/ProviderDAO.java

+	 * Gets all Provider Roles in the database
+	 *
+	 * @param includeRetired whether or not to include retired providers
+	 * @return list of al provider roles in the system


We do not need to duplicate this. We can use the @see annotation.

dkayiwa · 2025-06-13T09:39:42Z

api/src/main/java/org/openmrs/api/db/hibernate/HibernateProviderDAO.java

+	public ProviderRole getProviderRoleByUuid(String uuid) {
+		Session session = sessionFactory.getCurrentSession();
+		CriteriaBuilder cb = session.getCriteriaBuilder();
+		CriteriaQuery<ProviderRole> cq = cb.createQuery(ProviderRole.class);


Can you use something like HibernateUtil.getUniqueEntityByUUID(sessionFactory, clazz, uuid);

dkayiwa · 2025-06-13T09:53:18Z

api/src/main/java/org/openmrs/api/impl/ProviderServiceImpl.java

+	@Transactional
+	public void purgeProviderRole(ProviderRole role) throws ProviderRoleInUseException {
+		try {
+			dao.deleteProviderRole(role);


Can we use the same pattern as for the rest of the purge methods?

dkayiwa · 2025-06-13T09:53:45Z

api/src/main/java/org/openmrs/util/PrivilegeConstants.java

@@ -158,6 +158,10 @@ private PrivilegeConstants() {

 	@AddOnStartup(description = "Able to get Provider")
 	public static final String GET_PROVIDERS = "Get Providers";
+
+	public static final String PROVIDER_ROLE_API_PRIVILEGE = "Provider Role API";
+


These privileges do not match with our convention.

dkayiwa · 2025-06-13T09:55:14Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	@Test
+	public void getProviderRole_shouldGetProviderRole() {
+		ProviderRole role = service.getProviderRole(1002);
+		assertEquals(new Integer(1002), role.getId());


Why instantiate a new Integer?

dkayiwa · 2025-06-13T09:56:19Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	 * @see ProviderService#getProvider(Integer)
+	 */
+	@Test
+	public void getProvider_shouldGetProviderRoleGivenProviderId() {


Basing to your method name, we do not fetch provider roles by provider id.

dkayiwa · 2025-06-13T09:57:01Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	@Test
+	public void getProviderRoleByUuid_shouldGetProviderRoleByUuid() {
+		ProviderRole role = service.getProviderRoleByUuid("db7f523f-27ce-4bb2-86d6-6d1d05312bd5");
+		assertEquals(new Integer(1003), role.getId());


I do not like instantiating the new integer.

dkayiwa · 2025-06-13T09:57:32Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	}
+
+	@Test
+	public void getProviderRoleByUuid_shouldReturnNUllIfNoProviderForUuid() {


Null instead of NUll

dkayiwa · 2025-06-13T09:57:58Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	}
+
+	@Test
+	public void saveProviderRole_shouldSaveBasicProviderRole() {


What value is added by Basic?

dkayiwa · 2025-06-13T09:58:43Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+		ProviderRole role = new ProviderRole();
+		role.setName("Some provider role");
+		Context.getService(ProviderService.class).saveProviderRole(role);
+		assertEquals(13, service.getAllProviderRoles(true).size());


Within this method, how do we tell that the new provider role has been added?

dkayiwa · 2025-06-13T10:00:19Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	public void deleteProviderRole_shouldDeleteProviderRole() throws Exception {
+		ProviderRole role = service.getProviderRole(1012);
+		service.purgeProviderRole(role);
+		assertEquals(11, service.getAllProviderRoles(true).size());


From this method, how do we confirm that a provider role was deleted?

dkayiwa · 2025-06-13T10:01:16Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	public void retireProviderRole_shouldRetireProviderRole() {
+		ProviderRole role = service.getProviderRole(1002);
+		service.retireProviderRole(role, "test");
+		assertEquals(10, service.getAllProviderRoles(false).size());


Same question as for the above.

dkayiwa · 2025-06-13T10:01:31Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	public void unretireProviderRole_shouldUnretireProviderRole() {
+		ProviderRole role = service.getProviderRole(1002);
+		service.retireProviderRole(role, "test");
+		assertEquals(10, service.getAllProviderRoles(false).size());


Same question as for the above.

dkayiwa · 2025-06-13T11:23:44Z

api/src/main/java/org/openmrs/api/impl/ProviderServiceImpl.java

+	@Override
+	@Transactional
+	public void retireProviderRole(ProviderRole role, String reason) {
+		// BaseRetireHandler handles retiring the object


Are we going to be putting these comments in every service method where we retire or unretire objects?

dkayiwa · 2025-06-13T11:24:26Z

api/src/main/java/org/openmrs/util/PrivilegeConstants.java

@@ -512,6 +515,11 @@ private PrivilegeConstants() {

 	@AddOnStartup(description = "Able to edit Provider")
 	public static final String MANAGE_PROVIDERS = "Manage Providers";
+
+	@AddOnStartup(description = "Able to edit Provider Role")


dkayiwa · 2025-06-13T11:26:59Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	 */
+	@Test
+	public void purgeProvider_shouldDeleteProviderWithAProviderRole() {
+		Provider provider = service.getProvider(1009);


Is this test about deleting a provider or a provider role?

dkayiwa · 2025-06-13T11:28:26Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	 */
+	@Test
+	public void saveProvider_shouldSaveAProviderWithProviderRole() {
+		Provider provider = new Provider();


Is this how existing tests confirm that an entity was persisted to the database?

dkayiwa · 2025-06-13T11:33:31Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+		int roleCount = roles.size();
+		assertEquals(12, roleCount);
+
+		roles = service.getAllProviderRoles(true);


What is this duplication about?

dkayiwa · 2025-06-13T11:34:06Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	@Test
+	public void getAllProviderRoles_shouldGetAllProviderRolesExcludingRetired() {
+		List<ProviderRole> roles = service.getAllProviderRoles(false);
+		int roleCount = roles.size();


In this case we do not need the roleCount variable.

dkayiwa · 2025-06-13T11:39:16Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	}
+
+	@Test
+	public void getProviderRoleByUuid_shouldReturnNullIfNoProviderForUuid() {


If no provider or no provider role?

dkayiwa · 2025-06-13T11:41:10Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+		assertNotNull(saved.getProviderRoleId());
+		assertEquals("Some provider role", saved.getName());
+
+		ProviderRole fetched = service.getProviderRole(saved.getProviderRoleId());


What if this was already in the database even before you started saving?
Have you look at how other service methods test for saving?

dkayiwa · 2025-06-13T11:43:04Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+		assertTrue(retiredRole.getRetired());
+		assertEquals("test reason", retiredRole.getRetireReason());
+
+		List<ProviderRole> activeRoles = service.getAllProviderRoles(false);


This is not necessary after the above. But you want to keep it, then you need to assert that it originally contained it.

dkayiwa · 2025-06-13T11:43:49Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+		assertFalse(role.getRetired());
+		assertNull(role.getRetireReason());
+
+		List<ProviderRole> activeRoles = service.getAllProviderRoles(false);


Same as above.

dkayiwa · 2025-06-13T11:44:23Z

api/src/test/java/org/openmrs/api/ProviderServiceTest.java

+	@Test
+	public void unretireProviderRole_shouldUnretireProviderRole() {
+		ProviderRole role = service.getProviderRole(1002);
+		service.retireProviderRole(role, "testing retire");


This is better done by retrieving an already retired provider role.

mseaton · 2025-06-13T17:52:30Z

One additional thing I would hope / expect to see in this commit are service / DAO methods and/or changes that would allow getting all providers that have a particular role, and / or adding role to the list of search parameters that can be used to retrieve providers (and any other particularly useful service methods that are currently in the providermanagement module related to provider roles).

* TRUNK-6354: Add the ProviderRole domain object to core * add liquibase changesets for provider role * Update Liquibase changesets to use int instead of int(11) * Add fix as requested in reviews * remove the module prefix * use a more meaningful name for the foreign key * remove defaultvaluenumeric=0 from creator column

mherman22 force-pushed the TRUNK-6354-servicelayer branch from fd576d0 to fcc38f6 Compare June 12, 2025 15:06

mherman22 force-pushed the TRUNK-6354-servicelayer branch from 2a2d56d to cf31f12 Compare June 13, 2025 08:52

mherman22 mentioned this pull request Jun 13, 2025

TRUNK-6354: Add service layer tests for saving and retrieving provider roles #5071

Closed

5 tasks

mherman22 force-pushed the TRUNK-6354-servicelayer branch from cf31f12 to 9bae1f3 Compare June 13, 2025 09:12

dkayiwa reviewed Jun 13, 2025

View reviewed changes

mherman22 force-pushed the TRUNK-6354-servicelayer branch from 1c297f8 to 08c8416 Compare June 16, 2025 12:31

mherman22 mentioned this pull request Jun 16, 2025

Update adminui module to 2.8.0-SNAPSHOT openmrs/openmrs-module-adminui#67

Open

mherman22 force-pushed the TRUNK-6354-servicelayer branch from 94e1314 to 8946b97 Compare June 18, 2025 15:14

mherman22 force-pushed the TRUNK-6354-servicelayer branch 2 times, most recently from dd6f3ac to 775c52f Compare July 1, 2025 22:00

mherman22 added 10 commits July 2, 2025 14:37

TRUNK-6354: Add Provider Roles Service and Dao layer

f2970fc

utilize provider service/dao later instead of creating new classes

1ede62f

rename test dataset

4c4ea0d

Change provider roles methods in dao to use jpa criteria api

7a3c52b

adhere to omrs conventions

a233656

remove unused privilege

6eba377

fix privilege issues

e085398

improve the tests

a898f09

Add more service methods

f1188c4

fix javadoc on provider role

8d23519

mherman22 force-pushed the TRUNK-6354-servicelayer branch from 775c52f to 8d23519 Compare July 2, 2025 11:38

TRUNK-6354: Add Provider Roles Service and Dao layer #5073

Are you sure you want to change the base?

TRUNK-6354: Add Provider Roles Service and Dao layer #5073

Uh oh!

Conversation

mherman22 commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of what I changed

Issue I worked on

Checklist: I completed these to help reviewers :)

Uh oh!

dkayiwa commented Jun 12, 2025

Uh oh!

mherman22 commented Jun 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkayiwa Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkayiwa Jun 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseaton commented Jun 13, 2025

Uh oh!

Uh oh!

mherman22 commented Jun 12, 2025 •

edited

Loading

dkayiwa Jun 13, 2025 •

edited

Loading

dkayiwa Jun 13, 2025 •

edited

Loading