ZEN - Multiple bugfixes #231

sakshibobade21 · 2024-07-19T12:47:15Z

Proposed changes

This PR addresses several issues:

States of the stages not resetting correctly during a new reinstallation.
Unnecessary restart occurring when clicking 'Save and Close'.
Missing password dialog when clicking 'Save and Close'.
Job statement missing during new installation.
Incorrect status display in the connection tab.
Adding the warning dialog for new Installation

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Change in a documentation
Refactor the code
Chore, repository cleanup, updates the dependencies.
Breaking change (fix or feature that would cause existing functionality to not work as expected)

PR Checklist

Please delete options that are not relevant.

If the changes in this PR are meant for the next release / mainline, this PR targets a "staging" branch.
My code follows the style guidelines of this project (see: Contributing guideline)
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
New and existing unit tests pass locally with my changes
video or image is included if visual changes are made

My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works, or describe a test method below

Testing

Further comments

Signed-off-by: Sakshi Bobade <[email protected]>

skurnevich

Also when i use this PR "Upload Zowe PAX" never stops spinning, even when step is done and "Continue to Components Installation" button is unlocked, not sure why that happens but it wasn't like that when i tried PR 193

skurnevich · 2024-08-02T15:17:12Z

src/renderer/components/Home.tsx

-        {(showPasswordDialog && updatedConnection) && <Wizard initialization={false}/>}
-        {!showPasswordDialog && <Wizard initialization={false}/>}
+        {(showPasswordDialog && updatedConnection) && <Wizard initialization={isNewInstallation}/>}
+        {!showPasswordDialog && <Wizard initialization={isNewInstallation}/>}


These two lines look like over-complication, what is the use of updatedConnection? Why we can't use showPasswordDialog only?
Also there is inconsistency, if you resume progress after click Save & Close it will show the dialog, but if you close the app and click resume after re-opening it will start with Connection tab.
And line 73 has unused values const [showLoginDialog, setShowLogin] = useState(false); that are probably intended for the same purpose?

The following behavior is intentional:

If we click "Save & Close" and then resume in the same session, a dialog prompts for the password. Since we do not store the password in the app, we only ask for it.
Additionally, because it is the same session, the user does not need to see the entire connection tab—just the password is required.

If we restart the app (different session) and resume, the connection tab with all attributes is displayed before resuming.

This behavior originates from staging.

Great catch @skurnevich regarding the updatedConnection. This issue originates from staging and has been addressed in this PR now.

Thanks.

Additionally, I have removed unused states and variables in the Home component that originated from staging.

Ok thank you, sorry for me asking on things that are not part of this PR, but even if it is already in staging we still need to fix things that are not in a good shape.
So i do believe that we need to unify this different behavior for resuming the saved installation, because even if the app is reopened we sill know the host/port and user for that installation, so we just need to ask for password and validate the entire connection.
I can't see any reason to ask user for host or to change it, moreover if user gets to connection tab and changes the host for saved installation it will corrupt the entire installation as part of progress is done on a different machine.
So ok let's not have it as part of this PR as it does not belongs here, but i'll create an issue for fixing that.

Sure @skurnevich Thank you for the detailed review on my PRs.

We previously discussed these two different approaches for the resume. However, we can revisit and refine them further.
Thanks for creating the issue.

skurnevich · 2024-08-02T15:25:30Z

src/renderer/components/Home.tsx

    const handleClick = () => {
      let newInstallationArgs = installationSlice.getInitialState().installationArgs;
      if (id === "install") {
+        dispatch(setIsNewerInstallation(true));
+        setIsNewInstallation(true);


Having setIsNewInstallation and setIsNewerInstallation looks like an algorithmic issue, what is the reason to have both?

I added setIsNewerInstallation as a Redux state to be used globally across other stages.
The isNewInstallation is from v2.x/staging and it is not necessary.
I just updated the PR to rely solely on the Redux state.

Thanks!

skurnevich · 2024-08-02T15:27:19Z

src/renderer/components/common/Stepper.tsx

@@ -17,7 +17,7 @@ import StepLabel from '@mui/material/StepLabel';
 import Button from '@mui/material/Button';
 import Typography from '@mui/material/Typography';
 import { Link } from 'react-router-dom';
-import { selectConnectionStatus } from '../stages/progress/progressSlice';
+import { selectConnectionStatus, setConnectionStatus } from '../stages/progress/progressSlice';


setConnectionStatus does not seem to be used here, and should not be set from this component

Removed it.

Additionally, I have removed unused states, functions and variables in the Stepper component that originated from staging.

skurnevich · 2024-08-02T15:34:26Z

src/renderer/components/common/Stepper.tsx

@@ -29,25 +29,31 @@ import Warning from '@mui/icons-material/Warning';
 import CheckCircle from '@mui/icons-material/CheckCircle';
 import { TYPE_YAML, TYPE_OUTPUT, TYPE_JCL, INIT_STAGE_LABEL, REVIEW_INSTALL_STAGE_LABEL, UNPAX_STAGE_LABEL } from '../common/Utils';
 import { getProgress, getCompleteProgress, mapAndSetSkipStatus, mapAndGetSkipStatus } from '../stages/progress/StageProgressStatus';
-
+import {initProgressStatus} from '../stages/progress/progressConst';


So is this a set of defaults or replacement for the progress storage? Shouldn't defaults be part of the storage anyway?

Removed initProgressStatus

skurnevich · 2024-08-02T15:38:59Z

src/renderer/components/common/Stepper.tsx

 // TODO: define props, stages, stage interfaces
 // TODO: One rule in the store to enable/disable button

 export default function HorizontalLinearStepper({stages, initialization}:{stages: any, initialization?:boolean}) {

-  const connectionStatus = useSelector(selectConnectionStatus);
+  const connectionStatus = useState(useAppSelector(selectConnectionStatus));


Why the useSelector even appeared there over the useAppSelector? If it was by mistake i still can see it in the line 58 useSelector(selectConnectionStatus),
And again, any benefits of using useState here?

Ideally, it should be useAppSelector. This originated from staging, and I'm not sure how or when useSelector was added. I've fixed it in this PR. Additionally, useState is not necessary for connectionStatus here since it is not responsible for updating any state. So removed it.

skurnevich · 2024-08-02T15:42:16Z

src/renderer/components/common/Stepper.tsx


+  if(!isNewInstallation) {
+    completeProgress = getCompleteProgress();
+  }


I suggest to get rid of initProgressStatus, but even if not the ternary operator can be used here instead of 'completeProgress' re-assigning

Removed initProgressStatus. And added: const completeProgress = getCompleteProgress();
getCompleteProgress() takes care of it.

skurnevich · 2024-08-02T15:56:33Z

src/renderer/components/stages/progress/StageProgressStatus.ts

+  const keysArray = [progressStateKey, activeStateKey, planningStateKey, installationTypeKey, downloadUnpaxKey, datasetInstallationKey, apfAuthKey, securityKey, stcsKey, certificateKey, vsamKey, planningValidationDetailsKey, prevInstallationKey, skipStateKey, installationArgsKey]; 
+  keysArray.forEach(key => {
+    if(localStorage.getItem(key) !== null) {
+      localStorage.removeItem(key);


So all of above is happening just to reset the storage in case user click on New Zowe Installation when they have some Saved Installation? Can you share more details on how this works with the keys and ids?

The code is designed to reset various progress-related states and storage entries when a user initiates a new Zowe installation, even if they have saved installation data. The setProgressObjects function initializes all the necessary state objects to their default values, ensuring that the application starts fresh. The resetProgress function clears the relevant keys from local storage to remove any saved data associated with the previous installation. This process ensures that the application doesn't carry over any outdated or conflicting data from a previous setup. By using unique IDs based on the host and user, the code manages and isolates storage and state for different user sessions.

Ok, i still believe it is not correct, your idea is to split different sessions in the storage so user can continue with multiple instances installation is great and we should have it some eventually. But host and user combination is not enough to define a unique installation, for that we need to use unique session/instance id. Second, as of now it doesn't even work for me, when i save my session and then ask different user to start new one they still can see some values from my installation, and after that i can't return to my first installation as there is just one card for saved installation.
So in the current state i propose to keep multiple saved installations as a feature to be done in future, and just when user clicks on New Installation to show a warning popup that their previous progress will be deleted and after that actually delete all the contents of configuration storage

Yes, using just the host and username cannot fully distinguish between different installations, so this approach needs reworking. However, due to time constraints, we planned to proceed with this for the first release.
For future enhancements, we plan to support multiple saved installations.

In the meantime, I will implement a warning popup. Thanks again.

@skurnevich Added the warning dialog for the new installation

sakshibobade21 · 2024-08-07T07:28:32Z

Also when i use this PR "Upload Zowe PAX" never stops spinning, even when step is done and "Continue to Components Installation" button is unlocked, not sure why that happens but it wasn't like that when i tried PR 193

@skurnevich
The issue originates from v2.x/staging, and my PR specifically addresses this problem - #193

For reference, please see the attached screenshot:

Signed-off-by: Sakshi Bobade <[email protected]>

skurnevich · 2024-08-07T14:17:17Z

src/renderer/components/stages/progress/StageProgressStatus.ts

+  const keysArray = [progressStateKey, activeStateKey, planningStateKey, installationTypeKey, downloadUnpaxKey, datasetInstallationKey, apfAuthKey, securityKey, stcsKey, certificateKey, vsamKey, planningValidationDetailsKey, prevInstallationKey, skipStateKey, installationArgsKey]; 
+  keysArray.forEach(key => {
+    if(localStorage.getItem(key) !== null) {
+      localStorage.removeItem(key);


Ok, i still believe it is not correct, your idea is to split different sessions in the storage so user can continue with multiple instances installation is great and we should have it some eventually. But host and user combination is not enough to define a unique installation, for that we need to use unique session/instance id. Second, as of now it doesn't even work for me, when i save my session and then ask different user to start new one they still can see some values from my installation, and after that i can't return to my first installation as there is just one card for saved installation.
So in the current state i propose to keep multiple saved installations as a feature to be done in future, and just when user clicks on New Installation to show a warning popup that their previous progress will be deleted and after that actually delete all the contents of configuration storage

skurnevich · 2024-08-07T14:32:21Z

src/renderer/components/Home.tsx

-        {(showPasswordDialog && updatedConnection) && <Wizard initialization={false}/>}
-        {!showPasswordDialog && <Wizard initialization={false}/>}
+        {(showPasswordDialog && updatedConnection) && <Wizard initialization={isNewInstallation}/>}
+        {!showPasswordDialog && <Wizard initialization={isNewInstallation}/>}


Ok thank you, sorry for me asking on things that are not part of this PR, but even if it is already in staging we still need to fix things that are not in a good shape.
So i do believe that we need to unify this different behavior for resuming the saved installation, because even if the app is reopened we sill know the host/port and user for that installation, so we just need to ask for password and validate the entire connection.
I can't see any reason to ask user for host or to change it, moreover if user gets to connection tab and changes the host for saved installation it will corrupt the entire installation as part of progress is done on a different machine.
So ok let's not have it as part of this PR as it does not belongs here, but i'll create an issue for fixing that.

skurnevich · 2024-08-12T16:50:08Z

src/renderer/components/Dialogs/WarningDialog.tsx

+import { Box, Button, Dialog, DialogActions, DialogContent, DialogTitle, FormControl, TextField } from '@mui/material';
+import { useState } from 'react';
+
+const WarningDialog = ({onWarningDialogSubmit}: {onWarningDialogSubmit: any}) => {


This seems to be quite hardcoded again.
First it is better to have the WarningDialog.tsx in "common", no need to have separate structure for that.
Second we should have it as simple as possible, taking 3 properties "text", "onSubmit", "onCancel" and no logic inside, even visibility should be controlled in the parent component. In that case it could be reused in future in any other place. We actually can go even further and add 4th property to control dialog type, like error/warning/info etc.

skurnevich · 2024-08-12T16:53:38Z

src/renderer/components/Home.tsx

+import WarningDialog from './Dialogs/WarningDialog';
+import HomeCardComponent from './HomeCardComponent';
+import { ICard } from '../../types/interfaces';
+import { ROUTES } from '../../Routes/RouteConstant';


Similar with Routes, you have moved it out to a separate file structure Routes/RouteConstant.tsx which looks like an overkill to me, especially when there are just two links and routing is not actually needed, it is just a legacy leftover that should be cleaned up later. And definitely we do not need Routes outside of renderer.
It is fine to move constants that need to be shared between modules out to some distinct file, if there are not many, but in this case you can use something like generic constants.ts in the “renderer” or “src” folder.
Also cards constant still uses text "/wizard" on lines 46 and 53

skurnevich · 2024-08-12T17:31:20Z

src/renderer/components/Home.tsx

+    dispatch(setIsNewInstallation(status))
+    setNewInstallationClick(false);
+    setPreviousInstallation(!status);
+


Well, it does not clear previous information for me, it starts with a connection screen again, but all the data still there. I was going to recommend you to just drop the stores here, like every store has the deleteAll() function for that purpose, but when i tried that i realize it also has no effect until you close the app.
I believe we have some issues with the way we use storage, we duplicate it in memory and so just complicate things, creating multiple sources of data

skurnevich · 2024-08-12T17:41:50Z

src/renderer/components/stages/progress/StageProgressStatus.ts

@@ -10,126 +10,23 @@

 import { flatten, unflatten } from 'flat';
 import { ProgressState, PlanningState, InstallationType, ActiveState, DatasetInstallationState, InitSubStepsState, CertInitSubStepsState, PlanningValidationDetails, SkipState, InstallationArgs, DownloadUnpaxState} from '../../../../types/stateInterfaces';
+import { initProgressStatus, initInstallationTypeStatus, initDownloadUnpaxStatus, initActiveStatus, initPlanningStageStatus, initDatasetInstallationStatus, initApfAuthStatus, initSecurityInitStatus, initStcsInitStatus, initCertificateInitStatus, initVsamInitStatus, initPlanningValidationDetailsStatus, initStepSkipStatus, initInstallationArgsStatus } from './progressConst';


Why we even have StageProgressStatus.ts, why don't we use storage/ProgressStore.ts for that?

sakshibobade21 added 6 commits July 19, 2024 14:10

First commit

9a644e0

Signed-off-by: Sakshi Bobade <[email protected]>

Some code cleanup

0bb238d

Signed-off-by: Sakshi Bobade <[email protected]>

Addressing the changes for the new installation

ccf7108

Signed-off-by: Sakshi Bobade <[email protected]>

Adding the job statement on new installation

d97b60f

Signed-off-by: Sakshi Bobade <[email protected]>

reset connection status on new installation

3d1f1e4

Removing console logs

21692a6

Signed-off-by: Sakshi Bobade <[email protected]>

skurnevich mentioned this pull request Aug 2, 2024

Code Cleanup: Attempt 1 #234

Closed

14 tasks

skurnevich requested changes Aug 2, 2024

View reviewed changes

sakshibobade21 added 8 commits August 7, 2024 13:51

Updating the resume code

c1d74f4

Signed-off-by: Sakshi Bobade <[email protected]>

Updating the resume code

22c41f7

Signed-off-by: Sakshi Bobade <[email protected]>

Removing duplicate states for the newInstallation

a9897f7

Signed-off-by: Sakshi Bobade <[email protected]>

Resetting resume status

15e7661

Not initializing the state anymore in stepper

d57f685

Removing the useselector hook

3b2eb91

Removing the unused states

b9da937

Removing unused states, functions

0322389

sakshibobade21 requested a review from skurnevich August 7, 2024 11:18

Updating te fuction names

14421c0

skurnevich requested changes Aug 7, 2024

View reviewed changes

sakshibobade21 added 10 commits August 8, 2024 11:58

Merge remote-tracking branch 'origin' into bugfixes-restart

756dd98

Adding the new Warning Dialog

4727edb

Updating the Home comp

92ef950

Merge remote-tracking branch 'origin' into bugfixes-restart

d20c309

Adding the card component and updating the warning component

5a41955

Separating the code base

761551b

Home comp refactoring

028520b

Code organizing

1334659

Defining Routes

43299da

Updating the warning dialog

f4176a2

sakshibobade21 requested a review from skurnevich August 8, 2024 13:34

skurnevich requested changes Aug 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZEN - Multiple bugfixes #231

ZEN - Multiple bugfixes #231

sakshibobade21 commented Jul 19, 2024 •

edited

Loading

skurnevich left a comment

skurnevich Aug 2, 2024

sakshibobade21 Aug 7, 2024

sakshibobade21 Aug 7, 2024

sakshibobade21 Aug 7, 2024

skurnevich Aug 7, 2024

sakshibobade21 Aug 7, 2024

sakshibobade21 Aug 7, 2024

skurnevich Aug 2, 2024

sakshibobade21 Aug 7, 2024

skurnevich Aug 2, 2024

sakshibobade21 Aug 7, 2024

sakshibobade21 Aug 7, 2024

skurnevich Aug 2, 2024

sakshibobade21 Aug 7, 2024

skurnevich Aug 2, 2024

sakshibobade21 Aug 7, 2024 •

edited

Loading

skurnevich Aug 2, 2024 •

edited

Loading

sakshibobade21 Aug 7, 2024

skurnevich Aug 2, 2024

sakshibobade21 Aug 7, 2024 •

edited

Loading

skurnevich Aug 7, 2024

sakshibobade21 Aug 7, 2024

sakshibobade21 Aug 8, 2024

sakshibobade21 commented Aug 7, 2024

skurnevich Aug 7, 2024

skurnevich Aug 7, 2024

skurnevich Aug 12, 2024

skurnevich Aug 12, 2024

skurnevich Aug 12, 2024

skurnevich Aug 12, 2024

ZEN - Multiple bugfixes #231

Are you sure you want to change the base?

ZEN - Multiple bugfixes #231

Conversation

sakshibobade21 commented Jul 19, 2024 • edited Loading

Proposed changes

Type of change

PR Checklist

Testing

Further comments

skurnevich 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

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

sakshibobade21 Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

skurnevich Aug 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sakshibobade21 Aug 7, 2024 • edited Loading

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

sakshibobade21 commented Aug 7, 2024

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

sakshibobade21 commented Jul 19, 2024 •

edited

Loading

sakshibobade21 Aug 7, 2024 •

edited

Loading

skurnevich Aug 2, 2024 •

edited

Loading

sakshibobade21 Aug 7, 2024 •

edited

Loading