OBPIH-4921 Improve list pages/filter component not to make unnecessar… #3526

kchelstowski · 2022-10-04T21:39:34Z

…y fetches of data (fixes after QA)

It has not been that easy straight forward as it was supposed to be, as it was not working propely on obnav and I had hard time to reproduce the issue locally, but it turned out not to be working (not always) when freshly running the project and having clean local storage. Many problems combined - the redux persist is not always working - it does not always "remember" the values - in this case it hardly ever remembers currentLocation when switching between React tabs and on the other hand if it does remember, it fetches the session data including currentLocation again, which makes the filters run again multiple times (multiple times if initially the redux persist REMEMBERED the value, so the fetch of data on list pages could continue, because currentLocation exists), but if it gets "fresh" (the same) currentLocation, it makes the fetch work again, so I had to come up with some solution to be able to "recognize" if:

redux persist remembers the currentLocation
To be able to determine if we initially fetched anything (so we do not fetch two times if redux persist remembered the currentLocatoin so the fetch could continue without "waiting" for another fetch of session info (FETCH_SESSION_INFO).
To be able to store the previous value of currentLocation, so if it is the same as previous one, it means we don't have to fetch data again

The code, especially those if's might seem complicated, but it was the only way to prevent each of those scenarios (you can just imagine the complexity of it, if anytime you run the project again, the redux persist gave different results of either storing the session info propely or not).

I know it's not possible to fetch the session data only once when running the project if we don't have 100% React , because in the "meantime" for example our user's role might change and we should be able to refresh that data ASAP, but I think we should consider dividing the session stuff which can't change "in the meantime" without your interference (and currentLocation might be one of many), so we can avoid those complicated solutions in some places.

…y fetches of data (fixes after QA)

jmiranda · 2022-10-05T03:37:37Z

@awalkowiak @kchelstowski Let's discuss this on the tech huddle tomorrow.

kchelstowski · 2022-10-05T16:07:37Z

src/js/components/invoice/InvoiceListFilters.jsx

+    // Avoid unnecessary re-fetches if getAppContext triggers fetching session info
+    // but currentLocation doesn't change
+    // eslint-disable-next-line max-len
+    if ((!initiallyFetched && currentLocation?.id) || (prevLocation && prevLocation.id !== currentLocation?.id)) {


it had to be done like this because:
you might have a situation when you have currentLocation in store and you go to another list page -

if we left only

if (currentLocation.id)

we would refetch twice - once having already stored location and second time when the getAppContext made a refetch.
2. if we left only

if (prevLocation.id !== currentLocation.id)

we wouldn't make a fetch if we have currentLocation already stored, because the prevLocation would be the currentLocation (that was in store) and the currentLocation (being fetched from getAppContext) would be the same what prevLocation so we wouldn't make a single fetch.
That's why I added initiallyFetched, so if we have a situation where we have currentLocation in store, we make only one fetch, because we don't care about the currentLocation (the same) being fetched in the background from the getAppContext

We want to remove the complexity if we can just get to a place where the redux hook is not invoked unnecessarily when the current location has not changed. Our suspicion is that the the app context (being a complex object) might be too complex for whatever is detecting whether the current location hsa changed. Or maybe I misread that.

Either way, one solution is adding a new API endpoint to retrieve just the current location and user IDs so that we can provide a simpler object to go through the useEffect hook to determine if the location changed.

For the issue mentioned above re: refetching twice vs not fetching at all ... I think I understand, but I have no idea how to do deal with that.

@awalkowiak mentioned that he will try to download the branch locally to test it out and see if there's an easy solution

what I meant above is that this fix works, but I just wanted it to be understandable for your trio why I had to do it like this, because initially I also thought that it would be enough to check only prevLocation vs currentLocation but I then found some cases when it wasn't enough.
I think that separating current location from appContext would help a lot, but I do not know if it's worth including it in this ticket. We will see if @awalkowiak finds an easier solution for that.

kchelstowski · 2022-10-06T19:25:02Z

src/js/components/purchaseOrder/PurchaseOrderListFilters.jsx

-    // but currentLocation doesn't change
-    // eslint-disable-next-line max-len
-    if ((!initiallyFetched && currentLocation?.id) || (prevLocation && prevLocation.id !== currentLocation?.id)) {
+    if (currentLocation?.id && buyers) {


had to include buyers there as well, because it could also trigger refetch (you could have currentLocation.id and fetch, but the buyers fetch was still in progress and it caused another refetch then).

kchelstowski · 2022-10-06T19:26:33Z

src/js/components/purchaseOrder/PurchaseOrderListFilters.jsx

@@ -170,12 +162,12 @@ const PurchaseOrderListFilters = ({
      fetchStatuses();
    }

-    if (!buyers || buyers.length === 0) {
+    if (!buyers) {


I changed initialValue of buyers to null, because having it as [] was a bad idea, because if we had situation, when we would fetch none organization and made the if above like this: buyers.length > 0 we would actually never fetch if we don't have buyers' organizations (even if we don't actually need them (we don't have central purchasing enabled).

kchelstowski · 2022-10-06T19:26:49Z

src/js/reducers/organizationsReducer.jsx

@@ -6,7 +6,7 @@ const initialState = {
  data: [],
  fetched: false,
  suppliers: [],
-  buyers: [],
+  buyers: null,


described above why I decided to change it

awalkowiak · 2022-10-07T08:44:50Z

src/js/MainRouter.jsx

-      this.props.fetchMenuConfig();
-      TRANSLATION_PREFIXES.forEach(prefix => this.props.fetchTranslations('', prefix));
+    this.props.fetchSessionInfo();
+    this.props.initialize({


On the initial load this.props.supportedLocales and this.props.locale will be empty and because you are not waiting for the data from this.props.fetchSessionInfo() you might wrongly initialize translations.

I probably forgot to change it back, as I was trying to find any possible solution for my issue. Will bring it back

awalkowiak · 2022-10-07T08:45:58Z

src/js/actions/index.js

-  return {
-    type: FETCH_SESSION_INFO,
-    payload: request,
+  return (dispatch) => {


Why you refactored this action?

because it is not how redux-thunk is supposed to work - I don't understand why we were passing request in dispatch instead of payload from this request - in some cases in turned out to be understood as "fetched" when I console.logged sessionReducer, but in reality the payload didn't come yet.

@kchelstowski makes sense, but then we have to refactor all remaining actions too. So I think there should be at least a TODO combined with a tech debt ticket to not forget about it.

awalkowiak · 2022-10-07T08:47:57Z

src/js/components/invoice/InvoiceListFilters.jsx

-  }, [currentLocation]);
+    // Avoid unnecessary re-fetches if getAppContext triggers fetching session info
+    // but currentLocation doesn't change
+    // eslint-disable-next-line max-len


Is it required? Looks like the next line is not that long.

Forgot to delete it after refactoring if to shorter version

awalkowiak · 2022-10-07T08:48:56Z

src/js/components/products/ProductsListFilters.jsx

-  }, []);
+    // Avoid unnecessary re-fetches if getAppContext triggers fetching session info
+    // but currentLocation doesn't change
+    // eslint-disable-next-line max-len


Same as above

awalkowiak · 2022-10-07T08:50:53Z

src/js/components/stock-movement/inbound/StockMovementInboundList.jsx

-  }, [props.currentLocation]);
+    // Avoid unnecessary re-fetches if getAppContext triggers fetching session info
+    // but currentLocation doesn't change
+    // eslint-disable-next-line max-len


Same as above

awalkowiak · 2022-10-07T08:51:05Z

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

-  }, [props.currentLocation]);
+    // Avoid unnecessary re-fetches if getAppContext triggers fetching session info
+    // but currentLocation doesn't change
+    // eslint-disable-next-line max-len


same as above

awalkowiak · 2022-10-07T08:52:03Z

src/js/hooks/usePrevious.js

@@ -0,0 +1,11 @@
+import { useEffect, useRef } from 'react';
+
+export default function usePrevious(value) {


What is it for? Is is used anywhere?

I also forgot to delete it after refactoring if. Should we consider keeping it anyway, as this hook might be useful in the future or should I delete it?

awalkowiak · 2022-10-07T08:52:44Z

webpack.config.js

@@ -120,6 +120,7 @@ module.exports = {
      templates: path.resolve(SRC, 'templates'),
      store: path.resolve(SRC, 'store'),
      css: path.resolve(ROOT, 'css'),
+      hooks: path.resolve(SRC, 'hooks'),


Are we using this anywhere?

OBPIH-4921 Improve list pages/filter component not to make unnecessar…

2f6e3a6

…y fetches of data (fixes after QA)

kchelstowski requested review from awalkowiak and drodzewicz October 4, 2022 21:39

kchelstowski commented Oct 5, 2022

View reviewed changes

OBPIH-4921 Refactor checking if location id changed

6511c86

kchelstowski commented Oct 6, 2022

View reviewed changes

awalkowiak requested changes Oct 7, 2022

View reviewed changes

kchelstowski requested a review from awalkowiak October 7, 2022 12:03

OBPIH-4921 Fixes after review

ef670a1

kchelstowski force-pushed the OBPIH-4921 branch from ee80baa to ef670a1 Compare October 7, 2022 12:08

drodzewicz approved these changes Oct 10, 2022

View reviewed changes

OBPIH-4921 Fix translation action to await for session info

1fa2b65

kchelstowski force-pushed the OBPIH-4921 branch from 8ebc088 to 1fa2b65 Compare October 10, 2022 09:56

awalkowiak approved these changes Oct 10, 2022

View reviewed changes

awalkowiak merged commit 99258ec into feature/ui-redesign Oct 10, 2022

awalkowiak deleted the OBPIH-4921 branch October 10, 2022 10:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OBPIH-4921 Improve list pages/filter component not to make unnecessar… #3526

OBPIH-4921 Improve list pages/filter component not to make unnecessar… #3526

kchelstowski commented Oct 4, 2022

jmiranda commented Oct 5, 2022

kchelstowski Oct 5, 2022 •

edited

jmiranda Oct 5, 2022

kchelstowski Oct 5, 2022

kchelstowski Oct 6, 2022

kchelstowski Oct 6, 2022

kchelstowski Oct 6, 2022

awalkowiak Oct 7, 2022

kchelstowski Oct 7, 2022

awalkowiak Oct 7, 2022

kchelstowski Oct 7, 2022

awalkowiak Oct 7, 2022

awalkowiak Oct 7, 2022

kchelstowski Oct 7, 2022

awalkowiak Oct 7, 2022

awalkowiak Oct 7, 2022

awalkowiak Oct 7, 2022

awalkowiak Oct 7, 2022

kchelstowski Oct 7, 2022

awalkowiak Oct 7, 2022

		@@ -0,0 +1,11 @@
		import { useEffect, useRef } from 'react';

		export default function usePrevious(value) {

OBPIH-4921 Improve list pages/filter component not to make unnecessar… #3526

OBPIH-4921 Improve list pages/filter component not to make unnecessar… #3526

Conversation

kchelstowski commented Oct 4, 2022

jmiranda commented Oct 5, 2022

kchelstowski Oct 5, 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

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

kchelstowski Oct 5, 2022 •

edited