-
Notifications
You must be signed in to change notification settings - Fork 399
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
Cb 4364 remove spaces at the beginning of the input fields #2252
Conversation
@@ -204,6 +204,9 @@ export class ServerConfigurationService { | |||
return; | |||
} | |||
|
|||
data.state.serverConfig.serverName = data.state.serverConfig.serverName?.trim(); |
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.
Please do not modify state directly, use contexts config
const config = contexts.getContext(configContext);
config.serverConfig.serverName = data.state.serverConfig.serverName?.trim()
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.
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') { |
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.
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(); |
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.
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(); |
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.
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) { |
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.
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(); |
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.
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]; |
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.
Why do we need to delete the property first and trim the key 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.
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 => { |
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.
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) { |
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.
empty if block?
} | ||
} | ||
|
||
private trimSSHInputConfig(input: NetworkHandlerConfigInput) { |
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.
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(); |
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.
and better is do it in prepareConfig stage
state.credentials.credentials.user = state.credentials.credentials.user?.trim(); | ||
state.credentials.credentials.password = state.credentials.credentials.password?.trim(); |
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.
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]; |
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.
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)[]; |
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.
double type declaration remove one
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.
and this also looks like global const for not global const please use camelCase
…he-input-fields
@@ -187,15 +193,25 @@ export class UserFormInfoPart extends FormPart<IUserFormInfoState, IUserFormStat | |||
} | |||
|
|||
private async updateMetaParameters() { | |||
const tempMetaParameters = toJS(this.state.metaParameters); |
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.
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); |
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.
credentials have Record<string, any> type, so wee need more general aproach
…he-input-fields
@@ -132,10 +136,12 @@ export class UserFormInfoPart extends FormPart<IUserFormInfoState, IUserFormStat | |||
} | |||
|
|||
private async updateCredentials() { | |||
if (this.state.password) { | |||
const password = this.state.password.trim(); |
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.
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)
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 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)
...gin-authentication-administration/src/Administration/Users/UserForm/Info/UserFormInfoPart.ts
Outdated
Show resolved
Hide resolved
...ugin-connections/src/ConnectionForm/DriverProperties/ConnectionDriverPropertiesTabService.ts
Outdated
Show resolved
Hide resolved
...ugin-connections/src/ConnectionForm/DriverProperties/ConnectionDriverPropertiesTabService.ts
Outdated
Show resolved
Hide resolved
...gin-authentication-administration/src/Administration/Users/UserForm/Info/UserFormInfoPart.ts
Outdated
Show resolved
Hide resolved
...gin-authentication-administration/src/Administration/Users/UserForm/Info/UserFormInfoPart.ts
Outdated
Show resolved
Hide resolved
...ugin-connections/src/ConnectionForm/DriverProperties/ConnectionDriverPropertiesTabService.ts
Outdated
Show resolved
Hide resolved
…he-input-fields
…he-input-fields
…he-input-fields
No description provided.