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.

Sign up for GitHub

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

Jump to bottom

ZEN - Multiple bugfixes #231

Open
wants to merge 25 commits into
base: v2.x/staging
Choose a base branch
from
Open

ZEN - Multiple bugfixes #231

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
9a644e0
First commit
sakshibobade21 Jul 19, 2024
0bb238d
Some code cleanup
sakshibobade21 Jul 19, 2024
ccf7108
Addressing the changes for the new installation
sakshibobade21 Jul 19, 2024
d97b60f
Adding the job statement on new installation
sakshibobade21 Jul 19, 2024
3d1f1e4
reset connection status on new installation
sakshibobade21 Jul 19, 2024
21692a6
Removing console logs
sakshibobade21 Jul 19, 2024
c1d74f4
Updating the resume code
sakshibobade21 Aug 7, 2024
22c41f7
Updating the resume code
sakshibobade21 Aug 7, 2024
a9897f7
Removing duplicate states for the newInstallation
sakshibobade21 Aug 7, 2024
15e7661
Resetting resume status
sakshibobade21 Aug 7, 2024
d57f685
Not initializing the state anymore in stepper
sakshibobade21 Aug 7, 2024
3b2eb91
Removing the useselector hook
sakshibobade21 Aug 7, 2024
b9da937
Removing the unused states
sakshibobade21 Aug 7, 2024
0322389
Removing unused states, functions
sakshibobade21 Aug 7, 2024
14421c0
Updating te fuction names
sakshibobade21 Aug 7, 2024
756dd98
Merge remote-tracking branch 'origin' into bugfixes-restart
sakshibobade21 Aug 8, 2024
4727edb
Adding the new Warning Dialog
sakshibobade21 Aug 8, 2024
92ef950
Updating the Home comp
sakshibobade21 Aug 8, 2024
d20c309
Merge remote-tracking branch 'origin' into bugfixes-restart
sakshibobade21 Aug 8, 2024
5a41955
Adding the card component and updating the warning component
sakshibobade21 Aug 8, 2024
761551b
Separating the code base
sakshibobade21 Aug 8, 2024
028520b
Home comp refactoring
sakshibobade21 Aug 8, 2024
1334659
Code organizing
sakshibobade21 Aug 8, 2024
43299da
Defining Routes
sakshibobade21 Aug 8, 2024
f4176a2
Updating the warning dialog
sakshibobade21 Aug 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions src/renderer/components/Home.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@ import { Tooltip } from '@mui/material';
import installationImg from '../assets/installation.png'
import installationDryImg from '../assets/installation-dry-run.png'
import eventDispatcher from "../../services/eventDispatcher";
import { selectConnectionStatus} from './stages/progress/progressSlice';
import { selectConnectionStatus, setConnectionStatus} from './stages/progress/progressSlice';
import HorizontalLinearStepper from './common/Stepper';
import Wizard from './configuration-wizard/Wizard'
import { ActiveState } from '../../types/stateInterfaces';
import { getInstallationArguments, getPreviousInstallation } from './stages/progress/StageProgressStatus';
import { DEF_NO_OUTPUT, FALLBACK_SCHEMA, FALLBACK_YAML } from './common/Utils';
import { selectInstallationArgs, setInstallationArgs, installationSlice } from './stages/installation/installationSlice';
import { selectInstallationArgs, setInstallationArgs, installationSlice, setIsNewerInstallation } from './stages/installation/installationSlice';
import PasswordDialog from './common/passwordDialog';

// REVIEW: Get rid of routing
Expand Down Expand Up @@ -88,10 +88,13 @@ const Home = () => {

const makeCard = (card: ICard) => {
const {id, name, description, link, media} = card;

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

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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!

dispatch(setConnectionStatus(false));
newInstallationArgs = {...newInstallationArgs, dryRunMode: false};
} else if (id === "dry run") {
newInstallationArgs = {...newInstallationArgs, dryRunMode: true};
Expand Down Expand Up @@ -216,6 +219,7 @@ const Home = () => {
const resumeProgress = () => {
setShowWizard(true);
dispatch(setResumeProgress(true));
dispatch(setIsNewerInstallation(false));

if(connectionStatus) {
setShowPasswordDialog(true);
Expand Down Expand Up @@ -268,8 +272,8 @@ const Home = () => {
{showWizard &&
<>
{showPasswordDialog && <PasswordDialog onPasswordSubmit={confirmConnection}></PasswordDialog>}
{(showPasswordDialog && updatedConnection) && <Wizard initialization={false}/>}
{!showPasswordDialog && <Wizard initialization={false}/>}
{(showPasswordDialog && updatedConnection) && <Wizard initialization={isNewInstallation}/>}
{!showPasswordDialog && <Wizard initialization={isNewInstallation}/>}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The following behavior is intentional:

  1. 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.

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

This behavior originates from staging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

</>
}
</>
Expand Down
25 changes: 13 additions & 12 deletions src/renderer/components/common/Stepper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

import { useAppDispatch, useAppSelector } from '../../hooks';
import { selectNextStepEnabled, setLoading, setYaml } from '../configuration-wizard/wizardSlice';
import { selectActiveStepIndex, selectActiveSubStepIndex } from '../stages/progress/activeStepSlice';
Expand All @@ -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';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed initProgressStatus

import '../../styles/Stepper.css';
import { StepIcon } from '@mui/material';
import { getStageDetails } from '../../../services/StageDetails';
import { IResponse } from '../../../types/interfaces';
import { selectConnectionArgs, setPassword } from '../stages/connection/connectionSlice';
import { selectInstallationArgs } from '../stages/installation/installationSlice';
import { selectInstallationArgs, selectIsNewInstallation } from '../stages/installation/installationSlice';
// 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));
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@sakshibobade21 sakshibobade21 Aug 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.


const INIT_STAGE_ID = getStageDetails(INIT_STAGE_LABEL).id;
const REVIEW_STAGE_ID = getStageDetails(REVIEW_INSTALL_STAGE_LABEL).id;

const completeProgress = getCompleteProgress();
const isNewInstallation = useAppSelector(selectIsNewInstallation);

let completeProgress = {...initProgressStatus};

if(!isNewInstallation) {
completeProgress = getCompleteProgress();
}
Copy link
Collaborator

@skurnevich skurnevich Aug 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


const stageProgressStatus = [
useSelector(selectConnectionStatus),
completeProgress.planningStatus,
Expand All @@ -66,11 +72,9 @@ export default function HorizontalLinearStepper({stages, initialization}:{stages
completeProgress.certificateStatus,
completeProgress.launchConfigStatus
])



const [activeStep, setActiveStep] = initialization ? useState(0) : useState(useAppSelector(selectActiveStepIndex));
const [activeSubStep, setActiveSubStep] = initialization ? useState(0) : useState(useAppSelector(selectActiveSubStepIndex));
const [activeStep, setActiveStep] = isNewInstallation ? useState(0) : useState(useAppSelector(selectActiveStepIndex));
const [activeSubStep, setActiveSubStep] = isNewInstallation ? useState(0) : useState(useAppSelector(selectActiveSubStepIndex));
const [nextText, setNextText] = useState("Continue");
const [contentType, setContentType] = useState('output');
const [editorVisible, setEditorVisible] = useState(false);
Expand Down Expand Up @@ -239,9 +243,6 @@ export default function HorizontalLinearStepper({stages, initialization}:{stages
alertEmitter.emit('hideAlert');
eventDispatcher.emit('saveAndCloseEvent');
dispatch(setPassword(''));
// TODO: This is a workaround for same session + Save & Close + new install not resetting the Wizard properly.
// Fixed by reloading page. This is not ideal and should be investigated
window.location.reload();
}

const isNextStepEnabled = useAppSelector(selectNextStepEnabled);
Expand Down
5 changes: 2 additions & 3 deletions src/renderer/components/stages/Planning.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import { setYaml, setNextStepEnabled, setLoading, selectYaml } from '../configur
import { selectConnectionArgs, setConnectionArgs, setJobStatementVal } from './connection/connectionSlice';
import { setPlanningStatus, selectPlanningStatus } from './progress/progressSlice';
import { setZoweVersion, setInstallationArgs, selectInstallationArgs, selectZoweVersion } from './installation/installationSlice';
import { setJobStatement, setJobStatementValid, setJobStatementValidMsg, setLocationValidationDetails, setIsLocationValid, selectJobStatementValidMsg, selectLocValidationDetails } from "./PlanningSlice";
import { setJobStatement, setJobStatementValid, setJobStatementValidMsg, setLocationValidationDetails, setIsLocationValid, selectJobStatementValidMsg, selectLocValidationDetails, selectJobStatement } from "./PlanningSlice";
import { useAppDispatch, useAppSelector } from '../../hooks';
import { IResponse } from '../../../types/interfaces';
import { alertEmitter } from "../Header";
Expand Down Expand Up @@ -63,8 +63,7 @@ const Planning = () => {

const [jobHeaderSaved, setJobHeaderSaved] = useState(false);
const [isJobStatementUpdated, setIsJobStatementUpdated] = useState(false);
// const [jobStatementValue, setJobStatementValue] = useState(useAppSelector(selectJobStatement));
const [jobStatementValue, setJobStatementValue] = useState(getPlanningStageStatus()?.jobStatement);
const [jobStatementValue, setJobStatementValue] = useState(useAppSelector(selectJobStatement));

const [locationsValidated, setLocationsValidated] = useState(getPlanningStageStatus()?.isLocationValid || false);
const [isLocationsUpdated, setIsLocationsUpdated] = useState(false);
Expand Down
3 changes: 2 additions & 1 deletion src/renderer/components/stages/PlanningSlice.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import { createSlice, PayloadAction } from '@reduxjs/toolkit';
import { RootState } from '../../store';
import { setPlanningStageStatus, getPlanningStageStatus } from './progress/StageProgressStatus';
import { DEF_JOB_STATEMENT } from '../common/Utils';

export interface jobValidation {
jobStatement: string;
Expand All @@ -24,7 +25,7 @@ export interface locationValidation {
}

const initialState: jobValidation = {
jobStatement: getPlanningStageStatus()?.jobStatement || '',
jobStatement: getPlanningStageStatus()?.jobStatement || DEF_JOB_STATEMENT,
isJobStatementValid: getPlanningStageStatus()?.isJobStatementValid || false,
jobStatementValidMsg: ''
}
Expand Down
11 changes: 9 additions & 2 deletions src/renderer/components/stages/connection/Connection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,10 @@ import { setConnectionStatus, selectConnectionStatus} from '../progress/progres
import { Container } from "@mui/material";
import { alertEmitter } from "../../Header";
import { getStageDetails, initStageSkipStatus } from "../../../../services/StageDetails";
import { initializeProgress, getActiveStage, } from "../progress/StageProgressStatus";
import { initializeProgress, getActiveStage, resetProgress } from "../progress/StageProgressStatus";
import eventDispatcher from "../../../../services/eventDispatcher";
import { setLocationValidationDetails } from "../PlanningSlice";
import { selectInstallationArgs } from "../installation/installationSlice";
import { selectInstallationArgs, selectIsNewInstallation } from "../installation/installationSlice";

const Connection = () => {

Expand Down Expand Up @@ -125,13 +125,20 @@ const FTPConnectionForm = () => {
const [isFtpConnection, setIsFtpConnection] = useState(useAppSelector(selectConnectionSecure));
const [isAllCertificatesAccepted, setIsAllCertificatesAccepted] = useState(useAppSelector(selectAcceptAllCertificates));

const [isNewInstallation, setIsNewInstallation] = useState(useAppSelector(selectIsNewInstallation));
const [formProcessed, toggleFormProcessed] = React.useState(false);
const [validationDetails, setValidationDetails] = React.useState('');

const [isResume, setIsResume] = useState(useAppSelector(selectResumeProgress));

const installationArgs = useAppSelector(selectInstallationArgs);

useEffect(() => {
if(isNewInstallation) {
resetProgress(connectionArgs.host, connectionArgs.user);
}
}, []);

const handleFormChange = (ftpConnection?:boolean, acceptCerts?:boolean) => {
dispatch(setConnectionStatus(false));
dispatch(setNextStepEnabled(false));
Expand Down
8 changes: 7 additions & 1 deletion src/renderer/components/stages/installation/installationSlice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ interface InstallationState {
installationArgs: InstallationArgs;
zoweVersion: string;
licenseAgreement: boolean;
isNewInstallation?: boolean;
}

const initialState: InstallationState = {
Expand All @@ -42,6 +43,7 @@ const initialState: InstallationState = {
},
zoweVersion: '',
licenseAgreement: getInstallationTypeStatus()?.licenseAgreement || false,
isNewInstallation: false,
};

export const installationSlice = createSlice({
Expand All @@ -67,14 +69,18 @@ export const installationSlice = createSlice({
state.licenseAgreement = action.payload;
setInstallationTypeStatus('licenseAgreement', action.payload)
},
setIsNewerInstallation: (state, action: PayloadAction<boolean>) => {
state.isNewInstallation = action.payload;
}
}
});

export const { setInstallationArgs, setZoweVersion, setInstallationType, setLicenseAgreement, setUserUploadedPaxPath} = installationSlice.actions;
export const { setInstallationArgs, setZoweVersion, setInstallationType, setLicenseAgreement, setUserUploadedPaxPath, setIsNewerInstallation} = installationSlice.actions;

export const selectInstallationArgs = (state: RootState) => state.installation.installationArgs;
export const selectZoweVersion = (state: RootState) => state.installation.zoweVersion;
export const selectInstallationType = (state: RootState) => state.installation.installationArgs.installationType;
export const selectLicenseAgreement = (state: RootState) => state.installation.licenseAgreement;
export const selectIsNewInstallation = (state: RootState) => state.installation.isNewInstallation;

export default installationSlice.reducer;
Loading
Loading