BED-23: Bed Tags should be uniquely identified by name by jwnasambu · Pull Request #89 · openmrs/openmrs-module-bedmanagement

jwnasambu · 2025-09-25T20:06:02Z

Description of what changed
I added a BedTagValidator to enforce validation rules on BedTag objects. This prevents creating two active bed tags with the same name, while still allowing duplicate names if the bed tags is expired. The validator also checks that the name is not null/empty/whitespace and respects field length constraints.

Issued worked on
https://openmrs.atlassian.net/browse/BED-23

wikumChamith · 2025-09-26T05:57:17Z

api/src/main/java/org/openmrs/validator/BedTagValidator.java

+			return;
+		}
+
+		List<BedTag> allTags = (List<BedTag>) bedManagementService.getAllBedTags();


Do we need to the cast here?

@wikumChamith Thanks for the review. I have made a fix.

wikumChamith · 2025-09-29T15:30:34Z

api/src/test/java/org/openmrs/validator/BedTagValidatorTest.java

+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;


Can we try to avoid using PowerMock?

@wikumChamith Kindly feel free to review my PR with the proposed changes at your convinient time please!

wikumChamith · 2025-09-30T15:58:00Z

api/src/test/java/org/openmrs/validator/BedTagValidatorTest.java

+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.*;


https://openmrs.atlassian.net/wiki/spaces/docs/pages/25477044/Java+Conventions#Do-not-use-wildcard-imports

@wikumChamith Thanks for the point out. I have fixed the proposed changes. Kindly feel free to review my PR at your convinient time please!

ibacher · 2025-09-30T20:35:51Z

api/src/main/java/org/openmrs/validator/BedTagValidator.java

+
+		List<BedTag> existingTags = bedManagementService.getAllBedTags();
+		for (BedTag existingTag : existingTags) {
+			boolean isVoidedOrExpired = existingTag.isVoided() || existingTag.getDateVoided() != null;


It should be sufficient just to check this:

Suggested change

boolean isVoidedOrExpired = existingTag.isVoided() || existingTag.getDateVoided() != null;

boolean isVoided = existingTag.getVoided();

There's no reasonable case where dateVoided is not null but the tag isn't voided.

@ibacher after fixing this proposed changes, I get a build failure which I belief is because the validator treats the expired tag as active. The getVoided() method does not correctly reflect that dateVoided is set and by replacing existingTag.getVoided() with existingTag.getDateVoided() != null to properly detect expired/voided tags which solves the build failure. I stand to be corrected.

ibacher · 2025-09-30T20:36:18Z

api/src/main/java/org/openmrs/validator/BedTagValidator.java

+		BedTag tag = (BedTag) target;
+
+		if (tag.getName() == null || tag.getName().trim().isEmpty()) {
+			errors.rejectValue("name", "bedtag.name.required", "Name is required");


The strings added here should be added to the messages.properties or else they will never properly be translatable.

api/src/main/java/org/openmrs/validator/BedTagValidator.java

api/src/test/java/org/openmrs/validator/BedTagValidatorTest.java

jwnasambu · 2025-10-01T19:47:57Z

@ibacher I Kindly need your guidance on one of the requests please?

ibacher · 2025-10-07T16:34:17Z

api/src/main/java/org/openmrs/validator/BedTagValidator.java

+			if (existingTag.getDateVoided() != null) {
+				isVoided = true;
+			}


Again this shouldn't be needed.

ibacher · 2025-10-07T16:34:58Z

api/src/main/java/org/openmrs/validator/BedTagValidator.java

+			if (!isVoided && existingTag.getName().equals(tag.getName())) {
+				errors.rejectValue("name", "bedtag.name.exists");
+				break;
+			}


This needs to verify that the existingTag is different from the tag that we are validating. That's why the test is failing...

jwnasambu · 2025-10-07T21:45:02Z

@ibacher, @wikumChamith Kindly I have fixed all the proposed changes. Feel free to review my PR at your convinient time please!

ibacher · 2025-10-17T13:22:01Z

api/src/main/java/org/openmrs/validator/BedTagValidator.java

+import org.springframework.validation.Validator;
+
+@Handler(supports = {BedTag.class}, order = 50)
+public class BedTagValidator implements Validator {


So, this class also need to be registered as a bean in the applicationContext.xml.

ibacher · 2025-11-26T18:51:45Z

api/src/main/java/org/openmrs/module/bedmanagement/service/impl/BedManagementServiceImpl.java

 * http://license.openmrs.org
 *
 * Software distributed under the License is distributed on an "AS IS"
+


Most of the reformatting here is harmless, but this line is incorrect.

ibacher · 2025-11-26T18:52:05Z

api/src/main/java/org/openmrs/module/bedmanagement/service/impl/BedTagMapServiceImpl.java

 		return bedTagMapDao.getBedTagByUuid(bedTagUuid);
 	}
+
+	 @Override


Suggested change

@Override

@Override

ibacher · 2025-11-26T18:53:04Z

api/src/main/java/org/openmrs/module/bedmanagement/service/BedTagMapService.java

 	@Authorized(value = { "Get Bed Tags", "Get Beds" }, requireAll = true)
 	BedTag getBedTagByUuid(String bedTagUuid);
+
+    BedTag getBedTagByName(String name);


Shouldn't this have an @Authorized annotation? But also, I don't think it belongs in this class at all.

ibacher · 2025-11-26T18:54:36Z

api/src/main/java/org/openmrs/validator/BedTagValidator.java

+
+		BedTag existing = bedManagementService.getBedTagByName(tag.getName().trim());
+
+		if (existing != null && existing.getDateVoided() == null && !existing.getUuid().equals(tag.getUuid())) {


I still think we should be checking existing.getVoided(). The dateVoided property is informational, but not the determining factor for whether a value is voided or not.

jwnasambu · 2025-11-26T21:44:39Z

@dkayiwa, @ibacher kindly feel free to review my pull request at your convenient time please!

jwnasambu · 2025-11-30T14:45:32Z

@dkayiwa Kindly feel free to review my PR at your convinient time please!

dkayiwa · 2025-11-30T19:47:55Z

Can you discard the formatting changes?

jwnasambu · 2025-12-03T12:05:10Z

@dkayiwa Kindly feel free to review my PR at your convinient time please!

dkayiwa · 2025-12-03T12:13:00Z

Can you remove all those formatting changes?

jwnasambu · 2025-12-03T23:00:39Z

@dkayiwa Kindly feel free to review my PR at your convinient time please!

dkayiwa · 2025-12-03T23:02:37Z

api/src/main/java/org/openmrs/module/bedmanagement/dao/impl/BedTagMapDaoImpl.java

 		this.sessionFactory = sessionFactory;
 	}
-	
+


Can you remove all these formatting changes?

jwnasambu · 2025-12-10T11:12:13Z

@dkayiwa I’ve tried resolving the requested changes, but each time I update the files, more formatting changes keep appearing in the commits. Could you please advise on the best way to handle or remove these formatting changes? I tried to create a new commit but still facing the same challenge as reflected on this link https://github.com/openmrs/openmrs-module-bedmanagement/compare/master...jwnasambu:BED-23b?expand=1

jwnasambu force-pushed the new-BED-23 branch from 4e0b290 to 7f3559c Compare September 25, 2025 21:11

jwnasambu mentioned this pull request Sep 25, 2025

BED-23: Bed Tags should be uniquely identified by name #87

Closed

jwnasambu requested review from dkayiwa, ibacher and wikumChamith September 25, 2025 21:19

wikumChamith reviewed Sep 26, 2025

View reviewed changes

jwnasambu force-pushed the new-BED-23 branch from a868895 to a4571ce Compare September 28, 2025 13:21

wikumChamith reviewed Sep 29, 2025

View reviewed changes

jwnasambu force-pushed the new-BED-23 branch from a4c9583 to 5d17ceb Compare September 30, 2025 13:41

wikumChamith requested changes Sep 30, 2025

View reviewed changes

jwnasambu force-pushed the new-BED-23 branch from 6613614 to 329bb74 Compare September 30, 2025 16:45

ibacher reviewed Sep 30, 2025

View reviewed changes

jwnasambu force-pushed the new-BED-23 branch 2 times, most recently from 2da590a to 940e7ed Compare October 1, 2025 15:04

jwnasambu force-pushed the new-BED-23 branch 2 times, most recently from 70b24f8 to c7fdf98 Compare October 7, 2025 15:09

ibacher reviewed Oct 7, 2025

View reviewed changes

jwnasambu force-pushed the new-BED-23 branch 4 times, most recently from e6c8513 to ac0c752 Compare October 7, 2025 21:40

jwnasambu requested review from ibacher and wikumChamith October 7, 2025 21:43

jwnasambu force-pushed the new-BED-23 branch 2 times, most recently from ddb1b92 to 493fc39 Compare October 7, 2025 22:10

ibacher reviewed Oct 17, 2025

View reviewed changes

jwnasambu force-pushed the new-BED-23 branch from 493fc39 to 6bf84f8 Compare October 17, 2025 13:35

jwnasambu marked this pull request as draft October 17, 2025 13:56

Merge branch 'master' into new-BED-23

16d52b8

ibacher reviewed Nov 26, 2025

View reviewed changes

jwnasambu requested a review from ibacher November 26, 2025 21:41

BED-23: Clean trailing whitespace in previous commit

65a3563

jwnasambu force-pushed the new-BED-23 branch from eaef1ec to 65a3563 Compare December 3, 2025 11:36

BED-23: Revert the changes

daea5e3

Bed-23: Fix whitespaces

9d3cd86

dkayiwa reviewed Dec 3, 2025

View reviewed changes

jwnasambu force-pushed the new-BED-23 branch 2 times, most recently from 20046b3 to 958884e Compare December 4, 2025 22:07

jwnasambu marked this pull request as draft December 4, 2025 22:49

jwnasambu force-pushed the new-BED-23 branch from 8ecd655 to 61bd393 Compare December 4, 2025 22:58

BED-23: Remove trailing whitespaces and formatting changes

bd493d7

jwnasambu force-pushed the new-BED-23 branch from f261a01 to bd493d7 Compare December 9, 2025 00:07

jwnasambu added 7 commits December 9, 2025 03:12

BED-23:Remove unnecessary blank lines in BedTagMapService

15b5dfa

BED-23: Format BedTagMapDao interface for consistency

fa386c5

BED-23: Refactor BedManagementDao methods for clarity

60d4a0d

BED-23: Refactor BedTagMapServiceImpl methods and imports

3c8f62b

Update BedTagMapServiceImpl.java

5e5fca7

Merge branch 'master' into new-BED-23

07afb3b

Update BedTagMapServiceImpl.java

d3228fe

Merge branch 'master' into new-BED-23

3fc952e

jwnasambu closed this Dec 11, 2025

	boolean isVoidedOrExpired = existingTag.isVoided() \|\| existingTag.getDateVoided() != null;
	boolean isVoided = existingTag.getVoided();


		BedTag existing = bedManagementService.getBedTagByName(tag.getName().trim());

		if (existing != null && existing.getDateVoided() == null && !existing.getUuid().equals(tag.getUuid())) {

Conversation

jwnasambu commented Sep 25, 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!

jwnasambu Oct 1, 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!

Uh oh!

Uh oh!

jwnasambu commented Oct 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwnasambu commented Oct 7, 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!

jwnasambu commented Nov 26, 2025

Uh oh!

jwnasambu commented Nov 30, 2025

Uh oh!

dkayiwa commented Nov 30, 2025

Uh oh!

jwnasambu commented Dec 3, 2025

Uh oh!

dkayiwa commented Dec 3, 2025

Uh oh!

jwnasambu commented Dec 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwnasambu commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jwnasambu Oct 1, 2025 •

edited

Loading

jwnasambu commented Dec 10, 2025 •

edited

Loading