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: Loris configuration(M2-6644) #1927

Open
wants to merge 81 commits into
base: develop
Choose a base branch
from

Conversation

ramirlm
Copy link
Contributor

@ramirlm ramirlm commented Sep 23, 2024

  • Tests for the changes have been added
  • Delivered the fix or feature branches into develop or release branches via Squash and Merge (to keep clean history)

📝 Description

🔗 Jira Ticket M2-6644

📸 Screenshots

Before (Optional) After

IhorNesterenko99 and others added 30 commits December 5, 2023 15:14
# Conflicts:
#	src/modules/Builder/features/BuilderAppletSettings/BuilderAppletSettings.utils.tsx
#	src/resources/app-en.json
#	src/resources/app-fr.json
#	src/shared/consts.tsx
#	src/shared/state/Applet/Applet.schema.ts
# Conflicts:
#	src/shared/features/AppletSettings/LiveResponseStreamingSetting/LiveResponseStreamingSetting.test.tsx
# Conflicts:
#	src/modules/Builder/api/api.ts
# Conflicts:
#	src/modules/Builder/features/BuilderAppletSettings/BuilderAppletSettings.tsx
#	src/modules/Builder/features/BuilderAppletSettings/BuilderAppletSettings.types.ts
#	src/modules/Builder/features/BuilderAppletSettings/BuilderAppletSettings.utils.tsx
#	src/modules/Builder/pages/BuilderApplet/BuilderApplet.utils.tsx
nmialik and others added 21 commits July 26, 2024 12:03
# Conflicts:
#	src/modules/Dashboard/api/api.ts
#	src/modules/Dashboard/api/api.types.ts
#	src/shared/components/FormComponents/SelectController/SelectController.tsx
#	src/shared/components/FormComponents/SelectController/SelectController.types.ts
#	src/shared/components/Table/Table.tsx
#	src/shared/state/Applet/Applet.schema.ts
#	src/shared/state/Workspaces/Workspaces.schema.ts
#	src/shared/types/featureFlags.ts
# Conflicts:
#	src/modules/Auth/state/Auth.thunk.ts
#	src/modules/Builder/features/BuilderAppletSettings/BuilderAppletSettings.tsx
#	src/shared/layouts/BaseLayout/components/LeftBar/LeftBar.tsx
#	src/shared/layouts/BaseLayout/components/LeftBar/LeftBar.utils.ts
#	src/shared/utils/featureFlags.ts
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1927.d19gtpld8yi51u.amplifyapp.com

@ramirlm ramirlm marked this pull request as ready for review October 10, 2024 12:58
@ramirlm ramirlm requested a review from ChaconC October 10, 2024 12:58
Comment on lines 986 to 989
export const enableIntegrationApi = (integrations: Integration[], signal?: AbortSignal) =>
authApiClient.post('/integrations/', integrations, {
authApiClient.post('/integrations', integrations, {
signal,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

this endpoint doesn't exist anymore, it was replaced by saveIntegrationToApi

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You sure? This is the one to save 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.

Maybe you're referencing a different backend branch, as I can see in the one I point to it is being used

Copy link
Contributor

Choose a reason for hiding this comment

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

The one that saves the integration is the one you added as saveIntegrationToApi, which takes type, config, etc as parameters. This is the dummy endpoint which didn't include any additional server information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it! Makes sense now, will change. Thanks!

@@ -994,5 +994,5 @@ export const disableIntegrationApi = (integrations: string[], signal?: AbortSign
signal,
};

return authApiClient.delete('/integrations/', config);
return authApiClient.delete('/integrations', config);
Copy link
Contributor

Choose a reason for hiding this comment

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

same for this endpoint, though the integration is not part of this PR so it's ok if you keep it for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, will remove it

@@ -437,4 +437,6 @@ export const observerStyles = {

export const enum IntegrationTypes {
Loris = 'loris',
LorisUpperCase = 'LORIS', // In the API the integration type is in uppercase in some endpoints and lowercase in others
SampleIntegration = 'sample',
Copy link
Contributor

Choose a reason for hiding this comment

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

What is SampleIntegration? If it is meant as an example please add a comment explaining usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just the sample as I understood you requested me to add, did I misunderstand it?

Copy link
Contributor

Choose a reason for hiding this comment

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

The section I mentioned to include as example is for the configuration param (currently declared as SaveIntegrationParams)

Comment on lines +77 to +90
export type FetchIntegrationProjectsParams = {
hostname: string;
username: string;
password: string;
};

export type SaveIntegrationParams = {
appletId: string;
hostname: string;
project: string;
username: string;
password: string;
integrationType: IntegrationTypes;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be refactored to better align with the BE object where Integration has appletId, integrationType and configuration. Where configuration is different depending on the integrationType.

The type for FetchIntegrationProjectsParams is only valid for LorisIntegrationConfiguration, where the password is optional and only used for saving the new integration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I tried to do that, but was having issues. Will create a new interface and not use the applet one and it should be ok

Comment on lines +72 to +74
// currently set as snake case on the API
applet_id: string;
integration_type: IntegrationTypes;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you double check this please, in reviewing the backend response I see camelCase responses
CleanShot 2024-10-11 at 12 16 29@2x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only if it is in a different branch other than loris-integration-2. Is it? I tried it and I get errors

@@ -437,4 +437,6 @@ export const observerStyles = {

export const enum IntegrationTypes {
Loris = 'loris',
LorisUpperCase = 'LORIS', // In the API the integration type is in uppercase in some endpoints and lowercase in others
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not seeing where we use lowercase loris, and in any case it might be better to create a separate enum for that specific usage. I think all the new endpoints are using LORIS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the endpoints you mentioned as replaced, but I'll double check and understand why. If necessary I create a separate type

};

export const hasLorisIntegrationOnState = (integrations?: Integration[]): boolean =>
Boolean(integrations?.some((i) => i.integrationType === IntegrationTypes.Loris));
Copy link
Contributor

Choose a reason for hiding this comment

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

For example here the response is LORIS and you're comparing to loris
CleanShot 2024-10-11 at 12 16 29@2x

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the way it's being saved in the state

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