OBPIH-5826 comment indication approvals comment by drodzewicz · Pull Request #4429 · openboxes/openboxes

drodzewicz · 2023-12-20T12:04:24Z

follow the discussion in the ticket

add derived property currentEvent to stock movement
use currentEvent to get the comment for a approval status if (status in approvalStatus) display currentEvent.comment
remove requisitonComments and recentRequisitionComments

… list table

kchelstowski · 2023-12-22T09:16:37Z

grails-app/controllers/org/pih/warehouse/shipping/ShipmentController.groovy

+        if (!shipmentInstance) {
+            throw new Exception("Could not locate shipment with ID " + params.shipmentId)
        }
+        User recipient = (params.recipientId) ? User.get(params.recipientId) : null


I know it's been there and you might've not realized that, but I think User.get(params.recipientId) should be enough.... unless there is a possibility that user's id would be null..

I did not pay much attention toit, I'll try to mak sure that recipient is not null and if everything will be fine I'll fix it to be just User.get(recipientId)

kchelstowski · 2023-12-22T09:18:13Z

grails-app/services/org/pih/warehouse/shipping/ShipmentService.groovy

        return referenceNumber
    }
+
+    Comment addShipmentComment(Shipment shipment, String content, User sender, User recipient) {


I don't mind it, but probably according to our code conventions, we shouldn't have more than 3 arguments (?) 🤔
I believe we need to figure out a way how to deal with such cases (command object??)

kchelstowski · 2023-12-22T09:18:54Z

src/js/components/dashboard/StockRequestDashboard.jsx

+          {row.original.displayStatus}
+        </span>
+        {row.original.recentComment && (
+        <Tooltip


nitpicky: indent

it's weird that ESLint did not throw an error on it, thanks for noticing

kchelstowski · 2023-12-22T09:21:09Z

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

            trackingNumber      : trackingNumber,
            driverName          : driverName,
            comments            : comments,
+            recentComment       : recentComment,


I agree with you something doesn't feel right about that - I mean, we only care about the shipmentComments - what if we decided to care about the requisition comments also?
Would we then want to distinguish shipment/requisition recent comment or how would we want to approach that?
I don't know the scope much to be able to answer that question.

yes, the whole comments hierarchy should be reevaluated like, which comments does the StockMovement inherit? The Shipment comments? Requisition comments? Order Comments?
Maybe we should mash all of those comments in a common list and extract the most recent one regardless of it's origin?

Yeah, we need to rewrite the APIs completely. This is a case where we've overloaded StockMovement to do so many things and it's showing signs of collapse.

Can you explain what "recentComment" is for? I know it's probably related to the requisition approval, but I want to know specific use case without digging through the code. I suspect this is a case where the codes needs to be more specific about what we're trying to accomplish i.e. getApprovalComment. And that makes me feel like we need to do something similar to synonyms where we need to classify comments according to their purpose instead of just getting the latest comment.

kchelstowski · 2023-12-22T09:22:19Z

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

+    Comment getRecentComment() {
+        if (shipmentComments?.size() > 0) {
+            return shipmentComments?.sort({ a, b ->
+                b.dateCreated <=> a.dateCreated


shouldn't we look at lastUpdated instead of dateCreated? Maybe I don't know the scope very well, but I see that the compareTo method in Comment looks at the lastUpdated date.

If we do want to look at lastUpdated, a titbit: you could just write .sort(), since we have compareTo overwritten.

yeah, this is up for debate what do we consider "the most recent comment" in my opinion it's based on date created since that's usually how it is sorted in many comment sections and messengers.

kchelstowski · 2023-12-22T09:25:18Z

grails-app/controllers/org/pih/warehouse/shipping/ShipmentController.groovy

-            flash.message = "${warehouse.message(code: 'shipping.addedCommentToShipment.message', args: [params.comment, shipmentInstance.name])}"
+        Shipment shipmentInstance = Shipment.get(params.shipmentId)
+        if (!shipmentInstance) {
+            throw new Exception("Could not locate shipment with ID " + params.shipmentId)


maybe let's come up with a more specific exception? using the general Exception doesn't feel right to me.
Maybe we should start creating our own exceptions and define them? e.g. NotFoundException?

I believe org.hibernate.ObjectNotFoundException could be a good candidate

kchelstowski · 2023-12-22T09:27:20Z

grails-app/controllers/org/pih/warehouse/shipping/ShipmentController.groovy

        }
+        User recipient = (params.recipientId) ? User.get(params.recipientId) : null
+        shipmentService.addShipmentComment(shipmentInstance, params.comment,  session.user, recipient)
+        flash.message = "${warehouse.message(code: 'shipping.addedCommentToShipment.message', args: [params.comment, shipmentInstance.name])}"


the problem with that is since the ShipmentController is @Transactional, you start and end the transaction boundary in the controller, hence if anything goes wrong while persisting the comment, you will still get the success message in the view, because the exception will happen at the end of the session, not while executing the 896th line.

I know we may have encountered issues like this in the past, but let's confirm this the actual behavior.

jmiranda · 2023-12-21T16:04:02Z

grails-app/controllers/org/pih/warehouse/shipping/ShipmentController.groovy

+            throw new Exception("Could not locate shipment with ID " + params.shipmentId)
        }
+        User recipient = (params.recipientId) ? User.get(params.recipientId) : null
+        shipmentService.addShipmentComment(shipmentInstance, params.comment,  session.user, recipient)


Remove extra space between comment and user.

jmiranda · 2023-12-21T16:05:39Z

grails-app/controllers/org/pih/warehouse/shipping/ShipmentController.groovy

-            flash.message = "${warehouse.message(code: 'shipping.addedCommentToShipment.message', args: [params.comment, shipmentInstance.name])}"
+        Shipment shipmentInstance = Shipment.get(params.shipmentId)
+        if (!shipmentInstance) {
+            throw new Exception("Could not locate shipment with ID " + params.shipmentId)


Let's pass the shipmentId instead of the instance and do the lookup in the service method.

Note: this comment was written weeks ago before I stumbled upon @kchelstowski's command object proposal. So this can probably be safely ignored if we go the command object route.

jmiranda · 2023-12-30T15:21:12Z

grails-app/controllers/org/pih/warehouse/shipping/ShipmentController.groovy

-            flash.message = "${warehouse.message(code: 'shipping.addedCommentToShipment.message', args: [params.comment, shipmentInstance.name])}"
+        Shipment shipmentInstance = Shipment.get(params.shipmentId)
+        if (!shipmentInstance) {
+            throw new Exception("Could not locate shipment with ID " + params.shipmentId)


jmiranda · 2023-12-30T15:22:26Z

grails-app/controllers/org/pih/warehouse/shipping/ShipmentController.groovy

        }
+        User recipient = (params.recipientId) ? User.get(params.recipientId) : null
+        shipmentService.addShipmentComment(shipmentInstance, params.comment,  session.user, recipient)
+        flash.message = "${warehouse.message(code: 'shipping.addedCommentToShipment.message', args: [params.comment, shipmentInstance.name])}"


I know we may have encountered issues like this in the past, but let's confirm this the actual behavior.

jmiranda · 2023-12-30T15:44:41Z

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

            trackingNumber      : trackingNumber,
            driverName          : driverName,
            comments            : comments,
+            recentComment       : recentComment,


Yeah, we need to rewrite the APIs completely. This is a case where we've overloaded StockMovement to do so many things and it's showing signs of collapse.

jmiranda · 2023-12-30T15:47:15Z

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

            trackingNumber      : trackingNumber,
            driverName          : driverName,
            comments            : comments,
+            recentComment       : recentComment,


Can you explain what "recentComment" is for? I know it's probably related to the requisition approval, but I want to know specific use case without digging through the code. I suspect this is a case where the codes needs to be more specific about what we're trying to accomplish i.e. getApprovalComment. And that makes me feel like we need to do something similar to synonyms where we need to classify comments according to their purpose instead of just getting the latest comment.

jmiranda · 2023-12-30T15:50:36Z

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

                driverName: shipment.driverName,
                trackingNumber: trackingNumber?.identifier,
                comments: shipment.additionalInformation,
+                shipmentComments: shipment.comments,


If possible, I would like this to replace the "comments" property at some point. We can move "comments" to "additionalInformation" or try to figure out how we might allow the additionalInformation to be come an instance within the "comments" list. This is where we'd need to have a way to classify comments as we'd only ever want one of these "additionalInformation" comments to be added to stockMovement.comments.

- remove previous approach with recentRequsiitionComment - return currentEvent with comment

drodzewicz added 2 commits December 19, 2023 15:02

OBPIH-5826 Fix shipment unable to save shipment comment

ccdf8cd

OBPIH-5826 Add recent comment on stock request on requestor dashboard…

8f19d67

… list table

drodzewicz added warn: do not merge Marks a pull request that is not yet ready to be merged status: needs discussion An issue that needs further discussion before it can proceed. labels Dec 20, 2023

drodzewicz added this to the 0.9.1 milestone Dec 20, 2023

drodzewicz self-assigned this Dec 20, 2023

drodzewicz changed the title ~~Obpih 5826 comment indication approvals comment~~ OBPIH-5826 comment indication approvals comment Dec 20, 2023

drodzewicz marked this pull request as ready for review December 21, 2023 09:44

drodzewicz requested review from alannadolny, awalkowiak, jmiranda and kchelstowski December 21, 2023 09:44

kchelstowski reviewed Dec 22, 2023

View reviewed changes

jmiranda reviewed Dec 30, 2023

View reviewed changes

drodzewicz added 2 commits January 10, 2024 15:47

OBPIH-5826 rename shipmentComments to requisitionComments

77af51a

OBPIH-5826 Fixes after review

c812bb3

drodzewicz requested a review from kchelstowski January 31, 2024 13:49

OBPIH-5826 Return currentEvent for shipment in a list

ff4fcbc

- remove previous approach with recentRequsiitionComment - return currentEvent with comment

drodzewicz force-pushed the OBPIH-5826-comment-indication-approvals-comment branch from b691ac3 to ff4fcbc Compare January 31, 2024 13:51

awalkowiak removed warn: do not merge Marks a pull request that is not yet ready to be merged status: needs discussion An issue that needs further discussion before it can proceed. labels Feb 1, 2024

awalkowiak approved these changes Feb 1, 2024

View reviewed changes

awalkowiak merged commit a376456 into feature/upgrade-to-grails-3.3.10 Feb 1, 2024

awalkowiak deleted the OBPIH-5826-comment-indication-approvals-comment branch February 1, 2024 13:09

Uh oh!

Conversation

drodzewicz commented Dec 20, 2023 • edited by awalkowiak Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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

drodzewicz commented Dec 20, 2023 •

edited by awalkowiak

Loading