OBPIH-5264 Create a custom hook for fetching logic on new list pages #3741

alannadolny · 2022-12-28T22:36:37Z

No description provided.

kchelstowski · 2022-12-29T09:42:59Z

src/js/hooks/list-pages/product/useProductsListTableData.jsx

+        .catch(() => {
+          setLoading(false);
+          return Promise.reject(new Error(translate('react.productsList.fetch.fail.label', 'Unable to fetch products')));
+        });


I know you are not the author of this, but could we maybe add finally, so we setLoading(false) only once?
Apply for each hook if the same thing is there

kchelstowski · 2022-12-29T09:43:46Z

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

+            currentParams: params,
+          });
+        })
+        .catch(() => Promise.reject(new Error(translate('react.stockMovement.outbound.fetching.error', 'Unable to fetch outbound movements'))));


I don't see setLoading(false) if an error occurs? Consider using finally

kchelstowski · 2022-12-29T09:46:20Z

src/js/hooks/list-pages/useTableData.jsx

+  }));
+
+  const fireFetchData = () => {
+    sourceRef.current = CancelToken.source();


bring back the comment that was here

kchelstowski · 2022-12-29T09:48:42Z

src/js/hooks/list-pages/useTableData.jsx

+    if (currentLocation?.id) {
+      sourceRef.current.cancel('Fetching canceled');
+    }
+  }, [currentLocation?.id]);


I think you removed return from this useEffect - it was supposed to be a cleanup function (return statement inside useEffect)

drodzewicz

I feel like we can move quite a lot of stuff into useTable hook.
Since every TableHook has a loading state a tableData state with data, pages, totalCount and currentParams we can make it more generic and move it into useTable hook

const [loading, setLoading] = useState(true);
const [tableData, setTableData] = useState({
    data: [],
    pages: -1,
    totalCount: 0,
    currentParams: {},
});

same with fetching table data,a ll it does it fetches the data and set it in tabelData state which means we can just pass the url and params to the useTable hook like:

const url = "openboxes/api/invoice
const parmas = { ... }
const {} = useTable({ url, params })

I see that each list page has it own unique error when fetch is unsuccessful so this can also be an optional parameter that is passed to the useTable hook like an errorMessage
cc @kchelstowski

drodzewicz · 2023-01-02T08:42:50Z

src/js/hooks/list-pages/useTableData.jsx

+            totalCount: res.data.totalCount,
+            pages: Math.ceil(res.data.totalCount / tableState.pageSize),
+            currentParams: params,
+            totalPrice: res.data.totalPrice,


I don't really like that totalPrice is defined in this hook. This is a specific data that from what I can tell is used in only one case which is Purchase Orders.
I think a solution to that could be having an optional callback onFetchedData(data) => { ... } that is passed to the useTable hook like

in useTable.jsx

.then((res) => { setTableData({ data: res.data.data, totalCount: res.data.totalCount, pages: Math.ceil(res.data.totalCount / tableState.pageSize), currentParams: params, }); if (onFetchedData) onFetchedData(res.data)

and in usePurchaseOrderData.jsx

const [totalPrice, setTotalPrice] = useState(0); const { ... } = useTable({ params, url, onFetchedData: (data) => setTotalPrice(data.totalPrice) }) .... return { ..., tableData: { ...tableData, totalPrice } }

I feel like we should keep useTable hook as generic as possible and avoid having to add some single case solutions because eventually this component will grow out of control and will become messy

drodzewicz · 2023-01-02T09:20:42Z

src/js/hooks/list-pages/product/useProductsListTableData.jsx

+  const url = '/openboxes/api/products';
+  const messageId = 'react.productsList.fetch.fail.label';
+  const defaultMessage = 'Unable to fetch products';
+  const getSortingParams = tableState => (tableState.sorted.length > 0 ?


I also feel like getSortingParams can be moved to useTableData.
The logic for this seems very generic across all of the use*TableData hooks and you would only need to pass the sortColumn: 'lasUpdated' and sortOrder: 'desc' as params

…nsfer list

kchelstowski · 2023-01-04T14:21:35Z

src/js/hooks/list-pages/inbound/useInboundListTableData.jsx

+import { translateWithDefaultMessage } from 'utils/Translate';
+
+const useInboundListTableData = (filterParams) => {
+  const messageId = 'react.stockMovement.inbound.fetching.error';


it should be named as errorMessageId

kchelstowski · 2023-01-04T14:24:56Z

src/js/hooks/list-pages/inbound/useInboundListTableData.jsx

+    ...sortingParams,
+  }, (value) => {
+    if (typeof value === 'object' && _.isEmpty(value)) return true;
+    return !value;


I think that this function's arguments should be as ( { offset, currentLocation, state, sortingParams }), because having so many arguments it might be difficult to follow the order of arguments. Making this change, you'll allow yourself not to pass them in the order that you have now.
Moreover I think that for better readability, it would be good to destructure filterParams, having:

const { receiptStatusCode, origin, destination, requestedBy, createdBy, updatedBy } = filterParams

and proceed with that, so we have:

origin: origin?.id

kchelstowski · 2023-01-04T14:25:49Z

src/js/hooks/list-pages/inbound/useInboundListTableData.jsx

+
+
+  useEffect(() => {
+    if (!isShipmentStatusesFetched || shipmentStatuses.length === 0) {


no need for length === 0 , could be only if (shipmentStatuses.length)

kchelstowski · 2023-01-04T14:31:24Z

src/js/hooks/list-pages/useTableData.jsx

+            pages: Math.ceil(res.data.totalCount / tableState.pageSize),
+            currentParams: params,
+          });
+          if (onFetchedData) onFetchedData(res.data);


try onFetchedData?.(res.data) instead of having this if - if it doesn't work (but it should), at least follow the convention we have, so using brackets

I didn't know that optional chaining works with functions. Thanks for that

kchelstowski · 2023-01-05T09:41:56Z

src/js/hooks/list-pages/inbound/useInboundListTableData.jsx

@@ -14,24 +14,32 @@ import exportFileFromAPI from 'utils/file-download-util';
 import { translateWithDefaultMessage } from 'utils/Translate';

 const useInboundListTableData = (filterParams) => {
-  const messageId = 'react.stockMovement.inbound.fetching.error';
+  const errorMessageId = 'react.stockMovement.inbound.fetching.error';
  const defaultMessage = 'Unable to fetch inbound movements';


changing from messageId to errorMessageId as I requested, you should also change the defaultMessage naming to defaultErrorMessage

kchelstowski · 2023-01-05T09:47:39Z

src/js/hooks/list-pages/useTableData.jsx

@@ -77,9 +81,10 @@ const useTableData = ({
            pages: Math.ceil(res.data.totalCount / tableState.pageSize),
            currentParams: params,
          });
-          if (onFetchedData) onFetchedData(res.data);
+          // eslint-disable-next-line no-unused-expressions


is that necessary?

Without this comment eslint shows that there is an unused expression

bring it back to if version then, but remember about brackets I mentioned earlier. We should avoid disabling eslint even though it is working fine. Maybe some kind of bumping up eslint even higher would help, but it's not the scope of this ticket, I just wanted to know out of curiosity if it would work so thanks for checking that

alannadolny requested review from drodzewicz and kchelstowski December 28, 2022 22:36

alannadolny force-pushed the OBPIH-5264 branch from 10ee1a4 to b39257c Compare December 29, 2022 08:45

kchelstowski requested changes Dec 29, 2022

View reviewed changes

alannadolny requested a review from kchelstowski December 29, 2022 14:18

kchelstowski approved these changes Dec 29, 2022

View reviewed changes

drodzewicz requested changes Dec 30, 2022

View reviewed changes

alannadolny requested a review from drodzewicz December 31, 2022 00:08

drodzewicz requested changes Jan 2, 2023

View reviewed changes

alannadolny requested a review from drodzewicz January 2, 2023 12:34

drodzewicz approved these changes Jan 2, 2023

View reviewed changes

jmiranda added the needs attention label Jan 3, 2023

alannadolny added 11 commits January 4, 2023 10:12

OBPIH-5264 add hook for PurchaseOrderListTable

cccee7e

OBPIH-5264 add hook for InvoiceListTable

73b799e

OBPIH-5264 add hook for StockListTable

658be94

OBPIH-5264 add hook for inbound, outbound, product list and stock tra…

626fbe2

…nsfer list

OBPIH-5264 remove unused values from mapStateToProps

6570856

OBPIH-5264 move multiple useStates to one object

0629065

OBPIH-5264 change hooks structure

6c5448d

OBPIH-5264 fix after review

56295c0

OBPIH-5264 fixes after review

eb63cd5

OBPIH-5264 fix after review

359bc39

OBPIH-5264 rebase

c97730f

alannadolny force-pushed the OBPIH-5264 branch from 56f9d03 to c97730f Compare January 4, 2023 11:36

kchelstowski requested changes Jan 4, 2023

View reviewed changes

OBPIH-5264 fix after review

efce888

alannadolny requested a review from kchelstowski January 4, 2023 20:32

kchelstowski requested changes Jan 5, 2023

View reviewed changes

OBPIH-5264 fix after review

38e57a7

alannadolny requested a review from kchelstowski January 5, 2023 10:14

kchelstowski approved these changes Jan 5, 2023

View reviewed changes

kchelstowski requested a review from drodzewicz January 5, 2023 10:14

jmiranda removed the needs attention label Jan 7, 2023

drodzewicz approved these changes Jan 9, 2023

View reviewed changes

jmiranda merged commit 9c07e72 into develop Jan 10, 2023

jmiranda deleted the OBPIH-5264 branch January 10, 2023 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-5264 Create a custom hook for fetching logic on new list pages #3741

OBPIH-5264 Create a custom hook for fetching logic on new list pages #3741

alannadolny commented Dec 28, 2022

kchelstowski Dec 29, 2022

kchelstowski Dec 29, 2022

kchelstowski Dec 29, 2022

kchelstowski Dec 29, 2022

drodzewicz left a comment

drodzewicz Jan 2, 2023

drodzewicz Jan 2, 2023

kchelstowski Jan 4, 2023

kchelstowski Jan 4, 2023

kchelstowski Jan 4, 2023

kchelstowski Jan 4, 2023

alannadolny Jan 4, 2023

kchelstowski Jan 5, 2023

kchelstowski Jan 5, 2023

alannadolny Jan 5, 2023 •

edited

alannadolny Jan 5, 2023

kchelstowski Jan 5, 2023 •

edited



		useEffect(() => {
		if (!isShipmentStatusesFetched \|\| shipmentStatuses.length === 0) {

OBPIH-5264 Create a custom hook for fetching logic on new list pages #3741

OBPIH-5264 Create a custom hook for fetching logic on new list pages #3741

Conversation

alannadolny commented Dec 28, 2022

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

drodzewicz 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

alannadolny Jan 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchelstowski Jan 5, 2023 • edited

Choose a reason for hiding this comment

alannadolny Jan 5, 2023 •

edited

kchelstowski Jan 5, 2023 •

edited