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

Add mutualized api functions #455

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Add mutualized api functions #455

wants to merge 24 commits into from

Conversation

Meklo
Copy link
Contributor

@Meklo Meklo commented May 23, 2024

Removes the FilterContext, replaced by the following functions for API calls related to fetching directory data :

  • fetchRootFolders
  • fetchDirectoryContent
  • fetchDirectoryElementPath
  • elementExists
  • fetchElementsInfos

The following method used to retrieve the application metadata :

  • fetchAppsAndUrls (renamed to fetchAppsMetadata)

Also:

  • downloadZipFile (renamed to downloadFile)

Also adds common functions needed for data fetching mechanisms

  • backendFetch
  • backendFetchJson
  • backendFetchFile
  • backendFetchText

src/hooks/predefined-properties-hook.ts Outdated Show resolved Hide resolved
src/services/apps-metadata.ts Outdated Show resolved Hide resolved
src/services/directory.ts Show resolved Hide resolved
Copy link
Contributor

@Tristan-WorkGH Tristan-WorkGH left a comment

Choose a reason for hiding this comment

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

part 1

src/services/apps-metadata.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/explore.ts Outdated Show resolved Hide resolved
src/services/directory.ts Outdated Show resolved Hide resolved
@Meklo Meklo requested review from flomillot and Tristan-WorkGH May 27, 2024 14:25
src/services/metadata.ts Outdated Show resolved Hide resolved
src/services/directory.ts Outdated Show resolved Hide resolved
src/services/directory.ts Outdated Show resolved Hide resolved
src/services/directory.ts Show resolved Hide resolved
Copy link
Contributor

@Tristan-WorkGH Tristan-WorkGH left a comment

Choose a reason for hiding this comment

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

part2

Have you look to a more update version of utilities function for rest base from gridadmin ?


export type Url = string | URL;

export function fetchEnv() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Issue is, it can't resolve ../../public/env.json during compilation in commons-ui

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that was to not have to manage manually the type definition manually.
You just remove the link to the file in the definition and it's good.

src/services/index.ts Outdated Show resolved Hide resolved
src/services/metadata.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/explore.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/directory.ts Outdated Show resolved Hide resolved
src/services/directory.ts Outdated Show resolved Hide resolved
src/services/directory.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Tristan-WorkGH Tristan-WorkGH left a comment

Choose a reason for hiding this comment

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

part3

Comment on lines 69 to 88
const idsParams = getRequestParamFromList(
ids.filter((id) => id), // filter falsy elements
'ids'
);

const equipmentTypesParams = getRequestParamFromList(
equipmentTypes,
'equipmentTypes'
);

const elementTypesParams = getRequestParamFromList(
elementTypes,
'elementTypes'
);

const urlSearchParams = new URLSearchParams([
...idsParams,
...equipmentTypesParams,
...elementTypesParams,
]).toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a problem here because getRequestParamFromList() return an URLSearchParams...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on https://stackoverflow.com/questions/45126076/how-can-i-combine-two-urlsearchparams
It seems to be possible to combine them this way : new URLSearchParams({ ...Object.fromEntries(params1), ...Object.fromEntries(params2) });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nevermind, it broke the function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer instantiating only one URLSearchParams or a better system to merge multiple URLSearchParams into one ?

Comment on lines 193 to 337
.catch((error) => {
console.warn(
Copy link
Contributor

Choose a reason for hiding this comment

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

only a console log?

Copy link
Contributor Author

@Meklo Meklo May 28, 2024

Choose a reason for hiding this comment

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

How so ? Since it's in a catch bock I'd reckon a warn is warranted
EDIT : Ah I misunderstood, yea this far the error is only logged and not displayed in a snackbar

src/index.ts Outdated Show resolved Hide resolved
src/services/metadata.ts Outdated Show resolved Hide resolved
@Meklo Meklo requested review from flomillot and Tristan-WorkGH May 28, 2024 15:45
src/services/apps-metadata.ts Outdated Show resolved Hide resolved
src/services/apps-metadata.ts Outdated Show resolved Hide resolved
src/services/directory.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/metadata.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
src/services/utils.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Tristan-WorkGH Tristan-WorkGH May 29, 2024

Choose a reason for hiding this comment

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

If it's only to define a type, maybe not in src/utils/ but in src/types/ for example.

Also is it meant to be use multiple time or only once (actual case)? If the later I would move it where it's used.

src/services/utils.ts Outdated Show resolved Hide resolved
@flomillot flomillot mentioned this pull request May 29, 2024
@Meklo Meklo changed the title Add directory api Add mutualized api functions Jun 7, 2024
@dbraquart dbraquart self-requested a review July 3, 2024 14:14
Copy link
Contributor

@dbraquart dbraquart left a comment

Choose a reason for hiding this comment

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

code: ok
tests: to be continued

Copy link
Contributor

@Tristan-WorkGH Tristan-WorkGH left a comment

Choose a reason for hiding this comment

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

now assigned to me

@Tristan-WorkGH Tristan-WorkGH force-pushed the feat/add_directory_api branch from 5c232c8 to b4015aa Compare August 6, 2024 21:19
@Tristan-WorkGH Tristan-WorkGH force-pushed the feat/add_directory_api branch from b4015aa to 3d0d71f Compare August 6, 2024 21:28
@SlimaneAmar SlimaneAmar requested a review from flomillot August 7, 2024 06:18
@etiennehomer etiennehomer dismissed flomillot’s stale review August 7, 2024 08:47

All changes requested were resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants