OBPIH-6365 Add validation on quantity field by drodzewicz · Pull Request #4651 · openboxes/openboxes

drodzewicz · 2024-06-05T11:42:56Z

This PR was created to fix some points mentioned in #4650

As you can see it's not so straightforward to add a validation of type (not a number) on a quantity, let me explain.

So the case is that a user uploads a file with a potential "invalid" quantity number.
We want our API to process this data and return to the user all of the errors that come up in the File.

Now if we set quantity as a type Integer, we lose the ability to even get to the custom validators because it will throw an exception something along the lines of Can't cast String to an Integer in constructor when assigning this.quantity = quantity, which is why I changed the type of the quantity so we can assign the quantity and then our validator can validate the value.

Additionally, I have added a getQuantityAsNumber so we don't have to cast String to Integer every time we want to use this value, not sure if you guys will support this solution.

At the moment this is the best I could come up with so if you guys have any ideas then let me know

jmiranda

Let me know what you think. Also, if you've moved on I can work on the request changes as part of OBPIH-6331.

jmiranda · 2024-06-06T15:35:25Z

grails-app/controllers/org/pih/warehouse/picklist/ImportPickCommand.groovy

+            }
+            return true
+        })
+        quantityAsNumber(nullable: true)


Does this actually work?

More to the point, is this necessary to resolve some other issue? This feels like it might be related to the fact that the getQuantityAsNumber() getter should probably have a property defined in the transients list.

it was necessary because getQuantityAsNumber() returned a null and without it I got an error that getQuantityAsNumber cant be a nul value.
And as Kacper mentioned earlier, we don't have to define a transient for this getter

jmiranda · 2024-06-06T15:35:39Z

grails-app/controllers/org/pih/warehouse/picklist/ImportPickCommand.groovy

+                return ['notANumber.error']
+            }
+            if (Integer.parseInt(val) < 0) {
+                return ['negative.error']


Nice. I hadn't thought of that case.

jmiranda · 2024-06-06T15:36:20Z

grails-app/controllers/org/pih/warehouse/picklist/ImportPickCommand.groovy

-        quantity(nullable: false, min: 0)
+        quantity(nullable: false, validator: { val, obj ->
+            if (!NumberUtils.isNumber(val)) {
+                return ['notANumber.error']


Let's go with invalidNumber.error. I'll try to remember to make this change in OBPIH-6331.

jmiranda · 2024-06-06T15:39:42Z

grails-app/controllers/org/pih/warehouse/picklist/ImportPickCommand.groovy

        binLocation(nullable: true)
-        quantity(nullable: false, min: 0)
+        quantity(nullable: false, validator: { val, obj ->
+            if (!NumberUtils.isNumber(val)) {


I don't know the answer to this ... but make sure we're using the right class from the right library here. I remember there being two Apache Commons lang dependencies (lang, lang3).

Did a quick search and discovered that maybe this doesn't matter.
https://stackoverflow.com/questions/24158889/apache-commons-lang-2-vs-3

jmiranda · 2024-06-06T15:57:52Z

grails-app/controllers/org/pih/warehouse/picklist/ImportPickCommand.groovy

        expirationDate(nullable: true)
        binLocation(nullable: true)
-        quantity(nullable: false, min: 0)
+        quantity(nullable: false, validator: { val, obj ->


Could we do this instead?

Integer quantity quantity(nullable: false, validator: { val, obj -> if (!(val instanceof Integer)) { return ['invalid.error', val] } if (Integer.parseInt(val) < 0) { return ['negative.error', val] } })

I see what you mean now regarding this comment.

Now if we set quantity as a type Integer, we lose the ability to even get to the custom validators because it will throw an exception something along the lines of Can't cast String to an Integer in constructor when assigning this.quantity = quantity, which is why I changed the type of the quantity so we can assign the quantity and then our validator can validate the value.

So i think what you have is the right approach for now.

We could/should explore if there's a way to do some of the parsing / validation logic in the CSVMapReader to avoid these kinds of workarounds. Or use a different library.

But that's out of scope for this ticket.

This definitely sounds like something we should discuss in the design of importers ticket (OBID idea garden ticket)

jmiranda · 2024-06-06T15:59:56Z

grails-app/controllers/org/pih/warehouse/picklist/ImportPickCommand.groovy

+    }
+
+    Integer getQuantityAsNumber() {
+        return NumberUtils.isNumber(this.quantity) ? Integer.parseInt(this.quantity) : null


If we can define quantity as an Interger and handle the validation like I proposed then we should get rid of these methods.

jmiranda · 2024-06-06T16:01:22Z

grails-app/i18n/messages.properties

+importPickCommand.quantity.negative.error=Invalid quantity: "{2}". Quantity must not be a negative number.
 importPickCommand.requisitionItem.notFound.error=Requisition item with id: {0} not found
 importPickCommand.availableItem.notFound.error=The stock entry you selected: lot "{0}" and bin location "{1}" does not exist. Please review pick.
 importPickCommand.availableItem.notAvailable.error=The stock entry you selected: lot "{0}" and bin location "{1}" is not available. Please review pick.


In order to keep things consistent, let's change this to "unavailable" as well.

…ed quantity should be transient to avoid setters; fixed error code

jmiranda · 2024-06-06T19:06:08Z

Additionally, I have added a getQuantityAsNumber so we don't have to cast String to Integer every time we want to use this value, not sure if you guys will support this solution.

At the moment this is the best I could come up with so if you guys have any ideas then let me know

I pushed a commit that reverses those quantity mappings, so that the ugly property name is assigned the raw string field (String quantityAsText) rather than the field we'd like to expose (Integer quantity).

Otherwise, the PR looks good.

jmiranda · 2024-06-07T04:06:16Z

If/when this gets merged, please use a merge commit and NOT "squash and merge". I recreated my branch for OBPIH-6331 and I'm basing it off this branch.

kchelstowski

as far I as I understand the context, I'm fine with your solution, but for me, getting an exception "cannot cast ...." would be self-explanatory that I've done something wrong with the number.

kchelstowski · 2024-06-07T08:23:04Z

grails-app/controllers/org/pih/warehouse/picklist/ImportPickCommand.groovy

-    Integer quantity
+    String quantityAsText
+
+    static transients = ['quantity']


fyi: in Grails 3 we don't need to declare a transient anymore. The getter is enough.

I think I prefer transients over setters but we can discuss.

We don't need a setter neither. Since Grails 2.0 we can declare a getter without having to introduce a transient field:
https://docs.grails.org/6.1.2/ref/Domain%20Classes/transients.html

As of Grails 2.0 if there is only a getter or only a setter method, you don’t need to declare the property name of the method in the transients list. Only typed fields and get/set pairs that form a property but shouldn’t be persisted need to go in the transients list.

kchelstowski · 2024-06-07T08:24:37Z

src/main/groovy/org/pih/warehouse/api/StockMovementItem.groovy

    AVAILABLE, PICKED, RECALLED, HOLD, NOT_AVAILABLE

-    static getList() {
+    static list() {


have you made sure it is not used anywhere with the getList() version?

Yep I am sure, It was me who introduced this variable in the last PR

drodzewicz requested review from alannadolny, awalkowiak, ewaterman and jmiranda June 5, 2024 11:42

drodzewicz self-assigned this Jun 5, 2024

drodzewicz requested a review from kchelstowski June 5, 2024 11:56

drodzewicz force-pushed the OBPIH-6365-after-review-fixes branch from 9b2e651 to 64e0826 Compare June 5, 2024 11:58

OBPIH-6365 Add validation on quantity field

6b23e06

drodzewicz force-pushed the OBPIH-6365-after-review-fixes branch from 64e0826 to 6b23e06 Compare June 5, 2024 12:58

jmiranda reviewed Jun 6, 2024

View reviewed changes

OBPIH-6365 Reversed quantity property mappings for readability; deriv…

9eb3548

…ed quantity should be transient to avoid setters; fixed error code

kchelstowski reviewed Jun 7, 2024

View reviewed changes

awalkowiak merged commit 4e1b82f into develop Jun 7, 2024

awalkowiak deleted the OBPIH-6365-after-review-fixes branch June 7, 2024 14:37

Uh oh!

Conversation

drodzewicz commented Jun 5, 2024

Uh oh!

jmiranda left a comment

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!

jmiranda commented Jun 6, 2024

Uh oh!

jmiranda commented Jun 7, 2024

Uh oh!

kchelstowski left a comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants