Skip to content

OBGM-554 Unable to save adjustments when add them to PO #4133

Merged
awalkowiak merged 2 commits intofeature/upgrade-to-grails-3.3.10from
OBGM-554
Jul 4, 2023
Merged

OBGM-554 Unable to save adjustments when add them to PO #4133
awalkowiak merged 2 commits intofeature/upgrade-to-grails-3.3.10from
OBGM-554

Conversation

@alannadolny
Copy link
Collaborator

@alannadolny alannadolny commented Jul 3, 2023

No description provided.

@alannadolny alannadolny self-assigned this Jul 3, 2023
kchelstowski
kchelstowski reviewed Jul 3, 2023
View reviewed changes
grails-app/views/purchaseOrder/_showOrderItems.gsp Outdated
|| order?.status == OrderStatus.PENDING)}"/>
<g:set
var="canManageAdjustments"
value="${(order?.status >= OrderStatus.PLACED && isApprover) || (order?.status == OrderStatus.PENDING)}"
Copy link
Collaborator

@kchelstowski kchelstowski Jul 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't know the context, but it feels like this order?.status only works "by a fortune". It seems to be comparing statuses by alphabetic order, not by the sortOrder as it was probably meant to be.
Could you double check that?
In my opinion it works by a fortune, but in general it should be written using:

OrderStatus.compare(order?.status, OrderStatus.PLACED)

Be aware, that by using the compare method, a NullPointerException might be thrown if order.status is null, because we don't have null safe operators there:

openboxes/src/groovy/org/pih/warehouse/order/OrderStatus.groovy

Lines 33 to 35 in 1e77b28

static int compare(OrderStatus a, OrderStatus b) {
return a.sortOrder <=> b.sortOrder
}

Copy link
Collaborator Author

@alannadolny alannadolny Jul 4, 2023

Choose a reason for hiding this comment

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

Yes, it was compared by alphabetical order (>= placed was returning correct values -> placed, received, rejected). I changed it as you suggested to avoid future problems connected to the order of those statuses.

Copy link
Member

@jmiranda jmiranda Jul 7, 2023

Choose a reason for hiding this comment

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

It seems to be comparing statuses by alphabetic order, not by the sortOrder as it was probably meant to be.

I don't think this is true. Or at least it was not true in Grails 1.

The only reason it might be true is if within a GSP the order.status is somehow not being coerced into an OrderStatus enum and Grails thinks it's a String, forcing the OrderStatus.PLACED and OrderStatus.PENDING to be compared as Strings.

@alannadolny alannadolny requested a review from kchelstowski July 4, 2023 08:10
@awalkowiak awalkowiak merged commit 8318fb2 into feature/upgrade-to-grails-3.3.10 Jul 4, 2023
@awalkowiak awalkowiak deleted the OBGM-554 branch July 4, 2023 11:10
jmiranda
jmiranda reviewed Jul 7, 2023
View reviewed changes
Copy link
Member

@jmiranda jmiranda left a comment

Choose a reason for hiding this comment

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

I'd like for someone to review this and confirm the "enums compared as strings" behavior. We have enum comparisons all over the place so it would be annoying to have to change this everywhere if it's not necessary.

@jmiranda
Copy link
Member

jmiranda commented Jul 7, 2023

In Java 7, the following is true regarding enum compareTo.

The natural order implemented by this method is the order in which the constants are declared.

https://docs.oracle.com/javase/7/docs/api/java/lang/Enum.html#compareTo%28E%29

That means the order defined in the OrderStatus enum is the natural order. So this change was unnecessary since the order is correct here.

    PENDING(10, StatusType.WARNING),
    APPROVED(20, StatusType.PRIMARY),
    PLACED(30, StatusType.PRIMARY),
    PARTIALLY_RECEIVED(40, StatusType.PRIMARY),
    RECEIVED(50, StatusType.PRIMARY),
    COMPLETED(60, StatusType.SUCCESS),
    CANCELED(70, StatusType.DANGER),
    REJECTED(80, StatusType.DANGER)

Incidentally, this is also true for Java 8.

https://docs.oracle.com/javase/8/docs/api/java/lang/Enum.html#compareTo-E-

There might be something fishy going on with how the <g:set> tag is evaluating the expression, but comparing enums using relational operators (>, >=, ==, <=, <) is not the problem.

@jmiranda
Copy link
Member

jmiranda commented Jul 7, 2023

@awalkowiak @alannadolny @kchelstowski @drodzewicz

@kchelstowski
Copy link
Collaborator

kchelstowski commented Jul 7, 2023

@jmiranda that's very interesting. Thanks for pointing that out. I checked it in a console and it seems to be true what you've written. I remember we had a problem in some createCriteria where we compared the enum via ge/lt and it was giving some weird results (@awalkowiak should remember, as he caught it in one place).

Anyway to be always safe (e.g. accidentally we don't follow the sortOrder order while adding some new enum values or anything) I think we should override the compareTo methods to look at sort orders. What do you think guys?

@alannadolny alannadolny mentioned this pull request Oct 17, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jmiranda jmiranda jmiranda left review comments

@awalkowiak awalkowiak awalkowiak approved these changes

@kchelstowski kchelstowski kchelstowski approved these changes

@drodzewicz drodzewicz Awaiting requested review from drodzewicz

Assignees

@alannadolny alannadolny

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@alannadolny @jmiranda @kchelstowski @awalkowiak