Merge branch 'backport-5739-w2p-141627_search-instance-params-9.0' into w2p-141627_search-instance-params-10.x_contribute

Author: Ali Jaber

src/app/shared/search/search-configuration.service.spec.ts

@@ -310,6 +310,29 @@ describe('SearchConfigurationService', () => {
     });
   });
 
+  describe('isLegacySearchParam', () => {
+    it('should return true for unprefixed scoped search params', () => {
+      ['configuration', 'scope', 'query', 'dsoType', 'view'].forEach((param: string) => {
+        expect(service.isLegacySearchParam(param)).toBeTrue();
+      });
+    });
+
+    it('should return true for unprefixed filter params', () => {
+      expect(service.isLegacySearchParam('f.author')).toBeTrue();
+      expect(service.isLegacySearchParam('f.dateIssued.max')).toBeTrue();
+    });
+
+    it('should return false for params that are already prefixed with a search instance id', () => {
+      expect(service.isLegacySearchParam(`${defaults.pagination.id}.query`)).toBeFalse();
+      expect(service.isLegacySearchParam(`${defaults.pagination.id}.f.author`)).toBeFalse();
+    });
+
+    it('should return false for unrelated params', () => {
+      expect(service.isLegacySearchParam('page')).toBeFalse();
+      expect(service.isLegacySearchParam('spc.page')).toBeFalse();
+    });
+  });
+
   describe('unselectAppliedFilterParams', () => {
     let appliedFilter: AppliedFilter;
 

src/app/shared/search/search-configuration.service.ts

@@ -89,6 +89,13 @@ export class SearchConfigurationService implements OnDestroy {
    */
   public searchInstanceId = 'spc';
 
+  /**
+   * The names of the query parameters that are scoped to a search instance, but that still accept a
+   * legacy (unprefixed) value for backwards compatibility. Filter parameters (prefixed with `f.`) are
+   * handled separately, see {@link isLegacySearchParam}.
+   */
+  protected readonly legacyScopedQueryParams: string[] = ['configuration', 'scope', 'query', 'dsoType', 'view'];
+
   /**
    * Emits the current search options
    */
@@ -348,6 +355,18 @@ export class SearchConfigurationService implements OnDestroy {
     return this.getSearchInstanceParam(searchInstanceId, 'f.');
   }
 
+  /**
+   * Whether the given query parameter is a legacy (unprefixed) search parameter that has a
+   * search-instance-prefixed equivalent (e.g. `query`, `scope` or a `f.xxx` filter). These
+   * parameters are still read for backwards compatibility, but should be migrated to their
+   * prefixed form so they don't get mixed with newly added prefixed parameters.
+   *
+   * @param paramName The query parameter name
+   */
+  isLegacySearchParam(paramName: string): boolean {
+    return this.legacyScopedQueryParams.includes(paramName) || paramName.startsWith('f.');
+  }
+
   getCurrentPageParam(): string {
     return this.paginationService.getPageParam(this.searchInstanceId);
   }

src/app/shared/search/search.component.spec.ts

@@ -12,7 +12,10 @@ import {
 } from '@angular/core/testing';
 import { By } from '@angular/platform-browser';
 import { NoopAnimationsModule } from '@angular/platform-browser/animations';
-import { ActivatedRoute } from '@angular/router';
+import {
+  ActivatedRoute,
+  convertToParamMap,
+} from '@angular/router';
 import { RouterTestingModule } from '@angular/router/testing';
 import { APP_CONFIG } from '@dspace/config/app-config.interface';
 import { SearchManager } from '@dspace/core/browse/search-manager';
@@ -188,6 +191,9 @@ const routeServiceStub = {
   getQueryParamsWithPrefix: () => {
     return of(null);
   },
+  getQueryParamMap: () => {
+    return of(convertToParamMap({}));
+  },
   setParameter: (key: any, value: any) => {
     return;
   },
@@ -207,6 +213,7 @@ export function configureSearchComponentTestingModule(compType, additionalDeclar
     setSearchInstanceId: jasmine.createSpy('setSearchInstanceId'),
   });
   searchConfigurationServiceStub.getCurrentSearchInstanceParam = (param: string) => `${paginationId}.${param}`;
+  searchConfigurationServiceStub.isLegacySearchParam = (param: string) => ['configuration', 'scope', 'query', 'dsoType', 'view'].includes(param) || param.startsWith('f.');
 
   searchConfigurationServiceStub.setSearchInstanceId.and.callFake((pageId) => {
     paginatedSearchOptions$.next(Object.assign(paginatedSearchOptions$.value, {
@@ -348,6 +355,63 @@ describe('SearchComponent', () => {
     expect(comp.resultFound.emit).toHaveBeenCalledWith(expectedResults);
   }));
 
+  describe('migrateLegacySearchParams', () => {
+    let navigateSpy: jasmine.Spy;
+
+    beforeEach(() => {
+      comp.platformId = 'browser';
+      comp.searchInstanceId = paginationId;
+      navigateSpy = spyOn((comp as any).router, 'navigate');
+    });
+
+    it('should replace legacy (unprefixed) search params with their prefixed equivalent', () => {
+      spyOn((comp as any).routeService, 'getQueryParamMap').and.returnValue(of(convertToParamMap({
+        query: 'cats',
+        'f.author': 'Smith',
+      })));
+
+      (comp as any).migrateLegacySearchParams();
+
+      expect(navigateSpy).toHaveBeenCalledWith([], {
+        queryParams: {
+          query: null,
+          [`${paginationId}.query`]: 'cats',
+          'f.author': null,
+          [`${paginationId}.f.author`]: 'Smith',
+        },
+        queryParamsHandling: 'merge',
+        replaceUrl: true,
+      });
+    });
+
+    it('should not navigate when there are no legacy search params', () => {
+      spyOn((comp as any).routeService, 'getQueryParamMap').and.returnValue(of(convertToParamMap({
+        [`${paginationId}.query`]: 'cats',
+      })));
+
+      (comp as any).migrateLegacySearchParams();
+
+      expect(navigateSpy).not.toHaveBeenCalled();
+    });
+
+    it('should drop the legacy param without overriding an existing prefixed param', () => {
+      spyOn((comp as any).routeService, 'getQueryParamMap').and.returnValue(of(convertToParamMap({
+        query: 'legacy',
+        [`${paginationId}.query`]: 'prefixed',
+      })));
+
+      (comp as any).migrateLegacySearchParams();
+
+      expect(navigateSpy).toHaveBeenCalledWith([], {
+        queryParams: {
+          query: null,
+        },
+        queryParamsHandling: 'merge',
+        replaceUrl: true,
+      });
+    });
+  });
+
   describe('when the open sidebar button is clicked in mobile view', () => {
 
     beforeEach(() => {

src/app/shared/search/search.component.ts

@@ -1,5 +1,6 @@
 import {
   AsyncPipe,
+  isPlatformBrowser,
   isPlatformServer,
   NgTemplateOutlet,
 } from '@angular/common';
@@ -16,6 +17,7 @@ import {
 } from '@angular/core';
 import {
   NavigationStart,
+  Params,
   Router,
 } from '@angular/router';
 import {
@@ -67,6 +69,7 @@ import {
   filter,
   map,
   switchMap,
+  take,
 } from 'rxjs/operators';
 
 import { environment } from '../../../environments/environment';
@@ -383,6 +386,10 @@ export class SearchComponent implements OnDestroy, OnInit {
 
     this.searchConfigService.setSearchInstanceId(this.searchInstanceId);
 
+    if (isPlatformBrowser(this.platformId)) {
+      this.migrateLegacySearchParams();
+    }
+
     if (hasValue(this.configuration)) {
       this.routeService.setParameter(this.searchConfigService.getCurrentSearchInstanceParam('configuration'), this.configuration);
     }
@@ -589,6 +596,45 @@ export class SearchComponent implements OnDestroy, OnInit {
     }
   }
 
+  /**
+   * Detect legacy (unprefixed) search parameters in the current URL and replace them with their
+   * search-instance-prefixed equivalents (e.g. `query` becomes `spc.query`, `f.author` becomes
+   * `spc.f.author`). This keeps backwards compatibility with old search URLs while making sure that
+   * subsequent search interactions (entering a new query, adding a filter, ...) operate on the
+   * prefixed parameters only, instead of mixing or dropping the legacy ones.
+   * @private
+   */
+  private migrateLegacySearchParams(): void {
+    this.subs.push(this.routeService.getQueryParamMap().pipe(take(1)).subscribe((queryParamMap) => {
+      const prefix = `${this.searchInstanceId}.`;
+      const updatedParams: Params = {};
+      let hasLegacyParams = false;
+
+      queryParamMap.keys
+        .filter((key: string) => this.searchConfigService.isLegacySearchParam(key))
+        .forEach((key: string) => {
+          hasLegacyParams = true;
+          // Remove the legacy parameter from the URL
+          updatedParams[key] = null;
+          // Only move its value(s) to the prefixed parameter when no prefixed value is set yet,
+          // so an existing prefixed parameter always takes precedence over the legacy one.
+          const prefixedKey = `${prefix}${key}`;
+          if (!queryParamMap.has(prefixedKey)) {
+            const values: string[] = queryParamMap.getAll(key);
+            updatedParams[prefixedKey] = values.length > 1 ? values : values[0];
+          }
+        });
+
+      if (hasLegacyParams) {
+        void this.router.navigate([], {
+          queryParams: updatedParams,
+          queryParamsHandling: 'merge',
+          replaceUrl: true,
+        });
+      }
+    }));
+  }
+
   /**
    * Get the UUID from a DSO url
    * Return null if the url isn't an object page (allowedObjectPaths) or the UUID couldn't be found