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.

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

feat(ui-studies): add on click fetch and display list of non studies folder #2224

Merged
merged 44 commits into from
Dec 18, 2024

Conversation

smailio
Copy link
Contributor

@smailio smailio commented Nov 14, 2024

No description provided.

webapp/src/components/App/Studies/utils.ts Outdated Show resolved Hide resolved
webapp/src/services/api/study.ts Outdated Show resolved Hide resolved
webapp/src/services/api/study.ts Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudyTree.tsx Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudyTree.tsx Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudyTree.tsx Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudyTree.test.tsx Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudyTree.test.tsx Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudyTree.tsx Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudyTree.tsx Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudyTree.tsx Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudyTree.tsx Outdated Show resolved Hide resolved
@smailio smailio marked this pull request as ready for review November 21, 2024 10:56
Copy link
Member

@hdinia hdinia left a comment

Choose a reason for hiding this comment

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

You have an import error in components/Studies/SideNav.tsx

update this import StudyTree from "./StudyTree";
to: import StudyTree from "@/components/App/Studies/StudyTree";

do you have all frontend tools properly setup, eslint, typescript etc ?

Comment on lines 52 to 61
export const getWorkspaces = async (): Promise<string[]> => {
const res = await client.get(`/v1/private/explorer/_list_workspaces`);
return res.data.map((folder: { name: string }) => folder.name);
};
Copy link
Member

@hdinia hdinia Nov 21, 2024

Choose a reason for hiding this comment

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

you can pass types to axios methods, like this:

interface Workspace {
  name: string;
}

export const getWorkspaces = async => {
  const res = await client.get<Workspace[]>(
    `/v1/private/explorer/_list_workspaces`,
  );
  return res.data.map((workspace) => workspace.name);
};

@@ -48,6 +49,37 @@ export const getStudies = async (): Promise<StudyMetadata[]> => {
});
};

export const getWorkspaces = async (): Promise<string[]> => {
const res = await client.get(`/v1/private/explorer/_list_workspaces`);
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason why we have underscores in the api url ? /v1/private/explorer/_list_workspaces
We follow REST conventions, this should be something like /v1/private/explorer/workspaces, and if separators are needed we usually use a hyphen "-"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's as per the specs, convention for private routes

MartinBelthle
MartinBelthle previously approved these changes Nov 22, 2024
Copy link
Contributor

@MartinBelthle MartinBelthle left a comment

Choose a reason for hiding this comment

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

The back-end side seems legit

Copy link
Member

@hdinia hdinia left a comment

Choose a reason for hiding this comment

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

Some minor feedback, otherwise it looks good to me 👍🏼

webapp/public/locales/en/main.json Outdated Show resolved Hide resolved
webapp/public/locales/en/main.json Show resolved Hide resolved
webapp/src/components/App/Studies/StudiesList/index.tsx Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudiesList/index.tsx Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudiesList/index.tsx Outdated Show resolved Hide resolved
webapp/src/components/App/Studies/StudyTree/index.tsx Outdated Show resolved Hide resolved
hdinia
hdinia previously approved these changes Dec 4, 2024
@@ -667,7 +669,9 @@
"studies.exportOutput": "Exporter une sortie",
"studies.exportOutputFilter": "Exporter une sortie filtrée",
"studies.selectOutput": "Selectionnez une sortie",
"studies.variant": "Variante",
Copy link
Member

Choose a reason for hiding this comment

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

Remove also the key in the english file

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 think it was a duplicate in tis file or something

control={<Checkbox checked={isRecursiveScan} />}
label={t("studies.requestDeepScan")}
onChange={handleRecursiveScan}
/>{" "}
Copy link
Member

Choose a reason for hiding this comment

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

Remove {" "}

onConfirm={handleFolderScan}
alert="warning"
open
>
{`${t("studies.scanFolder")} ${folder}?`}
<FormControlLabel
Copy link
Member

Choose a reason for hiding this comment

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

Use our component CheckBoxFE

treeWithWorkspaces,
);
setStudiesTree(updatedTree);
failedPaths.forEach((path) => {
Copy link
Member

Choose a reason for hiding this comment

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

This can flood the user with popups. Include them in one snackbar instead.

setStudiesTree(initialStudiesTree);
enqueueErrorSnackbar(
t("studies.tree.error.failToFetchWorkspace"),
error as AxiosError,
Copy link
Member

Choose a reason for hiding this comment

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

Use toError(err)

@@ -0,0 +1,157 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

It's not better to have a test folder inside component @hdinia?

Copy link
Member

@hdinia hdinia Dec 4, 2024

Choose a reason for hiding this comment

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

indeed to not mix concerns if their is multiple files related to tests in a folder we should put them inside their __tests__ folder

t("studies.tree.error.detailsInConsole"),
);
}
};
Copy link
Member

Choose a reason for hiding this comment

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

Add a line break

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

);
}
}
const chidrenPaths = studyTreeNode.children.map(
Copy link
Member

Choose a reason for hiding this comment

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

This code and the code from the effect can be move in a common function

* @param path path of the subfolder to fetch, should sart with root, e.g. root/workspace/folder1
* @returns list of subfolders under the given path
*/
async function fetchSubfolders(path: string): Promise<NonStudyFolderDTO[]> {
Copy link
Member

Choose a reason for hiding this comment

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

async / await and return type are not necessary

@smailio smailio force-pushed the feature/615-optimize-listing-studies branch from f546844 to eb1a170 Compare December 4, 2024 12:48
@skamril skamril changed the title feat(ui): add on click fetch and display list of non studies folder feat(ui-studies): add on click fetch and display list of non studies folder Dec 17, 2024
skamril
skamril previously approved these changes Dec 17, 2024
@smailio smailio merged commit 763f370 into dev Dec 18, 2024
13 checks passed
@smailio smailio deleted the feature/615-optimize-listing-studies branch December 18, 2024 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants