Skip to content

ZEN - Multiple bugfixes #231

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

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 all 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
5 changes: 5 additions & 0 deletions src/Routes/RouteConstant.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export const ROUTES = {
HOME: '/',
WIZARD: '/wizard',
};

43 changes: 43 additions & 0 deletions src/renderer/components/Dialogs/WarningDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
/*
* This program and the accompanying materials are made available under the terms of the
* Eclipse Public License v2.0 which accompanies this distribution, and is available at
* https://www.eclipse.org/legal/epl-v20.html
*
* SPDX-License-Identifier: EPL-2.0
*
* Copyright Contributors to the Zowe Project.
*/
import { Box, Button, Dialog, DialogActions, DialogContent, DialogTitle, FormControl, TextField } from '@mui/material';
import { useState } from 'react';

const WarningDialog = ({onWarningDialogSubmit}: {onWarningDialogSubmit: any}) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.


const [isDialogVisible, setIsDialogVisible] = useState(true);

const handleClose = () => {
setIsDialogVisible(false);
onWarningDialogSubmit(false);
}

const handleSubmit = () => {
setIsDialogVisible(false);
onWarningDialogSubmit(true);
}

return (
<div>
<Dialog open={isDialogVisible} maxWidth={'xs'}>
<DialogTitle>Warning!</DialogTitle>
<DialogContent style={{ fontSize: '13px', fontFamily: 'Arial, sans-serif', color: 'black' }}> Starting a new installation will erase the previously stored installation data from the wizard. Do you wish to proceed? </DialogContent>
<DialogActions>
<div>
<Button onClick={handleSubmit}>Proceed</Button>
<Button onClick={handleClose}>Close</Button>
</div>
</DialogActions>
</Dialog>
</div>
)
}

export default WarningDialog;
188 changes: 101 additions & 87 deletions src/renderer/components/Home.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

import '../global.css';
import { useEffect, useState } from "react";
import { Link } from 'react-router-dom';
import { Link, useNavigate } from 'react-router-dom';
import { Box, Card, CardContent, CardMedia, Typography, Button } from '@mui/material';
import flatten, { unflatten } from 'flat';
import { IResponse, IIpcConnectionArgs } from '../../types/interfaces';
Expand All @@ -22,25 +22,22 @@ 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 { ActiveState, InstallationArgs } 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, setIsNewInstallation, selectIsNewInstallation } from './stages/installation/installationSlice';
import PasswordDialog from './common/passwordDialog';
import WarningDialog from './Dialogs/WarningDialog';
import HomeCardComponent from './HomeCardComponent';
import { ICard } from '../../types/interfaces';
import { ROUTES } from '../../Routes/RouteConstant';
Copy link
Collaborator

Choose a reason for hiding this comment

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

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


// REVIEW: Get rid of routing

interface ICard {
id: string,
name: string,
description: string,
link: string,
media: any,
}

// Cards Data
const cards: Array<ICard> = [
{
id: "install",
Expand All @@ -58,79 +55,39 @@ const cards: Array<ICard> = [
}
]

// Constants
const prevInstallationKey = "prev_installation";
const lastActiveState: ActiveState = {
activeStepIndex: 0,
isSubStep: false,
activeSubStepIndex: 0,
};
const defaultTooltip: string = "Resume";

// Helper Functions
const getNewInstallationArgs = (id: string, currentArgs: InstallationArgs) => {
return id === "install"
? { ...currentArgs, dryRunMode: false }
: { ...currentArgs, dryRunMode: true };
};

const Home = () => {

const dispatch = useAppDispatch();
const navigate = useNavigate();
const connectionStatus = useAppSelector(selectConnectionStatus);
const [showWizard, setShowWizard] = useState(false);
const [showLoginDialog, setShowLogin] = useState(false);
const [localYaml, setLocalYaml] = useState(useAppSelector(selectYaml));
const [schema, setLocalSchema] = useState(useAppSelector(selectSchema));
const installationArgs = useAppSelector(selectInstallationArgs);

const { activeStepIndex, isSubStep, activeSubStepIndex, lastActiveDate } = getPreviousInstallation();

const [isNewInstallation, setIsNewInstallation] = useState(false);

const schema = useAppSelector(selectSchema);
const stages: any = [];
const defaultTooltip: string = "Resume";
const resumeTooltip = connectionStatus ? defaultTooltip : `Validate Credentials & ${defaultTooltip}`;
const isNewInstallation = useAppSelector(selectIsNewInstallation);
const { lastActiveDate } = getPreviousInstallation();

const [showWizard, setShowWizard] = useState(false);
const [localYaml, setLocalYaml] = useState(useAppSelector(selectYaml));
const [showPasswordDialog, setShowPasswordDialog] = useState(false);
const [updatedConnection, setUpdatedConnection] = useState(false);
const [isResume, setIsResume] = useState(useAppSelector(selectResumeProgress));

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

const handleClick = () => {
let newInstallationArgs = installationSlice.getInitialState().installationArgs;
if (id === "install") {
newInstallationArgs = {...newInstallationArgs, dryRunMode: false};
} else if (id === "dry run") {
newInstallationArgs = {...newInstallationArgs, dryRunMode: true};
}
dispatch(setYaml(FALLBACK_YAML));
dispatch(setInstallationArgs(newInstallationArgs));
window.electron.ipcRenderer.setConfigByKeyNoValidate("installationArgs", newInstallationArgs);
setLocalYaml(FALLBACK_YAML);
window.electron.ipcRenderer.setConfig(FALLBACK_YAML);
// TODO: Ideally, reset connectionArgs too
// but this introduces bug with "self certificate chain" it's the checkbox, it looks checked but
// it acts like it's not unless you touch it
// dispatch(setConnectionArgs(connectionSlice.getInitialState().connectionArgs));
setIsResume(false);
};

return (
<Link key={`link-${id}`} to={link} >
<Box sx={{ width: '40vw', height: '40vh'}} onClick={handleClick}>
<Card id={`card-${id}`} square={true}>
<CardMedia
sx={{ height: 240 }}
image={media}
/>
<CardContent className="action-card">
<Box>
<Typography variant="subtitle1" component="div">
{name}
</Typography>
<Typography sx={{ mb: 1.5, mt: 1.5, fontSize: '0.875rem' }} color="text.secondary">
{description}
</Typography>
</Box>
</CardContent>
</Card>
</Box>
</Link>
)
}
const [newInstallationClicked, setNewInstallationClick] = useState(false);
const [previousInstallation, setPreviousInstallation] = useState(false);
const [defaultYaml, setDefaultYaml] = useState(FALLBACK_YAML);

useEffect(() => {
eventDispatcher.on('saveAndCloseEvent', () => setShowWizard(false));
Expand All @@ -142,8 +99,9 @@ const Home = () => {
window.electron.ipcRenderer.getConfig().then((res: IResponse) => {
if (res.status) {
dispatch(setYaml(res.details));
setDefaultYaml(res.details);
} else {
dispatch(setYaml(FALLBACK_YAML));
dispatch(setYaml(defaultYaml));
}
})
}
Expand Down Expand Up @@ -176,14 +134,10 @@ const Home = () => {
}
});

window.electron.ipcRenderer.setStandardOutput(DEF_NO_OUTPUT).then((res: any) => {
})

window.electron.ipcRenderer.findPreviousInstallations().then((res: IResponse) => {
const connectionStore = res.details;
if (connectionStore["connection-type"] === 'ftp') {
const jobStatement = connectionStore['ftp-details'].jobStatement.trim() || useAppSelector(selectInitJobStatement);
// console.log(JSON.stringify(connectionStore['ftp-details'],null,2));
const connectionArgs: IIpcConnectionArgs = {
...connectionStore["ftp-details"],
password: "",
Expand All @@ -200,13 +154,14 @@ const Home = () => {
if (!lastInstallation) {
const flattenedData = flatten(lastActiveState);
localStorage.setItem(prevInstallationKey, JSON.stringify(flattenedData));
setIsNewInstallation(true);
dispatch(setIsNewInstallation(true));
setPreviousInstallation(false);
} else {
const data: ActiveState = unflatten(JSON.parse(lastInstallation));
setIsNewInstallation(!(data && data.lastActiveDate));
dispatch(setIsNewInstallation(!(data && data.lastActiveDate)));
setPreviousInstallation(!!(data && data.lastActiveDate));
}


});
return () => {
eventDispatcher.off('saveAndCloseEvent', () => setShowWizard(true));
Expand All @@ -216,33 +171,93 @@ const Home = () => {
const resumeProgress = () => {
setShowWizard(true);
dispatch(setResumeProgress(true));

dispatch(setIsNewInstallation(false));

if(connectionStatus) {
setShowPasswordDialog(true);
setUpdatedConnection(false);
}
}

const confirmConnection = (status: boolean) => {
setUpdatedConnection(status);
setShowPasswordDialog(!status);
setShowWizard(status);
}

const handleNewInstallation = (newInstallationArgs: InstallationArgs) => {
dispatch(setYaml(defaultYaml));
dispatch(setInstallationArgs(newInstallationArgs));

window.electron.ipcRenderer.setConfigByKeyNoValidate("installationArgs", newInstallationArgs);
window.electron.ipcRenderer.setConfig(defaultYaml);

setLocalYaml(defaultYaml);

// TODO: Ideally, reset connectionArgs too
// but this introduces bug with "self certificate chain" it's the checkbox, it looks checked but
// it acts like it's not unless you touch it
// dispatch(setConnectionArgs(connectionSlice.getInitialState().connectionArgs));
}

const handleCardClick = (id: string) => {

const initialInstallationArgs = installationSlice.getInitialState().installationArgs;
const newInstallationArgs = getNewInstallationArgs(id, initialInstallationArgs);

if (id === "install") {
setNewInstallationClick(true);
if(previousInstallation) {
return;
}
dispatch(setIsNewInstallation(true));
dispatch(setConnectionStatus(false));
dispatch(setResumeProgress(false));
}
handleNewInstallation(newInstallationArgs);
};

const confirmNewInstallation = (status: boolean) => {
dispatch(setIsNewInstallation(status))
setNewInstallationClick(false);
setPreviousInstallation(!status);

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

if(status) {
dispatch(setConnectionStatus(false));
dispatch(setResumeProgress(false));
handleNewInstallation({ ...installationSlice.getInitialState().installationArgs, dryRunMode: false });
navigate(ROUTES.WIZARD);
}
}

return (
<>
{ previousInstallation && newInstallationClicked &&
<WarningDialog onWarningDialogSubmit={confirmNewInstallation}/>
}

{!showWizard && <div className="home-container" style={{ display: 'flex', flexDirection: 'column' }}>

<div style={{ position: 'absolute', left: '-9999px' }}>
<HorizontalLinearStepper stages={stages} />
{stages.length > 0 && <HorizontalLinearStepper stages={stages} />}
</div>

{!connectionStatus && <div style={{marginBottom: '20px'}}></div>}

<div style={{ display: 'flex', flexDirection: 'row', alignItems: 'center', marginLeft: '8%' }}>
{cards.map(card => makeCard(card))}
{cards.map(card => (
<HomeCardComponent
key={card.id}
id={card.id}
name={card.name}
description={card.description}
link={card.link}
media={card.media}
handleCardClick={handleCardClick}
previousInstallation={previousInstallation}
/>
))}
</div>

{!isNewInstallation && <div style={{marginBottom: '1px',marginTop: '120px',background: 'white', fontSize: 'small',marginLeft: 'calc(8% + 10px)', padding: '15px 0 15px 15px',width: 'calc(80% + 5px)', boxShadow: '1px 1px 3px #a6a6a6'}}>
{previousInstallation && <div style={{marginBottom: '1px',marginTop: '120px',background: 'white', fontSize: 'small',marginLeft: 'calc(8% + 10px)', padding: '15px 0 15px 15px',width: 'calc(80% + 5px)', boxShadow: '1px 1px 3px #a6a6a6'}}>
<Box sx={{display: 'flex', flexDirection: 'column'}}>

<div style={{paddingBottom: '10px', color: 'black'}}>
Expand All @@ -268,8 +283,7 @@ const Home = () => {
{showWizard &&
<>
{showPasswordDialog && <PasswordDialog onPasswordSubmit={confirmConnection}></PasswordDialog>}
{(showPasswordDialog && updatedConnection) && <Wizard initialization={false}/>}
{!showPasswordDialog && <Wizard initialization={false}/>}
{!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
27 changes: 27 additions & 0 deletions src/renderer/components/HomeCardComponent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { Box, Card, CardContent, CardMedia, Typography, Tooltip, Button } from '@mui/material';
import { Link } from 'react-router-dom';

const HomeCardComponent = ({ id, name, description, link, media, previousInstallation, handleCardClick }:{id: string, name: string, description: string, link: string, media: any, previousInstallation: boolean, handleCardClick: any}) => {

return (
<Link key={`link-${id}`} to={previousInstallation ? "/" : link}>
<Box sx={{ width: '40vw', height: '40vh' }} onClick={() => handleCardClick(id)}>
<Card id={`card-${id}`} square={true}>
<CardMedia sx={{ height: 240 }} image={media} />
<CardContent className="action-card">
<Box>
<Typography variant="subtitle1" component="div">
{name}
</Typography>
<Typography sx={{ mb: 1.5, mt: 1.5, fontSize: '0.875rem' }} color="text.secondary">
{description}
</Typography>
</Box>
</CardContent>
</Card>
</Box>
</Link>
);
};

export default HomeCardComponent;
Loading
Loading