Editor: Fix error when creating new record (#1061)

* feat(notifications-service): log errors in console when displaying error notifications * fix(editor): add missing unsubscribe on edit-page component teardown This avoid having unwanted navigations to /edit/uuid when loading a new record for instance. --------- Co-authored-by: Olivia Guyot <[email protected]>
geonetwork · Dec 19, 2024 · 8bec1c9 · 8bec1c9
1 parent da5f436
commit 8bec1c9
Show file tree

Hide file tree

Showing 16 changed files with 249 additions and 157 deletions.
diff --git a/apps/metadata-editor/src/app/duplicate-record.resolver.spec.ts b/apps/metadata-editor/src/app/duplicate-record.resolver.spec.ts
@@ -75,12 +75,16 @@ describe('DuplicateRecordResolver', () => {
       expect(resolvedData).toBeUndefined()
     })
     it('should show error notification', () => {
-      expect(notificationsService.showNotification).toHaveBeenCalledWith({
-        type: 'error',
-        title: 'editor.record.loadError.title',
-        text: 'editor.record.loadError.body oopsie',
-        closeMessage: 'editor.record.loadError.closeMessage',
-      })
+      expect(notificationsService.showNotification).toHaveBeenCalledWith(
+        {
+          type: 'error',
+          title: 'editor.record.loadError.title',
+          text: 'editor.record.loadError.body oopsie',
+          closeMessage: 'editor.record.loadError.closeMessage',
+        },
+        undefined,
+        expect.any(Error)
+      )
     })
   })
 })
diff --git a/apps/metadata-editor/src/app/duplicate-record.resolver.ts b/apps/metadata-editor/src/app/duplicate-record.resolver.ts
@@ -23,18 +23,22 @@ export class DuplicateRecordResolver {
       .openRecordForDuplication(route.paramMap.get('uuid'))
       .pipe(
         catchError((error) => {
-          this.notificationsService.showNotification({
-            type: 'error',
-            title: this.translateService.instant(
-              'editor.record.loadError.title'
-            ),
-            text: `${this.translateService.instant(
-              'editor.record.loadError.body'
-            )} ${error.message}`,
-            closeMessage: this.translateService.instant(
-              'editor.record.loadError.closeMessage'
-            ),
-          })
+          this.notificationsService.showNotification(
+            {
+              type: 'error',
+              title: this.translateService.instant(
+                'editor.record.loadError.title'
+              ),
+              text: `${this.translateService.instant(
+                'editor.record.loadError.body'
+              )} ${error.message}`,
+              closeMessage: this.translateService.instant(
+                'editor.record.loadError.closeMessage'
+              ),
+            },
+            undefined,
+            error
+          )
           return EMPTY
         })
       )

diff --git a/apps/metadata-editor/src/app/edit-record.resolver.spec.ts b/apps/metadata-editor/src/app/edit-record.resolver.spec.ts
@@ -75,12 +75,16 @@ describe('EditRecordResolver', () => {
       expect(resolvedData).toBeUndefined()
     })
     it('should show error notification', () => {
-      expect(notificationsService.showNotification).toHaveBeenCalledWith({
-        type: 'error',
-        title: 'editor.record.loadError.title',
-        text: 'editor.record.loadError.body oopsie',
-        closeMessage: 'editor.record.loadError.closeMessage',
-      })
+      expect(notificationsService.showNotification).toHaveBeenCalledWith(
+        {
+          type: 'error',
+          title: 'editor.record.loadError.title',
+          text: 'editor.record.loadError.body oopsie',
+          closeMessage: 'editor.record.loadError.closeMessage',
+        },
+        undefined,
+        expect.any(Error)
+      )
     })
   })
 })
diff --git a/apps/metadata-editor/src/app/edit-record.resolver.ts b/apps/metadata-editor/src/app/edit-record.resolver.ts
@@ -23,18 +23,22 @@ export class EditRecordResolver {
       .openRecordForEdition(route.paramMap.get('uuid'))
       .pipe(
         catchError((error) => {
-          this.notificationsService.showNotification({
-            type: 'error',
-            title: this.translateService.instant(
-              'editor.record.loadError.title'
-            ),
-            text: `${this.translateService.instant(
-              'editor.record.loadError.body'
-            )} ${error.message}`,
-            closeMessage: this.translateService.instant(
-              'editor.record.loadError.closeMessage'
-            ),
-          })
+          this.notificationsService.showNotification(
+            {
+              type: 'error',
+              title: this.translateService.instant(
+                'editor.record.loadError.title'
+              ),
+              text: `${this.translateService.instant(
+                'editor.record.loadError.body'
+              )} ${error.message}`,
+              closeMessage: this.translateService.instant(
+                'editor.record.loadError.closeMessage'
+              ),
+            },
+            undefined,
+            error
+          )
           return EMPTY
         })
       )

diff --git a/apps/metadata-editor/src/app/edit/edit-page.component.spec.ts b/apps/metadata-editor/src/app/edit/edit-page.component.spec.ts
@@ -121,24 +121,32 @@ describe('EditPageComponent', () => {
     describe('publish version error', () => {
       it('shows notification', () => {
         ;(facade.saveError$ as any).next(new PublicationVersionError('1.0.0'))
-        expect(notificationsService.showNotification).toHaveBeenCalledWith({
-          type: 'error',
-          title: 'editor.record.publishVersionError.title',
-          text: 'editor.record.publishVersionError.body',
-          closeMessage: 'editor.record.publishVersionError.closeMessage',
-        })
+        expect(notificationsService.showNotification).toHaveBeenCalledWith(
+          {
+            type: 'error',
+            title: 'editor.record.publishVersionError.title',
+            text: 'editor.record.publishVersionError.body',
+            closeMessage: 'editor.record.publishVersionError.closeMessage',
+          },
+          undefined,
+          expect.any(PublicationVersionError)
+        )
       })
     })
 
     describe('publish error', () => {
       it('shows notification', () => {
         ;(facade.saveError$ as any).next(new Error('oopsie'))
-        expect(notificationsService.showNotification).toHaveBeenCalledWith({
-          type: 'error',
-          title: 'editor.record.publishError.title',
-          text: 'editor.record.publishError.body oopsie',
-          closeMessage: 'editor.record.publishError.closeMessage',
-        })
+        expect(notificationsService.showNotification).toHaveBeenCalledWith(
+          {
+            type: 'error',
+            title: 'editor.record.publishError.title',
+            text: 'editor.record.publishError.body oopsie',
+            closeMessage: 'editor.record.publishError.closeMessage',
+          },
+          undefined,
+          expect.any(Error)
+        )
       })
     })
 
@@ -204,4 +212,25 @@ describe('EditPageComponent', () => {
       expect(await firstValueFrom(component.isLastPage$)).toBe(false)
     })
   })
+
+  describe('subscriptions', () => {
+    it('should add 3 subscriptions to component.subscription', () => {
+      const addSpy = jest.spyOn(component.subscription, 'add')
+      component.ngOnInit()
+      expect(addSpy).toHaveBeenCalledTimes(3)
+    })
+    it('should add 4 subscriptions to component.subscription when on /create route', () => {
+      const activatedRoute = TestBed.inject(ActivatedRoute)
+      activatedRoute.snapshot.routeConfig.path = '/create'
+      fixture.detectChanges()
+      const addSpy = jest.spyOn(component.subscription, 'add')
+      component.ngOnInit()
+      expect(addSpy).toHaveBeenCalledTimes(4)
+    })
+    it('unsubscribes', () => {
+      const unsubscribeSpy = jest.spyOn(component.subscription, 'unsubscribe')
+      component.ngOnDestroy()
+      expect(unsubscribeSpy).toHaveBeenCalled()
+    })
+  })
 })
diff --git a/apps/metadata-editor/src/app/edit/edit-page.component.ts b/apps/metadata-editor/src/app/edit/edit-page.component.ts
@@ -24,7 +24,6 @@ import { combineLatest, filter, firstValueFrom, Subscription, take } from 'rxjs'
 import { map } from 'rxjs/operators'
 import { SidebarComponent } from '../dashboard/sidebar/sidebar.component'
 import { PageSelectorComponent } from './components/page-selector/page-selector.component'
-import { PublishButtonComponent } from './components/publish-button/publish-button.component'
 import { TopToolbarComponent } from './components/top-toolbar/top-toolbar.component'
 
 marker('editor.record.form.bottomButtons.comeBackLater')
@@ -41,7 +40,6 @@ marker('editor.record.form.bottomButtons.next')
     CommonModule,
     ButtonComponent,
     MatProgressSpinnerModule,
-    PublishButtonComponent,
     TopToolbarComponent,
     NotificationsContainerComponent,
     PageSelectorComponent,
@@ -83,32 +81,40 @@ export class EditPageComponent implements OnInit, OnDestroy {
     this.subscription.add(
       this.facade.saveError$.subscribe((error) => {
         if (error instanceof PublicationVersionError) {
-          this.notificationsService.showNotification({
-            type: 'error',
-            title: this.translateService.instant(
-              'editor.record.publishVersionError.title'
-            ),
-            text: this.translateService.instant(
-              'editor.record.publishVersionError.body',
-              { currentVersion: error.detectedApiVersion }
-            ),
-            closeMessage: this.translateService.instant(
-              'editor.record.publishVersionError.closeMessage'
-            ),
-          })
+          this.notificationsService.showNotification(
+            {
+              type: 'error',
+              title: this.translateService.instant(
+                'editor.record.publishVersionError.title'
+              ),
+              text: this.translateService.instant(
+                'editor.record.publishVersionError.body',
+                { currentVersion: error.detectedApiVersion }
+              ),
+              closeMessage: this.translateService.instant(
+                'editor.record.publishVersionError.closeMessage'
+              ),
+            },
+            undefined,
+            error
+          )
         } else {
-          this.notificationsService.showNotification({
-            type: 'error',
-            title: this.translateService.instant(
-              'editor.record.publishError.title'
-            ),
-            text: `${this.translateService.instant(
-              'editor.record.publishError.body'
-            )} ${error.message}`,
-            closeMessage: this.translateService.instant(
-              'editor.record.publishError.closeMessage'
-            ),
-          })
+          this.notificationsService.showNotification(
+            {
+              type: 'error',
+              title: this.translateService.instant(
+                'editor.record.publishError.title'
+              ),
+              text: `${this.translateService.instant(
+                'editor.record.publishError.body'
+              )} ${error.message}`,
+              closeMessage: this.translateService.instant(
+                'editor.record.publishError.closeMessage'
+              ),
+            },
+            undefined,
+            error
+          )
         }
       })
     )
@@ -132,25 +138,29 @@ export class EditPageComponent implements OnInit, OnDestroy {
 
     // if we're on the /create route, go to /edit/{uuid} on first change
     if (this.route.snapshot.routeConfig?.path.includes('create')) {
-      this.facade.draftSaveSuccess$.pipe(take(1)).subscribe(() => {
-        this.router.navigate(['edit', currentRecord.uniqueIdentifier], {
-          replaceUrl: true,
+      this.subscription.add(
+        this.facade.draftSaveSuccess$.pipe(take(1)).subscribe(() => {
+          this.router.navigate(['edit', currentRecord.uniqueIdentifier], {
+            replaceUrl: true,
+          })
         })
-      })
+      )
     }
 
     // if the record unique identifier changes, navigate to /edit/newUuid
-    this.facade.record$
-      .pipe(
-        filter(
-          (record) =>
-            record?.uniqueIdentifier !== currentRecord.uniqueIdentifier
-        ),
-        take(1)
-      )
-      .subscribe((savedRecord) => {
-        this.router.navigate(['edit', savedRecord.uniqueIdentifier])
-      })
+    this.subscription.add(
+      this.facade.record$
+        .pipe(
+          filter(
+            (record) =>
+              record?.uniqueIdentifier !== currentRecord.uniqueIdentifier
+          ),
+          take(1)
+        )
+        .subscribe((savedRecord) => {
+          this.router.navigate(['edit', savedRecord.uniqueIdentifier])
+        })
+    )
   }
 
   ngOnDestroy() {

diff --git a/libs/feature/editor/src/lib/components/import-record/import-record.component.spec.ts b/libs/feature/editor/src/lib/components/import-record/import-record.component.spec.ts
@@ -112,7 +112,8 @@ describe('ImportRecordComponent', () => {
         title: 'editor.record.importFromExternalFile.failure.title',
         text: `editor.record.importFromExternalFile.failure.body `,
       }),
-      2500
+      2500,
+      mockError
     )
 
     expect(component.isRecordImportInProgress).toBe(false)

diff --git a/libs/feature/editor/src/lib/components/import-record/import-record.component.ts b/libs/feature/editor/src/lib/components/import-record/import-record.component.ts
@@ -142,7 +142,8 @@ export class ImportRecordComponent {
               'editor.record.importFromExternalFile.failure.body'
             )} ${error.message ?? ''}`,
           },
-          2500
+          2500,
+          error
         )
         this.isRecordImportInProgress = false
         this.cdr.markForCheck()

diff --git a/...field/form-field-online-link-resources/form-field-online-link-resources.component.spec.ts b/...field/form-field-online-link-resources/form-field-online-link-resources.component.spec.ts
@@ -149,12 +149,16 @@ describe('FormFieldOnlineLinkResourcesComponent', () => {
       expect(component.uploadProgress).toBeUndefined()
       component.handleFileChange(file)
       uploadSubject.error(new Error('something went wrong'))
-      expect(notificationsService.showNotification).toHaveBeenCalledWith({
-        type: 'error',
-        closeMessage: 'editor.record.onlineResourceError.closeMessage',
-        text: 'editor.record.onlineResourceError.body something went wrong',
-        title: 'editor.record.onlineResourceError.title',
-      })
+      expect(notificationsService.showNotification).toHaveBeenCalledWith(
+        {
+          type: 'error',
+          closeMessage: 'editor.record.onlineResourceError.closeMessage',
+          text: 'editor.record.onlineResourceError.body something went wrong',
+          title: 'editor.record.onlineResourceError.title',
+        },
+        undefined,
+        expect.any(Error)
+      )
     })
   })
   describe('handleUploadCancel', () => {

diff --git a/...form-field/form-field-online-link-resources/form-field-online-link-resources.component.ts b/...form-field/form-field-online-link-resources/form-field-online-link-resources.component.ts
@@ -143,18 +143,22 @@ export class FormFieldOnlineLinkResourcesComponent {
   private handleError(error: Error) {
     this.uploadProgress = undefined
     this.cd.detectChanges()
-    this.notificationsService.showNotification({
-      type: 'error',
-      title: this.translateService.instant(
-        'editor.record.onlineResourceError.title'
-      ),
-      text: `${this.translateService.instant(
-        'editor.record.onlineResourceError.body'
-      )} ${error.message}`,
-      closeMessage: this.translateService.instant(
-        'editor.record.onlineResourceError.closeMessage'
-      ),
-    })
+    this.notificationsService.showNotification(
+      {
+        type: 'error',
+        title: this.translateService.instant(
+          'editor.record.onlineResourceError.title'
+        ),
+        text: `${this.translateService.instant(
+          'editor.record.onlineResourceError.body'
+        )} ${error.message}`,
+        closeMessage: this.translateService.instant(
+          'editor.record.onlineResourceError.closeMessage'
+        ),
+      },
+      undefined,
+      error
+    )
   }
 
   private openEditDialog(resource: OnlineLinkResource, index: number) {