-
Notifications
You must be signed in to change notification settings - Fork 57
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
Add 'Manage' tab to Digital Twins page preview #957
Add 'Manage' tab to Digital Twins page preview #957
Conversation
setConfigData(digitalTwin.configFiles.map((name) => ({ id: name, name }))); | ||
}; | ||
|
||
const handleFileClick = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function handleFileClick
has a Cognitive Complexity of 7 (exceeds 5 allowed). Consider refactoring.
setShowLog(false); | ||
}; | ||
|
||
function ReconfigureDialog({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function ReconfigureDialog
has a Cognitive Complexity of 6 (exceeds 5 allowed). Consider refactoring.
setOpenCancelDialog(false); | ||
}; | ||
|
||
const handleConfirmSave = async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function handleConfirmSave
has 31 lines of code (exceeds 25 allowed). Consider refactoring.
@VanessaScherma remember to address issue #953 |
@VanessaScherma The tests are missing. Are you working on them? |
Testcases and error messages to check for the PR:
Question: can these go into end-to-end tests or should these be manual tests for now? |
Yes, I will complete them as soon as possible. |
); | ||
} | ||
|
||
const saveChanges = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function saveChanges
has 27 lines of code (exceeds 25 allowed). Consider refactoring.
@VanessaScherma This PR brings nice working features. I am only half way through the code review and can suggest the following changes for now.
|
@VanessaScherma update to text description To the top-of the DT preview page (before the React Tabs): const tabs: ITabs[] = [
{
...
{
label: 'Manage',
body: `Read the complete description of digital twins. If necessary, users can delete a digital twin, removing it from the workspace with all its associated data. Users can also reconfigure the digital twin.`,
},
{
label: 'Execute',
body: 'Execute the Digital Twins using Gitlab CI/CD workflows.',
},
...
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VanessaScherma The review is now complete. Please let me know when you are done with the changes and tests. Thanks.
name: string; | ||
} | ||
|
||
const handleCloseLog = (setShowLog: Dispatch<SetStateAction<boolean>>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be named handleDetailsDialogue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed into handleCloseDetailsDialog
. The name of this function has been also changed accordingly in the other Dialogs.
import { selectDigitalTwinByName } from '../../../store/digitalTwin.slice'; | ||
import DigitalTwin from '../../../util/gitlabDigitalTwin'; | ||
|
||
interface SidebarProps { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These seem to be DTItemProps
. These are rather the properties of a side bar item received from digital twin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are the props passed to the Sidebar
component from the parent component Editor
.
setShowLog: Dispatch<React.SetStateAction<boolean>>; | ||
} | ||
|
||
const handleToggleLog = (setShowLog: Dispatch<SetStateAction<boolean>>) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of the function is unclear. The delete button is on Manage tab where there is no need for log dialogue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function has been renamed handleToggleDeleteDialog
. Its purpose is to open the DeleteDialog
component when the button is clicked.
setShowLog: Dispatch<React.SetStateAction<boolean>>; | ||
} | ||
|
||
export const handleToggleLog = async ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it handleDetailsWindow
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function has been renamed handleToggleDetailsDialog
.
setShowReconfigure: Dispatch<SetStateAction<boolean>>; | ||
} | ||
|
||
const handleToggleReconfigure = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Reconfigure button is never inactive. The original html page and all the related React components are active in the background. Is there a need to deactivate the Details button. Perhaps you intend to use this click action to show README in which case the code structure and names have to reflect that intention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function has been changed into handleToggleReconfigureDialog
and it is used to open the ReconfigureDialog
by clicking on the ReconfigureButton
. Both the ReconfigureButton
and the DetailsButton
are never deactivated. If there are no files within the DT folder on GitLab, the ReconfigureDialog
shows the empty SimpleTreeView (while maintaining the ‘Description’, ‘Lifecycle’, ‘Configuration’ sections). If there is no README.md, the DetailsDialog
informs of the absence of that file within the GitLab folder. Is that what you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An updated message: "There is no README.md file for the hello-world digital twin"
{tab === 'Execute' ? ( | ||
<AssetCardExecute asset={asset} /> | ||
) : ( | ||
<AssetCardManage asset={asset} onDelete={() => onDelete(asset.path)} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also "details" and "reconfigure" buttons. Why are they treated differently from delete button?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onDelete
function is passed exclusively to AssetCardManage
, which includes the Delete button. This is because it is the only button that, in addition to having an API call via gitlabDigitalTwin
, must also modify the store via dispatch with the deleteAsset
function.
fileType: string; | ||
} | ||
|
||
function PreviewTab({ fileContent, fileType }: PreviewProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The md, json and yaml cases are handled. What happens if it is lifecycle/script?
- The hidden files (starting with dot character) should also be available. This allows us to edit
.gitlab-ci.yaml
file as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Added case for handling bash files, without extension.
- The yaml files are now included in the 'Configuration' section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not just the bash files. Please show all the files placed in lifecycle
directory
const handleDelete = async ( | ||
digitalTwin: DigitalTwin, | ||
setShowLog: Dispatch<SetStateAction<boolean>>, | ||
onDelete: () => void, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onDelete
function is void everywhere in the codebase. Is it needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does not have a return value but modifies the store via dispatch.
const filteredFiles = response | ||
.filter( | ||
(item: { type: string; name: string }) => | ||
item.type === 'blob' && item.name.endsWith('.json'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The yaml
file extension gets added here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
); | ||
}); | ||
|
||
it('should get description files', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
expect(dt.descriptionFiles).toEqual([]); | ||
}); | ||
|
||
it('should get lifecycle files', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
expect(dt.lifecycleFiles).toEqual([]); | ||
}); | ||
|
||
it('should get config files', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
expect(dt.descriptionFiles).toEqual(['file1.md']); | ||
}); | ||
|
||
it('should return empty array when fetching description files fails', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
expect(dt.lifecycleFiles).toEqual(['file2.json']); | ||
}); | ||
|
||
it('should return empty array when fetching lifecycle files fails', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
expect(dt.configFiles).toEqual(['file2.json']); | ||
}); | ||
|
||
it('should return empty array when fetching config files fails', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
import * as React from 'react'; | ||
|
||
describe('PreviewTab', () => { | ||
it('renders PreviewTab with markdown content', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
expect(screen.getByText('fileContent')).toBeInTheDocument(); | ||
}); | ||
|
||
it('renders PreviewTab with json content', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
expect(screen.getByText('fileContent')).toBeInTheDocument(); | ||
}); | ||
|
||
it('renders PreviewTab with yaml content', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
import { showSnackbar } from 'preview/store/snackbar.slice'; | ||
import { mockDigitalTwin } from 'test/preview/__mocks__/global_mocks'; | ||
|
||
jest.mock('react-redux', () => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
|
dt = new DigitalTwin('test-DTName', mockGitlabInstance); | ||
}); | ||
|
||
it('should get description files', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
); | ||
}); | ||
|
||
it('should get lifecycle files', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
); | ||
}); | ||
|
||
it('should get config files', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
); | ||
}); | ||
|
||
it('should get description files', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
}} | ||
> | ||
<SimpleTreeView> | ||
<TreeItem itemId="description" label="Description"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
))} | ||
</TreeItem> | ||
|
||
<TreeItem itemId="configuration" label="Configuration"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
))} | ||
</TreeItem> | ||
|
||
<TreeItem itemId="lifecycle" label="Lifecycle"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
|
||
const files = [{ name: 'Asset 1', content: 'content1', isModified: false }]; | ||
|
||
const store = configureStore({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
{ name: 'Asset 1', content: 'content1', isModified: false }, | ||
]; | ||
|
||
const store = configureStore({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
const digitalTwin = new DigitalTwin('Asset 1', mockGitlabInstance); | ||
digitalTwin.getFileContent = jest.fn().mockResolvedValueOnce(null); | ||
|
||
await React.act(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
jest.clearAllMocks(); | ||
}); | ||
|
||
it('calls handleFileClick when a description file is clicked', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
}); | ||
}); | ||
|
||
it('calls handleFileClick when a configuration file is clicked', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
}); | ||
}); | ||
|
||
it('calls handleFileClick when a lifecycle file is clicked', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
); | ||
}); | ||
|
||
it('calls handleFileClick when a description file is clicked', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
fireEvent.click(file); | ||
}); | ||
|
||
it('calls handleFileClick when a lifecycle file is clicked', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
jest.clearAllMocks(); | ||
}); | ||
|
||
it('calls handleFileClick when a description file is clicked', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
fireEvent.click(file); | ||
}); | ||
|
||
it('calls handleFileClick when a config file is clicked', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
}); | ||
}); | ||
|
||
it('calls handleFileClick when a configuration file is clicked', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
@@ -0,0 +1,311 @@ | |||
import AssetBoard from 'preview/components/asset/AssetBoard'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
File Manage.test.tsx
has 257 lines of code (exceeds 250 allowed). Consider refactoring.
}), | ||
}); | ||
|
||
const setupTest = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
}), | ||
}); | ||
|
||
const setupTest = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
}), | ||
}); | ||
|
||
const setupTest = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
{ name: 'Asset 1', content: 'content1', isModified: false }, | ||
]; | ||
|
||
const store = configureStore({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 4 locations. Consider refactoring.
{ name: 'Asset 1', content: 'content1', isModified: false }, | ||
]; | ||
|
||
const store = configureStore({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 4 locations. Consider refactoring.
describe('DeleteDialog', () => { | ||
let store: ReturnType<typeof setupStore>; | ||
|
||
beforeEach(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
describe('DetailsDialog', () => { | ||
let store: ReturnType<typeof setupStore>; | ||
|
||
beforeEach(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
describe('ReconfigureDialog', () => { | ||
let store: ReturnType<typeof setupStore>; | ||
|
||
beforeEach(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar blocks of code found in 3 locations. Consider refactoring.
@prasadtalasila, here are updates on testing. Rows uncovered, both in unit and integration tests:
Also, there remains a duplication problem on CodeClimate. Despite having created a util function for store setup, the call to the function, followed by rendering the component, is considered duplication. Any ideas on how to fix this? Thank you. |
@VanessaScherma |
Code Climate has analyzed commit dd7cc86 and detected 3 issues on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
54f6dbe
into
INTO-CPS-Association:feature/distributed-demo
It replaces PR #906. it adds the 'Manage' tab to Digital Twins page preview. It includes the functionality ‘Details’ to read the
README.md
file, ‘Delete’ to delete the DT folder from the GitLab profile, ‘Reconfigure’ to edit the DT files.