-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[libs, ME]: persist records selection #669
Changes from 4 commits
f704a00
0c138c3
bb0ea8f
5972e3b
4430fb9
d7a3c82
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,7 @@ import { Router } from '@angular/router' | |
import { BehaviorSubject } from 'rxjs' | ||
import { CommonModule } from '@angular/common' | ||
import { MatIconModule } from '@angular/material/icon' | ||
import { SelectionService } from '@geonetwork-ui/api/repository/gn4' | ||
|
||
const results = [{ md: true }] | ||
const currentPage = 5 | ||
|
@@ -23,6 +24,7 @@ export class RecordTableComponent { | |
@Input() records: CatalogRecord[] | ||
@Input() totalHits: number | ||
@Output() recordSelect = new EventEmitter<CatalogRecord>() | ||
@Output() recordsSelection = new EventEmitter<CatalogRecord[]>() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this output really needed? I didn't see any use of it |
||
} | ||
|
||
@Component({ | ||
|
@@ -55,12 +57,19 @@ class RouterMock { | |
navigate = jest.fn() | ||
} | ||
|
||
class SelectionServiceMock { | ||
selectRecords = jest.fn() | ||
deselectRecords = jest.fn() | ||
clearSelection = jest.fn() | ||
} | ||
|
||
describe('RecordsListComponent', () => { | ||
let component: RecordsListComponent | ||
let fixture: ComponentFixture<RecordsListComponent> | ||
let router: Router | ||
let searchService: SearchService | ||
let searchFacade: SearchFacade | ||
let selectionService: SelectionService | ||
|
||
beforeEach(() => { | ||
TestBed.configureTestingModule({ | ||
|
@@ -77,6 +86,10 @@ describe('RecordsListComponent', () => { | |
provide: SearchService, | ||
useClass: SearchServiceMock, | ||
}, | ||
{ | ||
provide: SelectionService, | ||
useClass: SelectionServiceMock, | ||
}, | ||
], | ||
}).overrideComponent(RecordsListComponent, { | ||
set: { | ||
|
@@ -90,6 +103,7 @@ describe('RecordsListComponent', () => { | |
}) | ||
router = TestBed.inject(Router) | ||
searchService = TestBed.inject(SearchService) | ||
selectionService = TestBed.inject(SelectionService) | ||
searchFacade = TestBed.inject(SearchFacade) | ||
fixture = TestBed.createComponent(RecordsListComponent) | ||
component = fixture.componentInstance | ||
|
@@ -121,10 +135,15 @@ describe('RecordsListComponent', () => { | |
describe('when click on a record', () => { | ||
beforeEach(() => { | ||
table.recordSelect.emit({ uniqueIdentifier: 123 }) | ||
table.recordsSelection.emit([{ uniqueIdentifier: 123 }]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This deserves a separate test I think there should be a distinction between "clicking on a record" and "selecting a record" |
||
}) | ||
it('routes to record edition', () => { | ||
expect(router.navigate).toHaveBeenCalledWith(['/edit', 123]) | ||
}) | ||
|
||
it('persists selection', () => { | ||
expect(selectionService.selectRecords).toHaveBeenCalled() | ||
}) | ||
}) | ||
describe('when click on pagination', () => { | ||
beforeEach(() => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import { CommonModule } from '@angular/common' | ||
import { Component, Input } from '@angular/core' | ||
import { Component, Input, OnDestroy } from '@angular/core' | ||
import { MatIconModule } from '@angular/material/icon' | ||
import { Router } from '@angular/router' | ||
import { CatalogRecord } from '@geonetwork-ui/common/domain/record' | ||
|
@@ -8,6 +8,8 @@ import { UiSearchModule } from '@geonetwork-ui/ui/search' | |
import { UiElementsModule } from '@geonetwork-ui/ui/elements' | ||
import { SortByField } from '@geonetwork-ui/common/domain/search' | ||
import { TranslateModule } from '@ngx-translate/core' | ||
import { SelectionService } from '@geonetwork-ui/api/repository/gn4' | ||
import { Subject, takeUntil } from 'rxjs' | ||
|
||
const includes = [ | ||
'uuid', | ||
|
@@ -34,18 +36,21 @@ const includes = [ | |
TranslateModule, | ||
], | ||
}) | ||
export class RecordsListComponent { | ||
export class RecordsListComponent implements OnDestroy { | ||
@Input() title: string | ||
@Input() logo: string | ||
@Input() recordCount: number | ||
@Input() userCount: number | ||
@Input() users | ||
@Input() linkToDatahub?: string | ||
|
||
private onDestroy$: Subject<void> = new Subject() | ||
|
||
constructor( | ||
private router: Router, | ||
public searchFacade: SearchFacade, | ||
public searchService: SearchService | ||
public searchService: SearchService, | ||
private selectionService: SelectionService | ||
) { | ||
this.searchFacade.setPageSize(15).setConfigRequestFields(includes) | ||
} | ||
|
@@ -72,4 +77,27 @@ export class RecordsListComponent { | |
showUsers() { | ||
this.router.navigate(['/users/my-org']) | ||
} | ||
|
||
getSelectedRecords() { | ||
return this.selectionService.selectedRecordsIdentifiers$ | ||
} | ||
|
||
handleRecordsSelection(records: CatalogRecord[]) { | ||
this.selectionService | ||
.selectRecords(records) | ||
.pipe(takeUntil(this.onDestroy$)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting, that looks like a valid approach to avoid having long-running observables stopping after a component is destroyed. Here though we shouldn't need it as the |
||
.subscribe() | ||
} | ||
|
||
handleRecordsDeselection(records: CatalogRecord[]) { | ||
this.selectionService | ||
.deselectRecords(records) | ||
.pipe(takeUntil(this.onDestroy$)) | ||
.subscribe() | ||
} | ||
|
||
ngOnDestroy(): void { | ||
this.onDestroy$.next() | ||
this.onDestroy$.complete() | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,107 @@ | ||
import { SelectionsApiService } from '@geonetwork-ui/data-access/gn4' | ||
import { SelectionService } from './selection.service' | ||
import { firstValueFrom, of } from 'rxjs' | ||
import { CatalogRecord } from '@geonetwork-ui/common/domain/record' | ||
|
||
function record(uuid: string): CatalogRecord { | ||
return { | ||
uniqueIdentifier: uuid, | ||
} as CatalogRecord | ||
} | ||
|
||
class SelectionsServiceMock { | ||
private selected = ['001', '002', '003'] | ||
add = jest.fn((bucket, ids) => { | ||
this.selected.push(...ids) | ||
return of(undefined) | ||
}) | ||
clear = jest.fn((bucket, ids) => { | ||
this.selected = this.selected.filter( | ||
(id) => !!ids && ids.indexOf(id) === -1 | ||
) | ||
return of(undefined) | ||
}) | ||
get = jest.fn(() => of(this.selected)) | ||
} | ||
|
||
describe('SelectionService', () => { | ||
let service: SelectionService | ||
let selectionsService: SelectionsApiService | ||
|
||
beforeEach(async () => { | ||
selectionsService = new SelectionsServiceMock() as any | ||
service = new SelectionService(selectionsService) | ||
}) | ||
|
||
it('should be created', () => { | ||
expect(service).toBeTruthy() | ||
}) | ||
|
||
describe('#selectRecords', () => { | ||
let selectedRecords | ||
beforeEach(async () => { | ||
service.selectedRecordsIdentifiers$.subscribe((value) => { | ||
selectedRecords = value | ||
}) | ||
await firstValueFrom( | ||
service.selectRecords([record('abcd'), record('efgh'), record('001')]) | ||
) | ||
}) | ||
it('calls the corresponding API', () => { | ||
expect(selectionsService.add).toHaveBeenCalledWith('gnui', [ | ||
'abcd', | ||
'efgh', | ||
'001', | ||
]) | ||
}) | ||
it('emits new records in selectedRecordsIdentifiers$', () => { | ||
expect(selectedRecords).toEqual(['001', '002', '003', 'abcd', 'efgh']) | ||
}) | ||
}) | ||
|
||
describe('#deselectRecords', () => { | ||
let selectedRecords | ||
beforeEach(async () => { | ||
service.selectedRecordsIdentifiers$.subscribe((value) => { | ||
selectedRecords = value | ||
}) | ||
await firstValueFrom( | ||
service.deselectRecords([record('abcd'), record('efgh'), record('001')]) | ||
) | ||
}) | ||
it('calls the corresponding API', () => { | ||
expect(selectionsService.clear).toHaveBeenCalledWith('gnui', [ | ||
'abcd', | ||
'efgh', | ||
'001', | ||
]) | ||
}) | ||
it('emits new records in selectedRecordsIdentifiers$', () => { | ||
expect(selectedRecords).toEqual(['002', '003']) | ||
}) | ||
}) | ||
|
||
describe('#clearSelection', () => { | ||
let selectedRecords | ||
beforeEach(async () => { | ||
service.selectedRecordsIdentifiers$.subscribe((value) => { | ||
selectedRecords = value | ||
}) | ||
await firstValueFrom(service.clearSelection()) | ||
}) | ||
it('calls the corresponding API', () => { | ||
expect(selectionsService.get).toHaveBeenCalledWith('gnui') | ||
|
||
expect(selectionsService.clear).toHaveBeenCalledWith('gnui', [ | ||
'001', | ||
'002', | ||
'003', | ||
]) | ||
}) | ||
it('emits new records in selectedRecordsIdentifiers$', () => { | ||
expect(selectedRecords).toEqual([]) | ||
}) | ||
}) | ||
|
||
// describe('selectedRecordsIdentifiers$', () => {}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
import { Injectable } from '@angular/core' | ||
import { CatalogRecord } from '@geonetwork-ui/common/domain/record' | ||
import { SelectionsApiService } from '@geonetwork-ui/data-access/gn4' | ||
import { BehaviorSubject, Observable, Subscription, map, tap } from 'rxjs' | ||
|
||
const BUCKET_ID = 'gnui' | ||
|
||
@Injectable({ | ||
providedIn: 'root', | ||
}) | ||
export class SelectionService { | ||
selectedRecordsIdentifiers$: BehaviorSubject<string[]> = new BehaviorSubject( | ||
[] | ||
) | ||
subscription: Subscription | ||
|
||
constructor(private selectionsApi: SelectionsApiService) { | ||
this.selectionsApi.get(BUCKET_ID).subscribe((selectedIds) => { | ||
this.addIdsToSelected(Array.from(selectedIds)) | ||
}) | ||
} | ||
|
||
private addIdsToSelected(ids: string[]) { | ||
const currentIds = this.selectedRecordsIdentifiers$.value | ||
const uniqueSet = new Set([...currentIds, ...ids]) | ||
this.selectedRecordsIdentifiers$.next([...uniqueSet]) | ||
} | ||
|
||
private removeIdsFromSelected(ids: string[]) { | ||
const filtered = this.selectedRecordsIdentifiers$.value.filter( | ||
(value) => !ids.includes(value) | ||
) | ||
this.selectedRecordsIdentifiers$.next(filtered) | ||
} | ||
|
||
selectRecords(records: CatalogRecord[]): Observable<void> { | ||
const newIds = [] | ||
records.map((record) => { | ||
newIds.push(record.uniqueIdentifier) | ||
}) | ||
const apiResponse = this.selectionsApi.add(BUCKET_ID, newIds) | ||
return apiResponse.pipe( | ||
tap(() => { | ||
this.addIdsToSelected(newIds) | ||
}), | ||
map(() => undefined) | ||
) | ||
} | ||
|
||
deselectRecords(records: CatalogRecord[]): Observable<void> { | ||
const idsToBeRemoved = [] | ||
records.map((record) => { | ||
idsToBeRemoved.push(record.uniqueIdentifier) | ||
}) | ||
const apiResponse = this.selectionsApi.clear(BUCKET_ID, idsToBeRemoved) | ||
return apiResponse.pipe( | ||
tap(() => { | ||
this.removeIdsFromSelected(idsToBeRemoved) | ||
}), | ||
map(() => undefined) | ||
) | ||
} | ||
|
||
clearSelection(): Observable<void> { | ||
const currentSelectedResponse = this.selectionsApi.get(BUCKET_ID) | ||
let currentSelection | ||
this.subscription = currentSelectedResponse.subscribe((value) => { | ||
currentSelection = [...value] | ||
}) | ||
this.selectionsApi.clear(BUCKET_ID, currentSelection) | ||
const apiResponse = this.selectionsApi.clear(BUCKET_ID, currentSelection) | ||
|
||
return apiResponse.pipe( | ||
tap(() => { | ||
this.removeIdsFromSelected(currentSelection) | ||
}), | ||
map(() => undefined) | ||
) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my other comment about event names, this might deserve a clarification as well :)