OBPIH-5854 New approval statuses in SM list pages - Approvals lists pages #4290

alannadolny · 2023-09-20T11:56:42Z

No description provided.

alannadolny · 2023-09-20T12:00:54Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

+    List<RequisitionStatus> getRequisitionStatusCodesList(boolean hasRequestApprovalActivity, boolean isRequestsList) {
+        if (isRequestsList) {
+            return hasRequestApprovalActivity ? RequisitionStatus.listRequisitionOptions() : RequisitionStatus.listOutboundOptions()
+        }
+
+        return hasRequestApprovalActivity ? RequisitionStatus.listOutboundOptionsWhenSupportingRequestApproval() : RequisitionStatus.listOutboundOptions()
+    }


The idea of this function is to use an appropriate list based on the list page that we are currently working on:

If we are on the requests list:
a) If our location supports request approval activity - add waiting for approval and approved
b) If our location doesn't support request approval - the options should be the same as we had before

If we are on the outbound list:
a) If our location supports request approval activity - add the approved status
b) If our location doesn't support request approval - the options should be the same as we had before

alannadolny · 2023-09-20T12:07:40Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

+        List options = statuses?.collect(mapStatusCodes)
+        List availableStatuses = RequisitionStatus.listAll().collect(mapStatusCodes)


I had to split options on the dropdown from statuses that we can have on the list page. In the frontend part, we used fetched statuses from the dropdown to display their names on the list, but when we removed the verified status we still had at least one stock movement with this status, and the error was thrown. So I split those two cases (We were trying to show the status that we didn't fetch). Regarding the validation with visible requests in "approved"/"waiting for approval" status - we have a ticket for this and I think this should be improved/resolved here

if options are still based on what getRequisitionStatusCodesList returns, can't we still run into this problem, since this method can filter out some of the statuses?

btw if I understand correctly - on the list we want to display SMs with some statuses that we can't filter by? E.g. On the list we'll have a SM with status "Verifying", but we would not be able to filter by this status?
If so, isn't it weird? Will it be handled in some other ticket or what?

Yup, you understand this correctly, the only ticket I can see is that one with validation https://pihemr.atlassian.net/browse/OBPIH-5857?atlOrigin=eyJpIjoiNjE4OTJhMzFmNTUwNDUzMzg2YWMxN2FiZmQwZWQ1MDUiLCJwIjoiamlyYS1zbGFjay1pbnQifQ
But I don't know if we have a ticket to handle the situation when we have a verifying status on the list when supporting request approval.

I think I understand the problem but not fully. To me, this approach doesn't seem right but maybe I am missing some context.
Anyway, we will probably need to discuss this part further.

alannadolny · 2023-09-20T12:09:41Z

src/js/hooks/list-pages/outbound/useOutboundFilters.jsx

+  useEffect(() => {
+    dispatch(fetchRequisitionStatusCodes(currentLocation?.id, isRequestsList));
+  }, [currentLocale, currentLocation, isRequestsList]);


When we have different statuses on the dropdown, we should fetch them when:

We are changing location (second location can not support requests approval, so this options should not be visible)

We are changing locale (different translations)

Changing list pages

kchelstowski

A general comment is that we probably need some comments for the RequisitionStatus methods.
In one place you use listRequisitionOptions, in other place listOutboundOptions, in other place listOutboundOptionsWhenSupportingRequestApproval, in other place listAll() - it's easy to get a headache while reading that and we should make sure to at least add comments above them to know, what they are supposed to exclude etc.
I agree the naming of them might say that, but for example - I don't know what we mean by listRequisitionOptions here, and what's the difference between this and listOutboundOptions since outbound is a requisition based SM 🤷‍♂️

kchelstowski · 2023-09-20T12:24:30Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

+        boolean isRequestsList = params.getBoolean("isRequestsList")
+        if (params.location) {
+            Location location = Location.get(params.location)
+            hasRequestApprovalActivity = location.supports(ActivityCode.APPROVE_REQUEST)


ideally, you could remove this hasRequestApprovalActivity flag and base it on what I've done in the draft PR #4288 - so to use location.approvalRequired transient.

kchelstowski · 2023-09-20T12:31:27Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

+        [CREATED, EDITING, APPROVED, PICKING, PICKED, CHECKING, ISSUED, CANCELED, PENDING, REQUESTED, DISPATCHED]
+    }
+
+    static listRequisitionOptions() {


I don't know if it's a good naming here - what do you mean by "Requisition" here?

I changed the naming here, but I am still open to better propositions

kchelstowski · 2023-09-20T12:32:45Z

src/js/actions/index.js

@@ -531,13 +531,13 @@ export function fetchShipmentStatusCodes() {
 }


-export function fetchRequisitionStatusCodes() {
+export function fetchRequisitionStatusCodes(locationId = null, isRequestsList = false) {


I don't mind the naming, but we used to distinguish outbound vs electronic request by using something connected to electronicType

never mind - I see that it is also used in the useOutboundFilters hook, but anyway, I'll leave this comment here

kchelstowski · 2023-09-20T12:35:16Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

+        List options = statuses?.collect(mapStatusCodes)
+        List availableStatuses = RequisitionStatus.listAll().collect(mapStatusCodes)


if options are still based on what getRequisitionStatusCodesList returns, can't we still run into this problem, since this method can filter out some of the statuses?

kchelstowski · 2023-09-20T12:39:09Z

src/js/actions/index.js

  return (dispatch) => {
-    apiClient.get('/openboxes/api/stockMovements/requisitionsStatusCodes')
+    apiClient.get(`/openboxes/api/stockMovements/requisitionsStatusCodes?location=${locationId}&isRequestsList=${isRequestsList}`)


I'm also wondering if the location property here should not be handled by the backend.
Since anyway it is always supposed to be a currentLocation

kchelstowski · 2023-09-20T13:50:03Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

    static listOutboundOptionsWhenSupportingRequestApproval() {
        [CREATED, EDITING, APPROVED, PICKING, PICKED, CHECKING, ISSUED, CANCELED, PENDING, REQUESTED, DISPATCHED]
    }

-    static listRequisitionOptions() {
+    // Options for request list when we are supporting request approval (Added approved and waiting for approval)


"we" => current location

kchelstowski · 2023-09-20T13:52:25Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

    static listOutboundOptionsWhenSupportingRequestApproval() {
        [CREATED, EDITING, APPROVED, PICKING, PICKED, CHECKING, ISSUED, CANCELED, PENDING, REQUESTED, DISPATCHED]
    }

-    static listRequisitionOptions() {
+    // Options for request list when we are supporting request approval (Added approved and waiting for approval)
+    static listRequestOptionsWhenSupportingRequestApproval() {
        [CREATED, EDITING, APPROVED, WAITING_FOR_APPROVAL, PICKING, PICKED, CHECKING, ISSUED, CANCELED, PENDING, REQUESTED, DISPATCHED]


looking at this stuff, I've now started to think if we should not refactor those statuses methods to:

create a common list method that would contain the intersection of all the methods

for particular methods just concat it with the statuses that we are adding, e.g: listCommon().concat([APPROVED])

or filter it if we want to exclude something

don't know if it should be part of this ticket, but I think it would improve the readability of those methods.

Good point, I think we can even implement some kind of a RequisitionStatusFactory to generate a list of the statuses based on some given parameters.
But for now, unless anyone disagrees I think we can proceed with the individual lists

kchelstowski · 2023-09-20T13:52:57Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

    static listOutboundOptionsWhenSupportingRequestApproval() {
        [CREATED, EDITING, APPROVED, PICKING, PICKED, CHECKING, ISSUED, CANCELED, PENDING, REQUESTED, DISPATCHED]
    }

-    static listRequisitionOptions() {
+    // Options for request list when we are supporting request approval (Added approved and waiting for approval)
+    static listRequestOptionsWhenSupportingRequestApproval() {


I'm stlil not sure about the naming for this one, but I don't have any suggestion so I'll leave it for others maybe

Yeah this is something that we could think about a bit

I assume we don't want to spend time thinking about naming this one? @awalkowiak ?

I do think we want to spend time thinking about naming.

drodzewicz · 2023-09-21T07:34:23Z

src/groovy/org/pih/warehouse/inventory/StockMovementStatusCode.groovy

@@ -18,8 +18,10 @@ enum StockMovementStatusCode {
    CREATED(0),
    REQUESTING(1, PENDING),
    REQUESTED(2, PENDING),
+    WAITING_FOR_APPROVAL(2),


Shouldn't this be PENDING_APPROVAL?
I saw Kacper has implemented these enums in #4288 so lets stick to the same name

drodzewicz · 2023-09-21T07:35:42Z

src/groovy/org/pih/warehouse/inventory/StockMovementStatusCode.groovy

    VALIDATING(2, PENDING),
    VALIDATED(3, PENDING),
+    APPROVED(3),


REJECTED is probably missing right?

I don't need this status here

drodzewicz · 2023-09-21T07:39:54Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

    static listOutboundOptionsWhenSupportingRequestApproval() {
        [CREATED, EDITING, APPROVED, PICKING, PICKED, CHECKING, ISSUED, CANCELED, PENDING, REQUESTED, DISPATCHED]
    }

-    static listRequisitionOptions() {
+    // Options for request list when we are supporting request approval (Added approved and waiting for approval)
+    static listRequestOptionsWhenSupportingRequestApproval() {
        [CREATED, EDITING, APPROVED, WAITING_FOR_APPROVAL, PICKING, PICKED, CHECKING, ISSUED, CANCELED, PENDING, REQUESTED, DISPATCHED]


Good point, I think we can even implement some kind of a RequisitionStatusFactory to generate a list of the statuses based on some given parameters.
But for now, unless anyone disagrees I think we can proceed with the individual lists

drodzewicz · 2023-09-21T07:42:48Z

src/js/actions/index.js

  return (dispatch) => {
-    apiClient.get('/openboxes/api/stockMovements/requisitionsStatusCodes')
+    apiClient.get(`/openboxes/api/stockMovements/requisitionsStatusCodes?isRequestsList=${isRequestsList}`)


let's use params attribute to pass parameters instead of template literals

Suggested change

apiClient.get(`/openboxes/api/stockMovements/requisitionsStatusCodes?isRequestsList=${isRequestsList}`)

apiClient.get('/openboxes/api/stockMovements/requisitionsStatusCodes', {

params: { isRequestsList },

})

awalkowiak · 2023-09-21T15:32:40Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

@@ -128,6 +139,8 @@ enum RequisitionStatus {
        return "(case status " +
            "when '${CREATED}' then ${CREATED.sortOrder} " +
            "when '${EDITING}' then ${EDITING.sortOrder} " +
+            "when '${PENDING_APPROVAL}' then ${PENDING_APPROVAL.sortOrder} " +


Kacper's PR might add the same thing, watch out for conflicts.

awalkowiak · 2023-09-21T15:43:33Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

+        render([data: options, allStatuses: allStatuses] as JSON)
+    }
+
+    List<RequisitionStatus> getRequisitionStatusCodesList(Boolean hasRequestApprovalActivity, Boolean isRequestsList) {


Would it be cleaner if it was one if + else if block in the action above? Is this used anywhere else? Plus you bring another confusion with hasRequestApprovalActivity for a moment I was wondering how this differs from isApprovalRequired

Yes, thank you. Please never name a variable after what it does. Always name the variable as close to the encapsulated behavior you're trying to capture. That way, if the behavior gets more complicated you don't rename hasRequestApprovalActivityAndHasSomeOtherActivity.

Instead, the variable should be isRequisitionApprovalEnabled or isApprovalRequired and if the logic of that becomes more complicated, we still really only care about whether we've enabled approvals. I might like @awalkowiak suggestion even more than mine.

Oh if "isRequestsList" is telling me where you are ("on the request list page") when you are making this request, then that's gotta go. There must be a better way of passing state than differentiating what page we're on. What if I needed these statuses on another page or the mobile app. Does that become "isRequestsListOrSomeOtherPageOrOnMobileApp"? What is the differentiating factor?

I might think about adding another enum RequisitionStatusWorkflow (??) to represent the different sub-workflows: RequestWorkflow, ApprovalWorkflow, PickPackWorkflow. Each status could have a workflow associated with it

CREATED(1, null, StatusType.SUCCESS, Workflow.REQUEST), EDITING(2, PENDING, StatusType.PRIMARY, Workflow.REQUEST), PENDING_APPROVAL(3, null, StatusType.WARNING, Workflow.APPROVAL), APPROVED(4, null, StatusType.SUCCESS, Workflow.APPROVAL), REJECTED(5, null, StatusType.DANGER, Workflow.APPROVAL), VERIFYING(6, PENDING, StatusType.WARNING, Workflow.REQUEST),

and you could pass a single workflow or a list of workflows to get the appropriate list back.

RequisitionStatus.listOptions(Workflow.ALL) RequisitionStatus.listOptions(Workflow.REQUEST) RequisitionStatus.listOptions([Workflow.REQUEST, Workflow.REQUEST_APPROVAL])

NOTE: I don't understand the need for listOutboundOptions. Could someone explain that to me?

Aside: This is one of the reasons I dislike the Stock Movement workflow so much. It's not just one workflow, it's anywhere from 3-5 different workflows, each with its own complex set of steps, that are being squashed into a single, barely usable flow.

awalkowiak · 2023-09-21T15:44:52Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

+        List options = statuses?.collect(mapStatusCodes)
+        List allStatuses = RequisitionStatus.listAll().collect(mapStatusCodes)
+
+        render([data: options, allStatuses: allStatuses] as JSON)


Why you can't just return [data: options] based on the case?

awalkowiak · 2023-09-21T15:45:59Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

    static listOutboundOptionsWhenSupportingRequestApproval() {
        [CREATED, EDITING, APPROVED, PICKING, PICKED, CHECKING, ISSUED, CANCELED, PENDING, REQUESTED, DISPATCHED]
    }

-    static listRequisitionOptions() {
+    // Options for request list when we are supporting request approval (Added approved and waiting for approval)
+    static listRequestOptionsWhenSupportingRequestApproval() {


Yeah this is something that we could think about a bit

kchelstowski · 2023-09-22T19:03:49Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

        }
-        render([data: options] as JSON)
+
+        return hasRequestApprovalActivity ? RequisitionStatus.listOutboundOptionsWhenSupportingRequestApproval() : RequisitionStatus.listOutboundOptions()


it'd be cleaner if you reverted the statement, so:

// If a location doesn't support the request approval, return listOutboundOptions no matter what list is displayed if (!hasRequestApprovalActivity) { return RequisitionStatus.listOutboundOptions() } // If supports request approval, check what type of list page it is and return appropriate statuses if (isRequestList) { return RequisitionStatus.listRequestOptionsWhenSupportingRequestApproval() } return RequisitionStatus.listOutboundOptionsWhenSupportingRequestApproval()

I prefer this version + changes in naming that @jmiranda suggested, than moving to the function higher + using if/else. So, I am leaving this part of the code in the version that you suggested, unless there is a strong opinion against it.

kchelstowski · 2023-09-22T19:05:10Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

    static listOutboundOptionsWhenSupportingRequestApproval() {
        [CREATED, EDITING, APPROVED, PICKING, PICKED, CHECKING, ISSUED, CANCELED, PENDING, REQUESTED, DISPATCHED]
    }

-    static listRequisitionOptions() {
+    // Options for request list when we are supporting request approval (Added approved and waiting for approval)
+    static listRequestOptionsWhenSupportingRequestApproval() {


I assume we don't want to spend time thinking about naming this one? @awalkowiak ?

kchelstowski · 2023-09-22T19:06:34Z

src/js/components/stock-movement/outbound/StockMovementOutboundTable.jsx

@@ -43,6 +43,8 @@ const StockMovementOutboundTable = ({
    deleteConfirmAlert,
  } = useOutboundListTableData(filterParams);

+  useTranslation('requisitionStatus');


nitpicky: I think this call should be placed in the useOutboundListTableData hook, but no need to change.

kchelstowski · 2023-09-22T19:09:29Z

src/js/components/stock-movement/outbound/StockMovementOutboundTable.jsx

+        >
+          <StatusIndicator
+            variant={row?.original?.statusVariant}
+            status={translate(`react.requisitionStatus.enum.${row?.original?.status}`, row?.original?.status)}


please make sure that the label will change when you change the language - I remember having difficult times with translate behavior when changing the language on list pages - most of the times I was just deciding to translate what I needed on the backend.

Eh, you're right, it doesn't work

jmiranda · 2023-09-25T04:38:36Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

+        render([data: options, allStatuses: allStatuses] as JSON)
+    }
+
+    List<RequisitionStatus> getRequisitionStatusCodesList(Boolean hasRequestApprovalActivity, Boolean isRequestsList) {


Yes, thank you. Please never name a variable after what it does. Always name the variable as close to the encapsulated behavior you're trying to capture. That way, if the behavior gets more complicated you don't rename hasRequestApprovalActivityAndHasSomeOtherActivity.

Instead, the variable should be isRequisitionApprovalEnabled or isApprovalRequired and if the logic of that becomes more complicated, we still really only care about whether we've enabled approvals. I might like @awalkowiak suggestion even more than mine.

jmiranda · 2023-09-25T04:41:44Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

-                    variant: it.variant.name
-            ]
+        Location currentLocation = Location.get(session.warehouse.id)
+        Boolean isRequestsList = params.getBoolean("isRequestsList")


If something isn't self-explanatory, let's add comments for all of the changes. For example, what is "isRequestsList" for?

Pretend I'm not reading the requirements in the tickets. Because I'm not.

jmiranda · 2023-09-25T04:44:47Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

+        Location currentLocation = Location.get(session.warehouse.id)
+        Boolean isRequestsList = params.getBoolean("isRequestsList")
+
+        List<String> statuses = getRequisitionStatusCodesList(currentLocation.isApprovalRequired(), isRequestsList).collect(mapStatusCodes)


I don't think we need "List" here. Feels like getOutboundRequisitionStatuses() or getOutboundRequisitionStatusCodes() might be enough. Although I honestly don't know what this code is trying to accomplish (even with the comment below).

jmiranda · 2023-09-25T04:48:39Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

+        render([data: options, allStatuses: allStatuses] as JSON)
+    }
+
+    List<RequisitionStatus> getRequisitionStatusCodesList(Boolean hasRequestApprovalActivity, Boolean isRequestsList) {


Oh if "isRequestsList" is telling me where you are ("on the request list page") when you are making this request, then that's gotta go. There must be a better way of passing state than differentiating what page we're on. What if I needed these statuses on another page or the mobile app. Does that become "isRequestsListOrSomeOtherPageOrOnMobileApp"? What is the differentiating factor?

jmiranda · 2023-09-25T04:50:57Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

+        return hasRequestApprovalActivity ? RequisitionStatus.listOutboundOptionsWhenSupportingRequestApproval() : RequisitionStatus.listOutboundOptions()
+    }
+
+    Closure mapStatusCodes = { RequisitionStatus status ->


Any chance we could move this as a static method on the enum?

jmiranda · 2023-09-25T04:51:44Z

grails-app/domain/org/pih/warehouse/inventory/OutboundStockMovementListItem.groovy

@@ -108,6 +108,7 @@ class OutboundStockMovementListItem implements Serializable {
                name                : name,
                description         : description,
                statusCode          : statusCode?.toString(),
+                statusVariant       : status?.variant?.name,


Can someone explain what variant is here?

The variant here is an indicator that can be as an example: "warning", "success", etc. It is represented as a colorful dot on our list pages:

I had to move it here, because of the implementation that we had. This implementation doesn't allow us to have different options on the filtering dropdown, than the statuses on the stock movement list. In this ticket I have to remove "Verified" from the filtering when the location supports "request approval", but on the current list we can have "verified" status

jmiranda · 2023-09-25T04:52:35Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

    static listOutboundOptionsWhenSupportingRequestApproval() {
        [CREATED, EDITING, APPROVED, PICKING, PICKED, CHECKING, ISSUED, CANCELED, PENDING, REQUESTED, DISPATCHED]
    }

-    static listRequisitionOptions() {
+    // Options for request list when we are supporting request approval (Added approved and waiting for approval)
+    static listRequestOptionsWhenSupportingRequestApproval() {


I do think we want to spend time thinking about naming.

jmiranda · 2023-09-25T05:09:37Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

+        render([data: options, allStatuses: allStatuses] as JSON)
+    }
+
+    List<RequisitionStatus> getRequisitionStatusCodesList(Boolean hasRequestApprovalActivity, Boolean isRequestsList) {


I might think about adding another enum RequisitionStatusWorkflow (??) to represent the different sub-workflows: RequestWorkflow, ApprovalWorkflow, PickPackWorkflow. Each status could have a workflow associated with it

CREATED(1, null, StatusType.SUCCESS, Workflow.REQUEST), EDITING(2, PENDING, StatusType.PRIMARY, Workflow.REQUEST), PENDING_APPROVAL(3, null, StatusType.WARNING, Workflow.APPROVAL), APPROVED(4, null, StatusType.SUCCESS, Workflow.APPROVAL), REJECTED(5, null, StatusType.DANGER, Workflow.APPROVAL), VERIFYING(6, PENDING, StatusType.WARNING, Workflow.REQUEST),

and you could pass a single workflow or a list of workflows to get the appropriate list back.

RequisitionStatus.listOptions(Workflow.ALL) RequisitionStatus.listOptions(Workflow.REQUEST) RequisitionStatus.listOptions([Workflow.REQUEST, Workflow.REQUEST_APPROVAL])

NOTE: I don't understand the need for listOutboundOptions. Could someone explain that to me?

Aside: This is one of the reasons I dislike the Stock Movement workflow so much. It's not just one workflow, it's anywhere from 3-5 different workflows, each with its own complex set of steps, that are being squashed into a single, barely usable flow.

jmiranda · 2023-09-25T05:11:40Z

grails-app/i18n/messages.properties

@@ -3481,6 +3484,22 @@ react.stockMovement.enum.AvailableItemStatus.PICKED=Picked
 react.stockMovement.enum.AvailableItemStatus.RECALLED=Recalled
 react.stockMovement.enum.AvailableItemStatus.HOLD=Hold
 react.stockMovement.enum.AvailableItemStatus.NOT_AVAILABLE=Not Available
+react.requisitionStatus.enum.CREATED=Created


Can we stop duplicating these? I don't understand why the react app can just use the existing key

enum.RequisitionStatus.STATUS

As I know we were using only those labels starting with "react" on the react side. For now, when I use our function for fetching translations, I have to pass "requisitionStatus" as an argument to fetch all of the translations that start with "react.requistionStatus...".

I am moving the translation to the backend because those translations don't work as @kchelstowski suggested in a comment. So, I am removing those duplications.

drodzewicz · 2023-09-25T10:24:15Z

src/groovy/org/pih/warehouse/requisition/RequisitionStatus.groovy

@@ -157,6 +158,15 @@ enum RequisitionStatus {
            "end)"
    }

+    static Closure mapStatusCodes = { RequisitionStatus status ->


I am wondering if we should specify what we are mapping to exactly.
Since we are mapping it to options maybe this should be called something like mapToOption or toOption and we can omit the "StatusCode" in the name since we are going to be calling it on RequisitionStatus so saying that we are mapping it to StatusCodes seems redundant, but that just my take.

To me this felts similar to what we are doing when mapping to JSON .toJson()

Good idea, thanks

drodzewicz · 2023-09-25T10:38:45Z

grails-app/domain/org/pih/warehouse/inventory/OutboundStockMovementListItem.groovy

        return [
                id                  : id,
                name                : name,
                description         : description,
                statusCode          : statusCode?.toString(),
                statusVariant       : status?.variant?.name,
+                statusLabel         : "${g.message(code: 'enum.RequisitionStatus.' + status.name())}",


Since status on OutboundStockMovementListItem is a RequisitionStatus, I feel like we can use the mapStatusCodes() or whatever the name for it will be, to extract the label from it.
I am not sure if it is going to work but lets try doing something like

Suggested change

statusLabel : "${g.message(code: 'enum.RequisitionStatus.' + status.name())}",

statusLabel : status.mapStatusCodes()?.label,

Yup, it works

jmiranda · 2023-09-25T15:26:21Z

src/js/hooks/list-pages/outbound/useOutboundFilters.jsx

    dispatch(fetchShipmentTypes());
  }, [currentLocale]);

+  useEffect(() => {
+    dispatch(fetchRequisitionStatusCodes(isRequestsList));
+  }, [currentLocale, currentLocation, isRequestsList]);


Should pass source type? Maybe not.

jmiranda

Minor changes if necessary otherwise LGTM

…pdown options

…nging location

…tend

awalkowiak · 2023-09-27T13:14:13Z

grails-app/controllers/org/pih/warehouse/api/StockMovementApiController.groovy

-        render([data: options] as JSON)
+        // If request approval is required, check what type of list it is and return appropriate statuses
+        if (isElectronicType) {
+            return RequisitionStatus.listRequestOptionsWhenSupportingRequestApproval()


Let's follow other namings and do it as listRequestOptionsWhenApprovalRequired and listOutboundOptionsWhenApprovalRequired

…ages (#4290) * OBPIH-5854 Add waiting for approval and approved statuses * OBPIH-5854 Add translations for new statuses * OBPIH-5854 Add fetching all available statuses and change sending dropdown options * OBPIH-5854 Make status colum wider and add fetching statuses when changing location * OBPIH-5854 Fixes after review * OBPIH-5854 Change comment in RequisitionStatus * OBPIH-5854 Fixes after review * OBPIH-5854 Remove fetching all statuses and add handling them on frontend * OBPIH-5854 Remove variant from mapping status codes * OBPIH-5854 Fix rejected status * OBPIH-5854 Fixes after review * OBPIH-5854 Rename static method in RequisitionStatus * OBPIH-5854 Change mapping status label in OutboundStockMovementListItem * OBPIH-5854 Change isElectronicType to sourceType * OBPIH-5854 Change naming of list methods in RequisitionStatus

alannadolny self-assigned this Sep 20, 2023

alannadolny commented Sep 20, 2023

View reviewed changes

alannadolny requested review from jmiranda, awalkowiak, drodzewicz and kchelstowski September 20, 2023 12:10

kchelstowski reviewed Sep 20, 2023

View reviewed changes

drodzewicz requested changes Sep 21, 2023

View reviewed changes

awalkowiak requested changes Sep 21, 2023

View reviewed changes

alannadolny requested review from awalkowiak and drodzewicz September 22, 2023 11:46

alannadolny force-pushed the OBPIH-5854 branch 2 times, most recently from 7c45247 to 4cb466d Compare September 22, 2023 14:31

kchelstowski reviewed Sep 22, 2023

View reviewed changes

jmiranda requested changes Sep 25, 2023

View reviewed changes

drodzewicz reviewed Sep 25, 2023

View reviewed changes

drodzewicz requested changes Sep 25, 2023

View reviewed changes

drodzewicz approved these changes Sep 25, 2023

View reviewed changes

jmiranda reviewed Sep 25, 2023

View reviewed changes

jmiranda approved these changes Sep 25, 2023

View reviewed changes

alannadolny added 9 commits September 26, 2023 13:17

OBPIH-5854 Add waiting for approval and approved statuses

b89ef87

OBPIH-5854 Add translations for new statuses

3a19246

OBPIH-5854 Add fetching all available statuses and change sending dro…

c32a7a3

…pdown options

OBPIH-5854 Make status colum wider and add fetching statuses when cha…

7aee6ee

…nging location

OBPIH-5854 Fixes after review

ffab132

OBPIH-5854 Change comment in RequisitionStatus

d3e24f4

OBPIH-5854 Fixes after review

9b2a180

OBPIH-5854 Remove fetching all statuses and add handling them on fron…

430ca55

…tend

OBPIH-5854 Remove variant from mapping status codes

b2586a0

alannadolny added 5 commits September 26, 2023 13:17

OBPIH-5854 Fix rejected status

d62ef42

OBPIH-5854 Fixes after review

be8bacb

OBPIH-5854 Rename static method in RequisitionStatus

fc0202c

OBPIH-5854 Change mapping status label in OutboundStockMovementListItem

5eb9add

OBPIH-5854 Change isElectronicType to sourceType

5a87c7b

alannadolny force-pushed the OBPIH-5854 branch from b869d4b to 5a87c7b Compare September 26, 2023 11:21

awalkowiak requested changes Sep 27, 2023

View reviewed changes

OBPIH-5854 Change naming of list methods in RequisitionStatus

a6a8b9f

awalkowiak approved these changes Sep 27, 2023

View reviewed changes

awalkowiak merged commit 18088af into develop Sep 27, 2023
3 checks passed

awalkowiak deleted the OBPIH-5854 branch September 27, 2023 13:34

		List options = statuses?.collect(mapStatusCodes)
		List availableStatuses = RequisitionStatus.listAll().collect(mapStatusCodes)

-    apiClient.get(`/openboxes/api/stockMovements/requisitionsStatusCodes?isRequestsList=${isRequestsList}`)
+    apiClient.get('/openboxes/api/stockMovements/requisitionsStatusCodes', {
+        params: { isRequestsList },
+    })

	statusLabel : "${g.message(code: 'enum.RequisitionStatus.' + status.name())}",
	statusLabel : status.mapStatusCodes()?.label,

OBPIH-5854 New approval statuses in SM list pages - Approvals lists pages #4290

OBPIH-5854 New approval statuses in SM list pages - Approvals lists pages #4290

Conversation

alannadolny commented Sep 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchelstowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmiranda Sep 25, 2023 • edited

Choose a reason for hiding this comment

jmiranda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmiranda Sep 25, 2023 •

edited