Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OBPIH-5300 Fix statuses on list pages/reason codes to be refetched after change of locale #3780

Merged
merged 7 commits into from
Jan 19, 2023

Conversation

kchelstowski
Copy link
Collaborator

@kchelstowski kchelstowski commented Jan 13, 2023

The issue was exactly what I've thought about during our planning - the statuses/reason codes are stored by redux store, and once they are fetched, they weren't refetched when the locale was changed. Obviously the easiest way (but not correct, which I'll explain) would be to make useEffect reacting to each locale change, but that wouldn't be enough here. Why? Because we still have GSP side - if we would change the locale on the GSP side and then go to any list page on the React side (assuming that we've been on this list page already before and we once fetched the statuses), we wouldn't get the correct label of the statuses, because changing the language on the GSP side wouldn't make our React's useEffect being triggered.
Going into details, it wouldn't be that easy to create a useEffect reacting for locale change and refetching the statuses, because we'd have to do it e.g. in the MainRouter and we would have to refetch ALL the statuses, for every single list page at once (because how would I know on what list page will I go in the future potentially?), which would be a bad idea, why would I need a fetch of statuses for Invoice list if I weren't to go there?
I needed to come up with some other solution which for now doesn't even matter, because we still don't have full React, and whenever we change the page, we don't use SPA (single page application routing), but we make the page being "refreshed" after each redirect, which makes the getAppContext run whenever we change the page, which we will get rid of if we have full React - we would have to fetch the session info only once. This is also why Artur has added the redux-persist, because without it, we were losing whole Redux data after each redirect, which actually was a negation of Redux's goal.

My preposition was to add some kind of "counter" to the sessionReducer and all of list pages reducers called sessionVersion - its usage is very simple - whenever we fetch the session info or change the locale, we increment the counter for the sessionReducer and then going to any list page we compare the sessionReducer's counter with the list page's counter - if they are different, it means that we either changed the language before going to that list page or refetched the session info - so with that information I know if I have to refetch the statuses/reason codes to have a proper label. The advantage of that solution is that if I go to that list page again and my counters are equal, I DO NOT have to refetch the statuses.
Of course for now anyway we will have to, because we don't have full React yet, but I wanted this solution to be "ready" for the full React, so we will have less "fetching refactor" in the future.
I also wanted to avoid refetching statuses for all of the list pages whenever I change the locale, because it would be bad in terms of performance especially if I weren't even ever to go on a specific list page.

I also fixed the stock transfer list's statuses, so they are translatable by crowdin. There was also one more mystery about the reason codes on the backend side - the locale to translate the label was taken from the session.user.locale, but after my latest update to localization mode, we make the session.locale, not the session.user.locale "higher" in order.

Copy link
Member

@jmiranda jmiranda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to get a better understanding of the options so let's discuss this on the tech huddle tomorrow or Wednesday.

Copy link
Collaborator

@awalkowiak awalkowiak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this session version approach. Just store in redux the languages that we got statuses for (or the latest one) and refetch it (or not) depending on the current language comparing to this value stored. Or if the issue is caused by the redux-persist and that approach still won't work, then simply pull the statuses on entering the list with todo to go back to pulling from redux once we will have everything covered in react.

@kchelstowski
Copy link
Collaborator Author

kchelstowski commented Jan 17, 2023

@awalkowiak What about changing the approach of translating the statuses that I proposed yesterday, so we just return the key to the frontend side and we use this key inside <Translate> instead of translating statuses on the backend side?

 def statusOptions = {
        def options = OrderSummaryStatus.derivedStatuses()?.collect {
            [ id: it.name(), value: it.name(), label: "${g.message(code: 'enum.OrderSummaryStatus.' + it.name())}", variant: it.variant?.name()  ]
        }
        render([data: options] as JSON)
    }

instead of label being done like this, I'd just return something like label: "react.enum.OrderSummaryStatus." + it.name() so that whenever we change the language, the translations would be refetched?

@awalkowiak
Copy link
Collaborator

@kchelstowski yeah that should be fine, i forgot about this discussion.

@kchelstowski
Copy link
Collaborator Author

@awalkowiak I tried to go with my proposed solution, but seeing how much complexed code I have to change/to add in order for this to work, I decided not to do it for now - I just had to edit many filterFields to be able to translate the labels there (because I didn't wanna do it through the Select components, as I'd have to add much additional logic), and as those statuses are just a few JSONs, it doesn't cost that much, so for now I just fetch them always on first render (even if it's already stored in Redux) and refetch each time we change the language. I added a TODO so if we have full React, we can always optimize it by checking if the current language differs from the language when we were fetching the statuses for the first time.

@jmiranda
Copy link
Member

Refetching statuses instead of trying to keep state about whether statuses need to be refetched.

@awalkowiak awalkowiak merged commit 9f51eb4 into release/0.8.21 Jan 19, 2023
@awalkowiak awalkowiak deleted the OBPIH-5300 branch January 19, 2023 20:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants