OBPIH-4923 Fix URL redirects with filters in params #3538

drodzewicz · 2022-10-11T15:04:25Z

No description provided.

drodzewicz · 2022-10-11T15:12:27Z

src/js/components/Layout/menu/Menu.jsx

+          }
+          return true;
+        });
+        return !!foundURL;


I needed to refactor this method because it match URLs with path that had search parameters in different order.
The problem that I noticed was with INBOUND/OUTBOUND stock movement

In example in config for menuUrls we have an object where key is the name of the section and value is the array of urls that the section contains.
For Inbound section we have a url for inbound list of /openboxes/stockMovements/lis?direction=INBOUND.
In the old method it would select it as active it the current URL was exactly the same as specified in the array, so for example such a URL /openboxes/stockMovements/lis?status=PENDING&direction=INBOUND. would not be matched.

In my implementation of this active section checked I check if section has search params we make sure that current URL has all required params and if it does we return it as matched URL.

yeah, checking active section was supposed to work for those 95% of cases back then not to spend too much time on it back then as it was not a priority, but the refactor must've happened some day. Good job

drodzewicz · 2022-10-11T15:21:00Z

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

@@ -60,8 +61,12 @@ const StockMovementOutboundList = (props) => {
    const queryProps = queryString.parse(props.history.location.search);
    // IF VALUE IS IN A SEARCH QUERY SET DEFAULT VALUES
    if (queryProps.requisitionStatusCode) {
-      defaultValues.requisitionStatusCode = props.requisitionStatuses
-        .filter(({ value }) => queryProps.requisitionStatusCode.includes(value));


This is a refactor that I had to implement in many places of the new List Table pages.
The problem was the used include(...) method.
Unfortunately I did not consider this before but this method does not return a exact match but as it implies it returns true if item is included in a value.

~~basicaly if we have a list ['PARTIALLY_RECEIVED', 'PENDING'].includes('RECEIVED') it would return true~~

here I agree that includes is not the same as find

I jumped to conclusions too fast, .include(...) works perfectly fine for this case.
The problem was in the props all along (that when a single prop was received where an array is expected it searched the single prop string instead of array of a single element)

kchelstowski · 2022-10-11T15:41:50Z

src/js/components/Filter/FilterForm.jsx

@@ -57,7 +57,7 @@ const FilterForm = ({
    }
    const clearedFilterList = Object.keys(defaultValues)
      .reduce((acc, key) => {
-        if (ignoreClearFilters.includes(key)) {
+        if (ignoreClearFilters.find(filter => filter === key)) {


doesn't includes work in the same way here like find? Both of them would return true in this if, wouldn't they?

yes it does.
I jump to conclusions too fast

kchelstowski · 2022-10-11T15:43:23Z

src/js/components/Layout/menu/Menu.jsx

+          }
+          return true;
+        });
+        return !!foundURL;


yeah, checking active section was supposed to work for those 95% of cases back then not to spend too much time on it back then as it was not a priority, but the refactor must've happened some day. Good job

kchelstowski · 2022-10-11T16:15:29Z

src/js/components/products/ProductsList.jsx

-        : [queryProps.catalogId];
-      defaultValues.catalogId = catalogList
-        .filter(({ id }) => catalogIdList.includes(id))
+      const catalogIdList = getList(queryProps.catalogId).map(it => ({ id: it }));


isn't it a bit "overthought"? queryProps.catalogId should be an Array, so what is that getList function responsible here for? Wouldn't simple queryProps.catalogId ?? [] work here?
I actually think that this map is useless. Check below

this is not entirely true that queryProps.catalogId is always an array.
The problem with query params is that in a URL such as /openboxes/products?catalogId=1 it does not return an array but a single value and URL /openboxes/products?catalogId=1&catalogId=2 does return an array.

So the goal of this utils method getList is to awlays return an array no matter how many values are in the param.
I've been inspired to do this by a similar case that we have on backend
def statuses = params?.list("receiptStatusCode") where params.list(...) does exactly that.

kchelstowski · 2022-10-11T16:21:39Z

src/js/components/products/ProductsList.jsx

-      defaultValues.catalogId = catalogList
-        .filter(({ id }) => catalogIdList.includes(id))
+      const catalogIdList = getList(queryProps.catalogId).map(it => ({ id: it }));
+      defaultValues.catalogId = _.intersectionBy(catalogList, catalogIdList, 'id')
        .map(({ id, label }) => ({
          id, label, name: label, value: id,
        }));


and this .map should be useless, as

catalogList.filter(catalog => catalogIdList.some(id => id === catalog.id))

this would return filtered catalogList which already has id, name, value?

this map is not useless because endpoint for catalog list return an array of [ { id: '...', label: '...' } ]
same for tags and categories

kchelstowski · 2022-10-11T16:21:49Z

src/js/components/products/ProductsList.jsx

-      defaultValues.catalogId = catalogList
-        .filter(({ id }) => catalogIdList.includes(id))
+      const catalogIdList = getList(queryProps.catalogId).map(it => ({ id: it }));
+      defaultValues.catalogId = _.intersectionBy(catalogList, catalogIdList, 'id')


catalogList.filter(catalog => catalogIdList.some(id => id === catalog.id))

kchelstowski · 2022-10-11T16:24:09Z

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

@@ -60,8 +61,12 @@ const StockMovementOutboundList = (props) => {
    const queryProps = queryString.parse(props.history.location.search);
    // IF VALUE IS IN A SEARCH QUERY SET DEFAULT VALUES
    if (queryProps.requisitionStatusCode) {
-      defaultValues.requisitionStatusCode = props.requisitionStatuses
-        .filter(({ value }) => queryProps.requisitionStatusCode.includes(value));


here I agree that includes is not the same as find

drodzewicz self-assigned this Oct 11, 2022

drodzewicz commented Oct 11, 2022

View reviewed changes

drodzewicz requested review from kchelstowski and awalkowiak October 11, 2022 15:21

drodzewicz changed the title ~~OBPIH-4923 Modify match active Menu section method~~ OBPIH-4923 Fix URL redirects with filters in params Oct 11, 2022

kchelstowski requested changes Oct 11, 2022

View reviewed changes

OBPIH-4923 Modify match active Menu section method

f8eff8c

drodzewicz force-pushed the OBPIH-4923-fix-redirects-from-dashboard-indicators-to-new-lists-with-filters-applied branch from dd09e02 to f8eff8c Compare October 12, 2022 09:21

drodzewicz requested a review from kchelstowski October 12, 2022 09:31

kchelstowski approved these changes Oct 12, 2022

View reviewed changes

kchelstowski merged commit dcc43d8 into feature/ui-redesign Oct 12, 2022

kchelstowski deleted the OBPIH-4923-fix-redirects-from-dashboard-indicators-to-new-lists-with-filters-applied branch October 12, 2022 10:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-4923 Fix URL redirects with filters in params #3538

OBPIH-4923 Fix URL redirects with filters in params #3538

drodzewicz commented Oct 11, 2022

drodzewicz Oct 11, 2022

kchelstowski Oct 11, 2022

drodzewicz Oct 11, 2022 •

edited

kchelstowski Oct 11, 2022

drodzewicz Oct 12, 2022

kchelstowski Oct 11, 2022

drodzewicz Oct 12, 2022 •

edited

kchelstowski Oct 11, 2022

kchelstowski Oct 11, 2022

drodzewicz Oct 12, 2022

kchelstowski Oct 11, 2022

drodzewicz Oct 12, 2022

kchelstowski Oct 11, 2022

kchelstowski Oct 11, 2022

OBPIH-4923 Fix URL redirects with filters in params #3538

OBPIH-4923 Fix URL redirects with filters in params #3538

Conversation

drodzewicz commented Oct 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drodzewicz Oct 11, 2022 • edited

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 Oct 12, 2022 • edited

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

drodzewicz Oct 11, 2022 •

edited

drodzewicz Oct 12, 2022 •

edited