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

Cb 4364 remove spaces at the beginning of the input fields #2252

Merged

Conversation

sergeyteleshev
Copy link
Contributor

No description provided.

@sergeyteleshev sergeyteleshev self-assigned this Dec 21, 2023
@@ -204,6 +204,9 @@ export class ServerConfigurationService {
return;
}

data.state.serverConfig.serverName = data.state.serverConfig.serverName?.trim();
Copy link
Member

Choose a reason for hiding this comment

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

Please do not modify state directly, use contexts config

const config = contexts.getContext(configContext);
config.serverConfig.serverName = data.state.serverConfig.serverName?.trim()

Copy link
Member

Choose a reason for hiding this comment

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

and better is do it in prepareConfig stage

@@ -70,14 +70,20 @@ export class AuthConfigurationOptionsTabService extends Bootstrap {

if (Object.keys(state.config.parameters).length) {
config.parameters = state.config.parameters;

for (const key of Object.keys(config.parameters)) {
if (typeof config.parameters[key] === 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

const value = config.parameters[key];
config.parameters[key] = value.trim();

@@ -76,7 +76,9 @@ export class UserFormInfoPart extends FormPart<IUserFormInfoState, IUserFormStat
);
}

protected override async saveChanges(): Promise<void> {
private async createUser() {
this.state.userId = this.state.userId.trim();
Copy link
Member

Choose a reason for hiding this comment

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

Please trim it in place, we don't want to modify initial object
userId: this.state.userId.trim()

}

protected override async saveChanges(): Promise<void> {
await this.createUser();
Copy link
Member

Choose a reason for hiding this comment

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

If you want to create a separate function, you can remove

this.initialState.userId = user.userId;
 this.formState.setMode(FormMode.Edit);

and move this.formState.mode === FormMode.Create check to saveChanges method, so you will always create user when you run createUser method

if(this.formState.mode === FormMode.Create) {
   const user = await this.createUser();
   this.initialState.userId = user.userId;
   this.formState.setMode(FormMode.Edit);
}

@@ -190,6 +198,12 @@ export class UserFormInfoPart extends FormPart<IUserFormInfoState, IUserFormStat
if (this.state.userId) {
const user = this.usersResource.get(this.state.userId);

for (const key in this.state.metaParameters) {
Copy link
Member

Choose a reason for hiding this comment

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

please do not modify state object, create a temp object instead

@@ -132,6 +138,8 @@ export class UserFormInfoPart extends FormPart<IUserFormInfoState, IUserFormStat
}

private async updateCredentials() {
this.state.password = this.state.password.trim();
Copy link
Member

Choose a reason for hiding this comment

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

please do not modify state object
const password = this.state.password.trim();


for (const key of Object.keys(config.properties)) {
const value = config.properties[key];
delete config.properties[key];
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to delete the property first and trim the key here?

Copy link
Member

Choose a reason for hiding this comment

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

maybe we need to check if property is custom or not and do not trim properties that we get from the server (keep as is)

@@ -304,6 +304,20 @@ export class ConnectionOptionsTabService extends Bootstrap {

const properties = await this.getConnectionAuthModelProperties(tempConfig.authModelId, state.info);

properties.forEach(property => {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like we should not trim properties here, as its from backend
we should trim credentials that we get from ui form

config.networkHandlersConfig.push(handlerConfig);
}

if (config.networkHandlersConfig?.length) {
Copy link
Member

Choose a reason for hiding this comment

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

empty if block?

}
}

private trimSSHInputConfig(input: NetworkHandlerConfigInput) {
Copy link
Member

Choose a reason for hiding this comment

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

probably we should rename it to the getPreparedSSHConfig and return object from it, because its kind of hard to keep track of object mutations this way
const preparedSSHConfig = this.getPreparedSSHConfig(handlerConfig);

@@ -204,6 +204,9 @@ export class ServerConfigurationService {
return;
}

data.state.serverConfig.serverName = data.state.serverConfig.serverName?.trim();
Copy link
Member

Choose a reason for hiding this comment

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

and better is do it in prepareConfig stage

Comment on lines 176 to 177
state.credentials.credentials.user = state.credentials.credentials.user?.trim();
state.credentials.credentials.password = state.credentials.credentials.password?.trim();
Copy link
Member

Choose a reason for hiding this comment

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

same here, please don't make modifications in functions like submit or save

form data -> prepare data -> submit data to the server -> load form data from the server

why?: let's imagine that we modified data before submitting to the server and now it's invalid or creates an exception on server side (user can't modify form to fix this error)


for (const key of Object.keys(config.properties)) {
const value = config.properties[key];
delete config.properties[key];
Copy link
Member

Choose a reason for hiding this comment

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

maybe we need to check if property is custom or not and do not trim properties that we get from the server (keep as is)

}

private trimSSHInputConfig(input: NetworkHandlerConfigInput) {
const ATTRIBUTES_TO_TRIM: (keyof NetworkHandlerConfigInput)[] = Object.keys(input) as (keyof NetworkHandlerConfigInput)[];
Copy link
Member

Choose a reason for hiding this comment

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

double type declaration remove one

Copy link
Member

Choose a reason for hiding this comment

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

and this also looks like global const for not global const please use camelCase

@@ -187,15 +193,25 @@ export class UserFormInfoPart extends FormPart<IUserFormInfoState, IUserFormStat
}

private async updateMetaParameters() {
const tempMetaParameters = toJS(this.state.metaParameters);
Copy link
Member

Choose a reason for hiding this comment

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

const metaParameters

@@ -173,6 +173,10 @@ export function useAuthDialogState(accessRequest: boolean, providerId: string |
provider = (provider || state.activeProvider) ?? undefined;
configuration = (configuration || state.activeConfiguration) ?? undefined;

const tempCredentials = toJS(state.credentials);
Copy link
Member

Choose a reason for hiding this comment

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

credentials have Record<string, any> type, so wee need more general aproach

@@ -132,10 +136,12 @@ export class UserFormInfoPart extends FormPart<IUserFormInfoState, IUserFormStat
}

private async updateCredentials() {
if (this.state.password) {
const password = this.state.password.trim();
Copy link
Member

Choose a reason for hiding this comment

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

i've added format function to FormPart please format all needed data in this function (you will need to mutate form state, lets do it for that case and if needed we will change this behaviour later)

(you need to merge devel to get this changes)

Copy link
Member

Choose a reason for hiding this comment

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

the main idea that we don't want to format data in different places because sometimes we need formatted data before we will submit it, for example to check if same name is taken. We need to format data first and then work with formatted data in all parts of our application (for validation, submitting and others)

@sergeyteleshev sergeyteleshev requested a review from Wroud January 3, 2024 11:14
@devnaumov devnaumov merged commit 9aace7c into devel Jan 3, 2024
3 of 4 checks passed
@serge-rider serge-rider deleted the CB-4364-remove-spaces-at-the-beginning-of-the-input-fields branch February 21, 2024 11:08
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.

4 participants