[OBPIH-6627] keep progress when reload on confirm page of full outbound import by drodzewicz · Pull Request #4802 · openboxes/openboxes

drodzewicz · 2024-08-23T13:17:06Z

✨ Description of Change

A concise summary of what is being changed. Please provide enough context for reviewers to be able to understand the change and why it is necessary. If the issue/ticket already provides enough information, you can put "See ticket" as the description.

Link to GitHub issue or Jira ticket: OBPIH-6627

Description:
A requested feature for the Full Outbound Import was to implement an ability for a user to refresh the page and not lose the filled-out form as we have in other workflows.

One big difference between full outbound import and other create stock movement workflows is that in the latter we save the data in the database and upon refreshing the page we full this created Pendning stock movement,but as for full outbound import, we are only saving it at the end so when we refresh the page we lose all the data.

To somehow save this filled-out data I decide to store it in the sessionStorage which is one of the browser storages that we can access.
I decided to use sessionStorage and localStorage because of the one main difference, sessionStorage expires once the tab is closed, unlike localStorage which has to be cleared manually. Since we are working with temporary data that we don't care to save for too long sessionStorage seemed like the approach to take.
To manage sessionStorage I implemented a custom useSessionStorage hook. kind of mimicking the useState

Another big change was a slight modification to the useWizzard hook.
Since we are dealing with page refreshes, we need to know what step we are refreshing, If user opens CONFIRM step and there is cached data we want to render that step with prefilled cached data, and if user opens DETAILS page and there is available cached data we want to assume that user wants to do a new import and we should not load cached data.

So I modified the useWizzard hook so we switch between steps based on the URL query parameter step instead of a value held in state

📷 Screenshots & Recordings (optional)

If this PR contains a UI change, consider adding one or more screenshots here or link to a screen recording to help reviewers visualize the change. Otherwise, you can remove this section.

- No screenshots -

📈 Test Plan

We require that all code changes come paired with a method of testing them. Please select which of the following testing approaches you've included with this change:

Frontend automation tests (unit)
Backend automation tests (unit, API, smoke)
End-to-end tests (Playwright)
Manual tests (please describe below)
Not Applicable

Description of test plan (if applicable):

✅ Quality Checks

Please confirm and check each of the following to ensure that your change conforms to the coding standards of the project:

The pull request title is prefixed with the issue/ticket number (Ex: [OBS-123] for Jira, [#0000] for GitHub, or [OBS-123, OBPIH-123] if there are multiple), or with [N/A] if not applicable
The pull request description has enough information for someone without context to be able to understand the change and why it is needed
The change has tests that prove the issue is fixed / the feature works as intended

…eters

… session storage

- move outbound import steps into a separate file - include label and name on requested by validation schema

…page refresh

kchelstowski · 2024-08-26T06:15:54Z

src/js/hooks/useSessionStorage.js

+  const setValue = (newValue) => sessionStorage.setItem(key, JSON.stringify(newValue));
+  const clearValue = () => sessionStorage.removeItem(key);
+
+  return [val, setValue, clearValue];


wouldn't it be better to return it as object, so we would be able to get rid of the confusing:

const [cachedData,]

it would get even more weird if we wanted to use the first and the third property:

const [cachedData, null, clearValue]

I modeled this useSessionStorage hook after useState where we destruct the value and setter from an array and accessing nth value like [,,clearFunction] = useSessionStorage() is rather common and is aligned with react conventions.

I don't feel like one solution is better than the other, it's what we prefer, both are correct.

ok, for me it just looks weird when we have a comma without anything after that, but if you feel like it's better, then I'm fine, it was not a change request

kchelstowski · 2024-08-26T06:17:24Z

src/js/components/stock-movement-wizard/outboundImport/OutboundImport.jsx

+    if (_.isEmpty(cachedData) && !is(OutboundImportStep.DETAILS)) {
+      navigateToStep(OutboundImportStep.DETAILS);
+    }
+  }, []);


let's move it to a separate small hook named e.g. useCachedData

shouldn't the cachedData (or any property of that, since this is probably an object that needs to be compared properly) be the dependency of the useEffect? Is there a possibility that those would be loaded/recognized after the useEffect is run?

values from localStorage and sessionStorage are not related to state and will not trigger the component rerender even if you put the in the dependency array.
Also these values are synchronous unlike useEffect which is asynchronous which means that these values will be available in the hook without a delay.

kchelstowski · 2024-08-26T06:22:24Z

src/js/components/stock-movement-wizard/outboundImport/OutboundImport.jsx

+    event.preventDefault();
+    submitMethod(getValues());
+  };
+


I kinda do not get this method. Why do we want to skip validation? And which validation exactly? Is it the frontend validation of the file from previous step? Even if so, how do you distinguish here whether the submit happens from the refreshed page or from regular usage of the workflow?

The problem is that I don't cache the uploaded file packingList which is a required value in the validation schema,
So after loading all of the values from sessionStorage I am missing the packingList File which prevents me from submitting the form.

I figured that there are no more fields that we need to fill out on the CONFIRM step and all of the validation should have happened in the previous step so we can assume that the data that we have on confirm step is correct and as a workaround we can skip the validation.

Ok, I supposed this was the case, thanks for explaining

kchelstowski · 2024-08-26T06:22:43Z

src/js/components/stock-movement-wizard/outboundImport/OutboundImportStep.js

+  CONFIRM: 'CONFIRM',
+};
+
+export default OutboundImportStep;


let's move it to consts directory, where we place all the enums.

kchelstowski · 2024-08-26T06:26:28Z