From b598f1b5ca9b54c4b5fe23daa3d7b502ee0dc6b2 Mon Sep 17 00:00:00 2001 From: Tim Donohue Date: Tue, 26 Sep 2023 12:07:30 -0500 Subject: [PATCH] Also remove unnecessary EpersonDtoModel from extending ReviewersListComponent. Remove "memberOfGroup" from EpersonDtoModel as it is no longer used --- .../members-list/members-list.component.ts | 12 +-- .../core/eperson/models/eperson-dto.model.ts | 5 - .../reviewers-list.component.spec.ts | 95 ++++++++----------- .../reviewers-list.component.ts | 51 ++++------ 4 files changed, 65 insertions(+), 98 deletions(-) diff --git a/src/app/access-control/group-registry/group-form/members-list/members-list.component.ts b/src/app/access-control/group-registry/group-form/members-list/members-list.component.ts index f49690f674..e828555a80 100644 --- a/src/app/access-control/group-registry/group-form/members-list/members-list.component.ts +++ b/src/app/access-control/group-registry/group-form/members-list/members-list.component.ts @@ -29,8 +29,8 @@ import { DSONameService } from '../../../../core/breadcrumbs/dso-name.service'; */ enum SubKey { ActiveGroup, - MembersDTO, - SearchResultsDTO, + Members, + SearchResults, } /** @@ -166,8 +166,8 @@ export class MembersListComponent implements OnInit, OnDestroy { * @private */ retrieveMembers(page: number): void { - this.unsubFrom(SubKey.MembersDTO); - this.subs.set(SubKey.MembersDTO, + this.unsubFrom(SubKey.Members); + this.subs.set(SubKey.Members, this.paginationService.getCurrentPagination(this.config.id, this.config).pipe( switchMap((currentPagination) => { return this.ePersonDataService.findListByHref(this.groupBeingEdited._links.epersons.href, { @@ -239,8 +239,8 @@ export class MembersListComponent implements OnInit, OnDestroy { * @param data Contains scope and query param */ search(data: any) { - this.unsubFrom(SubKey.SearchResultsDTO); - this.subs.set(SubKey.SearchResultsDTO, + this.unsubFrom(SubKey.SearchResults); + this.subs.set(SubKey.SearchResults, this.paginationService.getCurrentPagination(this.configSearch.id, this.configSearch).pipe( switchMap((paginationOptions) => { diff --git a/src/app/core/eperson/models/eperson-dto.model.ts b/src/app/core/eperson/models/eperson-dto.model.ts index 0e79902196..5fa6c7ed68 100644 --- a/src/app/core/eperson/models/eperson-dto.model.ts +++ b/src/app/core/eperson/models/eperson-dto.model.ts @@ -13,9 +13,4 @@ export class EpersonDtoModel { * Whether or not the linked EPerson is able to be deleted */ public ableToDelete: boolean; - /** - * Whether or not this EPerson is member of group on page it is being used on - */ - public memberOfGroup: boolean; - } diff --git a/src/app/workflowitems-edit-page/advanced-workflow-action/advanced-workflow-action-select-reviewer/reviewers-list/reviewers-list.component.spec.ts b/src/app/workflowitems-edit-page/advanced-workflow-action/advanced-workflow-action-select-reviewer/reviewers-list/reviewers-list.component.spec.ts index 7c8db782ce..bcbeef5683 100644 --- a/src/app/workflowitems-edit-page/advanced-workflow-action/advanced-workflow-action-select-reviewer/reviewers-list/reviewers-list.component.spec.ts +++ b/src/app/workflowitems-edit-page/advanced-workflow-action/advanced-workflow-action-select-reviewer/reviewers-list/reviewers-list.component.spec.ts @@ -17,7 +17,7 @@ import { Group } from '../../../../core/eperson/models/group.model'; import { PageInfo } from '../../../../core/shared/page-info.model'; import { FormBuilderService } from '../../../../shared/form/builder/form-builder.service'; import { NotificationsService } from '../../../../shared/notifications/notifications.service'; -import { GroupMock, GroupMock2 } from '../../../../shared/testing/group-mock'; +import { GroupMock } from '../../../../shared/testing/group-mock'; import { ReviewersListComponent } from './reviewers-list.component'; import { EPersonMock, EPersonMock2 } from '../../../../shared/testing/eperson.mock'; import { @@ -31,8 +31,10 @@ import { NotificationsServiceStub } from '../../../../shared/testing/notificatio import { RouterMock } from '../../../../shared/mocks/router.mock'; import { PaginationService } from '../../../../core/pagination/pagination.service'; import { PaginationServiceStub } from '../../../../shared/testing/pagination-service.stub'; -import { EpersonDtoModel } from '../../../../core/eperson/models/eperson-dto.model'; +// NOTE: Because ReviewersListComponent extends MembersListComponent, the below tests ONLY validate +// features which are *unique* to ReviewersListComponent. All other features are tested in the +// members-list.component.spec.ts file. describe('ReviewersListComponent', () => { let component: ReviewersListComponent; let fixture: ComponentFixture; @@ -40,31 +42,27 @@ describe('ReviewersListComponent', () => { let builderService: FormBuilderService; let ePersonDataServiceStub: any; let groupsDataServiceStub: any; - let activeGroup; - let allEPersons; - let allGroups; - let epersonMembers; - let subgroupMembers; + let activeGroup: Group; + let epersonMembers: EPerson[]; + let epersonNonMembers: EPerson[]; let paginationService; - let ePersonDtoModel1: EpersonDtoModel; - let ePersonDtoModel2: EpersonDtoModel; beforeEach(waitForAsync(() => { activeGroup = GroupMock; epersonMembers = [EPersonMock2]; - subgroupMembers = [GroupMock2]; - allEPersons = [EPersonMock, EPersonMock2]; - allGroups = [GroupMock, GroupMock2]; + epersonNonMembers = [EPersonMock]; ePersonDataServiceStub = { activeGroup: activeGroup, epersonMembers: epersonMembers, - subgroupMembers: subgroupMembers, + epersonNonMembers: epersonNonMembers, + // This method is used to get all the current members findListByHref(_href: string): Observable>> { return createSuccessfulRemoteDataObject$(buildPaginatedList(new PageInfo(), groupsDataServiceStub.getEPersonMembers())); }, + // This method is used to search across *non-members* searchByScope(scope: string, query: string): Observable>> { if (query === '') { - return createSuccessfulRemoteDataObject$(buildPaginatedList(new PageInfo(), allEPersons)); + return createSuccessfulRemoteDataObject$(buildPaginatedList(new PageInfo(), epersonNonMembers)); } return createSuccessfulRemoteDataObject$(buildPaginatedList(new PageInfo(), [])); }, @@ -81,22 +79,22 @@ describe('ReviewersListComponent', () => { groupsDataServiceStub = { activeGroup: activeGroup, epersonMembers: epersonMembers, - subgroupMembers: subgroupMembers, - allGroups: allGroups, + epersonNonMembers: epersonNonMembers, getActiveGroup(): Observable { return observableOf(activeGroup); }, getEPersonMembers() { return this.epersonMembers; }, - searchGroups(query: string): Observable>> { - if (query === '') { - return createSuccessfulRemoteDataObject$(buildPaginatedList(new PageInfo(), this.allGroups)); - } - return createSuccessfulRemoteDataObject$(buildPaginatedList(new PageInfo(), [])); - }, - addMemberToGroup(parentGroup, eperson: EPerson): Observable { - this.epersonMembers = [...this.epersonMembers, eperson]; + addMemberToGroup(parentGroup, epersonToAdd: EPerson): Observable { + // Add eperson to list of members + this.epersonMembers = [...this.epersonMembers, epersonToAdd]; + // Remove eperson from list of non-members + this.epersonNonMembers.forEach( (eperson: EPerson, index: number) => { + if (eperson.id === epersonToAdd.id) { + this.epersonNonMembers.splice(index, 1); + } + }); return observableOf(new RestResponse(true, 200, 'Success')); }, clearGroupsRequests() { @@ -109,21 +107,20 @@ describe('ReviewersListComponent', () => { return '/access-control/groups/' + group.id; }, deleteMemberFromGroup(parentGroup, epersonToDelete: EPerson): Observable { - this.epersonMembers = this.epersonMembers.find((eperson: EPerson) => { - if (eperson.id !== epersonToDelete.id) { - return eperson; + // Remove eperson from list of members + this.epersonMembers.forEach( (eperson: EPerson, index: number) => { + if (eperson.id === epersonToDelete.id) { + this.epersonMembers.splice(index, 1); } }); - if (this.epersonMembers === undefined) { - this.epersonMembers = []; - } + // Add eperson to list of non-members + this.epersonNonMembers = [...this.epersonNonMembers, epersonToDelete]; return observableOf(new RestResponse(true, 200, 'Success')); }, + // Used to find the currently active group findById(id: string) { - for (const group of allGroups) { - if (group.id === id) { - return createSuccessfulRemoteDataObject$(group); - } + if (activeGroup.id === id) { + return createSuccessfulRemoteDataObject$(activeGroup); } return createNoContentRemoteDataObject$(); }, @@ -135,7 +132,7 @@ describe('ReviewersListComponent', () => { translateService = getMockTranslateService(); paginationService = new PaginationServiceStub(); - TestBed.configureTestingModule({ + return TestBed.configureTestingModule({ imports: [CommonModule, NgbModule, FormsModule, ReactiveFormsModule, BrowserModule, TranslateModule.forRoot({ loader: { @@ -169,12 +166,6 @@ describe('ReviewersListComponent', () => { fixture.debugElement.nativeElement.remove(); })); - beforeEach(() => { - ePersonDtoModel1 = new EpersonDtoModel(); - ePersonDtoModel1.eperson = EPersonMock; - ePersonDtoModel2 = new EpersonDtoModel(); - ePersonDtoModel2.eperson = EPersonMock2; - }); describe('when no group is selected', () => { beforeEach(() => { @@ -218,34 +209,32 @@ describe('ReviewersListComponent', () => { it('should replace the value when a new member is added when multipleReviewers is false', () => { spyOn(component.selectedReviewersUpdated, 'emit'); component.multipleReviewers = false; - component.selectedReviewers = [ePersonDtoModel1]; + component.selectedReviewers = [EPersonMock]; - component.addMemberToGroup(ePersonDtoModel2); + component.addMemberToGroup(EPersonMock2); - expect(component.selectedReviewers).toEqual([ePersonDtoModel2]); - expect(component.selectedReviewersUpdated.emit).toHaveBeenCalledWith([ePersonDtoModel2.eperson]); + expect(component.selectedReviewers).toEqual([EPersonMock2]); + expect(component.selectedReviewersUpdated.emit).toHaveBeenCalledWith([EPersonMock2]); }); it('should add the value when a new member is added when multipleReviewers is true', () => { spyOn(component.selectedReviewersUpdated, 'emit'); component.multipleReviewers = true; - component.selectedReviewers = [ePersonDtoModel1]; + component.selectedReviewers = [EPersonMock]; - component.addMemberToGroup(ePersonDtoModel2); + component.addMemberToGroup(EPersonMock2); - expect(component.selectedReviewers).toEqual([ePersonDtoModel1, ePersonDtoModel2]); - expect(component.selectedReviewersUpdated.emit).toHaveBeenCalledWith([ePersonDtoModel1.eperson, ePersonDtoModel2.eperson]); + expect(component.selectedReviewers).toEqual([EPersonMock, EPersonMock2]); + expect(component.selectedReviewersUpdated.emit).toHaveBeenCalledWith([EPersonMock, EPersonMock2]); }); it('should delete the member when present', () => { spyOn(component.selectedReviewersUpdated, 'emit'); - ePersonDtoModel1.memberOfGroup = true; - component.selectedReviewers = [ePersonDtoModel1]; + component.selectedReviewers = [EPersonMock]; - component.deleteMemberFromGroup(ePersonDtoModel1); + component.deleteMemberFromGroup(EPersonMock); expect(component.selectedReviewers).toEqual([]); - expect(ePersonDtoModel1.memberOfGroup).toBeFalse(); expect(component.selectedReviewersUpdated.emit).toHaveBeenCalledWith([]); }); diff --git a/src/app/workflowitems-edit-page/advanced-workflow-action/advanced-workflow-action-select-reviewer/reviewers-list/reviewers-list.component.ts b/src/app/workflowitems-edit-page/advanced-workflow-action/advanced-workflow-action-select-reviewer/reviewers-list/reviewers-list.component.ts index 6984a1d86d..65a106e2e8 100644 --- a/src/app/workflowitems-edit-page/advanced-workflow-action/advanced-workflow-action-select-reviewer/reviewers-list/reviewers-list.component.ts +++ b/src/app/workflowitems-edit-page/advanced-workflow-action/advanced-workflow-action-select-reviewer/reviewers-list/reviewers-list.component.ts @@ -8,10 +8,7 @@ import { NotificationsService } from '../../../../shared/notifications/notificat import { PaginationService } from '../../../../core/pagination/pagination.service'; import { Group } from '../../../../core/eperson/models/group.model'; import { getFirstSucceededRemoteDataPayload } from '../../../../core/shared/operators'; -import { EpersonDtoModel } from '../../../../core/eperson/models/eperson-dto.model'; import { EPerson } from '../../../../core/eperson/models/eperson.model'; -import { Observable, of as observableOf } from 'rxjs'; -import { hasValue } from '../../../../shared/empty.util'; import { PaginatedList } from '../../../../core/data/paginated-list.model'; import { MembersListComponent, @@ -24,8 +21,8 @@ import { DSONameService } from '../../../../core/breadcrumbs/dso-name.service'; */ enum SubKey { ActiveGroup, - MembersDTO, - SearchResultsDTO, + Members, + SearchResults, } /** @@ -50,7 +47,7 @@ export class ReviewersListComponent extends MembersListComponent implements OnIn @Output() selectedReviewersUpdated: EventEmitter = new EventEmitter(); - selectedReviewers: EpersonDtoModel[] = []; + selectedReviewers: EPerson[] = []; constructor( protected groupService: GroupDataService, @@ -100,54 +97,40 @@ export class ReviewersListComponent extends MembersListComponent implements OnIn retrieveMembers(page: number): void { this.config.currentPage = page; if (this.groupId === null) { - this.unsubFrom(SubKey.MembersDTO); - const paginatedListOfDTOs: PaginatedList = new PaginatedList(); - paginatedListOfDTOs.page = this.selectedReviewers; - this.ePeopleMembersOfGroupDtos.next(paginatedListOfDTOs); + this.unsubFrom(SubKey.Members); + const paginatedListOfEPersons: PaginatedList = new PaginatedList(); + paginatedListOfEPersons.page = this.selectedReviewers; + this.ePeopleMembersOfGroup.next(paginatedListOfEPersons); } else { super.retrieveMembers(page); } } /** - * Checks whether the given {@link possibleMember} is part of the {@link selectedReviewers}. + * Removes the {@link eperson} from the {@link selectedReviewers} * - * @param possibleMember The {@link EPerson} that needs to be checked + * @param eperson The {@link EPerson} to remove */ - isMemberOfGroup(possibleMember: EPerson): Observable { - return observableOf(hasValue(this.selectedReviewers.find((reviewer: EpersonDtoModel) => reviewer.eperson.id === possibleMember.id))); - } - - /** - * Removes the {@link ePerson} from the {@link selectedReviewers} - * - * @param ePerson The {@link EpersonDtoModel} containg the {@link EPerson} to remove - */ - deleteMemberFromGroup(ePerson: EpersonDtoModel) { - ePerson.memberOfGroup = false; - const index = this.selectedReviewers.indexOf(ePerson); + deleteMemberFromGroup(eperson: EPerson) { + const index = this.selectedReviewers.indexOf(eperson); if (index !== -1) { this.selectedReviewers.splice(index, 1); } - this.selectedReviewersUpdated.emit(this.selectedReviewers.map((ePersonDtoModel: EpersonDtoModel) => ePersonDtoModel.eperson)); + this.selectedReviewersUpdated.emit(this.selectedReviewers); } /** - * Adds the {@link ePerson} to the {@link selectedReviewers} (or replaces it when {@link multipleReviewers} is + * Adds the {@link eperson} to the {@link selectedReviewers} (or replaces it when {@link multipleReviewers} is * `false`). Afterwards it will emit the list. * - * @param ePerson The {@link EPerson} to add to the list + * @param eperson The {@link EPerson} to add to the list */ - addMemberToGroup(ePerson: EpersonDtoModel) { - ePerson.memberOfGroup = true; + addMemberToGroup(eperson: EPerson) { if (!this.multipleReviewers) { - for (const selectedReviewer of this.selectedReviewers) { - selectedReviewer.memberOfGroup = false; - } this.selectedReviewers = []; } - this.selectedReviewers.push(ePerson); - this.selectedReviewersUpdated.emit(this.selectedReviewers.map((epersonDtoModel: EpersonDtoModel) => epersonDtoModel.eperson)); + this.selectedReviewers.push(eperson); + this.selectedReviewersUpdated.emit(this.selectedReviewers); } }