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 logic to allow warnings for input fields #3256

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 17 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions apps/keira/src/app/scss/_editor.scss
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
@import 'variables';

input.ng-invalid.ng-touched {
border-color: red;
}

.top-bar {
background-color: #000;
color: $content-block-bg-color;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,11 @@
<div class="form-group col-12 col-sm-6 col-md-4 col-lg-3 col-xl-2">
<label class="control-label" for="minlevel">minlevel</label>
<i class="fas fa-info-circle ms-1" placement="auto" [tooltip]="'CREATURE.TEMPLATE.MIN_MAX_LEVEL' | translate"></i>
<input [formControlName]="'minlevel'" id="minlevel" type="number" class="form-control form-control-sm" />
<input keiraInputValidation [formControlName]="'minlevel'" id="minlevel" type="number" class="form-control form-control-sm" />
</div>
<div class="form-group col-12 col-sm-6 col-md-4 col-lg-3 col-xl-2">
<label class="control-label" for="maxlevel">maxlevel</label>
<i class="fas fa-info-circle ms-1" placement="auto" [tooltip]="'CREATURE.TEMPLATE.MIN_MAX_LEVEL' | translate"></i>
<input [formControlName]="'maxlevel'" id="maxlevel" type="number" class="form-control form-control-sm" />
<label for="maxLevel">maxlevel</label>
<input keiraInputValidation [formControlName]="'maxlevel'" id="maxlevel" type="number" class="form-control form-control-sm" />
</div>
<div class="form-group col-12 col-sm-6 col-md-4 col-lg-3 col-xl-2">
<label class="control-label" for="exp">expansion</label>
Expand Down Expand Up @@ -231,7 +230,8 @@
<i class="fas fa-info-circle ms-1" placement="auto" [tooltip]="'CREATURE.TEMPLATE.MODEL_ID_TOOLTIP' | translate"></i>
</span>
<span
>Models are now available in the <a href="" [routerLink]="['/creature', 'creature-template-model']">Creature Template Model</a> editor.</span
>Models are now available in the
<a href="" [routerLink]="['/creature', 'creature-template-model']">Creature Template Model</a> editor.</span
>
</div>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import {
} from '@keira/shared/acore-world-model';
import { SingleRowEditorComponent } from '@keira/shared/base-abstract-classes';
import { QueryOutputComponent, TopBarComponent } from '@keira/shared/base-editor-components';
import { Model3DViewerComponent } from '@keira/shared/model-3d-viewer';
import {
BooleanOptionSelectorComponent,
CreatureSelectorBtnComponent,
Expand All @@ -41,6 +40,8 @@ import { TooltipModule } from 'ngx-bootstrap/tooltip';
import { CreatureHandlerService } from '../creature-handler.service';
import { CreatureTemplateService } from './creature-template.service';
import { RouterLink } from '@angular/router';
import { InputValidationDirective } from '@keira/shared/directives';
import { ValidationService } from '@keira/shared/common-services';

@Component({
changeDetection: ChangeDetectionStrategy.OnPush,
Expand All @@ -59,12 +60,13 @@ import { RouterLink } from '@angular/router';
FlagsSelectorBtnComponent,
SpellSelectorBtnComponent,
CreatureSelectorBtnComponent,
Model3DViewerComponent,
GenericOptionSelectorComponent,
BooleanOptionSelectorComponent,
IconSelectorComponent,
RouterLink,
InputValidationDirective,
],
providers: [ValidationService],
Copy link
Contributor

@Francesco-Borzi Francesco-Borzi Jan 29, 2025

Choose a reason for hiding this comment

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

why does it have to be a singleton?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because validation should happen on a page by page basis and not on the app level ?

Copy link
Member

Choose a reason for hiding this comment

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

It should not be necessary, we could keep one instance for the entire application, so provide it in "root" and don't put it here.

If you find a strong reason to provide it inside the component then we could put it back.

})
export class CreatureTemplateComponent extends SingleRowEditorComponent<CreatureTemplate> {
protected readonly UNIT_FLAGS = UNIT_FLAGS;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ describe('QuestPreviewService', () => {
expect(mysqlQueryService.getItemNameByStartQuest).toHaveBeenCalledTimes(1);
expect(mysqlQueryService.getItemNameByStartQuest).toHaveBeenCalledWith(mockID);
expect(mysqlQueryService.getReputationRewardByFaction).toHaveBeenCalledTimes(1);
expect(mysqlQueryService.getReputationRewardByFaction).toHaveBeenCalledWith(null as any);
expect(mysqlQueryService.getReputationRewardByFaction).toHaveBeenCalledWith(0);
});

it('sqliteQuery', async () => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
import { TestBed } from '@angular/core/testing';
import { RouterTestingModule } from '@angular/router/testing';

import { QueryError } from 'mysql2';
import { ToastrService } from 'ngx-toastr';
import { of, throwError } from 'rxjs';
import { instance, mock } from 'ts-mockito';
import { MysqlQueryService } from '@keira/shared/db-layer';
import { EditorService } from './editor.service';

import { mockChangeDetectorRef } from '@keira/shared/test-utils';
import { MockEntity, MockSingleRowEditorService } from '../../core.mock';
import Spy = jasmine.Spy;
Expand All @@ -18,7 +15,6 @@ describe('EditorService', () => {

beforeEach(() =>
TestBed.configureTestingModule({
imports: [RouterTestingModule],
providers: [
{ provide: MysqlQueryService, useValue: instance(mock(MysqlQueryService)) },
{ provide: ToastrService, useValue: instance(mock(ToastrService)) },
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { QueryError } from 'mysql2';
import { ToastrService } from 'ngx-toastr';
import { Observable } from 'rxjs';
import { FormControl, FormGroup } from '@angular/forms';
import { FormControl, FormGroup, Validators } from '@angular/forms';

import { Class, StringKeys, TableRow } from '@keira/shared/constants';
import { MysqlQueryService } from '@keira/shared/db-layer';
Expand Down Expand Up @@ -82,10 +82,15 @@ export abstract class EditorService<T extends TableRow> extends SubscriptionHand
}

protected initForm(): void {
const defaultValues = new this._entityClass();

// Initialize the FormGroup
this._form = new FormGroup<ModelForm<T>>({} as any);

// Loop through the fields and initialize controls with default values
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this comment, IMHO the code seems already quite self-explanatory :)

Suggested change
// Loop through the fields and initialize controls with default values

for (const field of this.fields) {
this._form.addControl(field, new FormControl());
const defaultValue = defaultValues[field];
this._form.addControl(field, new FormControl(defaultValue, [Validators.required]));
Comment on lines +91 to +92
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const defaultValue = defaultValues[field];
this._form.addControl(field, new FormControl(defaultValue, [Validators.required]));
this._form.addControl(field, new FormControl(defaultValues[field], [Validators.required]));

}

this.disableEntityIdField();
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import { TestBed } from '@angular/core/testing';
import { RouterTestingModule } from '@angular/router/testing';

import { ToastrService } from 'ngx-toastr';
import { instance, mock } from 'ts-mockito';
import { MysqlQueryService } from '@keira/shared/db-layer';
Expand All @@ -14,7 +12,6 @@ describe('SingleRowEditorService', () => {

beforeEach(() =>
TestBed.configureTestingModule({
imports: [RouterTestingModule],
providers: [
{ provide: MysqlQueryService, useValue: instance(mock(MysqlQueryService)) },
{ provide: ToastrService, useValue: instance(mock(ToastrService)) },
Expand Down Expand Up @@ -162,4 +159,77 @@ describe('SingleRowEditorService', () => {
expect(updateFullQuerySpy).toHaveBeenCalledTimes(1);
});
});

describe('updateFormAfterReload()', () => {
let consoleErrorSpy: Spy;
let consoleWarnSpy: Spy;
let mockForm: any;

beforeEach(() => {
// Mock the form and its controls
mockForm = {
controls: {
id: { setValue: jasmine.createSpy('setValue') },
name: { setValue: jasmine.createSpy('setValue') },
guid: { setValue: jasmine.createSpy('setValue') }, // Add guid control
},
};
service['_form'] = mockForm;

// Mock originalValue
service['_originalValue'] = { id: 123, name: 'Test Name', guid: 456 }; // Add guid value

// Spy on console.error and console.warn
consoleErrorSpy = spyOn(console, 'error');
consoleWarnSpy = spyOn(console, 'warn');

// Temporarily override `fields` for testing
Object.defineProperty(service, 'fields', {
value: ['id', 'name', 'guid', 123 as any, null as any],
writable: true,
});
});

it('should set values for valid fields in the form', () => {
service['updateFormAfterReload']();

expect(mockForm.controls.id.setValue).toHaveBeenCalledWith(123);
expect(mockForm.controls.name.setValue).toHaveBeenCalledWith('Test Name');
expect(mockForm.controls.guid.setValue).toHaveBeenCalledWith(456);
});

it('should log an error for missing controls', () => {
// Override `fields` to include an invalid field
Object.defineProperty(service, 'fields', {
value: ['id', 'missingField'],
});

service['updateFormAfterReload']();

expect(consoleErrorSpy).toHaveBeenCalledWith("Control 'missingField' does not exist!");
});

it('should log a warning for invalid field types', () => {
service['updateFormAfterReload']();

expect(consoleWarnSpy).toHaveBeenCalledWith("Field '123' is not a valid string key.");
expect(consoleWarnSpy).toHaveBeenCalledWith("Field 'null' is not a valid string key.");
});

it('should not throw errors for valid but empty fields', () => {
Object.defineProperty(service, 'fields', {
value: [], // No fields to iterate
});

expect(() => service['updateFormAfterReload']()).not.toThrow();
});

it('should reset loading to false after execution', () => {
service['_loading'] = true;

service['updateFormAfterReload']();

expect(service['_loading']).toBe(false); // Ensure loading is reset
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -86,19 +86,27 @@ export abstract class SingleRowEditorService<T extends TableRow> extends EditorS

protected updateFormAfterReload() {
this._loading = true;

for (const field of this.fields) {
const control = this._form.controls[field];
/* istanbul ignore else */
if (control) {
control.setValue(this._originalValue[field]);
} else {
console.error(`Control '${field}' does not exist!`);
console.log(`----------- DEBUG CONTROL KEYS:`);
for (const k of Object.keys(this._form.controls)) {
console.log(k);
// Ensure `field` is of type `string`
if (typeof field === 'string') {
const control = this._form.controls[field];
Comment on lines +92 to +93
Copy link
Contributor

Choose a reason for hiding this comment

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

is this change needed for the original purpose of the PR? in general, I'd prefer to split changes in several PRs, for example:

  • 1 PR to include new functionality (including its unit tests)
  • a separate PR to improve/refactor existing code that is not related to the functionality being introduced


if (control) {
const value = this._originalValue[field as keyof T]; // Ensure type safety here
control.setValue(value as T[typeof field]);
} else {
console.error(`Control '${field}' does not exist!`);
console.log(`----------- DEBUG CONTROL KEYS:`);
for (const k of Object.keys(this._form.controls)) {
console.log(k);
}
}
Copy link
Member

@Helias Helias Jan 28, 2025

Choose a reason for hiding this comment

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

not sure about this code inside the else block, it seems more a debug code , we could keep it, but could you give me more insight about it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@Exitare please do not mark comments as resolved without answering or addressing them

Copy link
Member Author

@Exitare Exitare Jan 29, 2025

Choose a reason for hiding this comment

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

I resolved it, because I removed it initially. But then one of the tests failed and I added it back, but didn't unresolve the issue here.

} else {
console.warn(`Field '${String(field)}' is not a valid string key.`);
}
}

this._loading = false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,16 +24,40 @@
<keira-highlightjs-wrapper id="diff-query" [code]="editorService.diffQuery" [hidden]="showFullQuery()" />
<keira-highlightjs-wrapper id="full-query" [code]="editorService.fullQuery" [hidden]="!showFullQuery()" />
<div>
<button type="button" class="btn btn-secondary btn-sm" (click)="copy()" id="copy-btn">
<button
type="button"
class="btn btn-secondary btn-sm"
(click)="copy()"
id="copy-btn"
[disabled]="(validationService.validationPassed$ | async) === false"
>
<i class="fa fa-copy fa-sm"></i> {{ 'COPY' | translate }}
</button>
<button type="button" class="btn btn-primary btn-sm" (click)="execute()" id="execute-btn">
<button
type="button"
class="btn btn-primary btn-sm"
(click)="execute()"
id="execute-btn"
[disabled]="(validationService.validationPassed$ | async) === false"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should avoid this repetition and wrap validationService.validationPassed$ in a variable. Even better to use a Signal.

Copy link
Member Author

@Exitare Exitare Jan 29, 2025

Choose a reason for hiding this comment

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

I don't know how this requested change should look like because I used a variable before and it was requested to use the async pipe. Now it's back to variable again?

Copy link
Member

Choose a reason for hiding this comment

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

Before it was a variable with a subscription running on ngOninit, this affects the component adding a dependency for the OnInit interface and moreover, it forces you to manage the subscribe/unsubscribe.
In the current situation you don't need ngOninit and to manage the subscribe/unsubscribe thanks to the async pipe.

Wrapping this into a variable can be done without ngOninit still and in the following way:

protected readonly isValidationPassed$ =  this.validationService.validationPassed$.pipe(map((val) => val === false));

Keeping in the template

[disabled]="isValidationPassed$ | async"

Or, you could use signals

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, for learning purposes. What's the undesirable part about having a dependency in the ngOnInit, besides the subscription management part. Is it a general best practice not to use the ngOnInit if possible?

Copy link
Member

@Helias Helias Jan 30, 2025

Choose a reason for hiding this comment

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

Personally I avoid any kind of dependencies, so if we can avoid ngOnInit, ngOnChanges etc. in an elegant way I would not add them because It increases the complexity of the logic component

>
<i class="fa fa-bolt fa-sm"></i> {{ 'EXECUTE' | translate }}
</button>
<button type="button" class="btn btn-success btn-sm" (click)="copy(); execute()" id="execute-and-copy-btn">
<button
type="button"
class="btn btn-success btn-sm"
(click)="copy(); execute()"
id="execute-and-copy-btn"
[disabled]="(validationService.validationPassed$ | async) === false"
>
<i class="fa fa-bolt fa-sm"></i> {{ 'EXECUTE_AND_COPY' | translate }} <i class="fa fa-copy fa-sm"></i>
</button>
<button type="button" class="btn btn-danger btn-sm float-end" (click)="reload()" id="reload-btn">
<button
type="button"
class="btn btn-danger btn-sm float-end"
(click)="reload()"
id="reload-btn"
[disabled]="(validationService.validationPassed$ | async) === false"
>
<i class="fa fa-sync fa-sm"></i> {{ 'RELOAD' | translate }}
</button>
</div>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { ChangeDetectionStrategy, ChangeDetectorRef, Component, EventEmitter, inject, Input, Output } from '@angular/core';

import { FormsModule } from '@angular/forms';
import { EditorService } from '@keira/shared/base-abstract-classes';
import { TableRow } from '@keira/shared/constants';
Expand All @@ -11,6 +10,8 @@ import { filter } from 'rxjs';
import { HighlightjsWrapperComponent } from '../highlightjs-wrapper/highlightjs-wrapper.component';
import { ModalConfirmComponent } from '../modal-confirm/modal-confirm.component';
import { QueryErrorComponent } from './query-error/query-error.component';
import { ValidationService } from '@keira/shared/common-services';
import { AsyncPipe } from '@angular/common';

@Component({
// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection
Expand All @@ -19,18 +20,18 @@ import { QueryErrorComponent } from './query-error/query-error.component';
templateUrl: './query-output.component.html',
styleUrls: ['./query-output.component.scss'],
standalone: true,
imports: [FormsModule, HighlightjsWrapperComponent, QueryErrorComponent, TranslateModule],
imports: [FormsModule, HighlightjsWrapperComponent, QueryErrorComponent, TranslateModule, AsyncPipe],
})
export class QueryOutputComponent<T extends TableRow> extends SubscriptionHandler {
private readonly clipboardService = inject(ClipboardService);
private readonly modalService = inject(BsModalService);
protected readonly validationService = inject(ValidationService);

@Input() docUrl!: string;
@Input() editorService!: EditorService<T>;
@Output() executeQuery = new EventEmitter<string>();
selectedQuery: 'diff' | 'full' = 'diff';
private modalRef!: BsModalRef;

private readonly changeDetectorRef = inject(ChangeDetectorRef);

showFullQuery(): boolean {
Expand Down
1 change: 1 addition & 0 deletions libs/shared/common-services/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export * from './electron.service';
export * from './config.service';
export * from './location.service';
export * from './validation.service';
Loading