diff --git a/docs/testing.md b/docs/testing.md index 3de20e198..e05d231a4 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -1,359 +1,908 @@ -# OSF Angular Testing Strategy - -## Index - -- [Overview](#overview) - - [Pro-tips](#pro-tips) -- [Best Practices](#best-practices) -- [Summary Table](#summary-table) -- [Test Coverage Enforcement (100%)](#test-coverage-enforcement-100) -- [Key Structure](#key-structure) -- [Testing Angular Services (with HTTP)](#testing-angular-services-with-http) -- [Testing Angular Components](#testing-angular-components) -- [Testing Angular Pipes](#testing-angular-pipes) -- [Testing Angular Directives](#testing-angular-directives) -- [Testing Angular NGXS](#testing-ngxs) +# OSF Angular Unit Testing Guide + +## Table of Contents + +1. [Test Stack](#1-test-stack) +2. [Project Testing Infrastructure](#2-project-testing-infrastructure) +3. [Test File Structure](#3-test-file-structure) +4. [TestBed Configuration](#4-testbed-configuration) +5. [Mocking Strategies](#5-mocking-strategies) +6. [Store Mocking](#6-store-mocking) +7. [Router & Route Mocking](#7-router--route-mocking) +8. [Service Mocking](#8-service-mocking) +9. [Signal-Based Testing](#9-signal-based-testing) +10. [Async Operations](#10-async-operations) +11. [Form Testing](#11-form-testing) +12. [Dialog Testing](#12-dialog-testing) +13. [Edge Cases](#13-edge-cases) +14. [Testing Angular Services (HTTP)](#14-testing-angular-services-http) +15. [Testing NGXS State](#15-testing-ngxs-state) +16. [Test Data](#16-test-data) +17. [Coverage Enforcement](#17-coverage-enforcement) +18. [Best Practices](#18-best-practices) +19. [Appendix: Assertion Patterns](#appendix-assertion-patterns) --- -## Overview +## 1. Test Stack -The OSF Angular project uses a modular and mock-driven testing strategy. A shared `testing/` folder provides reusable mocks, mock data, and testing module configuration to support consistent and maintainable unit tests across the codebase. +| Tool | Purpose | +| ------------------------- | --------------------------------------------------------------------------------------------- | +| **Jest** | Test runner & assertion library | +| **Angular TestBed** | Component / service compilation | +| **ng-mocks** | `MockComponents`, `MockModule`, `MockProvider` | +| **NGXS** | State management — mocked via `provideMockStore()` for components, real store for state tests | +| **RxJS** | Observable / Subject-based async testing | +| **HttpTestingController** | HTTP interception for service and state integration tests | +| **Custom utilities** | `src/testing/` — builders, factories, mock data | --- -### Pro-tips +## 2. Project Testing Infrastructure -**What to test** +### Directory: `src/testing/` -The OSF Angular testing strategy enforces 100% coverage while also serving as a guardrail for future engineers. Each test should highlight the most critical aspect of your code — what you’d want the next developer to understand before making changes. If a test fails during a refactor, it should clearly signal that a core feature was impacted, prompting them to investigate why and preserve the intended behavior. +``` +src/testing/ +├── osf.testing.provider.ts ← provideOSFCore(), provideOSFHttp() +├── osf.testing.module.ts ← OSFTestingModule (legacy — prefer providers) +├── providers/ ← Builder-pattern mocks for services +│ ├── store-provider.mock.ts +│ ├── route-provider.mock.ts +│ ├── router-provider.mock.ts +│ ├── toast-provider.mock.ts +│ ├── custom-confirmation-provider.mock.ts +│ ├── custom-dialog-provider.mock.ts +│ ├── component-provider.mock.ts +│ ├── loader-service.mock.ts +│ └── dialog-provider.mock.ts +├── mocks/ ← Mock domain models (89+ files) +│ ├── registries.mock.ts +│ ├── draft-registration.mock.ts +│ └── ... +└── data/ ← JSON API response fixtures + ├── dashboard/ + ├── addons/ + └── files/ +``` + +### `provideOSFCore()` — mandatory base provider + +Every component test must include `provideOSFCore()`. It configures animations, translations, and environment tokens. + +```typescript +export function provideOSFCore() { + return [ + provideNoopAnimations(), + importProvidersFrom(TranslateModule.forRoot()), + TranslationServiceMock, + EnvironmentTokenMock, + ]; +} +``` + +> **Never** import `OSFTestingModule` directly in new tests. It is retained for legacy compatibility only. Use `provideOSFCore()` instead. --- -**Test Data** +## 3. Test File Structure + +### Core rules + +- Prefer a single flat `describe` block per file to keep tests searchable and prevent state leakage. Use nested `describe` blocks when it significantly simplifies setup or groups logically distinct behaviors. +- For specs where all tests share a single configuration, use `beforeEach` with `TestBed.configureTestingModule` directly. Use a `setup()` helper when tests need different selector values, route configs, or other overrides. +- No `TestBed.resetTestingModule()` in `afterEach` — Angular auto-resets. +- Use actual interfaces/types for mock data instead of `any`. +- Co-locate unit tests with components using `*.spec.ts`. + +### Standard structure + +```typescript +describe('MyComponent', () => { + let component: MyComponent; + let fixture: ComponentFixture; + let store: Store; + + beforeEach(() => { + TestBed.configureTestingModule({ ... }); + store = TestBed.inject(Store); + fixture = TestBed.createComponent(MyComponent); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + it('should create', () => { + expect(component).toBeTruthy(); + }); +}); +``` + +### `setup()` helper — parameterised tests + +Use when tests need different selector values or route configs. Avoids duplicating `TestBed` configuration across tests. -The OSF Angular Test Data module provides a centralized and consistent source of data across all unit tests. It is intended solely for use within unit tests. By standardizing test data, any changes to underlying data models will produce cascading failures, which help expose the full scope of a refactor. This is preferable to isolated or hardcoded test values, which can lead to false positives and missed regressions. +Extend `BaseSetupOverrides` from `@testing/providers/store-provider.mock` when the spec only needs standard route/selector overrides. Add component-specific fields as needed. -The strategy for structuring test data follows two principles: +Use `mergeSignalOverrides` from `@testing/providers/store-provider.mock` to apply selector overrides on top of default signal values. -1. Include enough data to cover all relevant permutations required by the test suite. -2. Ensure the data reflects all possible states (stati) of the model. +Use `withNoParent()` on `ActivatedRouteMockBuilder` when testing components that guard against a missing parent route. -**Test Scope** +```typescript +import { BaseSetupOverrides, mergeSignalOverrides, provideMockStore } from '@testing/providers/store-provider.mock'; -The OSF Angular project defines a `@testing` scope that can be used for importing all testing-related modules. +interface SetupOverrides extends BaseSetupOverrides { + routerUrl?: string; +} + +function setup(overrides: SetupOverrides = {}) { + const routeBuilder = ActivatedRouteMockBuilder.create().withParams(overrides.routeParams ?? { id: 'draft-1' }); + if (overrides.hasParent === false) routeBuilder.withNoParent(); + const mockRoute = routeBuilder.build(); + + const mockRouter = RouterMockBuilder.create() + .withUrl(overrides.routerUrl ?? '/registries/drafts/reg-1/1') + .build(); + + const defaultSignals = [{ selector: MySelectors.getData, value: mockData }]; + const signals = mergeSignalOverrides(defaultSignals, overrides.selectorOverrides); + + TestBed.configureTestingModule({ + imports: [MyComponent], + providers: [ + provideOSFCore(), + MockProvider(ActivatedRoute, mockRoute), + MockProvider(Router, mockRouter), + provideMockStore({ signals }), + ], + }); + + const store = TestBed.inject(Store); + const fixture = TestBed.createComponent(MyComponent); + return { fixture, component: fixture.componentInstance, store }; +} + +// Usage +it('should handle missing data', () => { + const { component } = setup({ + selectorOverrides: [{ selector: MySelectors.getData, value: null }], + }); + expect(component.hasData()).toBe(false); +}); + +it('should not dispatch when parent route is absent', () => { + const { store } = setup({ hasParent: false }); + expect(store.dispatch).not.toHaveBeenCalled(); +}); +``` --- -## Best Practices +## 4. TestBed Configuration + +### Standalone components (standard) + +```typescript +TestBed.configureTestingModule({ + imports: [ + ComponentUnderTest, + ...MockComponents(ChildA, ChildB), + MockModule(PrimeNGModule), + ], + providers: [ + provideOSFCore(), + MockProvider(ActivatedRoute, mockRoute), + MockProvider(Router, mockRouter), + MockProvider(ToastService, ToastServiceMock.simple()), + provideMockStore({ signals: [...] }), + ], +}); +``` -- Always import `OsfTestingModule` or `OsfTestingStoreModule` to minimize boilerplate and get consistent mock behavior. -- Use mocks and mock-data from `testing/` to avoid repeating test setup. -- Avoid real HTTP, translation, or store dependencies in unit tests by default. -- Co-locate unit tests with components using `*.spec.ts`. +### Components with signal-input children + +Use `overrideComponent` when a child uses Angular signal viewChild and `MockComponents` cannot stub it correctly. + +```typescript +TestBed.configureTestingModule({ ... }) + .overrideComponent(FilesControlComponent, { + remove: { imports: [FilesTreeComponent] }, + add: { + imports: [ + MockComponentWithSignal('osf-files-tree', [ + 'files', + 'selectionMode', + 'totalCount', + 'storage', + 'currentFolder', + 'isLoading', + 'scrollHeight', + 'viewOnly', + 'resourceId', + 'provider', + 'selectedFiles', + ]), + ], + }, + }); +``` --- -## Summary Table +## 5. Mocking Strategies + +### Priority order -| Location | Purpose | -| ----------------------- | -------------------------------------- | -| `osf.testing.module.ts` | Unified test module for shared imports | -| `src/mocks/*.mock.ts` | Mock services and tokens | -| `src/data/*.data.ts` | Static mock data for test cases | +Always check `@testing/` before writing inline mocks. Builders and factories almost certainly exist. + +1. Use existing builders/factories from `@testing/providers/` +2. Use `MockProvider` with an explicit mock object +3. Use `MockComponents` / `MockModule` from ng-mocks +4. Use `MockComponentWithSignal` for signal-input children +5. Inline `jest.fn()` mocks as a last resort + +### Quick reference + +| Need | Use | +| -------------------------- | ------------------------------------------------------- | +| Store selectors / dispatch | `provideMockStore()` | +| Router | `RouterMockBuilder` | +| ActivatedRoute | `ActivatedRouteMockBuilder` | +| ToastService | `ToastServiceMock.simple()` | +| CustomConfirmationService | `CustomConfirmationServiceMock.simple()` | +| CustomDialogService | `CustomDialogServiceMockBuilder` | +| LoaderService | `new LoaderServiceMock()` | +| Child components | `MockComponents(...)` or `MockComponentWithSignal(...)` | +| PrimeNG modules | `MockModule(...)` | + +> **Rule:** Bare `MockProvider(Service)` creates ng-mocks stubs, not `jest.fn()`. When you need `.mockImplementation`, `.mockClear`, or assertion checking, always pass an explicit mock as the second argument. --- -## Test Coverage Enforcement (100%) +## 6. Store Mocking + +### `provideMockStore` configuration options + +| Config key | Maps to | Use case | +| ----------- | ------------------------------------- | ------------------------------------ | +| `signals` | `store.selectSignal()` | Signal-based selectors (most common) | +| `selectors` | `store.select()` / `selectSnapshot()` | Observable-based selectors | +| `actions` | `store.dispatch()` return value | When component reads dispatch result | + +```typescript +provideMockStore({ + signals: [ + { selector: RegistriesSelectors.getDraftRegistration, value: mockDraft }, + { selector: RegistriesSelectors.getStepsState, value: stepsStateSignal }, + ], + actions: [ + { action: new CreateDraft({ ... }), value: { id: 'new-draft' } }, + ], +}) +``` + +### `mergeSignalOverrides` — applying selector overrides in `setup()` -This project **strictly enforces 100% test coverage** through the following mechanisms: +Use `mergeSignalOverrides(defaults, overrides)` from `@testing/providers/store-provider.mock` instead of inlining the merge logic. It replaces matching selectors and preserves the rest. -### Husky Pre-Push Hook +```typescript +import { mergeSignalOverrides } from '@testing/providers/store-provider.mock'; -Before pushing any code, Husky runs a **pre-push hook** that executes: +const defaultSignals = [ + { selector: MySelectors.getData, value: [] }, + { selector: MySelectors.isLoading, value: false }, +]; +const signals = mergeSignalOverrides(defaultSignals, overrides.selectorOverrides); +``` + +### Dispatch assertions + +```typescript +expect(store.dispatch).toHaveBeenCalledWith(new MyAction('id')); +expect(store.dispatch).not.toHaveBeenCalledWith(expect.any(MyAction)); -```bash -npm run test:coverage +// Filter by action type across multiple dispatches +const calls = (store.dispatch as jest.Mock).mock.calls.filter(([a]: [any]) => a instanceof GetProjects); +expect(calls.length).toBe(1); ``` -This command: +### Clearing init dispatches -- Runs the full test suite with `--coverage`. -- Fails the push if **coverage drops below 100%**. -- Ensures developers never bypass test coverage enforcement locally. +When `ngOnInit` dispatches and you need isolated per-test assertions: -> Pro Tip: Use `npm run test:watch` during development to maintain coverage incrementally. +```typescript +(store.dispatch as jest.Mock).mockClear(); +component.doSomething(); +expect(store.dispatch).toHaveBeenCalledWith(new SpecificAction()); +``` --- -### GitHub Actions CI +## 7. Router & Route Mocking -Every pull request and push runs GitHub Actions that: +### ActivatedRoute -- Run `npm run test:coverage`. -- Verify test suite passes with **100% code coverage**. -- Fail the build if even **1 uncovered branch/line/function** exists. +```typescript +const mockRoute = ActivatedRouteMockBuilder.create() + .withParams({ id: 'draft-1' }) + .withQueryParams({ projectId: 'proj-1' }) + .withData({ feature: 'registries' }) + .build(); -This guarantees **test integrity in CI** and **prevents regressions**. +// Nested child routes +const mockRoute = ActivatedRouteMockBuilder.create() + .withParams({ id: 'reg-1' }) + .withFirstChild((child) => child.withParams({ step: '2' })) + .build(); ---- +// No parent route (for testing components that guard against missing parent) +const mockRoute = ActivatedRouteMockBuilder.create().withParams({ id: 'reg-1' }).withNoParent().build(); +``` -### Coverage Expectations +### Router -| File Type | Coverage Requirement | -| ----------- | ------------------------------------------ | -| `*.ts` | 100% line & branch | -| `*.spec.ts` | Required per file | -| Services | Must mock HTTP via `HttpTestingController` | -| Components | DOM + Input + Output event coverage | -| Pipes/Utils | All edge cases tested | +```typescript +const mockRouter = RouterMockBuilder.create().withUrl('/registries/drafts/reg-1/metadata').build(); + +expect(mockRouter.navigate).toHaveBeenCalledWith(['../1'], expect.objectContaining({ relativeTo: expect.anything() })); +expect(mockRouter.navigateByUrl).toHaveBeenCalledWith('/registries/prov-1/new'); +``` --- -### Summary +## 8. Service Mocking -- **Zero exceptions** for test coverage. -- **Push blocked** without passing 100% tests. -- GitHub CI double-checks every PR. +### Simple factories + +```typescript +const toastService = ToastServiceMock.simple(); +const confirmationService = CustomConfirmationServiceMock.simple(); +// Returns plain objects with jest.fn() methods — safe to assert on directly +``` + +### Builder pattern + +```typescript +const mockDialog = CustomDialogServiceMockBuilder.create() + .withOpen( + jest.fn().mockReturnValue({ + onClose: dialogClose$.pipe(), + close: jest.fn(), + }) + ) + .build(); +``` + +### Inline mock (no builder exists) + +```typescript +const mockFilesService = { + uploadFile: jest.fn(), + getFileGuid: jest.fn(), +}; +MockProvider(FilesService, mockFilesService); +``` --- -## Key Structure +## 9. Signal-Based Testing -### `src/testing/osf.testing.module.ts` +### `WritableSignal` for dynamic state -This module centralizes commonly used providers, declarations, and test utilities. It's intended to be imported into any `*.spec.ts` test file to avoid repetitive boilerplate. +Pass a `WritableSignal` as the selector value to change state mid-test. The mock store detects `isSignal(value)` and returns it as-is, so updates propagate automatically. -Example usage: +```typescript +let stepsStateSignal: WritableSignal<{ invalid: boolean }[]>; -```ts -import { TestBed } from '@angular/core/testing'; -import { OsfTestingModule } from '@testing/osf.testing.module'; +beforeEach(() => { + stepsStateSignal = signal([{ invalid: true }]); + provideMockStore({ + signals: [{ selector: RegistriesSelectors.getStepsState, value: stepsStateSignal }], + }); +}); -beforeEach(async () => { - await TestBed.configureTestingModule({ - imports: [OsfTestingModule], - }).compileComponents(); +it('should react to signal changes', () => { + expect(component.isDraftInvalid()).toBe(true); + stepsStateSignal.set([{ invalid: false }]); + expect(component.isDraftInvalid()).toBe(false); }); ``` -### OSFTestingModule +### Setting signal inputs + +```typescript +fixture.componentRef.setInput('attachedFiles', []); +fixture.componentRef.setInput('projectId', 'project-1'); +fixture.detectChanges(); -**Imports:** +// Never use direct property assignment for signal inputs +``` -- `NoopAnimationsModule` – disables Angular animations for clean, predictable unit tests. -- `BrowserModule` – required for bootstrapping Angular features. -- `CommonModule` – provides core Angular directives (e.g., `ngIf`, `ngFor`). -- `TranslateModule.forRoot()` – sets up the translation layer for template-based testing with `@ngx-translate`. +--- -**Providers:** +## 10. Async Operations -- `provideNoopAnimations()` – disables animation via the new standalone provider API. -- `provideRouter([])` – injects an empty router config for component-level testing. -- `provideHttpClient(withInterceptorsFromDi())` – ensures DI-compatible HTTP interceptors are respected in tests. -- `provideHttpClientTesting()` – injects `HttpTestingController` for mocking HTTP requests in unit tests. -- `TranslationServiceMock` – mocks i18n service methods. -- `EnvironmentTokenMock` – mocks environment config values. +### `fakeAsync` + `tick` for debounced operations ---- +```typescript +it('should dispatch after debounce', fakeAsync(() => { + (store.dispatch as jest.Mock).mockClear(); + component.onProjectFilter('abc'); + tick(300); + expect(store.dispatch).toHaveBeenCalledWith(new GetProjects('user-1', 'abc')); +})); -### OSFTestingStoreModule +// Deduplication — only the last value dispatches +it('should debounce rapid calls', fakeAsync(() => { + (store.dispatch as jest.Mock).mockClear(); + component.onProjectFilter('a'); + component.onProjectFilter('ab'); + component.onProjectFilter('abc'); + tick(300); + const calls = (store.dispatch as jest.Mock).mock.calls.filter(([a]: [any]) => a instanceof GetProjects); + expect(calls.length).toBe(1); +})); +``` + +### `done` callback for output emissions + +```typescript +it('should emit attachFile', (done) => { + component.attachFile.subscribe((f) => { + expect(f).toEqual({ id: 'file-1' }); + done(); + }); + component.selectFile({ id: 'file-1' } as FileModel); +}); +``` -**Imports:** +--- -- `OSFTestingModule` – reuses core mocks and modules. +## 11. Form Testing -**Providers:** +### Validation and submit -- `StoreMock` – mocks NgRx Store for selector and dispatch testing. -- `ToastServiceMock` – injects a mock version of the UI toast service. +```typescript +it('should be invalid when title is empty', () => { + component.metadataForm.patchValue({ title: '' }); + expect(component.metadataForm.get('title')?.valid).toBe(false); +}); -### Testing Mocks +it('should trim values on submit', () => { + component.metadataForm.patchValue({ + title: ' Padded Title ', + description: ' Padded Desc ', + }); + (store.dispatch as jest.Mock).mockClear(); + component.submitMetadata(); + expect(store.dispatch).toHaveBeenCalledWith( + new UpdateDraft('draft-1', expect.objectContaining({ title: 'Padded Title' })) + ); +}); +``` -The `src/testing/mocks/` directory provides common service and token mocks to isolate unit tests from real implementations. +### Validator toggling & touched state -**examples** +```typescript +it('should toggle validator', () => { + component.toggleFromProject(); + expect(component.draftForm.get('project')?.validator).toBeTruthy(); + component.toggleFromProject(); + expect(component.draftForm.get('project')?.validator).toBeNull(); +}); -- `environment.token.mock.ts` – Mocks environment tokens like base API URLs. -- `store.mock.ts` – NGXS or other store-related mocks. -- `translation.service.mock.ts` – Prevents needing actual i18n setup during testing. -- `toast.service.mock.ts` – Mocks user feedback services to track invocations without UI. +it('should mark form touched on init when invalid', () => { + expect(component.metadataForm.touched).toBe(true); +}); +``` --- -### Test Data +## 12. Dialog Testing -The `src/testing/data/` directory includes fake/mock data used by tests to simulate external API responses or internal state. +### Subject-based `onClose` -The OSF Angular Test Data module provides a centralized and consistent source of data across all unit tests. It is intended solely for use within unit tests. By standardizing test data, any changes to underlying data models will produce cascading failures, which help expose the full scope of a refactor. This is preferable to isolated or hardcoded test values, which can lead to false positives and missed regressions. +Always use a real `Subject` for `onClose` — `MockProvider` cannot auto-generate reactive streams. Use `provideDynamicDialogRefMock()` where applicable. -The strategy for structuring test data follows two principles: +```typescript +const dialogClose$ = new Subject(); +const mockDialog = CustomDialogServiceMockBuilder.create() + .withOpen( + jest.fn().mockReturnValue({ + onClose: dialogClose$.pipe(), + close: jest.fn(), + }) + ) + .build(); -1. Include enough data to cover all relevant permutations required by the test suite. -2. Ensure the data reflects all possible states (stati) of the model. +it('should navigate on confirm', () => { + component.openConfirmDialog(); + dialogClose$.next(true); + expect(mockRouter.navigate).toHaveBeenCalledWith(['/new-reg-1/overview']); +}); -**examples** +it('should not navigate on cancel', () => { + component.openConfirmDialog(); + dialogClose$.next(false); + expect(mockRouter.navigate).not.toHaveBeenCalled(); +}); +``` + +### Chained dialogs + +```typescript +it('should pass data between dialogs', () => { + const selectClose$ = new Subject(); + const confirmClose$ = new Subject(); + let callCount = 0; + + (dialog.open as jest.Mock).mockImplementation(() => { + callCount++; + const subj = callCount === 1 ? selectClose$ : confirmClose$; + return { onClose: subj.pipe(), close: jest.fn() }; + }); + + component.openSelectComponentsDialog(); + selectClose$.next(['comp-1']); -- `addons.authorized-storage.data.ts` -- `addons.external-storage.data.ts` -- `addons.configured.data.ts` -- `addons.operation-invocation.data.ts` + expect(dialog.open).toHaveBeenCalledTimes(2); + const secondArgs = (dialog.open as jest.Mock).mock.calls[1]; + expect(secondArgs[1].data.components).toEqual(['comp-1']); +}); +``` + +### Confirmation service (auto-confirm pattern) + +```typescript +it('should dispatch on confirm', () => { + mockConfirmation.confirmDelete.mockImplementation(({ onConfirm }: any) => onConfirm()); + (store.dispatch as jest.Mock).mockClear(); + component.deleteDraft(); + expect(store.dispatch).toHaveBeenCalledWith(new DeleteDraft('draft-1')); +}); +``` --- -## Testing Angular Services (with HTTP) +## 13. Edge Cases -All OSF Angular services that make HTTP requests must be tested using `HttpClientTestingModule` and `HttpTestingController`. This testing style verifies both the API call itself and the logic that maps the response into application data. +### `ngOnDestroy` — conditional cleanup -When using HttpTestingController to flush HTTP requests in tests, only use data from the @testing/data mocks to ensure consistency and full test coverage. +Components that auto-save on destroy must skip saves when the resource was already deleted. Test both paths. -Any error handling will also need to be tested. +```typescript +it('should skip updates on destroy when draft was deleted', () => { + (store.dispatch as jest.Mock).mockClear(); + component.isDraftDeleted = true; + component.ngOnDestroy(); + expect(store.dispatch).not.toHaveBeenCalled(); +}); -### Setup +it('should dispatch update on destroy when fields changed', () => { + component.metadataForm.patchValue({ title: 'Changed Title' }); + (store.dispatch as jest.Mock).mockClear(); + component.ngOnDestroy(); + expect(store.dispatch).toHaveBeenCalledWith( + new UpdateDraft('draft-1', expect.objectContaining({ title: 'Changed Title' })) + ); +}); -```ts -import { HttpTestingController } from '@angular/common/http/testing'; -import { OSFTestingModule } from '@testing/osf.testing.module'; +it('should not dispatch update on destroy when fields are unchanged', () => { + (store.dispatch as jest.Mock).mockClear(); + component.ngOnDestroy(); + expect(store.dispatch).not.toHaveBeenCalledWith(expect.any(UpdateDraft)); +}); +``` -let service: YourService; +### Null / undefined selector values -beforeEach(() => { - TestBed.configureTestingModule({ - imports: [OSFTestingModule], - providers: [YourService], +```typescript +it('should handle null draft', () => { + const { component } = setup({ + selectorOverrides: [{ selector: Selectors.getDraft, value: null }], }); + expect(component).toBeTruthy(); +}); +``` - service = TestBed.inject(YourService); +### Empty arrays vs populated arrays + +```typescript +it('should mark invalid when required field has empty array', () => { + const { component } = setup({ + selectorOverrides: [{ selector: Selectors.getStepsData, value: { field1: [] } }], + }); + expect(component.steps()[1].invalid).toBe(true); +}); + +it('should not mark invalid with non-empty array', () => { + const { component } = setup({ + selectorOverrides: [{ selector: Selectors.getStepsData, value: { field1: ['item'] } }], + }); + expect(component.steps()[1].invalid).toBe(false); }); ``` -### Example Test +### Missing links / properties -```ts -it('should call correct endpoint and return expected data', inject( - [HttpTestingController], - (httpMock: HttpTestingController) => { - service.getSomething().subscribe((data) => { - expect(data).toEqual(mockData); - }); +```typescript +it('should not upload when no upload link', () => { + currentFolderSignal.set({ links: {} } as FileFolderModel); + component.uploadFiles(file); + expect(mockFilesService.uploadFile).not.toHaveBeenCalled(); +}); +``` - const req = httpMock.expectOne('/api/endpoint'); - expect(req.request.method).toBe('GET'); - req.flush(getMockDataFromTestingData()); +### File size limits - httpMock.verify(); // Verify no outstanding HTTP calls - } -)); +```typescript +it('should warn on oversized file', () => { + const oversizedFile = new File([''], 'big.bin'); + Object.defineProperty(oversizedFile, 'size', { value: FILE_SIZE_LIMIT }); + component.onFileSelected({ target: { files: [oversizedFile] } } as unknown as Event); + expect(toastService.showWarn).toHaveBeenCalledWith('shared.files.limitText'); +}); ``` -### Key Rules +### Deduplication -- Use `OSFTestingModule` to isolate the service -- Inject and use `HttpTestingController` -- Always call `httpMock.expectOne()` to verify the URL and method -- Always call `req.flush()` to simulate the backend response -- Add `httpMock.verify()` in each `it` to catch unflushed requests +```typescript +it('should deduplicate file selection', () => { + const file = { id: 'file-1' } as FileModel; + component.onFileTreeSelected(file); + component.onFileTreeSelected(file); + expect(component.filesSelection).toEqual([file]); +}); +``` + +### Conditional dispatch based on state + +```typescript +it('should not dispatch when submitting', () => { + const { store } = setup({ + selectorOverrides: [ + { selector: Selectors.isDraftSubmitting, value: true }, + { selector: Selectors.getDraft, value: { ...DEFAULT_DRAFT, hasProject: true } }, + ], + }); + expect(store.dispatch).not.toHaveBeenCalledWith(expect.any(FetchProjectChildren)); +}); +``` --- -## Testing Angular Components +## 14. Testing Angular Services (HTTP) -- coming soon +All services that make HTTP requests must be tested using `HttpClientTestingModule` and `HttpTestingController`. Only use data from `@testing/data` mocks when flushing requests — never hardcode response values inline. ---- +### Setup -## Testing Angular Pipes +```typescript +import { HttpTestingController } from '@angular/common/http/testing'; +import { provideOSFCore, provideOSFHttp } from '@testing/osf.testing.provider'; -- coming soon +let service: YourService; ---- +beforeEach(() => { + TestBed.configureTestingModule({ + imports: [], + providers: [provideOSFCore(), provideOSFHttp(), YourService], + }); + service = TestBed.inject(YourService); +}); +``` -## Testing Angular Directives +### Example test -- coming soon +```typescript +it('should call correct endpoint and return expected data', () => { + const httpMock = TestBed.inject(HttpTestingController); ---- + service.getSomething().subscribe((data) => { + expect(data).toEqual(mockData); + }); + + const req = httpMock.expectOne('/api/endpoint'); + expect(req.request.method).toBe('GET'); + req.flush(getMockDataFromTestingData()); -## NGXS State Testing Strategy + httpMock.verify(); +}); +``` -The OSF Angular strategy for NGXS state testing is to create **small integration test scenarios**. This is a deliberate departure from traditional **black box isolated** testing. The rationale is: +### Key rules -1. **NGXS actions** tested in isolation are difficult to mock and result in garbage-in/garbage-out tests. -2. **NGXS selectors** tested in isolation are easy to mock but also lead to garbage-in/garbage-out outcomes. -3. **NGXS states** tested in isolation are easy to invoke but provide no meaningful validation. -4. **Mocking service calls** during state testing introduces false positives, since the mocked service responses may not reflect actual backend behavior. +- Use `provideOSFCore() + provideOSFHttp()` to isolate the service +- Always call `httpMock.expectOne()` to verify the URL and method +- Always call `req.flush()` with data from `@testing/data` — never hardcode responses inline +- Add `httpMock.verify()` at the end of each test to catch unflushed requests +- Error handling paths must also be tested -This approach favors realism and accuracy over artificial test isolation. +--- -### Test Outline Strategy +## 15. Testing NGXS State -1. **Dispatch the primary action** – Kick off the state logic under test. -2. **Dispatch any dependent actions** – Include any secondary actions that rely on the primary action's outcome. -3. **Verify the loading selector is `true`** – Ensure the loading state is activated during the async flow. -4. **Verify the service call using `HttpTestingController` and `@testing/data` mocks** – Confirm that the correct HTTP request is made and flushed with known mock data. -5. **Verify the loading selector is `false`** – Ensure the loading state deactivates after the response is handled. -6. **Verify the primary data selector** – Check that the core selector related to the dispatched action returns the expected state. -7. **Verify any additional selectors** – Assert the output of other derived selectors relevant to the action. -8. **Validate the test with `httpMock.verify()`** – Confirm that all HTTP requests were flushed and none remain unhandled: +The OSF Angular strategy for NGXS state testing is to create **small integration test scenarios** rather than isolated unit tests. This is a deliberate design decision. -```ts -expect(httpMock.verify).toBeTruthy(); -``` +### Why integration testing for NGXS? -### Example +- Actions tested in isolation are hard to mock and produce garbage-in/garbage-out tests +- Selectors tested in isolation are easy to mock but equally produce false positives +- States tested in isolation are easy to invoke but provide no meaningful validation +- Mocking service calls during state tests introduces false positives — mocked responses may not reflect actual backend behaviour -This is an example of an NGXS action test that involves both a **primary action** and a **dependent action**. The dependency must be dispatched first to ensure the test environment mimics the actual runtime behavior. This pattern helps validate not only the action effects but also the full selector state after updates. All HTTP requests are flushed using the centralized `@testing/data` mocks. +### Test outline — required steps -```ts -it('should test action, state and selectors', inject([HttpTestingController], (httpMock: HttpTestingController) => { +1. **Dispatch the primary action** — kick off the state logic under test +2. **Dispatch any dependent actions** — include secondary actions that rely on the primary action's outcome +3. **Verify the loading selector is `true`** — ensure loading state activates during the async flow +4. **Flush HTTP requests with `@testing/data` mocks** — confirm correct requests are made and flushed with known data +5. **Verify the loading selector is `false`** — ensure loading deactivates after the response is handled +6. **Verify the primary data selector** — check the core selector returns expected state +7. **Verify additional selectors** — assert derived selectors relevant to the action +8. **Call `httpMock.verify()`** — confirm no HTTP requests remain unhandled + +### Example + +```typescript +it('should test action, state and selectors', () => { + const httpMock = TestBed.inject(HttpTestingController); let result: any[] = []; - // Dependency Action + + // 1. Dispatch dependent action first store.dispatch(new GetAuthorizedStorageAddons('reference-id')).subscribe(); - // Primary Action + // 2. Dispatch primary action store.dispatch(new GetAuthorizedStorageOauthToken('account-id')).subscribe(() => { result = store.selectSnapshot(AddonsSelectors.getAuthorizedStorageAddons); }); - // Loading selector is true + // 3. Loading selector is true const loading = store.selectSignal(AddonsSelectors.getAuthorizedStorageAddonsLoading); expect(loading()).toBeTruthy(); - // Http request for service for dependency action - let request = httpMock.expectOne('api/path/dependency/action'); - expect(request.request.method).toBe('GET'); - // @testing/data response mock - request.flush(getAddonsAuthorizedStorageData()); + // 4a. Flush dependent action HTTP request + let req = httpMock.expectOne('api/path/dependency/action'); + expect(req.request.method).toBe('GET'); + req.flush(getAddonsAuthorizedStorageData()); - // Http request for service for primary action - let request = httpMock.expectOne('api/path/primary/action'); - expect(request.request.method).toBe('PATCH'); - // @testing/data response mock with updates + // 4b. Flush primary action HTTP request + req = httpMock.expectOne('api/path/primary/action'); + expect(req.request.method).toBe('PATCH'); const addonWithToken = getAddonsAuthorizedStorageData(1); addonWithToken.data.attributes.oauth_token = 'ya2.34234324534'; - request.flush(addonWithToken); + req.flush(addonWithToken); - // Full testing of the dependency selector - expect(result[1]).toEqual( - Object({ - accountOwnerId: '0b441148-83e5-4f7f-b302-b07b528b160b', - }) - ); + // 5. Loading selector is false + expect(loading()).toBeFalsy(); - // Full testing of the primary selector - let oauthToken = store.selectSnapshot(AddonsSelectors.getAuthorizedStorageAddonOauthToken(result[0].id)); + // 6. Primary selector — verify only the targeted record was updated + const oauthToken = store.selectSnapshot(AddonsSelectors.getAuthorizedStorageAddonOauthToken(result[0].id)); expect(oauthToken).toBe('ya29.A0AS3H6NzDCKgrUx'); - // Verify only the requested `account-id` was updated - oauthToken = store.selectSnapshot(AddonsSelectors.getAuthorizedStorageAddonOauthToken(result[1].id)); - expect(oauthToken).toBe(result[1].oauthToken); + // 7. Other selector — verify untargeted record is unchanged + const otherToken = store.selectSnapshot(AddonsSelectors.getAuthorizedStorageAddonOauthToken(result[1].id)); + expect(otherToken).toBe(result[1].oauthToken); - // Loading selector is false - expect(loading()).toBeFalsy(); - - // httpMock.verify to ensure no other api calls are called. - expect(httpMock.verify).toBeTruthy(); -})); + // 8. No outstanding requests + httpMock.verify(); +}); ``` --- + +## 16. Test Data + +Test data lives in two directories under `src/testing/`. Always use these — never hardcode response values inline in tests. + +### `testing/mocks/` — domain model mocks (89+ files) + +Pre-built mock objects for domain models used directly in component tests. Imported via `@testing/mocks/*`. + +| File | Purpose | +| ---------------------------- | ---------------------------------------------- | +| `registries.mock.ts` | `MOCK_DRAFT_REGISTRATION`, `MOCK_PAGES_SCHEMA` | +| `draft-registration.mock.ts` | `MOCK_DRAFT_REGISTRATION` with full shape | +| `schema-response.mock.ts` | Schema response fixtures | +| `contributors.mock.ts` | Contributor model mocks | +| `project.mock.ts` | Project model mocks | + +### `testing/data/` — JSON API response fixtures + +Centralised raw JSON API responses used for HTTP flush in service and state integration tests. Imported via `@testing/data/*`. + +| File | Purpose | +| ------------------------------------- | --------------------------------- | +| `addons.authorized-storage.data.ts` | Authorised storage addon fixtures | +| `addons.external-storage.data.ts` | External storage addon fixtures | +| `addons.configured.data.ts` | Configured addon state fixtures | +| `addons.operation-invocation.data.ts` | Operation invocation fixtures | + +### Why centralised test data matters + +- Any change to an underlying data model produces cascading test failures, exposing the full scope of a refactor +- Hardcoded inline values lead to false positives and missed regressions +- Consistent data across tests makes selector and state assertions directly comparable + +### Data structure principles + +1. Include enough data to cover all relevant permutations required by the test suite +2. Ensure data reflects all possible states of the model + +--- + +## 17. Coverage Enforcement + +This project strictly enforces 90%+ test coverage through GitHub Actions CI. + +### Coverage requirements + +| File type | Requirement | Notes | +| ------------- | ------------------ | ------------------------------------------ | +| `*.ts` | 90%+ line & branch | Zero exceptions | +| Services | 90%+ | Must mock HTTP via `HttpTestingController` | +| Components | 90%+ | DOM + Input + Output event coverage | +| Pipes / utils | 90%+ | All edge cases tested | +| NGXS state | 90%+ | Integration test approach required | + +### Enforcement pipeline + +- **GitHub Actions CI:** runs on every PR and push — build fails if a single uncovered branch, line, or function exists + +> **Tip:** Use `npm run test:watch` during development to maintain coverage incrementally rather than discovering gaps at push time. + +--- + +## 18. Best Practices + +1. **Always use `provideOSFCore()`** — never import `OSFTestingModule` directly in new tests. +2. **Always use `provideMockStore()`** — never mock `component.actions` via `Object.defineProperty`. +3. **Always pass explicit mocks to `MockProvider`** when you need `jest.fn()` assertions. Bare `MockProvider(Service)` creates ng-mocks stubs. +4. **Check `@testing/` before creating inline mocks** — builders and factories almost certainly exist. +5. **Prefer a single flat `describe` block** per file to keep tests searchable and prevent state leakage. Use nested `describe` blocks when it significantly simplifies setup or groups logically distinct behaviors. No `afterEach`. +6. **No redundant tests** — merge tests that cover the same code path. +7. **Use `(store.dispatch as jest.Mock).mockClear()`** when `ngOnInit` dispatches and you need isolated per-test assertions. +8. **Use `WritableSignal` for dynamic state** — pass `signal()` values to `provideMockStore` when tests need to mutate state mid-test. +9. **Use `Subject` for dialog `onClose`** — gives explicit control over dialog result timing. Use `provideDynamicDialogRefMock()` where applicable. +10. **Use `fakeAsync` + `tick`** for debounced operations — specify the exact debounce duration. +11. **Use `fixture.componentRef.setInput()`** for signal inputs — never direct property assignment. +12. **Use `ngMocks.faster()`** when all tests in a file share identical `TestBed` config — reuses the compiled module for speed. Do not use if any test requires a different config: shared state will cause subtle test pollution. +13. **Use typed mock interfaces** (`ToastServiceMockType`, `RouterMockType`, etc.) — avoid `any`. +14. **Test both positive and negative paths** — confirm an action fires AND confirm it does not fire when conditions are not met. +15. **Only use `@testing/data` fixtures in HTTP flushes** — never hardcode response values inline in service or state tests. +16. **Each test should highlight the most critical aspect of the code** — if a test fails during a refactor, it should clearly signal that a core feature was impacted. + +--- + +## Appendix: Assertion Patterns + +### Action dispatch + +```typescript +expect(store.dispatch).toHaveBeenCalledWith(new MyAction('id')); +expect(store.dispatch).not.toHaveBeenCalledWith(expect.any(MyAction)); +expect(store.dispatch).toHaveBeenCalledWith(new UpdateDraft('draft-1', expect.objectContaining({ title: 'Changed' }))); +``` + +### Router navigation + +```typescript +expect(mockRouter.navigate).toHaveBeenCalledWith(['../1'], expect.objectContaining({ relativeTo: expect.anything() })); +expect(mockRouter.navigateByUrl).toHaveBeenCalledWith('/target'); +``` + +### Dialog open calls + +```typescript +expect(mockDialog.open).toHaveBeenCalled(); +const callArgs = (mockDialog.open as jest.Mock).mock.calls[0]; +expect(callArgs[1].header).toBe('expected.title'); +expect(callArgs[1].data.draftId).toBe('draft-1'); +``` + +### Filtering dispatch calls by action type + +```typescript +const calls = (store.dispatch as jest.Mock).mock.calls.filter(([a]: [any]) => a instanceof GetProjects); +expect(calls.length).toBe(1); +expect(calls[0][0]).toEqual(new GetProjects('user-1', 'abc')); +``` diff --git a/jest.config.js b/jest.config.js index a59db139c..ff42f9ea5 100644 --- a/jest.config.js +++ b/jest.config.js @@ -54,10 +54,10 @@ module.exports = { extensionsToTreatAsEsm: ['.ts'], coverageThreshold: { global: { - branches: 34.8, - functions: 38.0, - lines: 65.5, - statements: 66.0, + branches: 39.5, + functions: 41.1, + lines: 68.0, + statements: 68.4, }, }, watchPathIgnorePatterns: [ diff --git a/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.html b/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.html index 9f8a88dc4..5ca0e95f2 100644 --- a/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.html +++ b/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.html @@ -7,13 +7,12 @@ @let resourceType = currentResource()?.type; @let iconName = resourceType === 'analytic_code' ? 'code' : resourceType; @let icon = `custom-icon-${iconName} icon-resource-size`; - @let resourceName = resourceType === RegistryResourceType.Code ? 'Analytic Code' : resourceType;
-

{{ resourceName }}

+

{{ resourceTypeTranslationKey() | translate }}

{{ doiLink() }} diff --git a/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.spec.ts b/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.spec.ts index b934deae5..8ad13dace 100644 --- a/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.spec.ts +++ b/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.spec.ts @@ -1,89 +1,92 @@ import { Store } from '@ngxs/store'; -import { MockComponent, MockProvider } from 'ng-mocks'; +import { MockComponents, MockProvider } from 'ng-mocks'; import { DynamicDialogConfig, DynamicDialogRef } from 'primeng/dynamicdialog'; -import { signal } from '@angular/core'; -import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { TestBed } from '@angular/core/testing'; import { IconComponent } from '@osf/shared/components/icon/icon.component'; import { LoadingSpinnerComponent } from '@osf/shared/components/loading-spinner/loading-spinner.component'; +import { RegistryResourceType } from '@osf/shared/enums/registry-resource.enum'; +import { RegistryResource } from '../../models'; import { RegistryResourcesSelectors } from '../../store/registry-resources'; import { ResourceFormComponent } from '../resource-form/resource-form.component'; import { AddResourceDialogComponent } from './add-resource-dialog.component'; -import { DynamicDialogRefMock } from '@testing/mocks/dynamic-dialog-ref.mock'; -import { TranslateServiceMock } from '@testing/mocks/translate.service.mock'; -import { OSFTestingModule } from '@testing/osf.testing.module'; -import { provideMockStore } from '@testing/providers/store-provider.mock'; +import { provideDynamicDialogRefMock } from '@testing/mocks/dynamic-dialog-ref.mock'; +import { provideOSFCore } from '@testing/osf.testing.provider'; +import { mergeSignalOverrides, provideMockStore, SignalOverride } from '@testing/providers/store-provider.mock'; + +const MOCK_RESOURCE: RegistryResource = { + id: 'res-1', + description: 'Test', + finalized: false, + type: RegistryResourceType.Data, + pid: '10.1234/test', +}; + +interface SetupOverrides { + registryId?: string; + selectorOverrides?: SignalOverride[]; +} + +function setup(overrides: SetupOverrides = {}) { + const mockDialogConfig = { data: { id: overrides.registryId ?? 'registry-123' } }; + + const defaultSignals = [ + { selector: RegistryResourcesSelectors.getCurrentResource, value: null }, + { selector: RegistryResourcesSelectors.isCurrentResourceLoading, value: false }, + ]; + + const signals = mergeSignalOverrides(defaultSignals, overrides.selectorOverrides); + + TestBed.configureTestingModule({ + imports: [ + AddResourceDialogComponent, + ...MockComponents(LoadingSpinnerComponent, ResourceFormComponent, IconComponent), + ], + providers: [ + provideOSFCore(), + provideDynamicDialogRefMock(), + MockProvider(DynamicDialogConfig, mockDialogConfig), + provideMockStore({ signals }), + ], + }); + + const store = TestBed.inject(Store); + const dialogRef = TestBed.inject(DynamicDialogRef); + const fixture = TestBed.createComponent(AddResourceDialogComponent); + const component = fixture.componentInstance; + fixture.detectChanges(); + + return { fixture, component, store, dialogRef }; +} describe('AddResourceDialogComponent', () => { - let component: AddResourceDialogComponent; - let fixture: ComponentFixture; - let store: Store; - let dialogRef: jest.Mocked; - let mockDialogConfig: jest.Mocked; - - const mockRegistryId = 'registry-123'; - - beforeEach(async () => { - mockDialogConfig = { - data: { - id: mockRegistryId, - }, - } as jest.Mocked; - - await TestBed.configureTestingModule({ - imports: [ - AddResourceDialogComponent, - OSFTestingModule, - MockComponent(LoadingSpinnerComponent), - MockComponent(ResourceFormComponent), - MockComponent(IconComponent), - ], - providers: [ - DynamicDialogRefMock, - TranslateServiceMock, - MockProvider(DynamicDialogConfig, mockDialogConfig), - provideMockStore({ - signals: [ - { selector: RegistryResourcesSelectors.getCurrentResource, value: signal(null) }, - { selector: RegistryResourcesSelectors.isCurrentResourceLoading, value: signal(false) }, - ], - }), - ], - }).compileComponents(); - - fixture = TestBed.createComponent(AddResourceDialogComponent); - component = fixture.componentInstance; - store = TestBed.inject(Store); - dialogRef = TestBed.inject(DynamicDialogRef) as jest.Mocked; - fixture.detectChanges(); - }); - - it('should create', () => { - expect(component).toBeTruthy(); - }); + it('should create with default values', () => { + const { component } = setup(); - it('should initialize with default values', () => { + expect(component).toBeTruthy(); expect(component.doiDomain).toBe('https://doi.org/'); - expect(component.inputLimits).toBeDefined(); expect(component.isResourceConfirming()).toBe(false); expect(component.isPreviewMode()).toBe(false); - expect(component.resourceOptions()).toBeDefined(); }); it('should initialize form with empty values', () => { + const { component } = setup(); + expect(component.form.get('pid')?.value).toBe(''); expect(component.form.get('resourceType')?.value).toBe(''); expect(component.form.get('description')?.value).toBe(''); }); it('should validate pid with DOI validator', () => { + const { component } = setup(); const pidControl = component.form.get('pid'); + pidControl?.setValue('invalid-doi'); pidControl?.updateValueAndValidity(); @@ -92,53 +95,130 @@ describe('AddResourceDialogComponent', () => { }); it('should accept valid DOI format', () => { + const { component } = setup(); const pidControl = component.form.get('pid'); + pidControl?.setValue('10.1234/valid.doi'); expect(pidControl?.hasError('doi')).toBe(false); }); it('should not preview resource when form is invalid', () => { - const dispatchSpy = jest.spyOn(store, 'dispatch'); - component.form.get('pid')?.setValue(''); + const { component, store } = setup(); + + (store.dispatch as jest.Mock).mockClear(); + component.previewResource(); + + expect(store.dispatch).not.toHaveBeenCalled(); + expect(component.isPreviewMode()).toBe(false); + }); + + it('should not preview resource when currentResource is null', () => { + const { component, store } = setup(); + component.form.patchValue({ pid: '10.1234/test', resourceType: 'data' }); + (store.dispatch as jest.Mock).mockClear(); component.previewResource(); - expect(dispatchSpy).not.toHaveBeenCalled(); + expect(store.dispatch).not.toHaveBeenCalled(); expect(component.isPreviewMode()).toBe(false); }); - it('should throw error when previewing resource without current resource', () => { - component.form.patchValue({ - pid: '10.1234/test', - resourceType: 'dataset', + it('should preview resource and set preview mode on success', () => { + const { component, store } = setup({ + selectorOverrides: [{ selector: RegistryResourcesSelectors.getCurrentResource, value: MOCK_RESOURCE }], }); - expect(() => component.previewResource()).toThrow(); + component.form.patchValue({ pid: '10.1234/test', resourceType: 'data', description: 'desc' }); + (store.dispatch as jest.Mock).mockClear(); + component.previewResource(); + + expect(store.dispatch).toHaveBeenCalled(); + expect(component.isPreviewMode()).toBe(true); }); it('should set isPreviewMode to false when backToEdit is called', () => { - component.isPreviewMode.set(true); + const { component } = setup(); + component.isPreviewMode.set(true); component.backToEdit(); expect(component.isPreviewMode()).toBe(false); }); - it('should throw error when adding resource without current resource', () => { - expect(() => component.onAddResource()).toThrow(); + it('should not add resource when currentResource is null', () => { + const { component, store, dialogRef } = setup(); + + (store.dispatch as jest.Mock).mockClear(); + component.onAddResource(); + + expect(store.dispatch).not.toHaveBeenCalled(); + expect(dialogRef.close).not.toHaveBeenCalled(); }); - it('should close dialog without deleting when closeDialog is called without current resource', () => { - const dispatchSpy = jest.spyOn(store, 'dispatch'); + it('should confirm add resource and close dialog on success', () => { + const { component, store, dialogRef } = setup({ + selectorOverrides: [{ selector: RegistryResourcesSelectors.getCurrentResource, value: MOCK_RESOURCE }], + }); + + (store.dispatch as jest.Mock).mockClear(); + component.onAddResource(); + expect(component.isResourceConfirming()).toBe(false); + expect(store.dispatch).toHaveBeenCalled(); + expect(dialogRef.close).toHaveBeenCalledWith(true); + }); + + it('should close dialog without deleting when currentResource is null', () => { + const { component, store, dialogRef } = setup(); + + (store.dispatch as jest.Mock).mockClear(); + component.closeDialog(); + + expect(dialogRef.close).toHaveBeenCalled(); + expect(store.dispatch).not.toHaveBeenCalled(); + }); + + it('should delete resource and close dialog when currentResource exists', () => { + const { component, store, dialogRef } = setup({ + selectorOverrides: [{ selector: RegistryResourcesSelectors.getCurrentResource, value: MOCK_RESOURCE }], + }); + + (store.dispatch as jest.Mock).mockClear(); component.closeDialog(); + expect(store.dispatch).toHaveBeenCalled(); expect(dialogRef.close).toHaveBeenCalled(); - expect(dispatchSpy).not.toHaveBeenCalled(); }); - it('should compute doiLink as undefined when current resource does not exist', () => { - expect(component.doiLink()).toBe('https://doi.org/undefined'); + it('should compute doiLink from currentResource pid', () => { + const { component } = setup({ + selectorOverrides: [{ selector: RegistryResourcesSelectors.getCurrentResource, value: MOCK_RESOURCE }], + }); + + expect(component.doiLink()).toBe('https://doi.org/10.1234/test'); + }); + + it('should return empty string for resourceTypeTranslationKey when currentResource is null', () => { + const { component } = setup(); + + expect(component.resourceTypeTranslationKey()).toBe(''); + }); + + it('should return translation key for resourceTypeTranslationKey when resource type matches', () => { + const { component } = setup({ + selectorOverrides: [{ selector: RegistryResourcesSelectors.getCurrentResource, value: MOCK_RESOURCE }], + }); + + expect(component.resourceTypeTranslationKey()).toBe('resources.typeOptions.data'); + }); + + it('should return empty string for resourceTypeTranslationKey when type is unknown', () => { + const unknownResource = { ...MOCK_RESOURCE, type: 'unknown_type' as RegistryResourceType }; + const { component } = setup({ + selectorOverrides: [{ selector: RegistryResourcesSelectors.getCurrentResource, value: unknownResource }], + }); + + expect(component.resourceTypeTranslationKey()).toBe(''); }); }); diff --git a/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.ts b/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.ts index 814a8cdf8..7d69a244a 100644 --- a/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.ts +++ b/src/app/features/registry/components/add-resource-dialog/add-resource-dialog.component.ts @@ -1,11 +1,11 @@ import { createDispatchMap, select } from '@ngxs/store'; -import { TranslatePipe, TranslateService } from '@ngx-translate/core'; +import { TranslatePipe } from '@ngx-translate/core'; import { Button } from 'primeng/button'; import { DynamicDialogConfig, DynamicDialogRef } from 'primeng/dynamicdialog'; -import { finalize, take } from 'rxjs'; +import { finalize } from 'rxjs'; import { ChangeDetectionStrategy, Component, computed, inject, signal } from '@angular/core'; import { FormControl, FormGroup, ReactiveFormsModule, Validators } from '@angular/forms'; @@ -38,7 +38,6 @@ export class AddResourceDialogComponent { readonly dialogRef = inject(DynamicDialogRef); readonly currentResource = select(RegistryResourcesSelectors.getCurrentResource); readonly isCurrentResourceLoading = select(RegistryResourcesSelectors.isCurrentResourceLoading); - private readonly translateService = inject(TranslateService); private dialogConfig = inject(DynamicDialogConfig); private registryId: string = this.dialogConfig.data.id; @@ -61,11 +60,20 @@ export class AddResourceDialogComponent { deleteResource: SilentDelete, }); - public resourceOptions = signal(resourceTypeOptions); - public isPreviewMode = signal(false); + resourceOptions = signal(resourceTypeOptions); + isPreviewMode = signal(false); readonly RegistryResourceType = RegistryResourceType; + readonly resourceTypeTranslationKey = computed(() => { + const type = this.currentResource()?.type; + const options = this.resourceOptions(); + + if (!type || !options.length) return ''; + + return options.find((opt) => opt.value === type)?.label ?? ''; + }); + previewResource(): void { if (this.form.invalid) { return; @@ -79,7 +87,7 @@ export class AddResourceDialogComponent { const currentResource = this.currentResource(); if (!currentResource) { - throw new Error(this.translateService.instant('resources.errors.noCurrentResource')); + return; } this.actions.previewResource(currentResource.id, addResource).subscribe(() => this.isPreviewMode.set(true)); @@ -94,28 +102,23 @@ export class AddResourceDialogComponent { const currentResource = this.currentResource(); if (!currentResource) { - throw new Error(this.translateService.instant('resources.errors.noRegistryId')); + return; } this.isResourceConfirming.set(true); this.actions .confirmAddResource(addResource, currentResource.id, this.registryId) - .pipe( - take(1), - finalize(() => { - this.dialogRef.close(true); - this.isResourceConfirming.set(false); - }) - ) - .subscribe({}); + .pipe(finalize(() => this.isResourceConfirming.set(false))) + .subscribe(() => this.dialogRef.close(true)); } closeDialog(): void { - this.dialogRef.close(); const currentResource = this.currentResource(); if (currentResource) { this.actions.deleteResource(currentResource.id); } + + this.dialogRef.close(); } } diff --git a/src/app/features/registry/components/archiving-message/archiving-message.component.spec.ts b/src/app/features/registry/components/archiving-message/archiving-message.component.spec.ts index 6cf0e635c..5afea533d 100644 --- a/src/app/features/registry/components/archiving-message/archiving-message.component.spec.ts +++ b/src/app/features/registry/components/archiving-message/archiving-message.component.spec.ts @@ -1,106 +1,38 @@ import { MockComponents } from 'ng-mocks'; -import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { TestBed } from '@angular/core/testing'; import { ENVIRONMENT } from '@core/provider/environment.provider'; -import { IconComponent } from '@osf/shared/components/icon/icon.component'; -import { RegistryStatus } from '@osf/shared/enums/registry-status.enum'; -import { RegistrationOverviewModel } from '../../models'; import { ShortRegistrationInfoComponent } from '../short-registration-info/short-registration-info.component'; import { ArchivingMessageComponent } from './archiving-message.component'; import { MOCK_REGISTRATION_OVERVIEW_MODEL } from '@testing/mocks/registration-overview-model.mock'; -import { OSFTestingModule } from '@testing/osf.testing.module'; +import { provideOSFCore } from '@testing/osf.testing.provider'; describe('ArchivingMessageComponent', () => { - let component: ArchivingMessageComponent; - let fixture: ComponentFixture; - let environment: any; - - const mockRegistration: RegistrationOverviewModel = MOCK_REGISTRATION_OVERVIEW_MODEL; - - beforeEach(async () => { - await TestBed.configureTestingModule({ - imports: [ - ArchivingMessageComponent, - OSFTestingModule, - ...MockComponents(IconComponent, ShortRegistrationInfoComponent), - ], - }).compileComponents(); - - fixture = TestBed.createComponent(ArchivingMessageComponent); - component = fixture.componentInstance; - environment = TestBed.inject(ENVIRONMENT); - - fixture.componentRef.setInput('registration', mockRegistration); - fixture.detectChanges(); - }); - - it('should have support email from environment', () => { - expect(component.supportEmail).toBeDefined(); - expect(typeof component.supportEmail).toBe('string'); - expect(component.supportEmail).toBe(environment.supportEmail); - }); - - it('should receive registration input', () => { - expect(component.registration()).toEqual(mockRegistration); - }); - - it('should have registration as a required input', () => { - expect(component.registration()).toBeDefined(); - expect(component.registration()).toEqual(mockRegistration); - }); - - it('should handle different registration statuses', () => { - const statuses = [ - RegistryStatus.Accepted, - RegistryStatus.Pending, - RegistryStatus.Withdrawn, - RegistryStatus.Embargo, - ]; - - statuses.forEach((status) => { - const registrationWithStatus = { ...mockRegistration, status }; - fixture.componentRef.setInput('registration', registrationWithStatus); - fixture.detectChanges(); - - expect(component.registration().status).toBe(status); + beforeEach(() => { + TestBed.configureTestingModule({ + imports: [ArchivingMessageComponent, ...MockComponents(ShortRegistrationInfoComponent)], + providers: [provideOSFCore()], }); }); - it('should be reactive to registration input changes', () => { - const updatedRegistration = { ...mockRegistration, title: 'Updated Title' }; - - fixture.componentRef.setInput('registration', updatedRegistration); + it('should create and receive registration input', () => { + const fixture = TestBed.createComponent(ArchivingMessageComponent); + fixture.componentRef.setInput('registration', MOCK_REGISTRATION_OVERVIEW_MODEL); fixture.detectChanges(); - expect(component.registration().title).toBe('Updated Title'); + expect(fixture.componentInstance).toBeTruthy(); + expect(fixture.componentInstance.registration()).toEqual(MOCK_REGISTRATION_OVERVIEW_MODEL); }); - it('should update when registration properties change', () => { - const updatedRegistration = { - ...mockRegistration, - title: 'New Title', - description: 'New Description', - }; - - fixture.componentRef.setInput('registration', updatedRegistration); - fixture.detectChanges(); - - expect(component.registration().title).toBe('New Title'); - expect(component.registration().description).toBe('New Description'); - }); - - it('should maintain supportEmail reference when registration changes', () => { - const initialSupportEmail = component.supportEmail; - const updatedRegistration = { ...mockRegistration, title: 'Different Title' }; - - fixture.componentRef.setInput('registration', updatedRegistration); + it('should have supportEmail from environment', () => { + const fixture = TestBed.createComponent(ArchivingMessageComponent); + fixture.componentRef.setInput('registration', MOCK_REGISTRATION_OVERVIEW_MODEL); fixture.detectChanges(); - expect(component.supportEmail).toBe(initialSupportEmail); - expect(component.supportEmail).toBe(environment.supportEmail); + expect(fixture.componentInstance.supportEmail).toBe(TestBed.inject(ENVIRONMENT).supportEmail); }); }); diff --git a/src/app/features/registry/components/archiving-message/archiving-message.component.ts b/src/app/features/registry/components/archiving-message/archiving-message.component.ts index 3525ed5aa..bf66ce905 100644 --- a/src/app/features/registry/components/archiving-message/archiving-message.component.ts +++ b/src/app/features/registry/components/archiving-message/archiving-message.component.ts @@ -13,7 +13,7 @@ import { ShortRegistrationInfoComponent } from '../short-registration-info/short @Component({ selector: 'osf-archiving-message', - imports: [TranslatePipe, Card, IconComponent, Divider, ShortRegistrationInfoComponent], + imports: [Card, Divider, IconComponent, ShortRegistrationInfoComponent, TranslatePipe], templateUrl: './archiving-message.component.html', styleUrl: './archiving-message.component.scss', changeDetection: ChangeDetectionStrategy.OnPush, diff --git a/src/app/features/registry/components/edit-resource-dialog/edit-resource-dialog.component.spec.ts b/src/app/features/registry/components/edit-resource-dialog/edit-resource-dialog.component.spec.ts index c4f9447ff..e334b961c 100644 --- a/src/app/features/registry/components/edit-resource-dialog/edit-resource-dialog.component.spec.ts +++ b/src/app/features/registry/components/edit-resource-dialog/edit-resource-dialog.component.spec.ts @@ -2,11 +2,11 @@ import { Store } from '@ngxs/store'; import { MockComponents, MockProvider } from 'ng-mocks'; -import { DynamicDialogConfig } from 'primeng/dynamicdialog'; +import { DynamicDialogConfig, DynamicDialogRef } from 'primeng/dynamicdialog'; -import { of } from 'rxjs'; +import { of, throwError } from 'rxjs'; -import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { TestBed } from '@angular/core/testing'; import { LoadingSpinnerComponent } from '@osf/shared/components/loading-spinner/loading-spinner.component'; import { RegistryResourceType } from '@osf/shared/enums/registry-resource.enum'; @@ -17,168 +17,94 @@ import { ResourceFormComponent } from '../resource-form/resource-form.component' import { EditResourceDialogComponent } from './edit-resource-dialog.component'; -import { DynamicDialogRefMock } from '@testing/mocks/dynamic-dialog-ref.mock'; -import { OSFTestingModule } from '@testing/osf.testing.module'; +import { provideDynamicDialogRefMock } from '@testing/mocks/dynamic-dialog-ref.mock'; +import { provideOSFCore } from '@testing/osf.testing.provider'; import { provideMockStore } from '@testing/providers/store-provider.mock'; +const MOCK_RESOURCE: RegistryResource = { + id: 'resource-123', + pid: '10.1234/test.doi', + type: RegistryResourceType.Data, + description: 'Test resource description', + finalized: false, +}; + describe('EditResourceDialogComponent', () => { - let component: EditResourceDialogComponent; - let fixture: ComponentFixture; - let store: Store; - let mockDialogConfig: jest.Mocked; - - const mockRegistryId = 'registry-123'; - const mockResource: RegistryResource = { - id: 'resource-123', - pid: '10.1234/test.doi', - type: RegistryResourceType.Data, - description: 'Test resource description', - finalized: false, - }; - - beforeEach(async () => { - mockDialogConfig = { - data: { - id: mockRegistryId, - resource: mockResource, - }, - } as jest.Mocked; - - await TestBed.configureTestingModule({ - imports: [ - EditResourceDialogComponent, - OSFTestingModule, - ...MockComponents(LoadingSpinnerComponent, ResourceFormComponent), - ], + beforeEach(() => { + TestBed.configureTestingModule({ + imports: [EditResourceDialogComponent, ...MockComponents(LoadingSpinnerComponent, ResourceFormComponent)], providers: [ - DynamicDialogRefMock, - MockProvider(DynamicDialogConfig, mockDialogConfig), + provideOSFCore(), + provideDynamicDialogRefMock(), + MockProvider(DynamicDialogConfig, { data: { id: 'registry-123', resource: MOCK_RESOURCE } }), provideMockStore({ signals: [{ selector: RegistryResourcesSelectors.isCurrentResourceLoading, value: false }], }), ], - }).compileComponents(); - - fixture = TestBed.createComponent(EditResourceDialogComponent); - component = fixture.componentInstance; - store = TestBed.inject(Store); - fixture.detectChanges(); + }); }); it('should initialize form with resource data', () => { - expect(component.form.get('pid')?.value).toBe(mockResource.pid); - expect(component.form.get('resourceType')?.value).toBe(mockResource.type); - expect(component.form.get('description')?.value).toBe(mockResource.description); - }); - - it('should have required validators on pid and resourceType', () => { - const pidControl = component.form.get('pid'); - const resourceTypeControl = component.form.get('resourceType'); - - expect(pidControl?.hasError('required')).toBe(false); - expect(resourceTypeControl?.hasError('required')).toBe(false); - }); - - it('should validate pid with DOI validator when invalid format', () => { - const pidControl = component.form.get('pid'); - pidControl?.setValue('invalid-doi'); - pidControl?.updateValueAndValidity(); - - const hasDoiError = pidControl?.hasError('doi') || pidControl?.hasError('invalidDoi'); - expect(hasDoiError).toBe(true); - }); - - it('should accept valid DOI format', () => { - const pidControl = component.form.get('pid'); - pidControl?.setValue('10.1234/valid.doi'); + const fixture = TestBed.createComponent(EditResourceDialogComponent); + fixture.detectChanges(); - expect(pidControl?.hasError('doi')).toBe(false); + expect(fixture.componentInstance.form.value).toEqual({ + pid: MOCK_RESOURCE.pid, + resourceType: MOCK_RESOURCE.type, + description: MOCK_RESOURCE.description, + }); }); - it('should mark form as invalid when pid is empty', () => { - component.form.get('pid')?.setValue(''); - component.form.get('pid')?.markAsTouched(); - - expect(component.form.invalid).toBe(true); - }); + it('should not dispatch when form is invalid', () => { + const fixture = TestBed.createComponent(EditResourceDialogComponent); + fixture.detectChanges(); + const store = TestBed.inject(Store); + const dispatchSpy = jest.spyOn(store, 'dispatch'); - it('should mark form as invalid when resourceType is empty', () => { - component.form.get('resourceType')?.setValue(''); - component.form.get('resourceType')?.markAsTouched(); + fixture.componentInstance.form.get('pid')?.setValue(''); + fixture.componentInstance.save(); - expect(component.form.invalid).toBe(true); + expect(dispatchSpy).not.toHaveBeenCalled(); }); - it('should mark form as valid when all required fields are filled with valid values', () => { - component.form.patchValue({ - pid: '10.1234/test', - resourceType: 'dataset', - description: 'Test description', - }); - - expect(component.form.valid).toBe(true); - }); + it('should dispatch UpdateResource and close dialog on success', () => { + const fixture = TestBed.createComponent(EditResourceDialogComponent); + fixture.detectChanges(); + const store = TestBed.inject(Store); + const dialogRef = TestBed.inject(DynamicDialogRef); + jest.spyOn(store, 'dispatch').mockReturnValue(of(undefined)); - it('should dispatch UpdateResource action with correct parameters when form is valid', () => { - const dispatchSpy = jest.spyOn(store, 'dispatch').mockReturnValue(of()); - component.form.patchValue({ + fixture.componentInstance.form.patchValue({ pid: '10.1234/updated', resourceType: 'paper', description: 'Updated description', }); + fixture.componentInstance.save(); - component.save(); - - expect(dispatchSpy).toHaveBeenCalledWith( + expect(store.dispatch).toHaveBeenCalledWith( expect.objectContaining({ - registryId: mockRegistryId, - resourceId: mockResource.id, - resource: expect.objectContaining({ - pid: '10.1234/updated', - resource_type: 'paper', - description: 'Updated description', - }), + registryId: 'registry-123', + resourceId: MOCK_RESOURCE.id, + resource: { pid: '10.1234/updated', resource_type: 'paper', description: 'Updated description' }, }) ); + expect(dialogRef.close).toHaveBeenCalledWith(true); }); - it('should handle empty description', () => { - const dispatchSpy = jest.spyOn(store, 'dispatch').mockReturnValue(of()); - component.form.patchValue({ - pid: '10.1234/test', - resourceType: 'dataset', - description: '', - }); - - component.save(); - - expect(dispatchSpy).toHaveBeenCalledWith( - expect.objectContaining({ - resource: expect.objectContaining({ - description: '', - }), - }) - ); - }); + it('should not close dialog on dispatch error', () => { + const fixture = TestBed.createComponent(EditResourceDialogComponent); + fixture.detectChanges(); + const store = TestBed.inject(Store); + const dialogRef = TestBed.inject(DynamicDialogRef); + jest.spyOn(store, 'dispatch').mockReturnValue(throwError(() => new Error('fail'))); - it('should handle null form values by converting to empty strings', () => { - const dispatchSpy = jest.spyOn(store, 'dispatch').mockReturnValue(of()); - component.form.patchValue({ - pid: '10.1234/test', - resourceType: 'dataset', - description: null, + fixture.componentInstance.form.patchValue({ + pid: '10.1234/updated', + resourceType: 'paper', + description: '', }); + fixture.componentInstance.save(); - component.save(); - - expect(dispatchSpy).toHaveBeenCalledWith( - expect.objectContaining({ - resource: expect.objectContaining({ - pid: '10.1234/test', - resource_type: 'dataset', - description: '', - }), - }) - ); + expect(dialogRef.close).not.toHaveBeenCalled(); }); }); diff --git a/src/app/features/registry/components/edit-resource-dialog/edit-resource-dialog.component.ts b/src/app/features/registry/components/edit-resource-dialog/edit-resource-dialog.component.ts index a6e84c692..247da58b1 100644 --- a/src/app/features/registry/components/edit-resource-dialog/edit-resource-dialog.component.ts +++ b/src/app/features/registry/components/edit-resource-dialog/edit-resource-dialog.component.ts @@ -2,8 +2,6 @@ import { createDispatchMap, select } from '@ngxs/store'; import { DynamicDialogConfig, DynamicDialogRef } from 'primeng/dynamicdialog'; -import { finalize, take } from 'rxjs'; - import { ChangeDetectionStrategy, Component, inject } from '@angular/core'; import { FormControl, FormGroup, ReactiveFormsModule, Validators } from '@angular/forms'; @@ -16,7 +14,7 @@ import { ResourceFormComponent } from '../resource-form/resource-form.component' @Component({ selector: 'osf-edit-resource-dialog', - imports: [LoadingSpinnerComponent, ReactiveFormsModule, ResourceFormComponent], + imports: [ReactiveFormsModule, LoadingSpinnerComponent, ResourceFormComponent], templateUrl: './edit-resource-dialog.component.html', styleUrl: './edit-resource-dialog.component.scss', changeDetection: ChangeDetectionStrategy.OnPush, @@ -25,7 +23,7 @@ export class EditResourceDialogComponent { readonly dialogRef = inject(DynamicDialogRef); readonly isCurrentResourceLoading = select(RegistryResourcesSelectors.isCurrentResourceLoading); - private dialogConfig = inject(DynamicDialogConfig); + private readonly dialogConfig = inject(DynamicDialogConfig); private registryId: string = this.dialogConfig.data.id; private resource: RegistryResource = this.dialogConfig.data.resource as RegistryResource; @@ -58,12 +56,6 @@ export class EditResourceDialogComponent { this.actions .updateResource(this.registryId, this.resource.id, addResource) - .pipe( - take(1), - finalize(() => { - this.dialogRef.close(true); - }) - ) - .subscribe(); + .subscribe(() => this.dialogRef.close(true)); } } diff --git a/src/app/features/registry/components/registration-links-card/registration-links-card.component.html b/src/app/features/registry/components/registration-links-card/registration-links-card.component.html index 612052d22..6e2f582b3 100644 --- a/src/app/features/registry/components/registration-links-card/registration-links-card.component.html +++ b/src/app/features/registry/components/registration-links-card/registration-links-card.component.html @@ -9,7 +9,7 @@ styleClass="text-lg" link [label]="registrationData().title || 'project.registrations.card.noTitle' | translate" - (click)="reviewEmitRegistrationData.emit(registrationData()!.id)" + (onClick)="reviewEmitRegistrationData.emit(registrationData()!.id)" >
@@ -82,17 +82,13 @@ - @if ( - registrationDataTyped() && - registrationDataTyped()?.currentUserPermissions && - registrationDataTyped()!.currentUserPermissions.length > 1 - ) { + @if (hasWriteAccess()) { } diff --git a/src/app/features/registry/components/registration-links-card/registration-links-card.component.spec.ts b/src/app/features/registry/components/registration-links-card/registration-links-card.component.spec.ts index f10170db1..1155f3d51 100644 --- a/src/app/features/registry/components/registration-links-card/registration-links-card.component.spec.ts +++ b/src/app/features/registry/components/registration-links-card/registration-links-card.component.spec.ts @@ -1,6 +1,6 @@ import { MockComponents } from 'ng-mocks'; -import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { TestBed } from '@angular/core/testing'; import { ContributorsListComponent } from '@osf/shared/components/contributors-list/contributors-list.component'; import { DataResourcesComponent } from '@osf/shared/components/data-resources/data-resources.component'; @@ -11,110 +11,77 @@ import { RegistrationLinksCardComponent } from './registration-links-card.compon import { createMockLinkedNode } from '@testing/mocks/linked-node.mock'; import { createMockLinkedRegistration } from '@testing/mocks/linked-registration.mock'; import { createMockRegistryComponent } from '@testing/mocks/registry-component.mock'; -import { OSFTestingModule } from '@testing/osf.testing.module'; +import { provideOSFCore } from '@testing/osf.testing.provider'; +import { MockComponentWithSignal } from '@testing/providers/component-provider.mock'; describe('RegistrationLinksCardComponent', () => { - let component: RegistrationLinksCardComponent; - let fixture: ComponentFixture; - - beforeEach(async () => { - await TestBed.configureTestingModule({ + beforeEach(() => { + TestBed.configureTestingModule({ imports: [ RegistrationLinksCardComponent, - OSFTestingModule, ...MockComponents(DataResourcesComponent, IconComponent, ContributorsListComponent), + MockComponentWithSignal('osf-truncated-text'), ], - }).compileComponents(); - - fixture = TestBed.createComponent(RegistrationLinksCardComponent); - component = fixture.componentInstance; - }); - - it('should create', () => { - fixture.componentRef.setInput('registrationData', createMockLinkedRegistration()); - fixture.detectChanges(); - expect(component).toBeTruthy(); - }); - - it('should set registrationData input correctly with LinkedRegistration', () => { - const mockLinkedRegistration = createMockLinkedRegistration(); - fixture.componentRef.setInput('registrationData', mockLinkedRegistration); - fixture.detectChanges(); - - expect(component.registrationData()).toEqual(mockLinkedRegistration); - }); - - it('should set registrationData input correctly with LinkedNode', () => { - const mockLinkedNode = createMockLinkedNode(); - fixture.componentRef.setInput('registrationData', mockLinkedNode); - fixture.detectChanges(); - - expect(component.registrationData()).toEqual(mockLinkedNode); + providers: [provideOSFCore()], + }); }); - it('should set registrationData input correctly with RegistryComponentModel', () => { - const mockRegistryComponent = createMockRegistryComponent(); - fixture.componentRef.setInput('registrationData', mockRegistryComponent); - fixture.detectChanges(); - - expect(component.registrationData()).toEqual(mockRegistryComponent); - }); - - it('should return true when data has reviewsState property', () => { + it('should identify LinkedRegistration data', () => { + const fixture = TestBed.createComponent(RegistrationLinksCardComponent); fixture.componentRef.setInput('registrationData', createMockLinkedRegistration()); fixture.detectChanges(); - expect(component.isRegistrationData()).toBe(true); + expect(fixture.componentInstance.isRegistrationData()).toBe(true); + expect(fixture.componentInstance.registrationDataTyped()).toEqual(createMockLinkedRegistration()); }); - it('should return false when data does not have reviewsState property', () => { + it('should identify LinkedNode data', () => { + const fixture = TestBed.createComponent(RegistrationLinksCardComponent); fixture.componentRef.setInput('registrationData', createMockLinkedNode()); fixture.detectChanges(); - expect(component.isRegistrationData()).toBe(false); + expect(fixture.componentInstance.isRegistrationData()).toBe(false); + expect(fixture.componentInstance.isComponentData()).toBe(false); + expect(fixture.componentInstance.registrationDataTyped()).toBeNull(); + expect(fixture.componentInstance.componentsDataTyped()).toBeNull(); }); - it('should return true when data has registrationSupplement property', () => { + it('should identify RegistryComponentModel data', () => { + const fixture = TestBed.createComponent(RegistrationLinksCardComponent); fixture.componentRef.setInput('registrationData', createMockRegistryComponent()); fixture.detectChanges(); - expect(component.isComponentData()).toBe(true); - }); - - it('should return true for LinkedRegistration with registrationSupplement', () => { - fixture.componentRef.setInput('registrationData', createMockLinkedRegistration()); - fixture.detectChanges(); - - expect(component.isComponentData()).toBe(true); + expect(fixture.componentInstance.isComponentData()).toBe(true); + expect(fixture.componentInstance.componentsDataTyped()).toEqual(createMockRegistryComponent()); }); - it('should return false when data does not have registrationSupplement property', () => { - fixture.componentRef.setInput('registrationData', createMockLinkedNode()); + it('should return true for hasWriteAccess when user has write permission', () => { + const fixture = TestBed.createComponent(RegistrationLinksCardComponent); + fixture.componentRef.setInput( + 'registrationData', + createMockLinkedRegistration({ currentUserPermissions: ['read', 'write'] }) + ); fixture.detectChanges(); - expect(component.isComponentData()).toBe(false); + expect(fixture.componentInstance.hasWriteAccess()).toBe(true); }); - it('should return LinkedRegistration when data has reviewsState', () => { - const mockLinkedRegistration = createMockLinkedRegistration(); - fixture.componentRef.setInput('registrationData', mockLinkedRegistration); + it('should return false for hasWriteAccess when user has read-only permission', () => { + const fixture = TestBed.createComponent(RegistrationLinksCardComponent); + fixture.componentRef.setInput( + 'registrationData', + createMockLinkedRegistration({ currentUserPermissions: ['read'] }) + ); fixture.detectChanges(); - expect(component.registrationDataTyped()).toEqual(mockLinkedRegistration); + expect(fixture.componentInstance.hasWriteAccess()).toBe(false); }); - it('should return null when data does not have reviewsState', () => { + it('should return false for hasWriteAccess for non-registration data', () => { + const fixture = TestBed.createComponent(RegistrationLinksCardComponent); fixture.componentRef.setInput('registrationData', createMockLinkedNode()); fixture.detectChanges(); - expect(component.registrationDataTyped()).toBeNull(); - }); - - it('should return RegistryComponentModel when data has registrationSupplement', () => { - const mockRegistryComponent = createMockRegistryComponent(); - fixture.componentRef.setInput('registrationData', mockRegistryComponent); - fixture.detectChanges(); - - expect(component.componentsDataTyped()).toEqual(mockRegistryComponent); + expect(fixture.componentInstance.hasWriteAccess()).toBe(false); }); }); diff --git a/src/app/features/registry/components/registration-links-card/registration-links-card.component.ts b/src/app/features/registry/components/registration-links-card/registration-links-card.component.ts index a050662ce..f585d316a 100644 --- a/src/app/features/registry/components/registration-links-card/registration-links-card.component.ts +++ b/src/app/features/registry/components/registration-links-card/registration-links-card.component.ts @@ -10,7 +10,7 @@ import { ContributorsListComponent } from '@osf/shared/components/contributors-l import { DataResourcesComponent } from '@osf/shared/components/data-resources/data-resources.component'; import { IconComponent } from '@osf/shared/components/icon/icon.component'; import { TruncatedTextComponent } from '@osf/shared/components/truncated-text/truncated-text.component'; -import { RevisionReviewStates } from '@osf/shared/enums/revision-review-states.enum'; +import { UserPermissions } from '@osf/shared/enums/user-permissions.enum'; import { LinkedNode, LinkedRegistration, RegistryComponentModel } from '../../models'; @@ -36,8 +36,6 @@ export class RegistrationLinksCardComponent { readonly updateEmitRegistrationData = output(); readonly reviewEmitRegistrationData = output(); - readonly RevisionReviewStates = RevisionReviewStates; - readonly isRegistrationData = computed(() => { const data = this.registrationData(); return 'reviewsState' in data; @@ -57,4 +55,8 @@ export class RegistrationLinksCardComponent { const data = this.registrationData(); return this.isComponentData() ? (data as RegistryComponentModel) : null; }); + + readonly hasWriteAccess = computed( + () => this.registrationDataTyped()?.currentUserPermissions?.includes(UserPermissions.Write) ?? false + ); } diff --git a/src/app/features/registry/components/registration-overview-toolbar/registration-overview-toolbar.component.spec.ts b/src/app/features/registry/components/registration-overview-toolbar/registration-overview-toolbar.component.spec.ts index 56a81a601..765ac611f 100644 --- a/src/app/features/registry/components/registration-overview-toolbar/registration-overview-toolbar.component.spec.ts +++ b/src/app/features/registry/components/registration-overview-toolbar/registration-overview-toolbar.component.spec.ts @@ -4,7 +4,7 @@ import { MockComponent, MockProvider } from 'ng-mocks'; import { of } from 'rxjs'; -import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { TestBed } from '@angular/core/testing'; import { UserSelectors } from '@core/store/user'; import { SocialsShareButtonComponent } from '@osf/shared/components/socials-share-button/socials-share-button.component'; @@ -14,121 +14,123 @@ import { BookmarksSelectors } from '@osf/shared/stores/bookmarks'; import { RegistrationOverviewToolbarComponent } from './registration-overview-toolbar.component'; -import { OSFTestingModule } from '@testing/osf.testing.module'; -import { provideMockStore } from '@testing/providers/store-provider.mock'; -import { ToastServiceMockBuilder } from '@testing/providers/toast-provider.mock'; - -describe('RegistrationOverviewToolbarComponent', () => { - let component: RegistrationOverviewToolbarComponent; - let fixture: ComponentFixture; - let store: jest.Mocked; - let toastService: ReturnType; - - const mockResourceId = 'registration-123'; - const mockResourceTitle = 'Test Registration'; - const mockBookmarksCollectionId = 'bookmarks-123'; - - beforeEach(async () => { - toastService = ToastServiceMockBuilder.create().build(); - - await TestBed.configureTestingModule({ - imports: [RegistrationOverviewToolbarComponent, OSFTestingModule, MockComponent(SocialsShareButtonComponent)], - providers: [ - provideMockStore({ - signals: [ - { selector: BookmarksSelectors.getBookmarksCollectionId, value: mockBookmarksCollectionId }, - { selector: BookmarksSelectors.getBookmarks, value: [] }, - { selector: BookmarksSelectors.areBookmarksLoading, value: false }, - { selector: BookmarksSelectors.getBookmarksCollectionIdSubmitting, value: false }, - { selector: UserSelectors.isAuthenticated, value: true }, - ], - }), - MockProvider(ToastService, toastService), - ], - }).compileComponents(); - - store = TestBed.inject(Store) as jest.Mocked; - store.dispatch = jest.fn().mockReturnValue(of(true)); - - fixture = TestBed.createComponent(RegistrationOverviewToolbarComponent); - component = fixture.componentInstance; - - fixture.componentRef.setInput('resourceId', mockResourceId); - fixture.componentRef.setInput('resourceTitle', mockResourceTitle); - fixture.componentRef.setInput('isPublic', true); - }); - - it('should set resourceId input correctly', () => { - fixture.detectChanges(); - expect(component.resourceId()).toBe(mockResourceId); +import { provideOSFCore } from '@testing/osf.testing.provider'; +import { mergeSignalOverrides, provideMockStore, SignalOverride } from '@testing/providers/store-provider.mock'; +import { ToastServiceMock } from '@testing/providers/toast-provider.mock'; + +const MOCK_RESOURCE_ID = 'registration-123'; +const MOCK_BOOKMARKS_COLLECTION_ID = 'bookmarks-123'; + +interface SetupOverrides { + bookmarks?: { id: string }[]; + bookmarksCollectionId?: string | null; + isAuthenticated?: boolean; + selectorOverrides?: SignalOverride[]; +} + +function setup(overrides: SetupOverrides = {}) { + const mockToastService = ToastServiceMock.simple(); + + const defaultSignals = [ + { + selector: BookmarksSelectors.getBookmarksCollectionId, + value: 'bookmarksCollectionId' in overrides ? overrides.bookmarksCollectionId : MOCK_BOOKMARKS_COLLECTION_ID, + }, + { selector: BookmarksSelectors.getBookmarks, value: overrides.bookmarks ?? [] }, + { selector: BookmarksSelectors.areBookmarksLoading, value: false }, + { selector: BookmarksSelectors.getBookmarksCollectionIdSubmitting, value: false }, + { selector: UserSelectors.isAuthenticated, value: overrides.isAuthenticated ?? true }, + ]; + + const signals = mergeSignalOverrides(defaultSignals, overrides.selectorOverrides); + + TestBed.configureTestingModule({ + imports: [RegistrationOverviewToolbarComponent, MockComponent(SocialsShareButtonComponent)], + providers: [provideOSFCore(), MockProvider(ToastService, mockToastService), provideMockStore({ signals })], }); - it('should set resourceTitle input correctly', () => { - fixture.detectChanges(); - expect(component.resourceTitle()).toBe(mockResourceTitle); - }); + const store = TestBed.inject(Store); + const fixture = TestBed.createComponent(RegistrationOverviewToolbarComponent); + fixture.componentRef.setInput('resourceId', MOCK_RESOURCE_ID); + fixture.componentRef.setInput('resourceTitle', 'Test Registration'); + fixture.componentRef.setInput('isPublic', true); + fixture.detectChanges(); - it('should set isPublic input correctly', () => { - fixture.detectChanges(); - expect(component.isPublic()).toBe(true); - }); + return { fixture, component: fixture.componentInstance, store, mockToastService }; +} - it('should dispatch GetResourceBookmark when bookmarksCollectionId and resourceId exist', () => { - fixture.detectChanges(); +describe('RegistrationOverviewToolbarComponent', () => { + it('should dispatch GetResourceBookmark on init', () => { + const { store } = setup(); expect(store.dispatch).toHaveBeenCalledWith( expect.objectContaining({ - bookmarkCollectionId: mockBookmarksCollectionId, - resourceId: mockResourceId, + bookmarkCollectionId: MOCK_BOOKMARKS_COLLECTION_ID, + resourceId: MOCK_RESOURCE_ID, resourceType: ResourceType.Registration, }) ); }); - it('should set isBookmarked to false when bookmarks array is empty', () => { - fixture.detectChanges(); - expect(component.isBookmarked()).toBe(false); + it('should not dispatch GetResourceBookmark when bookmarksCollectionId is null', () => { + const { store } = setup({ bookmarksCollectionId: null }); + + expect(store.dispatch).not.toHaveBeenCalled(); }); - it('should not do anything when resourceId is missing', () => { - fixture.componentRef.setInput('resourceId', ''); - fixture.detectChanges(); + it('should compute isBookmarked from bookmarks', () => { + const { component } = setup({ bookmarks: [{ id: MOCK_RESOURCE_ID }] }); - component.toggleBookmark(); + expect(component.isBookmarked()).toBe(true); + }); - expect(store.dispatch).not.toHaveBeenCalled(); - expect(toastService.showSuccess).not.toHaveBeenCalled(); + it('should compute isBookmarked as false when not in bookmarks', () => { + const { component } = setup({ bookmarks: [{ id: 'other-id' }] }); + + expect(component.isBookmarked()).toBe(false); }); - it('should add bookmark when isBookmarked is false', () => { - fixture.detectChanges(); - component.isBookmarked.set(false); - jest.clearAllMocks(); + it('should dispatch add bookmark when not bookmarked', () => { + const { component, store, mockToastService } = setup(); + jest.spyOn(store, 'dispatch').mockReturnValue(of(undefined)); component.toggleBookmark(); expect(store.dispatch).toHaveBeenCalledWith( expect.objectContaining({ - bookmarkCollectionId: mockBookmarksCollectionId, - resourceId: mockResourceId, + bookmarkCollectionId: MOCK_BOOKMARKS_COLLECTION_ID, + resourceId: MOCK_RESOURCE_ID, resourceType: ResourceType.Registration, }) ); + expect(mockToastService.showSuccess).toHaveBeenCalledWith('project.overview.dialog.toast.bookmark.add'); }); - it('should remove bookmark when isBookmarked is true', () => { - fixture.detectChanges(); - component.isBookmarked.set(true); - jest.clearAllMocks(); + it('should dispatch remove bookmark when already bookmarked', () => { + const { component, store, mockToastService } = setup({ bookmarks: [{ id: MOCK_RESOURCE_ID }] }); + jest.spyOn(store, 'dispatch').mockReturnValue(of(undefined)); component.toggleBookmark(); expect(store.dispatch).toHaveBeenCalledWith( expect.objectContaining({ - bookmarkCollectionId: mockBookmarksCollectionId, - resourceId: mockResourceId, + bookmarkCollectionId: MOCK_BOOKMARKS_COLLECTION_ID, + resourceId: MOCK_RESOURCE_ID, resourceType: ResourceType.Registration, }) ); + expect(mockToastService.showSuccess).toHaveBeenCalledWith('project.overview.dialog.toast.bookmark.remove'); + }); + + it('should not dispatch toggleBookmark when resourceId is empty', () => { + const { fixture, store, mockToastService } = setup(); + fixture.componentRef.setInput('resourceId', ''); + fixture.detectChanges(); + jest.spyOn(store, 'dispatch').mockClear().mockReturnValue(of(undefined)); + + fixture.componentInstance.toggleBookmark(); + + expect(store.dispatch).not.toHaveBeenCalled(); + expect(mockToastService.showSuccess).not.toHaveBeenCalled(); }); }); diff --git a/src/app/features/registry/components/registration-overview-toolbar/registration-overview-toolbar.component.ts b/src/app/features/registry/components/registration-overview-toolbar/registration-overview-toolbar.component.ts index 0038252f3..00640ee98 100644 --- a/src/app/features/registry/components/registration-overview-toolbar/registration-overview-toolbar.component.ts +++ b/src/app/features/registry/components/registration-overview-toolbar/registration-overview-toolbar.component.ts @@ -5,7 +5,7 @@ import { TranslatePipe } from '@ngx-translate/core'; import { Button } from 'primeng/button'; import { Tooltip } from 'primeng/tooltip'; -import { ChangeDetectionStrategy, Component, DestroyRef, effect, inject, input, signal } from '@angular/core'; +import { ChangeDetectionStrategy, Component, computed, DestroyRef, effect, inject, input } from '@angular/core'; import { takeUntilDestroyed } from '@angular/core/rxjs-interop'; import { UserSelectors } from '@core/store/user'; @@ -27,23 +27,26 @@ import { changeDetection: ChangeDetectionStrategy.OnPush, }) export class RegistrationOverviewToolbarComponent { - private toastService = inject(ToastService); - private destroyRef = inject(DestroyRef); + private readonly toastService = inject(ToastService); + private readonly destroyRef = inject(DestroyRef); - resourceId = input.required(); - resourceTitle = input.required(); - isPublic = input(false); + readonly resourceId = input.required(); + readonly resourceTitle = input.required(); + readonly isPublic = input(false); - isBookmarked = signal(false); - resourceType = ResourceType.Registration; + readonly resourceType = ResourceType.Registration; - bookmarksCollectionId = select(BookmarksSelectors.getBookmarksCollectionId); - bookmarks = select(BookmarksSelectors.getBookmarks); - isBookmarksLoading = select(BookmarksSelectors.areBookmarksLoading); - isBookmarksSubmitting = select(BookmarksSelectors.getBookmarksCollectionIdSubmitting); - isAuthenticated = select(UserSelectors.isAuthenticated); + readonly bookmarksCollectionId = select(BookmarksSelectors.getBookmarksCollectionId); + readonly bookmarks = select(BookmarksSelectors.getBookmarks); + readonly isBookmarksLoading = select(BookmarksSelectors.areBookmarksLoading); + readonly isBookmarksSubmitting = select(BookmarksSelectors.getBookmarksCollectionIdSubmitting); + readonly isAuthenticated = select(UserSelectors.isAuthenticated); - actions = createDispatchMap({ + readonly isBookmarked = computed( + () => this.bookmarks()?.some((bookmark) => bookmark.id === this.resourceId()) ?? false + ); + + private readonly actions = createDispatchMap({ getResourceBookmark: GetResourceBookmark, addResourceToBookmarks: AddResourceToBookmarks, removeResourceFromBookmarks: RemoveResourceFromBookmarks, @@ -57,17 +60,6 @@ export class RegistrationOverviewToolbarComponent { this.actions.getResourceBookmark(bookmarksCollectionId, this.resourceId(), this.resourceType); }); - - effect(() => { - const bookmarks = this.bookmarks(); - - if (!this.resourceId() || !bookmarks?.length) { - this.isBookmarked.set(false); - return; - } - - this.isBookmarked.set(bookmarks.some((bookmark) => bookmark.id === this.resourceId())); - }); } toggleBookmark(): void { @@ -75,24 +67,14 @@ export class RegistrationOverviewToolbarComponent { if (!this.resourceId() || !bookmarksId) return; - const newBookmarkState = !this.isBookmarked(); - - if (newBookmarkState) { - this.actions - .addResourceToBookmarks(bookmarksId, this.resourceId(), this.resourceType) - .pipe(takeUntilDestroyed(this.destroyRef)) - .subscribe(() => { - this.isBookmarked.set(newBookmarkState); - this.toastService.showSuccess('project.overview.dialog.toast.bookmark.add'); - }); - } else { - this.actions - .removeResourceFromBookmarks(bookmarksId, this.resourceId(), this.resourceType) - .pipe(takeUntilDestroyed(this.destroyRef)) - .subscribe(() => { - this.isBookmarked.set(newBookmarkState); - this.toastService.showSuccess('project.overview.dialog.toast.bookmark.remove'); - }); - } + const action = this.isBookmarked() + ? this.actions.removeResourceFromBookmarks(bookmarksId, this.resourceId(), this.resourceType) + : this.actions.addResourceToBookmarks(bookmarksId, this.resourceId(), this.resourceType); + + const toastKey = this.isBookmarked() + ? 'project.overview.dialog.toast.bookmark.remove' + : 'project.overview.dialog.toast.bookmark.add'; + + action.pipe(takeUntilDestroyed(this.destroyRef)).subscribe(() => this.toastService.showSuccess(toastKey)); } } diff --git a/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.html b/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.html index a615d5e11..508d4540c 100644 --- a/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.html +++ b/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.html @@ -15,7 +15,7 @@ severity="danger" [disabled]="form.invalid" (onClick)="withdrawRegistration()" - [loading]="submitting" + [loading]="submitting()" >
diff --git a/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.spec.ts b/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.spec.ts index 5e05c64ca..c9f30842c 100644 --- a/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.spec.ts +++ b/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.spec.ts @@ -1,74 +1,83 @@ +import { Store } from '@ngxs/store'; + import { MockComponent, MockProvider } from 'ng-mocks'; -import { DynamicDialogConfig } from 'primeng/dynamicdialog'; +import { DynamicDialogConfig, DynamicDialogRef } from 'primeng/dynamicdialog'; + +import { of, throwError } from 'rxjs'; -import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { TestBed } from '@angular/core/testing'; import { TextInputComponent } from '@osf/shared/components/text-input/text-input.component'; import { RegistrationWithdrawDialogComponent } from './registration-withdraw-dialog.component'; -import { DynamicDialogRefMock } from '@testing/mocks/dynamic-dialog-ref.mock'; -import { OSFTestingModule } from '@testing/osf.testing.module'; +import { provideDynamicDialogRefMock } from '@testing/mocks/dynamic-dialog-ref.mock'; +import { provideOSFCore } from '@testing/osf.testing.provider'; import { provideMockStore } from '@testing/providers/store-provider.mock'; -describe('RegistrationWithdrawDialogComponent', () => { - let component: RegistrationWithdrawDialogComponent; - let fixture: ComponentFixture; - let mockDialogConfig: jest.Mocked; - - beforeEach(async () => { - mockDialogConfig = { - data: { registryId: 'test-registry-id' }, - } as jest.Mocked; - - await TestBed.configureTestingModule({ - imports: [RegistrationWithdrawDialogComponent, OSFTestingModule, MockComponent(TextInputComponent)], - providers: [ - DynamicDialogRefMock, - MockProvider(DynamicDialogConfig, mockDialogConfig), - provideMockStore({ - signals: [], - }), - ], - }).compileComponents(); - - fixture = TestBed.createComponent(RegistrationWithdrawDialogComponent); - component = fixture.componentInstance; - fixture.detectChanges(); +function setup(registryId = 'reg-123') { + TestBed.configureTestingModule({ + imports: [RegistrationWithdrawDialogComponent, MockComponent(TextInputComponent)], + providers: [ + provideOSFCore(), + provideDynamicDialogRefMock(), + MockProvider(DynamicDialogConfig, { data: { registryId } }), + provideMockStore(), + ], }); - it('should create', () => { + const store = TestBed.inject(Store); + const dialogRef = TestBed.inject(DynamicDialogRef); + const fixture = TestBed.createComponent(RegistrationWithdrawDialogComponent); + fixture.detectChanges(); + + return { fixture, component: fixture.componentInstance, store, dialogRef }; +} + +describe('RegistrationWithdrawDialogComponent', () => { + it('should create with default form state', () => { + const { component } = setup(); + expect(component).toBeTruthy(); + expect(component.submitting()).toBe(false); + expect(component.form.controls.text.value).toBe(''); }); - it('should initialize with default values', () => { - expect(component.submitting).toBe(false); - expect(component.form.get('text')?.value).toBe(''); + it('should dispatch withdraw and close dialog on success', () => { + const { component, store, dialogRef } = setup(); + jest.spyOn(store, 'dispatch').mockReturnValue(of(undefined)); + + component.form.controls.text.setValue('Withdrawal reason'); + component.withdrawRegistration(); + + expect(store.dispatch).toHaveBeenCalledWith( + expect.objectContaining({ + registryId: 'reg-123', + justification: 'Withdrawal reason', + }) + ); + expect(component.submitting()).toBe(false); + expect(dialogRef.close).toHaveBeenCalled(); }); - it('should have form validators', () => { - const textControl = component.form.get('text'); + it('should not close dialog on dispatch error', () => { + const { component, store, dialogRef } = setup(); + jest.spyOn(store, 'dispatch').mockReturnValue(throwError(() => new Error('fail'))); - expect(textControl?.hasError('required')).toBe(true); + component.form.controls.text.setValue('Reason'); + component.withdrawRegistration(); - textControl?.setValue('Valid withdrawal reason'); - expect(textControl?.hasError('required')).toBe(false); + expect(component.submitting()).toBe(false); + expect(dialogRef.close).not.toHaveBeenCalled(); }); - it('should handle form validation state', () => { - expect(component.form.valid).toBe(false); - - component.form.patchValue({ - text: 'Valid withdrawal reason', - }); - - expect(component.form.valid).toBe(true); + it('should not dispatch when registryId is missing', () => { + const { component, store, dialogRef } = setup(''); - component.form.patchValue({ - text: '', - }); + component.withdrawRegistration(); - expect(component.form.valid).toBe(false); + expect(store.dispatch).not.toHaveBeenCalled(); + expect(dialogRef.close).not.toHaveBeenCalled(); }); }); diff --git a/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.ts b/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.ts index 8ace79288..b189debd7 100644 --- a/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.ts +++ b/src/app/features/registry/components/registration-withdraw-dialog/registration-withdraw-dialog.component.ts @@ -5,9 +5,9 @@ import { TranslatePipe } from '@ngx-translate/core'; import { Button } from 'primeng/button'; import { DynamicDialogConfig, DynamicDialogRef } from 'primeng/dynamicdialog'; -import { finalize, take } from 'rxjs'; +import { finalize } from 'rxjs'; -import { ChangeDetectionStrategy, Component, inject } from '@angular/core'; +import { ChangeDetectionStrategy, Component, inject, signal } from '@angular/core'; import { FormControl, FormGroup } from '@angular/forms'; import { WithdrawRegistration } from '@osf/features/registry/store/registry'; @@ -17,7 +17,7 @@ import { CustomValidators } from '@osf/shared/helpers/custom-form-validators.hel @Component({ selector: 'osf-registration-withdraw-dialog', - imports: [TranslatePipe, TextInputComponent, Button], + imports: [Button, TextInputComponent, TranslatePipe], templateUrl: './registration-withdraw-dialog.component.html', styleUrl: './registration-withdraw-dialog.component.scss', changeDetection: ChangeDetectionStrategy.OnPush, @@ -32,22 +32,16 @@ export class RegistrationWithdrawDialogComponent { }); readonly inputLimits = InputLimits; - submitting = false; + readonly submitting = signal(false); withdrawRegistration(): void { const registryId = this.config.data.registryId; - if (registryId) { - this.submitting = true; - this.actions - .withdrawRegistration(registryId, this.form.controls.text.value ?? '') - .pipe( - take(1), - finalize(() => { - this.submitting = false; - this.dialogRef.close(); - }) - ) - .subscribe(); - } + if (!registryId) return; + + this.submitting.set(true); + this.actions + .withdrawRegistration(registryId, this.form.controls.text.value ?? '') + .pipe(finalize(() => this.submitting.set(false))) + .subscribe(() => this.dialogRef.close()); } } diff --git a/src/app/features/registry/components/registry-blocks-section/registry-blocks-section.component.spec.ts b/src/app/features/registry/components/registry-blocks-section/registry-blocks-section.component.spec.ts index 595364d11..9e835c38c 100644 --- a/src/app/features/registry/components/registry-blocks-section/registry-blocks-section.component.spec.ts +++ b/src/app/features/registry/components/registry-blocks-section/registry-blocks-section.component.spec.ts @@ -1,118 +1,52 @@ import { MockComponent } from 'ng-mocks'; -import { ComponentFixture, TestBed } from '@angular/core/testing'; +import { TestBed } from '@angular/core/testing'; import { RegistrationBlocksDataComponent } from '@osf/shared/components/registration-blocks-data/registration-blocks-data.component'; import { RevisionReviewStates } from '@osf/shared/enums/revision-review-states.enum'; -import { PageSchema } from '@osf/shared/models/registration/page-schema.model'; import { RegistryBlocksSectionComponent } from './registry-blocks-section.component'; import { createMockPageSchema } from '@testing/mocks/page-schema.mock'; import { createMockSchemaResponse } from '@testing/mocks/schema-response.mock'; -import { OSFTestingModule } from '@testing/osf.testing.module'; +import { provideOSFCore } from '@testing/osf.testing.provider'; describe('RegistryBlocksSectionComponent', () => { - let component: RegistryBlocksSectionComponent; - let fixture: ComponentFixture; - - beforeEach(async () => { - await TestBed.configureTestingModule({ - imports: [RegistryBlocksSectionComponent, OSFTestingModule, MockComponent(RegistrationBlocksDataComponent)], - }).compileComponents(); - - fixture = TestBed.createComponent(RegistryBlocksSectionComponent); - component = fixture.componentInstance; + beforeEach(() => { + TestBed.configureTestingModule({ + imports: [RegistryBlocksSectionComponent, MockComponent(RegistrationBlocksDataComponent)], + providers: [provideOSFCore()], + }); }); - it('should create', () => { - fixture.componentRef.setInput('schemaBlocks', []); + it('should create with required inputs', () => { + const fixture = TestBed.createComponent(RegistryBlocksSectionComponent); + fixture.componentRef.setInput('schemaBlocks', [createMockPageSchema()]); fixture.componentRef.setInput('schemaResponse', null); fixture.detectChanges(); - expect(component).toBeTruthy(); + expect(fixture.componentInstance).toBeTruthy(); + expect(fixture.componentInstance.isLoading()).toBe(false); }); - it('should set schemaBlocks input correctly', () => { - const mockBlocks: PageSchema[] = [createMockPageSchema()]; - fixture.componentRef.setInput('schemaBlocks', mockBlocks); - fixture.componentRef.setInput('schemaResponse', null); - fixture.detectChanges(); - - expect(component.schemaBlocks()).toEqual(mockBlocks); - }); - - it('should set schemaResponse input correctly', () => { + it('should compute updatedFields from schemaResponse', () => { + const fixture = TestBed.createComponent(RegistryBlocksSectionComponent); const mockResponse = createMockSchemaResponse('response-1', RevisionReviewStates.Approved); - fixture.componentRef.setInput('schemaBlocks', []); - fixture.componentRef.setInput('schemaResponse', mockResponse); - fixture.detectChanges(); + mockResponse.updatedResponseKeys = ['key1', 'key2']; - expect(component.schemaResponse()).toEqual(mockResponse); - }); - - it('should default isLoading to false', () => { - fixture.componentRef.setInput('schemaBlocks', []); - fixture.componentRef.setInput('schemaResponse', null); - fixture.detectChanges(); - - expect(component.isLoading()).toBe(false); - }); - - it('should compute updatedFields from schemaResponse with updatedResponseKeys', () => { - const mockResponse = createMockSchemaResponse('response-1', RevisionReviewStates.Approved); - mockResponse.updatedResponseKeys = ['key1', 'key2', 'key3']; fixture.componentRef.setInput('schemaBlocks', []); fixture.componentRef.setInput('schemaResponse', mockResponse); fixture.detectChanges(); - expect(component.updatedFields()).toEqual(['key1', 'key2', 'key3']); + expect(fixture.componentInstance.updatedFields()).toEqual(['key1', 'key2']); }); - it('should return empty array when schemaResponse is null', () => { + it('should return empty updatedFields when schemaResponse is null', () => { + const fixture = TestBed.createComponent(RegistryBlocksSectionComponent); fixture.componentRef.setInput('schemaBlocks', []); fixture.componentRef.setInput('schemaResponse', null); fixture.detectChanges(); - expect(component.updatedFields()).toEqual([]); - }); - - it('should handle single updatedResponseKey', () => { - const mockResponse = createMockSchemaResponse('response-1', RevisionReviewStates.Approved); - mockResponse.updatedResponseKeys = ['single-key']; - fixture.componentRef.setInput('schemaBlocks', []); - fixture.componentRef.setInput('schemaResponse', mockResponse); - fixture.detectChanges(); - - expect(component.updatedFields()).toEqual(['single-key']); - }); - - it('should initialize with all required inputs', () => { - const mockBlocks: PageSchema[] = [createMockPageSchema()]; - const mockResponse = createMockSchemaResponse('response-1', RevisionReviewStates.Approved); - - fixture.componentRef.setInput('schemaBlocks', mockBlocks); - fixture.componentRef.setInput('schemaResponse', mockResponse); - fixture.detectChanges(); - - expect(component.schemaBlocks()).toEqual(mockBlocks); - expect(component.schemaResponse()).toEqual(mockResponse); - expect(component.isLoading()).toBe(false); - }); - - it('should handle all inputs being set together', () => { - const mockBlocks: PageSchema[] = [createMockPageSchema()]; - const mockResponse = createMockSchemaResponse('response-1', RevisionReviewStates.Approved); - mockResponse.updatedResponseKeys = ['test-key']; - - fixture.componentRef.setInput('schemaBlocks', mockBlocks); - fixture.componentRef.setInput('schemaResponse', mockResponse); - fixture.componentRef.setInput('isLoading', true); - fixture.detectChanges(); - - expect(component.schemaBlocks()).toEqual(mockBlocks); - expect(component.schemaResponse()).toEqual(mockResponse); - expect(component.isLoading()).toBe(true); - expect(component.updatedFields()).toEqual(['test-key']); + expect(fixture.componentInstance.updatedFields()).toEqual([]); }); }); diff --git a/src/app/features/registry/components/registry-make-decision/registry-make-decision.component.html b/src/app/features/registry/components/registry-make-decision/registry-make-decision.component.html index e079f1667..480e1d155 100644 --- a/src/app/features/registry/components/registry-make-decision/registry-make-decision.component.html +++ b/src/app/features/registry/components/registry-make-decision/registry-make-decision.component.html @@ -67,7 +67,7 @@

- {{ requestForm.controls[ModerationDecisionFormControls.Comment].value.length }}/{{ decisionCommentLimit }} + {{ requestForm.controls[ModerationDecisionFormControls.Comment].value?.length }}/{{ decisionCommentLimit }}