From b8e28f9cccf7d7e6685143b6b657e7df64168076 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Henry=20T=C3=A4schner?= <129834483+HenryT-CG@users.noreply.github.com> Date: Sun, 10 Nov 2024 07:18:54 +0100 Subject: [PATCH] Use a form on theme import (#296) * fix: use a form on theme import * fix: code cleanup --- .../theme-import/theme-import.component.html | 34 +++++--- .../theme-import.component.spec.ts | 81 +++++++++---------- .../theme-import/theme-import.component.ts | 67 +++++++-------- .../theme-search/theme-search.component.html | 6 +- .../theme-search.component.spec.ts | 16 +++- .../theme-search/theme-search.component.ts | 8 +- src/assets/i18n/de.json | 3 +- src/assets/i18n/en.json | 3 +- 8 files changed, 123 insertions(+), 95 deletions(-) diff --git a/src/app/theme/theme-import/theme-import.component.html b/src/app/theme/theme-import/theme-import.component.html index f8c17b3..2b4513a 100644 --- a/src/app/theme/theme-import/theme-import.component.html +++ b/src/app/theme/theme-import/theme-import.component.html @@ -6,6 +6,7 @@ [closable]="true" [modal]="true" (onHide)="onImportThemeHide()" + [style]="{ 'max-width': '700px' }" > -
+
{{ 'THEME.NO_PROPERTIES' | translate }}
+ + { let component: ThemeImportComponent let fixture: ComponentFixture + const themes: Theme[] = [ + { name: 'theme1', displayName: 'Theme-1' }, + { name: 'theme2', displayName: 'Theme-2' } + ] + const formGroup = new FormGroup({ + themeName: new FormControl(null), + displayName: new FormControl(null) + }) const msgServiceSpy = jasmine.createSpyObj('PortalMessageService', ['success', 'error']) - - const themeApiSpy = jasmine.createSpyObj('ThemesAPIService', ['getThemes', 'importThemes']) + const themeApiSpy = jasmine.createSpyObj('ThemesAPIService', ['importThemes']) beforeEach(waitForAsync(() => { TestBed.configureTestingModule({ @@ -37,7 +45,6 @@ describe('ThemeImportComponent', () => { ], schemas: [NO_ERRORS_SCHEMA] }).compileComponents() - themeApiSpy.getThemes.and.returnValue(of({ stream: [] }) as any) msgServiceSpy.success.calls.reset() msgServiceSpy.error.calls.reset() })) @@ -45,37 +52,25 @@ describe('ThemeImportComponent', () => { beforeEach(() => { fixture = TestBed.createComponent(ThemeImportComponent) component = fixture.componentInstance + component.displayThemeImport = true + component.themes = themes + component.formGroup = formGroup fixture.detectChanges() }) it('should create', () => { expect(component).toBeTruthy() - }) - - it('should initialize themes and headers onInit', () => { - const themeArr: Theme[] = [ - { name: 'theme1', displayName: 'Theme-1' }, - { name: 'theme2', displayName: 'Theme-2' } - ] - const themesResponse: GetThemesResponse = { stream: themeArr } - - themeApiSpy.getThemes.and.returnValue(of(themesResponse as any)) - - component.displayThemeImport = true - component.ngOnInit() - - expect(component.themes).toEqual(themeArr) + component.ngOnChanges() }) it('should read file on theme import select', async () => { const themeSnapshot = JSON.stringify({ themes: { themeName: { + displayName: 'themeDisplayName', logoUrl: 'logo_url', properties: { - general: { - 'primary-color': '#000000' - } + general: { 'primary-color': '#000000' } } } } @@ -87,11 +82,9 @@ describe('ThemeImportComponent', () => { expect(component.themeImportError).toBe(false) expect(component.themeSnapshot).toBeDefined() - expect(component.properties).toEqual({ - general: { - 'primary-color': '#000000' - } - }) + expect(component.properties).toEqual({ general: { 'primary-color': '#000000' } }) + expect(component.formGroup.controls['themeName'].value).toEqual('themeName') + expect(component.formGroup.controls['displayName'].value).toEqual('themeDisplayName') expect(component.themeNameExists).toBe(false) }) @@ -105,7 +98,6 @@ describe('ThemeImportComponent', () => { expect(component.themeImportError).toBe(true) expect(component.themeSnapshot).toBe(null) expect(console.error).toHaveBeenCalledOnceWith('Theme Import Error: not valid data ') - // TODO: if error is visible }) it('should log error on data parsing error', async () => { @@ -121,10 +113,9 @@ describe('ThemeImportComponent', () => { }) it('should indicate theme name existance if already present', async () => { - component.themes = [{ name: 'themeName', displayName: 'Theme-1' }] const themeSnapshot = JSON.stringify({ themes: { - themeName: { + theme1: { displayName: 'Theme-1', logoUrl: 'logo_url', properties: {} @@ -142,12 +133,12 @@ describe('ThemeImportComponent', () => { expect(component.displayNameExists).toBe(true) }) - it('should emit displayThemeImportChange on import hide', () => { - spyOn(component.displayThemeImportChange, 'emit') + it('should emit uploadEmitter false on closing import dialog', () => { + spyOn(component.uploadEmitter, 'emit') component.onImportThemeHide() - expect(component.displayThemeImportChange.emit).toHaveBeenCalledOnceWith(false) + expect(component.uploadEmitter.emit).toHaveBeenCalledOnceWith(false) }) it('should clear error and import data on import clear', () => { @@ -182,22 +173,22 @@ describe('ThemeImportComponent', () => { created: 'created', themes: { ['theme']: { description: 'themeDescription' } } } - component.themeName = 'themeName' - component.displayName = 'themeDisplayName' + component.formGroup.controls['themeName'].setValue('themeName') + component.formGroup.controls['displayName'].setValue('themeDisplayName') component.properties = {} component.onThemeUpload() expect(msgServiceSpy.success).toHaveBeenCalledOnceWith({ summaryKey: 'THEME.IMPORT.IMPORT_THEME_SUCCESS' }) - expect(component.uploadEmitter.emit).toHaveBeenCalledTimes(1) + expect(component.uploadEmitter.emit).toHaveBeenCalledOnceWith(true) }) it('should return if no themes available', () => { themeApiSpy.importThemes.and.returnValue(of(new HttpResponse({ body: { id: 'id' } }))) spyOn(component.uploadEmitter, 'emit') - component.themeName = 'themeName' - component.displayName = 'themeDisplayName' + component.formGroup.controls['themeName'].setValue('themeName') + component.formGroup.controls['displayName'].setValue('themeDisplayName') component.properties = {} component.onThemeUpload() @@ -210,7 +201,7 @@ describe('ThemeImportComponent', () => { themeApiSpy.importThemes.and.returnValue(of(new HttpResponse({ body: { id: 'id' } }))) spyOn(component.uploadEmitter, 'emit') - component.themeName = 'themeName' + component.formGroup.controls['themeName'].setValue('themeName') component.onThemeUpload() expect(component.uploadEmitter.emit).not.toHaveBeenCalled() @@ -224,11 +215,19 @@ describe('ThemeImportComponent', () => { themes: { ['theme']: { description: 'themeDescription' } } } - component.themeName = 'themeName' - component.displayName = 'themeDisplayName' + component.formGroup.controls['themeName'].setValue('themeName') + component.formGroup.controls['displayName'].setValue('themeDisplayName') component.properties = {} component.onThemeUpload() expect(msgServiceSpy.error).toHaveBeenCalledOnceWith({ summaryKey: 'THEME.IMPORT.IMPORT_THEME_FAIL' }) }) + + it('should not check existence if form is not ready', () => { + component.themeNameExists = true + spyOnProperty(component.formGroup, 'valid').and.returnValue(false) + component.onThemeNameChange() + + expect(component.themeNameExists).toBeTrue() + }) }) diff --git a/src/app/theme/theme-import/theme-import.component.ts b/src/app/theme/theme-import/theme-import.component.ts index ab40a6d..18d48ba 100644 --- a/src/app/theme/theme-import/theme-import.component.ts +++ b/src/app/theme/theme-import/theme-import.component.ts @@ -4,12 +4,13 @@ import { ChangeDetectorRef, EventEmitter, Input, - OnInit, Output, - ViewChild + ViewChild, + OnChanges } from '@angular/core' import { HttpHeaders } from '@angular/common/http' import { TranslateService } from '@ngx-translate/core' +import { FormControl, FormGroup, Validators } from '@angular/forms' import { ActivatedRoute, Router } from '@angular/router' import { PortalMessageService } from '@onecx/portal-integration-angular' @@ -22,22 +23,20 @@ import { FileSelectEvent } from 'primeng/fileupload' templateUrl: './theme-import.component.html', styleUrls: ['./theme-import.component.scss'] }) -export class ThemeImportComponent implements OnInit, AfterViewInit { +export class ThemeImportComponent implements OnChanges, AfterViewInit { @Input() public displayThemeImport = false - @Output() public displayThemeImportChange = new EventEmitter() - @Output() public uploadEmitter = new EventEmitter() + @Input() public themes: Theme[] = [] + @Output() public uploadEmitter = new EventEmitter() @ViewChild('themeNameInput') themeNameInput!: HTMLInputElement - public themes!: Theme[] - public themeName: string | undefined = undefined - public displayName: string | undefined = undefined public themeNameExists = false public displayNameExists = false public themeImportError = false public themeSnapshot: ThemeSnapshot | null = null public httpHeaders!: HttpHeaders public properties: any = null + public formGroup: FormGroup constructor( private readonly route: ActivatedRoute, @@ -46,13 +45,17 @@ export class ThemeImportComponent implements OnInit, AfterViewInit { public readonly translate: TranslateService, private readonly msgService: PortalMessageService, private readonly cd: ChangeDetectorRef - ) {} + ) { + this.formGroup = new FormGroup({ + themeName: new FormControl(null, [Validators.required, Validators.minLength(2), Validators.maxLength(100)]), + displayName: new FormControl(null, [Validators.required, Validators.minLength(2), Validators.maxLength(100)]) + }) + } - ngOnInit(): void { + ngOnChanges(): void { if (this.displayThemeImport) { this.httpHeaders = new HttpHeaders() this.httpHeaders = this.httpHeaders.set('Content-Type', 'application/json') - this.getThemes() } } @@ -61,6 +64,7 @@ export class ThemeImportComponent implements OnInit, AfterViewInit { } public async onImportThemeSelect(event: FileSelectEvent): Promise { + this.formGroup.reset() return event.files[0].text().then((text) => { this.themeSnapshot = null try { @@ -70,11 +74,11 @@ export class ThemeImportComponent implements OnInit, AfterViewInit { this.themeImportError = false if (themeSnapshot.themes) { const key: string[] = Object.keys(themeSnapshot.themes) - this.themeName = key[0] - this.displayName = themeSnapshot.themes[key[0]].displayName this.properties = themeSnapshot.themes[key[0]].properties + this.formGroup.controls['themeName'].setValue(key[0]) + this.formGroup.controls['displayName'].setValue(themeSnapshot.themes[key[0]].displayName) } - this.checkThemeExistence() + this.onThemeNameChange() } else { console.error('Theme Import Error: not valid data ') this.themeImportError = true @@ -85,31 +89,31 @@ export class ThemeImportComponent implements OnInit, AfterViewInit { }) } - public checkThemeExistence() { - this.themeNameExists = false - this.displayNameExists = false - if (this.themeName) this.themeNameExists = this.themes.filter((theme) => theme.name === this.themeName).length > 0 - if (this.displayName) - this.displayNameExists = this.themes.filter((theme) => theme.displayName === this.displayName).length > 0 + public onThemeNameChange() { + if (this.themes.length === 0 || !this.formGroup.valid) return + this.themeNameExists = + this.themes.filter((theme) => theme.name === this.formGroup.controls['themeName'].value).length > 0 + this.displayNameExists = + this.themes.filter((theme) => theme.displayName === this.formGroup.controls['displayName'].value).length > 0 } public onImportThemeHide(): void { - this.displayThemeImportChange.emit(false) + this.uploadEmitter.emit(false) } public onImportThemeClear(): void { this.themeSnapshot = null this.themeImportError = false } public onThemeUpload(): void { - if (!this.themeName || !this.displayName || !this.properties) return + if (!this.formGroup.valid || !this.properties) return if (!this.themeSnapshot?.themes) return // Import data preparation const key: string[] = Object.keys(this.themeSnapshot?.themes) - this.themeSnapshot.themes[key[0]].displayName = this.displayName - if (key[0] !== this.themeName) { + this.themeSnapshot.themes[key[0]].displayName = this.formGroup.controls['displayName'].value + if (key[0] !== this.formGroup.controls['themeName'].value) { // save the theme properties to be reassigned on new key const themeProps = Object.getOwnPropertyDescriptor(this.themeSnapshot.themes, key[0]) - Object.defineProperty(this.themeSnapshot.themes, this.themeName, themeProps!) + Object.defineProperty(this.themeSnapshot.themes, this.formGroup.controls['themeName'].value, themeProps!) delete this.themeSnapshot.themes[key[0]] } // Import execution: upload @@ -121,9 +125,8 @@ export class ThemeImportComponent implements OnInit, AfterViewInit { next: () => { this.msgService.success({ summaryKey: 'THEME.IMPORT.IMPORT_THEME_SUCCESS' }) this.onImportThemeClear() - this.displayThemeImport = false - this.uploadEmitter.emit() - this.router.navigate([`./${this.themeName}`], { relativeTo: this.route }) + this.uploadEmitter.emit(true) + this.router.navigate([`./${this.formGroup.controls['themeName'].value}`], { relativeTo: this.route }) }, error: () => { this.msgService.error({ summaryKey: 'THEME.IMPORT.IMPORT_THEME_FAIL' }) @@ -135,12 +138,4 @@ export class ThemeImportComponent implements OnInit, AfterViewInit { const dto = obj as ThemeSnapshot return !!(typeof dto === 'object' && dto?.themes) } - - private getThemes(): void { - this.themeApi.getThemes({}).subscribe((themes) => { - if (themes.stream) { - this.themes = themes.stream - } - }) - } } diff --git a/src/app/theme/theme-search/theme-search.component.html b/src/app/theme/theme-search/theme-search.component.html index 4566727..7e1f4c0 100644 --- a/src/app/theme/theme-search/theme-search.component.html +++ b/src/app/theme/theme-search/theme-search.component.html @@ -149,6 +149,10 @@
+ - diff --git a/src/app/theme/theme-search/theme-search.component.spec.ts b/src/app/theme/theme-search/theme-search.component.spec.ts index 9c98fa2..22ba4fd 100644 --- a/src/app/theme/theme-search/theme-search.component.spec.ts +++ b/src/app/theme/theme-search/theme-search.component.spec.ts @@ -195,8 +195,20 @@ describe('ThemeSearchComponent', () => { }) it('should show import dialog on import theme click', () => { - component.themeImportDialogVisible = false + component.importDialogVisible = false component.onImportThemeClick() - expect(component.themeImportDialogVisible).toBe(true) + expect(component.importDialogVisible).toBe(true) + }) + it('should hide import dialog on import close', () => { + component.importDialogVisible = true + component.onThemeUpload(false) + expect(component.importDialogVisible).toBe(false) + }) + it('should hide import dialog on import close and reload', () => { + spyOn(component, 'loadThemes') + component.importDialogVisible = true + component.onThemeUpload(true) + expect(component.importDialogVisible).toBe(false) + expect(component.loadThemes).toHaveBeenCalledTimes(1) }) }) diff --git a/src/app/theme/theme-search/theme-search.component.ts b/src/app/theme/theme-search/theme-search.component.ts index 75c7f1b..f6fe576 100644 --- a/src/app/theme/theme-search/theme-search.component.ts +++ b/src/app/theme/theme-search/theme-search.component.ts @@ -24,7 +24,7 @@ export class ThemeSearchComponent implements OnInit { public sortOrder = 1 public limitText = limitText - public themeImportDialogVisible = false + public importDialogVisible = false public dataViewControlsTranslations: DataViewControlTranslations = {} @ViewChild(DataView) dv: DataView | undefined @@ -147,6 +147,10 @@ export class ThemeSearchComponent implements OnInit { this.sortOrder = asc ? -1 : 1 } public onImportThemeClick(): void { - this.themeImportDialogVisible = true + this.importDialogVisible = true + } + public onThemeUpload(uploaded: boolean) { + this.importDialogVisible = false + if (uploaded) this.loadThemes() } } diff --git a/src/assets/i18n/de.json b/src/assets/i18n/de.json index 15c8702..d0e6acd 100644 --- a/src/assets/i18n/de.json +++ b/src/assets/i18n/de.json @@ -160,7 +160,8 @@ "HEADER": "Import Theme", "IMPORT_THEME_SUCCESS": "Theme wurde erfolgreich importiert.", "IMPORT_THEME_FAIL": "Ein Fehler ist aufgetreten. Das Theme wurde nicht importiert.", - "THEME_EXISTS_MESSAGE": "Ein Theme mit diesem Namen existiert bereits. Dessen Definition wird überschrieben.", + "NAME_EXISTS_MESSAGE": "Ein Theme mit diesem Namen existiert bereits. Dessen Definition wird überschrieben.", + "DISPLAY_NAME_EXISTS_MESSAGE": "Ein Theme mit diesem Anzeigenamen existiert bereits. Importing is possible but results in themes with the same display name.", "THEME_COLOR_PRIMARY": "Primärfarbe", "THEME_COLOR_SECONDARY": "Sekundärfarbe", "THEME_COLOR_TEXT": "Textfarbe", diff --git a/src/assets/i18n/en.json b/src/assets/i18n/en.json index dc4db8e..c88c658 100644 --- a/src/assets/i18n/en.json +++ b/src/assets/i18n/en.json @@ -160,7 +160,8 @@ "HEADER": "Import Theme", "IMPORT_THEME_SUCCESS": "The Theme was sucessfully imported.", "IMPORT_THEME_FAIL": "An error occurred, the Theme was not imported.", - "THEME_EXISTS_MESSAGE": "A Theme with this name already exists. Its definition is overwritten.", + "NAME_EXISTS_MESSAGE": "A Theme with this name already exists. Its definition is overwritten.", + "DISPLAY_NAME_EXISTS_MESSAGE": "A Theme with this display name already exists. ", "THEME_COLOR_PRIMARY": "Primary color", "THEME_COLOR_SECONDARY": "Secondary color", "THEME_COLOR_TEXT": "Text color",