OBPIH-7001 Add “Active” column to Product export/import by SebastianLib · Pull Request #5556 · openboxes/openboxes

SebastianLib · 2025-10-16T08:12:13Z

✨ Description of Change

Link to GitHub issue or Jira ticket:
https://pihemr.atlassian.net/browse/OBPIH-7001

Description:

📷 Screenshots & Recordings (optional)

SebastianLib · 2025-10-16T08:18:17Z

grails-app/services/org/pih/warehouse/product/ProductService.groovy

+    Boolean parseCsvBooleanField(String value, int rowCount) {
+        // If the value is empty we treat it as true
+        if (!value) {
+            return true
+        }
+        String parsedValue = value.toLowerCase()
+        Set<String> validValues = ['true', 'false', '1', '0']
+        if (!(parsedValue in validValues)) {
+            throw new RuntimeException("Active field has to be either empty or a boolean value (true/false/1/0) at row " + rowCount)
+        }
+        return parsedValue in ['true', '1']


I'm wondering if we could do this in a better way 🤔

why not use String.toBoolean()?

This returns:

assert "true".toBoolean() == true assert "TRUE".toBoolean() == true assert " TrUe ".toBoolean() == true assert "1".toBoolean() == true assert "y".toBoolean() == true assert "yes".toBoolean() == true

and is false otherwise. Then all you need to do is:

return value == null ? true : value.toBolean()

But if you're not using it because you want to throw an error in case of invalid input, then a few suggestions:

make the method static and put it in a util class that we can re-use

do String parsedValue = value.trim().toLowerCase() (whitespace shouldn't matter)

add the following static constants:
Set<String> validBooleanValues = ['t', 'f', 'true', 'false', '1', '0', 'y', 'n', 'yes', 'no'] Set<String> trueBooleanValues = ['t, 'true', '1', 'y', 'yes']

codecov · 2025-10-16T08:25:54Z

Codecov Report

❌ Patch coverage is 2.70270% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 8.54%. Comparing base (1bb7314) to head (63e1936).
⚠️ Report is 153 commits behind head on develop.

Files with missing lines	Patch %	Lines
...es/org/pih/warehouse/product/ProductService.groovy	0.00%	27 Missing ⚠️
.../groovy/org/pih/warehouse/importer/CSVUtils.groovy	0.00%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             develop   #5556      +/-   ##
============================================
- Coverage       9.12%   8.54%   -0.59%     
+ Complexity      1170    1113      -57     
============================================
  Files            701     702       +1     
  Lines          45281   45346      +65     
  Branches       10851   10869      +18     
============================================
- Hits            4131    3873     -258     
- Misses         40497   40895     +398     
+ Partials         653     578      -75

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ewaterman · 2025-10-17T22:33:00Z

grails-app/services/org/pih/warehouse/product/ProductService.groovy

            // FIXME make relation to Constants.EXPORT_PRODUCT_COLUMNS explicit
            def row = [
                Id                  : product?.id,
+                Active              : product.active ?: Boolean.FALSE,


parseCsvBooleanField defaults to true. Why is this default different?

ewaterman · 2025-10-17T22:52:34Z

grails-app/services/org/pih/warehouse/product/ProductService.groovy

+    Boolean parseCsvBooleanField(String value, int rowCount) {
+        // If the value is empty we treat it as true
+        if (!value) {
+            return true
+        }
+        String parsedValue = value.toLowerCase()
+        Set<String> validValues = ['true', 'false', '1', '0']
+        if (!(parsedValue in validValues)) {
+            throw new RuntimeException("Active field has to be either empty or a boolean value (true/false/1/0) at row " + rowCount)
+        }
+        return parsedValue in ['true', '1']


why not use String.toBoolean()?

This returns:

assert "true".toBoolean() == true assert "TRUE".toBoolean() == true assert " TrUe ".toBoolean() == true assert "1".toBoolean() == true assert "y".toBoolean() == true assert "yes".toBoolean() == true

and is false otherwise. Then all you need to do is:

return value == null ? true : value.toBolean()

But if you're not using it because you want to throw an error in case of invalid input, then a few suggestions:

make the method static and put it in a util class that we can re-use

do String parsedValue = value.trim().toLowerCase() (whitespace shouldn't matter)

add the following static constants:
Set<String> validBooleanValues = ['t', 'f', 'true', 'false', '1', '0', 'y', 'n', 'yes', 'no'] Set<String> trueBooleanValues = ['t, 'true', '1', 'y', 'yes']

ewaterman · 2025-10-22T14:52:20Z

grails-app/utils/org/pih/warehouse/CsvUtil.groovy

+     */
+    static Boolean parseCsvBooleanField(String value, int rowCount) {
+        if (!value) {
+            return true


suggestion: it would probably be best for this to be a param so that it's configurable. You also might want to use StringUtils so that " " is also considered falsey:

static Boolean parseCsvBooleanField(String value, int rowCount, Boolean defaultValue) { if (StringUtils.isBlank(value)) { return defaultValue } ....

ewaterman · 2025-10-22T14:54:32Z

grails-app/utils/org/pih/warehouse/CsvUtil.groovy

+        String parsedValue = value.trim().toLowerCase()
+
+        if (!(parsedValue in validBooleanValues)) {
+            throw new RuntimeException("Active field has to be either empty or a boolean value (${validBooleanValues.join(', ')}) at row ${rowCount}")


nitpick: I think you can just do ${validBooleanValues} because the Set.toString() method will output properly.

Oh, I didn’t know that 😄 I thought it worked like in JavaScript and that you had to add .join().

Maybe it’s better to stick with .join(", "), because without it we display [t, f, true, false, 1, 0, y, n, yes, no], and we probably don’t want to display the square brackets. 🤔

What's wrong with the square brackets? Your error message adds round brackets so you could just remove those and let it use the square ones instead:

"Active field has to be either empty or a boolean value ${validBooleanValues} at row ${rowCount}"

ewaterman · 2025-10-22T14:56:05Z

grails-app/utils/org/pih/warehouse/CsvUtil.groovy

+
+class CsvUtil {
+    static final Set<String> validBooleanValues = ['t', 'f', 'true', 'false', '1', '0', 'y', 'n', 'yes', 'no']
+    static final Set<String> trueBooleanValues = ['t', 'true', '1', 'y', 'yes']


nitpick: please use all caps for constants: VALID_BOOLEAN_VALUES, TRUE_BOOLEAN_VALUES

Initially, that’s what I wanted to do, but when I noticed that you wrote it in camelCase in previous comment, I decided to do the same. I will change it.

SebastianLib requested review from alannadolny, awalkowiak, ewaterman and kchelstowski October 16, 2025 08:12

SebastianLib force-pushed the feature/OBPIH-7001 branch from 0fdc308 to bff08c2 Compare October 16, 2025 08:14

OBPIH-7001 Add “Active” column to Product export/import

55160bc

SebastianLib force-pushed the feature/OBPIH-7001 branch from bff08c2 to 55160bc Compare October 16, 2025 08:17

SebastianLib commented Oct 16, 2025

View reviewed changes

ewaterman reviewed Oct 17, 2025

View reviewed changes

OBPIH-7001 Corrections after code review

63dabd1

SebastianLib force-pushed the feature/OBPIH-7001 branch from 7619b36 to 63dabd1 Compare October 22, 2025 13:43

SebastianLib requested a review from ewaterman October 22, 2025 13:44

ewaterman reviewed Oct 22, 2025

View reviewed changes

OBPIH-7001 Corrections after code review

6146997

SebastianLib requested a review from ewaterman October 22, 2025 15:25

ewaterman approved these changes Oct 22, 2025

View reviewed changes

OBPIH-7001 Move parseCsvBooleanField method to CsvUtils file

63e1936

alannadolny merged commit 6783694 into develop Oct 24, 2025
7 checks passed

alannadolny deleted the feature/OBPIH-7001 branch October 24, 2025 08:45

Uh oh!

Conversation

SebastianLib commented Oct 16, 2025

✨ Description of Change

📷 Screenshots & Recordings (optional)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Oct 16, 2025 •

edited

Loading