From f54ef66317d53d64791838ee61bf7a2e9a3e4ad2 Mon Sep 17 00:00:00 2001 From: chiku Date: Tue, 29 Sep 2026 01:17:23 +0900 Subject: [PATCH] feat(files): add dynamic file provider registry for foreign addon support The Files route was guarded by a build-time list of provider names, so a storage service that GravyValet serves under any other name (a foreign addon imp) could not be opened. Changes: - add `FileProviderRegistryService`, which accepts the built-in names without a request and fetches the external storage service names from GravyValet the first time it is asked for a name it does not know - `metadata` is reserved and always refused as a file provider name - the first time access of file detail page also triggers this request because file GUIDs under `files/` are unknown names too - make `isFileProvider` ask the registry asynchronously and it refuses the name when the request fails - fall back to the provider/display name in the connect toasts and the disconnect dialog when the service has no entry in `AddonServiceNames` - add `FileProviderType` to document that provider names can be dynamic Tests: - add a spec for `FileProviderRegistryService`: built-in and reserved names need no request, one request serves concurrent and later lookups, and the failure and timeout paths (report, loader, retry) - rewrite the `isFileProvider` guard spec against a mocked registry, including the refusal when the registry rejects - add a spec for `AddonDialogService`, covering the name in the disconnect dialog header - revive the skipped specs of `ConnectConfiguredAddonComponent` and `ConnectAddonComponent`: the router mock supplies the navigation state that both components read in their constructor. They now cover the create/update flows and the name in the success toast --- .../guards/is-file-provider.guard.spec.ts | 86 ++++-- src/app/core/guards/is-file-provider.guard.ts | 21 +- .../file-provider-registry.service.spec.ts | 266 ++++++++++++++++++ .../file-provider-registry.service.ts | 100 +++++++ src/app/features/files/store/files.model.ts | 16 +- ...connect-configured-addon.component.spec.ts | 111 ++++++-- .../connect-configured-addon.component.ts | 3 +- .../services/addon-dialog.service.spec.ts | 90 ++++++ .../services/addon-dialog.service.ts | 2 +- .../connect-addon.component.spec.ts | 158 ++++++++++- .../connect-addon/connect-addon.component.ts | 5 +- 11 files changed, 785 insertions(+), 73 deletions(-) create mode 100644 src/app/core/services/file-provider-registry.service.spec.ts create mode 100644 src/app/core/services/file-provider-registry.service.ts create mode 100644 src/app/features/project/project-addons/services/addon-dialog.service.spec.ts diff --git a/src/app/core/guards/is-file-provider.guard.spec.ts b/src/app/core/guards/is-file-provider.guard.spec.ts index 49e107f84..238e79031 100644 --- a/src/app/core/guards/is-file-provider.guard.spec.ts +++ b/src/app/core/guards/is-file-provider.guard.spec.ts @@ -1,49 +1,79 @@ -import { ParamMap, UrlSegment } from '@angular/router'; +import { MockProvider } from 'ng-mocks'; +import { Mock } from 'vitest'; + +import { HttpErrorResponse } from '@angular/common/http'; +import { TestBed } from '@angular/core/testing'; +import { Route, UrlSegment } from '@angular/router'; + +import { FileProviderRegistryService } from '@core/services/file-provider-registry.service'; import { FileProvider } from '@osf/features/files/constants'; import { isFileProvider } from './is-file-provider.guard'; describe('isFileProvider', () => { - const createMockParamMap = (): ParamMap => ({ - get: () => null, - getAll: () => [], - has: () => false, - keys: [], + let registry: { isValidProvider: Mock }; + + const FOREIGN_PROVIDER = 's3compat'; + const route: Route = {}; + + const createSegments = (...paths: string[]): UrlSegment[] => paths.map((path) => new UrlSegment(path, {})); + + const runGuard = (segments: UrlSegment[]) => TestBed.runInInjectionContext(() => isFileProvider(route, segments)); + + beforeEach(() => { + const validProviders: string[] = [...Object.values(FileProvider), FOREIGN_PROVIDER]; + + registry = { + isValidProvider: vi.fn(async (providerName: string) => validProviders.includes(providerName)), + }; + + TestBed.configureTestingModule({ + providers: [MockProvider(FileProviderRegistryService, registry)], + }); }); - const createMockSegment = (path: string): UrlSegment => ({ - path, - parameters: {}, - parameterMap: createMockParamMap(), + it('should return true when id matches a built-in FileProvider value', async () => { + for (const provider of Object.values(FileProvider)) { + await expect(runGuard(createSegments(provider))).resolves.toBe(true); + expect(registry.isValidProvider).toHaveBeenCalledWith(provider); + } }); - const createMockSegments = (path: string) => [createMockSegment(path)]; + it('should return true when id matches an external provider registered in gravyvalet', async () => { + await expect(runGuard(createSegments(FOREIGN_PROVIDER))).resolves.toBe(true); + expect(registry.isValidProvider).toHaveBeenCalledWith(FOREIGN_PROVIDER); + }); - it('should return true when id matches a FileProvider value', () => { - Object.values(FileProvider).forEach((provider) => { - const result = isFileProvider({} as any, createMockSegments(provider)); - expect(result).toBe(true); - }); + it('should return false when id does not match any registered provider', async () => { + await expect(runGuard(createSegments('invalid-provider'))).resolves.toBe(false); + expect(registry.isValidProvider).toHaveBeenCalledWith('invalid-provider'); + }); + + it('should return false when the registry cannot ask gravyvalet', async () => { + registry.isValidProvider.mockRejectedValue(new HttpErrorResponse({ status: 503 })); + + await expect(runGuard(createSegments(FOREIGN_PROVIDER))).resolves.toBe(false); }); - it('should return false when id does not match any FileProvider value', () => { - const result = isFileProvider({} as any, createMockSegments('invalid-provider')); - expect(result).toBe(false); + it('should only check the first segment', async () => { + await expect(runGuard(createSegments(FileProvider.GoogleDrive, 'subfolder', 'file.txt'))).resolves.toBe(true); + expect(registry.isValidProvider).toHaveBeenCalledTimes(1); + expect(registry.isValidProvider).toHaveBeenCalledWith(FileProvider.GoogleDrive); }); - it('should return false when segments array is empty', () => { - const result = isFileProvider({} as any, []); - expect(result).toBe(false); + it('should return false when segments array is empty', async () => { + await expect(runGuard([])).resolves.toBe(false); + expect(registry.isValidProvider).not.toHaveBeenCalled(); }); - it('should return false when first segment has no path', () => { - const result = isFileProvider({} as any, [createMockSegment('')]); - expect(result).toBe(false); + it('should return false when first segment has no path', async () => { + await expect(runGuard(createSegments(''))).resolves.toBe(false); + expect(registry.isValidProvider).not.toHaveBeenCalled(); }); - it('should return false when first segment is undefined', () => { - const result = isFileProvider({} as any, [undefined as any]); - expect(result).toBe(false); + it('should return false when first segment is undefined', async () => { + await expect(runGuard([undefined as unknown as UrlSegment])).resolves.toBe(false); + expect(registry.isValidProvider).not.toHaveBeenCalled(); }); }); diff --git a/src/app/core/guards/is-file-provider.guard.ts b/src/app/core/guards/is-file-provider.guard.ts index 0dd88d330..c65f4b8e4 100644 --- a/src/app/core/guards/is-file-provider.guard.ts +++ b/src/app/core/guards/is-file-provider.guard.ts @@ -1,9 +1,24 @@ +import { inject } from '@angular/core'; import { CanMatchFn, Route, UrlSegment } from '@angular/router'; -import { FileProvider } from '@osf/features/files/constants'; +import { FileProviderRegistryService } from '@core/services/file-provider-registry.service'; -export const isFileProvider: CanMatchFn = (route: Route, segments: UrlSegment[]) => { +/** + * Route guard that checks if a file provider is valid. + * Supports both built-in providers (osfstorage, googledrive, etc.) and + * dynamically discovered external storage services (foreign addons like s3compat). + */ +export const isFileProvider: CanMatchFn = async (route: Route, segments: UrlSegment[]) => { const id = segments[0]?.path; + if (!id) { + return false; + } - return !!(id && Object.values(FileProvider).some((provider) => provider === id)); + const registry = inject(FileProviderRegistryService); + + try { + return await registry.isValidProvider(id); + } catch { + return false; + } }; diff --git a/src/app/core/services/file-provider-registry.service.spec.ts b/src/app/core/services/file-provider-registry.service.spec.ts new file mode 100644 index 000000000..1d7795d13 --- /dev/null +++ b/src/app/core/services/file-provider-registry.service.spec.ts @@ -0,0 +1,266 @@ +import { MockProvider } from 'ng-mocks'; + +import { TimeoutError } from 'rxjs'; + +import { HttpTestingController } from '@angular/common/http/testing'; +import { TestBed } from '@angular/core/testing'; + +import { BYPASS_ERROR_INTERCEPTOR } from '@core/interceptors/error-interceptor.tokens'; +import { FileProvider } from '@osf/features/files/constants'; +import { AddonGetListResponseJsonApi } from '@osf/shared/models/addons/external-addon-json-api.model'; +import { ToastService } from '@osf/shared/services/toast.service'; + +import { getAddonsExternalStorageData } from '@testing/data/addons/addons.external-storage.data'; +import { provideOSFCore, provideOSFHttp } from '@testing/osf.testing.provider'; +import { LoaderServiceMock, provideLoaderServiceMock } from '@testing/providers/loader-service.mock'; +import { ToastServiceMock, ToastServiceMockType } from '@testing/providers/toast-provider.mock'; + +import { FILE_PROVIDER_REGISTRY_TIMEOUT_MS, FileProviderRegistryService } from './file-provider-registry.service'; + +describe('FileProviderRegistryService', () => { + let service: FileProviderRegistryService; + let httpMock: HttpTestingController; + let loaderService: LoaderServiceMock; + let toastService: ToastServiceMockType; + + const FOREIGN_PROVIDER = 's3compat'; + const isRegistryRequest = (request: { url: string; method: string }) => + request.url === 'http://addons.localhost:8000/external-storage-services' && request.method === 'GET'; + const expectRequest = () => httpMock.expectOne(isRegistryRequest); + + const getListingWithForeignProvider = (externalServiceName: string = FOREIGN_PROVIDER) => { + const listing = getAddonsExternalStorageData() as unknown as AddonGetListResponseJsonApi; + listing.data[0].attributes.external_service_name = externalServiceName; + return listing; + }; + + beforeEach(() => { + loaderService = new LoaderServiceMock(); + toastService = ToastServiceMock.simple(); + + TestBed.configureTestingModule({ + providers: [ + provideOSFCore(), + provideOSFHttp(), + provideLoaderServiceMock(loaderService), + MockProvider(ToastService, toastService), + ], + }); + + service = TestBed.inject(FileProviderRegistryService); + httpMock = TestBed.inject(HttpTestingController); + }); + + afterEach(() => { + httpMock.verify(); + vi.useRealTimers(); + }); + + it('should not request anything when it is created', () => { + httpMock.expectNone(isRegistryRequest); + }); + + it('should accept every built-in provider without asking gravyvalet', async () => { + for (const provider of Object.values(FileProvider)) { + await expect(service.isValidProvider(provider)).resolves.toBe(true); + } + + httpMock.expectNone(isRegistryRequest); + }); + + it('should refuse a built-in name written in another case', async () => { + const result = service.isValidProvider('OsfStorage'); + expectRequest().flush(getListingWithForeignProvider()); + + await expect(result).resolves.toBe(false); + await expect(service.isValidProvider('DropBox')).resolves.toBe(false); + }); + + it('should refuse a reserved name without asking gravyvalet', async () => { + await expect(service.isValidProvider('metadata')).resolves.toBe(false); + + httpMock.expectNone(isRegistryRequest); + }); + + it('should refuse a reserved name even when gravyvalet lists a service of that name', async () => { + const result = service.isValidProvider(FOREIGN_PROVIDER); + expectRequest().flush(getListingWithForeignProvider('metadata')); + await result; + + await expect(service.isValidProvider('metadata')).resolves.toBe(false); + }); + + it('should ask gravyvalet for the external storage services when a name is not built-in', async () => { + const result = service.isValidProvider(FOREIGN_PROVIDER); + + const request = expectRequest(); + expect(request.request.params.get('fields[external-storage-services]')).toBe('external_service_name'); + request.flush(getListingWithForeignProvider()); + + await expect(result).resolves.toBe(true); + }); + + it('should leave the request to the global error interceptor', () => { + service.isValidProvider(FOREIGN_PROVIDER); + + const request = expectRequest(); + expect(request.request.context.get(BYPASS_ERROR_INTERCEPTOR)).toBe(false); + request.flush(getListingWithForeignProvider()); + }); + + it('should show the full-screen loader while it waits for gravyvalet', async () => { + const result = service.isValidProvider(FOREIGN_PROVIDER); + + const request = expectRequest(); + expect(loaderService.show).toHaveBeenCalledTimes(1); + expect(loaderService.hide).not.toHaveBeenCalled(); + + request.flush(getListingWithForeignProvider()); + await result; + + expect(loaderService.hide).toHaveBeenCalledTimes(1); + }); + + it('should hide the full-screen loader when the request fails', async () => { + const result = service.isValidProvider(FOREIGN_PROVIDER); + expectRequest().flush('Service Unavailable', { status: 503, statusText: 'Service Unavailable' }); + await expect(result).rejects.toBeDefined(); + + expect(loaderService.show).toHaveBeenCalledTimes(1); + expect(loaderService.hide).toHaveBeenCalledTimes(1); + }); + + it('should not show the full-screen loader for a name that needs no request', async () => { + await service.isValidProvider(FileProvider.OsfStorage); + await service.isValidProvider('metadata'); + + const first = service.isValidProvider(FOREIGN_PROVIDER); + expectRequest().flush(getListingWithForeignProvider()); + await first; + await service.isValidProvider(FOREIGN_PROVIDER); + + expect(loaderService.show).toHaveBeenCalledTimes(1); + }); + + it('should refuse an external provider name written in another case', async () => { + const result = service.isValidProvider('S3Compat'); + expectRequest().flush(getListingWithForeignProvider()); + + await expect(result).resolves.toBe(false); + await expect(service.isValidProvider(FOREIGN_PROVIDER)).resolves.toBe(true); + }); + + it('should reject a name that gravyvalet does not list', async () => { + const result = service.isValidProvider('unknownprovider'); + expectRequest().flush(getListingWithForeignProvider()); + + await expect(result).resolves.toBe(false); + }); + + it('should skip external services without a service name', async () => { + const result = service.isValidProvider(''); + expectRequest().flush(getListingWithForeignProvider('')); + + await expect(result).resolves.toBe(false); + }); + + it('should keep the external storage services after the first request', async () => { + const first = service.isValidProvider(FOREIGN_PROVIDER); + expectRequest().flush(getListingWithForeignProvider()); + await first; + + await expect(service.isValidProvider(FOREIGN_PROVIDER)).resolves.toBe(true); + await expect(service.isValidProvider('unknownprovider')).resolves.toBe(false); + + httpMock.expectNone(isRegistryRequest); + }); + + it('should send one request for concurrent lookups', async () => { + const results = Promise.all([ + service.isValidProvider(FOREIGN_PROVIDER), + service.isValidProvider('unknownprovider'), + service.isValidProvider(FOREIGN_PROVIDER), + ]); + expectRequest().flush(getListingWithForeignProvider()); + + await expect(results).resolves.toEqual([true, false, true]); + }); + + it('should reject when the request fails', async () => { + const result = service.isValidProvider(FOREIGN_PROVIDER); + expectRequest().flush('Service Unavailable', { status: 503, statusText: 'Service Unavailable' }); + + await expect(result).rejects.toMatchObject({ status: 503 }); + }); + + it('should still accept built-in providers after a failed request', async () => { + const result = service.isValidProvider(FOREIGN_PROVIDER); + expectRequest().flush('Service Unavailable', { status: 503, statusText: 'Service Unavailable' }); + await expect(result).rejects.toBeDefined(); + + await expect(service.isValidProvider(FileProvider.OsfStorage)).resolves.toBe(true); + + httpMock.expectNone(isRegistryRequest); + }); + + it('should ask gravyvalet again after a failed request', async () => { + const failed = service.isValidProvider(FOREIGN_PROVIDER); + expectRequest().flush('Service Unavailable', { status: 503, statusText: 'Service Unavailable' }); + await expect(failed).rejects.toBeDefined(); + + const retried = service.isValidProvider(FOREIGN_PROVIDER); + expectRequest().flush(getListingWithForeignProvider()); + + await expect(retried).resolves.toBe(true); + }); + + it('should not report a failed request itself, as the error interceptor does it', async () => { + const result = service.isValidProvider(FOREIGN_PROVIDER); + expectRequest().flush('Service Unavailable', { status: 503, statusText: 'Service Unavailable' }); + await expect(result).rejects.toBeDefined(); + + expect(toastService.showError).not.toHaveBeenCalled(); + }); + + it('should give the request up and tell the user when gravyvalet does not answer in time', async () => { + vi.useFakeTimers(); + + const result = service.isValidProvider(FOREIGN_PROVIDER); + const rejection = expect(result).rejects.toBeInstanceOf(TimeoutError); + const request = expectRequest(); + await vi.advanceTimersByTimeAsync(FILE_PROVIDER_REGISTRY_TIMEOUT_MS); + await rejection; + + expect(request.cancelled).toBe(true); + expect(toastService.showError).toHaveBeenCalledTimes(1); + expect(toastService.showError).toHaveBeenCalledWith('common.errorMessages.serverError'); + expect(loaderService.hide).toHaveBeenCalledTimes(1); + }); + + it('should accept an answer that arrives just before the time is up', async () => { + vi.useFakeTimers(); + + const result = service.isValidProvider(FOREIGN_PROVIDER); + const request = expectRequest(); + await vi.advanceTimersByTimeAsync(FILE_PROVIDER_REGISTRY_TIMEOUT_MS - 1); + request.flush(getListingWithForeignProvider()); + + await expect(result).resolves.toBe(true); + expect(toastService.showError).not.toHaveBeenCalled(); + }); + + it('should ask gravyvalet again after it did not answer in time', async () => { + vi.useFakeTimers(); + + const timedOut = service.isValidProvider(FOREIGN_PROVIDER); + const rejection = expect(timedOut).rejects.toBeInstanceOf(TimeoutError); + expectRequest(); + await vi.advanceTimersByTimeAsync(FILE_PROVIDER_REGISTRY_TIMEOUT_MS); + await rejection; + + const retried = service.isValidProvider(FOREIGN_PROVIDER); + expectRequest().flush(getListingWithForeignProvider()); + + await expect(retried).resolves.toBe(true); + }); +}); diff --git a/src/app/core/services/file-provider-registry.service.ts b/src/app/core/services/file-provider-registry.service.ts new file mode 100644 index 000000000..54cdb148c --- /dev/null +++ b/src/app/core/services/file-provider-registry.service.ts @@ -0,0 +1,100 @@ +import { finalize, firstValueFrom, tap, timeout, TimeoutError } from 'rxjs'; + +import { inject, Injectable } from '@angular/core'; + +import { ENVIRONMENT } from '@core/provider/environment.provider'; +import { FileProvider } from '@osf/features/files/constants'; +import { AddonGetListResponseJsonApi } from '@osf/shared/models/addons/external-addon-json-api.model'; +import { JsonApiService } from '@osf/shared/services/json-api.service'; +import { LoaderService } from '@osf/shared/services/loader.service'; +import { ToastService } from '@osf/shared/services/toast.service'; + +export const FILE_PROVIDER_REGISTRY_TIMEOUT_MS = 60000; + +/** + * Registry service that maintains a set of valid file provider names. + * Combines built-in providers with dynamically discovered external storage services. + */ +@Injectable({ + providedIn: 'root', +}) +export class FileProviderRegistryService { + private readonly jsonApiService = inject(JsonApiService); + private readonly environment = inject(ENVIRONMENT); + private readonly loaderService = inject(LoaderService); + private readonly toastService = inject(ToastService); + + private readonly builtInProviders = new Set(Object.values(FileProvider)); + private readonly reservedNames = new Set(['metadata']); + private externalProviders: Promise> | null = null; + + /** + * Check if a provider name is valid (built-in or external). + * + * Names are matched exactly: gravyvalet serves its names in lower case, and the Files URLs carry + * those names as they are. A reserved name is refused and a built-in name is accepted, both + * without any request. For any other name the external storage services are fetched from + * gravyvalet first. If that request fails, or is not answered within + * `FILE_PROVIDER_REGISTRY_TIMEOUT_MS`, the promise rejects. + */ + async isValidProvider(providerName: string): Promise { + if (this.reservedNames.has(providerName)) { + return false; + } + + if (this.builtInProviders.has(providerName)) { + return true; + } + + const externalProviders = await this.getExternalProviders(); + + return externalProviders.has(providerName); + } + + private getExternalProviders(): Promise> { + if (!this.externalProviders) { + this.externalProviders = this.fetchExternalProviders().catch((error) => { + // Keep only a successful result, so that the next unknown name asks gravyvalet again + this.externalProviders = null; + throw error; + }); + } + + return this.externalProviders; + } + + private async fetchExternalProviders(): Promise> { + const params = { 'fields[external-storage-services]': 'external_service_name' }; + + this.loaderService.show(); + + const response = await firstValueFrom( + this.jsonApiService + .get(`${this.environment.addonsApiUrl}/external-storage-services`, params) + .pipe( + timeout(FILE_PROVIDER_REGISTRY_TIMEOUT_MS), + tap({ + error: (error) => { + // A timeout does not pass the error interceptor, so the registry reports it itself + if (error instanceof TimeoutError) { + this.toastService.showError('common.errorMessages.serverError'); + } + }, + }), + finalize(() => this.loaderService.hide()) + ) + ); + + const providers = new Set(); + + for (const service of response.data) { + const serviceName = service.attributes.external_service_name; + + if (serviceName) { + providers.add(serviceName); + } + } + + return providers; + } +} diff --git a/src/app/features/files/store/files.model.ts b/src/app/features/files/store/files.model.ts index 9c5dcb4b1..9ba38d3b9 100644 --- a/src/app/features/files/store/files.model.ts +++ b/src/app/features/files/store/files.model.ts @@ -11,6 +11,20 @@ import { FileProvider } from '../constants'; import { OsfFileCustomMetadata } from '../models/file-custom-metadata.model'; import { OsfFileRevision } from '../models/file-revisions.model'; +/** + * Type alias for built-in file providers. + * `FileProvider` is declared without `as const`, so this resolves to `string` today. + */ +type BuiltInFileProvider = (typeof FileProvider)[keyof typeof FileProvider]; + +/** + * A file provider name: a built-in one, or a dynamic one that gravyvalet lists + * (e.g., s3compat from a foreign addon). + * Resolves to `string` today. `(string & {})` keeps dynamic names accepted, and + * lets the IDE suggest the built-in names, if `FileProvider` is ever declared `as const`. + */ +export type FileProviderType = BuiltInFileProvider | (string & {}); + export interface FilesStateModel { files: AsyncStateWithTotalCount; moveDialogFiles: AsyncStateWithTotalCount; @@ -18,7 +32,7 @@ export interface FilesStateModel { moveDialogCurrentFolder: FileFolderModel | null; search: string; sort: string; - provider: (typeof FileProvider)[keyof typeof FileProvider]; + provider: FileProviderType; openedFile: AsyncStateModel; fileMetadata: AsyncStateModel; resourceMetadata: AsyncStateModel; diff --git a/src/app/features/project/project-addons/components/connect-configured-addon/connect-configured-addon.component.spec.ts b/src/app/features/project/project-addons/components/connect-configured-addon/connect-configured-addon.component.spec.ts index 0cef8ccfe..9212c4062 100644 --- a/src/app/features/project/project-addons/components/connect-configured-addon/connect-configured-addon.component.spec.ts +++ b/src/app/features/project/project-addons/components/connect-configured-addon/connect-configured-addon.component.spec.ts @@ -1,51 +1,112 @@ +import { Store } from '@ngxs/store'; + import { MockComponents, MockProvider } from 'ng-mocks'; -import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { DialogService } from 'primeng/dynamicdialog'; + +import { TestBed } from '@angular/core/testing'; import { ActivatedRoute, Router } from '@angular/router'; import { AddonSetupAccountFormComponent } from '@osf/shared/components/addons/addon-setup-account-form/addon-setup-account-form.component'; +import { AddonTermsComponent } from '@osf/shared/components/addons/addon-terms/addon-terms.component'; import { StorageItemSelectorComponent } from '@osf/shared/components/addons/storage-item-selector/storage-item-selector.component'; import { SubHeaderComponent } from '@osf/shared/components/sub-header/sub-header.component'; -import { CredentialsFormat } from '@osf/shared/enums/addons-credentials-format.enum'; +import { AddonCategory } from '@osf/shared/enums/addons-category.enum'; import { AddonModel } from '@osf/shared/models/addons/addon.model'; +import { ToastService } from '@osf/shared/services/toast.service'; +import { AddonsSelectors, CreateConfiguredAddon } from '@osf/shared/stores/addons'; +import { MOCK_ADDON } from '@testing/mocks/addon.mock'; +import { MOCK_CONFIGURED_ADDON } from '@testing/mocks/configured-addon.mock'; import { provideOSFCore } from '@testing/osf.testing.provider'; +import { ActivatedRouteMockBuilder } from '@testing/providers/route-provider.mock'; +import { RouterMockBuilder } from '@testing/providers/router-provider.mock'; import { provideMockStore } from '@testing/providers/store-provider.mock'; +import { ToastServiceMock } from '@testing/providers/toast-provider.mock'; import { ConnectConfiguredAddonComponent } from './connect-configured-addon.component'; -describe.skip('ConnectAddonComponent', () => { - let component: ConnectConfiguredAddonComponent; - let fixture: ComponentFixture; - - const mockAddon: AddonModel = { - id: 'test-addon-id', - type: 'external-storage-services', - displayName: 'Test Addon', - credentialsFormat: CredentialsFormat.OAUTH2, - supportedFeatures: ['ACCESS'], - providerName: 'Test Provider', - authUrl: 'https://test.com/auth', - externalServiceName: 'test-service', - }; - - beforeEach(() => { +describe('ConnectConfiguredAddonComponent', () => { + const storageAddon: AddonModel = { ...MOCK_ADDON, type: AddonCategory.EXTERNAL_STORAGE_SERVICES }; + + function setup(addon: AddonModel = storageAddon) { + const toastService = ToastServiceMock.simple(); + const mockRouter = { + ...RouterMockBuilder.create().withUrl('/abc12/addons/connect-addon').build(), + getCurrentNavigation: vi.fn().mockReturnValue({ extras: { state: { addon } } }), + }; + TestBed.configureTestingModule({ imports: [ ConnectConfiguredAddonComponent, - ...MockComponents(SubHeaderComponent, AddonSetupAccountFormComponent, StorageItemSelectorComponent), + ...MockComponents( + SubHeaderComponent, + AddonTermsComponent, + AddonSetupAccountFormComponent, + StorageItemSelectorComponent + ), + ], + providers: [ + provideOSFCore(), + provideMockStore({ + signals: [ + { selector: AddonsSelectors.getAddonsUserReference, value: [{ id: 'user-reference-id', userUri: '' }] }, + { selector: AddonsSelectors.getCreatedOrUpdatedConfiguredAddon, value: MOCK_CONFIGURED_ADDON }, + ], + }), + MockProvider(Router, mockRouter), + MockProvider(ActivatedRoute, ActivatedRouteMockBuilder.create().withParams({ id: 'abc12' }).build()), + MockProvider(ToastService, toastService), + MockProvider(DialogService), ], - providers: [provideOSFCore(), provideMockStore(), MockProvider(Router), MockProvider(ActivatedRoute)], }); - fixture = TestBed.createComponent(ConnectConfiguredAddonComponent); - component = fixture.componentInstance; + const store = TestBed.inject(Store); + const fixture = TestBed.createComponent(ConnectConfiguredAddonComponent); fixture.detectChanges(); - }); + + return { component: fixture.componentInstance, store, toastService, mockRouter }; + } it('should create and initialize with addon data from router state', () => { + const { component } = setup(); + expect(component).toBeTruthy(); - expect(component['addon']()).toEqual(mockAddon); - expect(component['terms']().length).toBeGreaterThan(0); + expect(component.addon()).toEqual(storageAddon); + }); + + it('should create the configured addon and return to the addons list', () => { + const { component, store, mockRouter } = setup(); + + component.handleCreateConfiguredAddon(); + + expect(store.dispatch).toHaveBeenCalledWith(expect.any(CreateConfiguredAddon)); + expect(mockRouter.navigate).toHaveBeenCalledWith(['/abc12/addons'], { + queryParams: { activeTab: 1, addonType: 'storage' }, + }); + }); + + it('should name a built-in service in the success toast', () => { + const { component, toastService } = setup({ ...storageAddon, externalServiceName: 'dropbox' }); + + component.handleCreateConfiguredAddon(); + + expect(toastService.showSuccess).toHaveBeenCalledWith('settings.addons.toast.createSuccess', { + addonName: 'Dropbox', + }); + }); + + it('should fall back to the provider name in the success toast when the service has no built-in name', () => { + const { component, toastService } = setup({ + ...storageAddon, + externalServiceName: 's3compat', + providerName: 'S3 Compatible Storage', + }); + + component.handleCreateConfiguredAddon(); + + expect(toastService.showSuccess).toHaveBeenCalledWith('settings.addons.toast.createSuccess', { + addonName: 'S3 Compatible Storage', + }); }); }); diff --git a/src/app/features/project/project-addons/components/connect-configured-addon/connect-configured-addon.component.ts b/src/app/features/project/project-addons/components/connect-configured-addon/connect-configured-addon.component.ts index b7907d439..99b72bd01 100644 --- a/src/app/features/project/project-addons/components/connect-configured-addon/connect-configured-addon.component.ts +++ b/src/app/features/project/project-addons/components/connect-configured-addon/connect-configured-addon.component.ts @@ -216,7 +216,8 @@ export class ConnectConfiguredAddonComponent { }, }); this.toastService.showSuccess('settings.addons.toast.createSuccess', { - addonName: AddonServiceNames[addon.externalServiceName as keyof typeof AddonServiceNames], + addonName: + AddonServiceNames[addon.externalServiceName as keyof typeof AddonServiceNames] || addon.providerName, }); } }, diff --git a/src/app/features/project/project-addons/services/addon-dialog.service.spec.ts b/src/app/features/project/project-addons/services/addon-dialog.service.spec.ts new file mode 100644 index 000000000..1b29fba46 --- /dev/null +++ b/src/app/features/project/project-addons/services/addon-dialog.service.spec.ts @@ -0,0 +1,90 @@ +import { TranslateService } from '@ngx-translate/core'; +import { MockProvider } from 'ng-mocks'; + +import { DialogService } from 'primeng/dynamicdialog'; + +import { Subject } from 'rxjs'; + +import { Mock } from 'vitest'; + +import { TestBed } from '@angular/core/testing'; + +import { ConfiguredAddonModel } from '@shared/models/addons/configured-addon.model'; + +import { MOCK_CONFIGURED_ADDON } from '@testing/mocks/configured-addon.mock'; +import { provideOSFCore } from '@testing/osf.testing.provider'; + +import { DisconnectAddonModalComponent } from '../components/disconnect-addon-modal/disconnect-addon-modal.component'; + +import { AddonDialogService } from './addon-dialog.service'; + +describe('AddonDialogService', () => { + let service: AddonDialogService; + let translateService: TranslateService; + let dialogService: { open: Mock }; + let dialogClose$: Subject<{ success: boolean }>; + + const builtInAddon: ConfiguredAddonModel = { + ...MOCK_CONFIGURED_ADDON, + externalServiceName: 'dropbox', + displayName: 'My Dropbox folder', + }; + + const foreignAddon: ConfiguredAddonModel = { + ...MOCK_CONFIGURED_ADDON, + externalServiceName: 's3compat', + displayName: 'S3 Compatible Storage', + }; + + beforeEach(() => { + dialogClose$ = new Subject<{ success: boolean }>(); + dialogService = { open: vi.fn().mockReturnValue({ onClose: dialogClose$ }) }; + + TestBed.configureTestingModule({ + providers: [provideOSFCore(), MockProvider(DialogService, dialogService)], + }); + + service = TestBed.inject(AddonDialogService); + translateService = TestBed.inject(TranslateService); + }); + + it('should open the disconnect dialog for the given addon', () => { + service.openDisconnectDialog(builtInAddon); + + expect(dialogService.open).toHaveBeenCalledTimes(1); + const [component, config] = dialogService.open.mock.calls[0]; + expect(component).toBe(DisconnectAddonModalComponent); + expect(config.data.addon).toBe(builtInAddon); + }); + + it('should name a built-in service in the disconnect dialog header', () => { + service.openDisconnectDialog(builtInAddon); + + expect(translateService.instant).toHaveBeenCalledWith('settings.addons.configureAddon.disconnect', { + addonName: 'Dropbox', + }); + }); + + it('should fall back to the addon display name when the service has no built-in name', () => { + service.openDisconnectDialog(foreignAddon); + + expect(translateService.instant).toHaveBeenCalledWith('settings.addons.configureAddon.disconnect', { + addonName: 'S3 Compatible Storage', + }); + }); + + it('should return the result of the disconnect dialog', () => { + const results: { success: boolean }[] = []; + + service.openDisconnectDialog(foreignAddon).subscribe((result) => results.push(result)); + dialogClose$.next({ success: true }); + + expect(results).toEqual([{ success: true }]); + }); + + it('should throw when the disconnect dialog cannot be opened', () => { + dialogService.open.mockReturnValue(null); + + expect(() => service.openDisconnectDialog(foreignAddon)).toThrow('common.errorMessages.dialogOpenError'); + }); +}); diff --git a/src/app/features/project/project-addons/services/addon-dialog.service.ts b/src/app/features/project/project-addons/services/addon-dialog.service.ts index 15b551576..cad0428bc 100644 --- a/src/app/features/project/project-addons/services/addon-dialog.service.ts +++ b/src/app/features/project/project-addons/services/addon-dialog.service.ts @@ -24,7 +24,7 @@ export class AddonDialogService { const dialogRef = this.dialogService.open(DisconnectAddonModalComponent, { focusOnShow: false, header: this.translateService.instant('settings.addons.configureAddon.disconnect', { - addonName: AddonServiceNames[addon.externalServiceName as keyof typeof AddonServiceNames], + addonName: AddonServiceNames[addon.externalServiceName as keyof typeof AddonServiceNames] || addon.displayName, }), closeOnEscape: true, modal: true, diff --git a/src/app/features/settings/settings-addons/components/connect-addon/connect-addon.component.spec.ts b/src/app/features/settings/settings-addons/components/connect-addon/connect-addon.component.spec.ts index 1729311f2..dbdfc7203 100644 --- a/src/app/features/settings/settings-addons/components/connect-addon/connect-addon.component.spec.ts +++ b/src/app/features/settings/settings-addons/components/connect-addon/connect-addon.component.spec.ts @@ -1,39 +1,171 @@ -import { MockComponents } from 'ng-mocks'; +import { Store } from '@ngxs/store'; -import { ComponentFixture, TestBed } from '@angular/core/testing'; -import { provideRouter } from '@angular/router'; +import { MockComponents, MockProvider } from 'ng-mocks'; + +import { TestBed } from '@angular/core/testing'; +import { Router } from '@angular/router'; import { AddonSetupAccountFormComponent } from '@osf/shared/components/addons/addon-setup-account-form/addon-setup-account-form.component'; import { AddonTermsComponent } from '@osf/shared/components/addons/addon-terms/addon-terms.component'; import { SubHeaderComponent } from '@osf/shared/components/sub-header/sub-header.component'; +import { AuthorizedAccountType } from '@osf/shared/enums/addon-type.enum'; +import { AddonCategory } from '@osf/shared/enums/addons-category.enum'; +import { AuthorizedAddonRequestJsonApi } from '@osf/shared/models/addons/authorized-addon-json-api.model'; +import { AddonOAuthService } from '@osf/shared/services/addons/addon-oauth.service'; +import { ToastService } from '@osf/shared/services/toast.service'; +import { AddonModel } from '@shared/models/addons/addon.model'; +import { AuthorizedAccountModel } from '@shared/models/addons/authorized-account.model'; +import { AddonsSelectors, CreateAuthorizedAddon, UpdateAuthorizedAddon } from '@shared/stores/addons'; import { MOCK_ADDON } from '@testing/mocks/addon.mock'; import { provideOSFCore } from '@testing/osf.testing.provider'; +import { RouterMockBuilder } from '@testing/providers/router-provider.mock'; import { provideMockStore } from '@testing/providers/store-provider.mock'; +import { ToastServiceMock } from '@testing/providers/toast-provider.mock'; import { ConnectAddonComponent } from './connect-addon.component'; -describe.skip('ConnectAddonComponent', () => { - let component: ConnectAddonComponent; - let fixture: ComponentFixture; +interface SetupOverrides { + addon?: AddonModel | AuthorizedAccountModel; + createdAccount?: AuthorizedAccountModel; +} + +describe('ConnectAddonComponent', () => { + const storageAddon: AddonModel = { ...MOCK_ADDON, type: AddonCategory.EXTERNAL_STORAGE_SERVICES }; + + const authorizedAccount: AuthorizedAccountModel = { + ...MOCK_ADDON, + type: AuthorizedAccountType.STORAGE, + id: 'account-id', + displayName: 'My account', + authUrl: null, + authorizedCapabilities: ['ACCESS', 'UPDATE'], + authorizedOperationNames: ['list_root_items'], + credentialsAvailable: true, + apiBaseUrl: '', + defaultRootFolder: '', + oauthToken: '', + accountOwnerId: 'user-reference-id', + externalStorageServiceId: 'service-id', + }; + + const payload: AuthorizedAddonRequestJsonApi = { + data: { + type: AuthorizedAccountType.STORAGE, + attributes: { + api_base_url: '', + auth_url: null, + authorized_capabilities: ['ACCESS', 'UPDATE'], + credentials: {}, + credentials_available: false, + display_name: 'My account', + initiate_oauth: false, + }, + relationships: { + account_owner: { data: { type: 'user-references', id: 'user-reference-id' } }, + external_storage_service: { data: { type: 'external-storage-services', id: 'service-id' } }, + }, + }, + }; + + function setup(overrides: SetupOverrides = {}) { + const addon = overrides.addon ?? storageAddon; + const toastService = ToastServiceMock.simple(); + const mockRouter = { + ...RouterMockBuilder.create().withUrl('/settings/addons/connect-addon').build(), + getCurrentNavigation: vi.fn().mockReturnValue({ extras: { state: { addon } } }), + }; - beforeEach(() => { TestBed.configureTestingModule({ imports: [ ConnectAddonComponent, ...MockComponents(SubHeaderComponent, AddonTermsComponent, AddonSetupAccountFormComponent), ], - providers: [provideOSFCore(), provideRouter([]), provideMockStore()], + providers: [ + provideOSFCore(), + provideMockStore({ + signals: [ + { selector: AddonsSelectors.getAddonsUserReference, value: [{ id: 'user-reference-id', userUri: '' }] }, + { + selector: AddonsSelectors.getCreatedOrUpdatedAuthorizedAddon, + value: overrides.createdAccount ?? authorizedAccount, + }, + ], + }), + MockProvider(Router, mockRouter), + MockProvider(ToastService, toastService), + MockProvider(AddonOAuthService, { startOAuthTracking: vi.fn(), stopOAuthTracking: vi.fn() }), + ], }); - fixture = TestBed.createComponent(ConnectAddonComponent); - component = fixture.componentInstance; + const store = TestBed.inject(Store); + const fixture = TestBed.createComponent(ConnectAddonComponent); fixture.detectChanges(); - }); + + return { component: fixture.componentInstance, store, toastService, mockRouter }; + } it('should create and initialize with addon data from router state', () => { + const { component } = setup(); + expect(component).toBeTruthy(); - expect(component['addon']()).toEqual(MOCK_ADDON); - expect(component['terms']().length).toBeGreaterThan(0); + expect(component.addon()).toEqual(storageAddon); + }); + + it('should create the authorized addon and return to the addons list', () => { + const { component, store, mockRouter } = setup(); + + component.handleConnectAuthorizedAddon(payload); + + expect(store.dispatch).toHaveBeenCalledWith(new CreateAuthorizedAddon(payload, 'storage')); + expect(mockRouter.navigate).toHaveBeenCalledWith(['/settings/addons'], { + queryParams: { activeTab: 1, addonType: 'storage' }, + }); + }); + + it('should update the authorized addon when the page was opened for an account', () => { + const { component, store } = setup({ addon: authorizedAccount }); + + component.handleConnectAuthorizedAddon(payload); + + expect(store.dispatch).toHaveBeenCalledWith(new UpdateAuthorizedAddon(payload, 'storage', 'account-id')); + }); + + it('should name a built-in service in the success toast', () => { + const { component, toastService } = setup({ + createdAccount: { ...authorizedAccount, externalServiceName: 'dropbox' }, + }); + + component.handleConnectAuthorizedAddon(payload); + + expect(toastService.showSuccess).toHaveBeenCalledWith('settings.addons.toast.createSuccess', { + addonName: 'Dropbox', + }); + }); + + it('should fall back to the provider name of the addon being connected when the service has no built-in name', () => { + const { component, toastService } = setup({ + addon: { ...storageAddon, externalServiceName: 's3compat', providerName: 'S3 Compatible Storage' }, + createdAccount: { ...authorizedAccount, externalServiceName: 's3compat', providerName: '' }, + }); + + component.handleConnectAuthorizedAddon(payload); + + expect(toastService.showSuccess).toHaveBeenCalledWith('settings.addons.toast.createSuccess', { + addonName: 'S3 Compatible Storage', + }); + }); + + it('should fall back to the provider name of the updated account when the page was opened for an account', () => { + const { component, toastService } = setup({ + addon: { ...authorizedAccount, externalServiceName: 's3compat', providerName: '' }, + createdAccount: { ...authorizedAccount, externalServiceName: 's3compat', providerName: 'S3 Compatible Storage' }, + }); + + component.handleConnectAuthorizedAddon(payload); + + expect(toastService.showSuccess).toHaveBeenCalledWith('settings.addons.toast.createSuccess', { + addonName: 'S3 Compatible Storage', + }); }); }); diff --git a/src/app/features/settings/settings-addons/components/connect-addon/connect-addon.component.ts b/src/app/features/settings/settings-addons/components/connect-addon/connect-addon.component.ts index 95d65c98a..059f576e7 100644 --- a/src/app/features/settings/settings-addons/components/connect-addon/connect-addon.component.ts +++ b/src/app/features/settings/settings-addons/components/connect-addon/connect-addon.component.ts @@ -161,7 +161,10 @@ export class ConnectAddonComponent { }, }); this.toastService.showSuccess('settings.addons.toast.createSuccess', { - addonName: AddonServiceNames[createdAddon?.externalServiceName as keyof typeof AddonServiceNames], + addonName: + AddonServiceNames[createdAddon?.externalServiceName as keyof typeof AddonServiceNames] || + this.addon()?.providerName || + createdAddon?.providerName, }); } }