From 627da93e23ab242425fe0dfaefe7e7159a687d2c Mon Sep 17 00:00:00 2001 From: Koen Pauwels Date: Wed, 30 Nov 2022 15:25:36 +0100 Subject: [PATCH 1/8] 97183 Workaround: reorder admin sidebar sections The issue described at https://github.com/DSpace/dspace-angular/issues/1643 was no longer reproducible: The menu component ultimately retrieves menu section information from the store, but in the `MenuComponent#ngOnInit` method, this information is piped through `distinctUntilChanged(compareArraysUsingIds())`, which discards an update that sets these menu elements to be visible. The behavior of this pipe is probably incorrect, but a proper fix is out of scope for the current task. For now, we work around the problem by adding top-level menu sections _after_ their children while initializing the menu section store, which side-steps this issue. --- src/app/menu.resolver.ts | 44 ++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/app/menu.resolver.ts b/src/app/menu.resolver.ts index 8630150c58..60f2d1cca2 100644 --- a/src/app/menu.resolver.ts +++ b/src/app/menu.resolver.ts @@ -171,17 +171,6 @@ export class MenuResolver implements Resolve { ]).subscribe(([isCollectionAdmin, isCommunityAdmin, isSiteAdmin]) => { const menuList = [ /* News */ - { - id: 'new', - active: false, - visible: true, - model: { - type: MenuItemType.TEXT, - text: 'menu.section.new' - } as TextMenuItemModel, - icon: 'plus', - index: 0 - }, { id: 'new_community', parentID: 'new', @@ -232,6 +221,17 @@ export class MenuResolver implements Resolve { link: '/processes/new' } as LinkMenuItemModel, }, + { + id: 'new', + active: false, + visible: true, + model: { + type: MenuItemType.TEXT, + text: 'menu.section.new' + } as TextMenuItemModel, + icon: 'plus', + index: 0 + }, // TODO: enable this menu item once the feature has been implemented // { // id: 'new_item_version', @@ -246,17 +246,6 @@ export class MenuResolver implements Resolve { // }, /* Edit */ - { - id: 'edit', - active: false, - visible: true, - model: { - type: MenuItemType.TEXT, - text: 'menu.section.edit' - } as TextMenuItemModel, - icon: 'pencil-alt', - index: 1 - }, { id: 'edit_community', parentID: 'edit', @@ -296,6 +285,17 @@ export class MenuResolver implements Resolve { } } as OnClickMenuItemModel, }, + { + id: 'edit', + active: false, + visible: true, + model: { + type: MenuItemType.TEXT, + text: 'menu.section.edit' + } as TextMenuItemModel, + icon: 'pencil-alt', + index: 1 + }, /* Statistics */ // TODO: enable this menu item once the feature has been implemented From 68bac1f400154b4909486eacb77fa3aa181f4371 Mon Sep 17 00:00:00 2001 From: Koen Pauwels Date: Tue, 6 Dec 2022 11:30:46 +0100 Subject: [PATCH 2/8] 97183 Hide sidebar items w/o appropriate permissions. See https://github.com/DSpace/dspace-angular/issues/1643 Sidebar now omits "New" and/or "Edit" options in the sidebar if the user does not have permissions to respectively create new Items or edit Items. If this results in an empty sidebar, the sidebar is hidden in its entirety. --- .../admin-sidebar/admin-sidebar.component.ts | 1 + .../data/feature-authorization/feature-id.ts | 4 +- src/app/menu.resolver.ts | 91 ++++++++++--------- src/app/root/root.component.ts | 2 +- src/app/shared/menu/menu.service.ts | 25 +++++ 5 files changed, 80 insertions(+), 43 deletions(-) diff --git a/src/app/admin/admin-sidebar/admin-sidebar.component.ts b/src/app/admin/admin-sidebar/admin-sidebar.component.ts index 8eb288daa2..36f9250504 100644 --- a/src/app/admin/admin-sidebar/admin-sidebar.component.ts +++ b/src/app/admin/admin-sidebar/admin-sidebar.component.ts @@ -100,6 +100,7 @@ export class AdminSidebarComponent extends MenuComponent implements OnInit { } } }); + this.menuVisible = this.menuService.isMenuVisibleWithVisibleSections(this.menuID); } @HostListener('focusin') diff --git a/src/app/core/data/feature-authorization/feature-id.ts b/src/app/core/data/feature-authorization/feature-id.ts index 3cb18bf515..9e73ca9a2b 100644 --- a/src/app/core/data/feature-authorization/feature-id.ts +++ b/src/app/core/data/feature-authorization/feature-id.ts @@ -29,5 +29,7 @@ export enum FeatureID { CanViewUsageStatistics = 'canViewUsageStatistics', CanSendFeedback = 'canSendFeedback', CanClaimItem = 'canClaimItem', - CanSynchronizeWithORCID = 'canSynchronizeWithORCID' + CanSynchronizeWithORCID = 'canSynchronizeWithORCID', + CanSubmit = 'canSubmit', + CanEditItem = 'canEditItem', } diff --git a/src/app/menu.resolver.ts b/src/app/menu.resolver.ts index 60f2d1cca2..19413ab4be 100644 --- a/src/app/menu.resolver.ts +++ b/src/app/menu.resolver.ts @@ -167,10 +167,11 @@ export class MenuResolver implements Resolve { combineLatest([ this.authorizationService.isAuthorized(FeatureID.IsCollectionAdmin), this.authorizationService.isAuthorized(FeatureID.IsCommunityAdmin), - this.authorizationService.isAuthorized(FeatureID.AdministratorOf) - ]).subscribe(([isCollectionAdmin, isCommunityAdmin, isSiteAdmin]) => { - const menuList = [ - /* News */ + this.authorizationService.isAuthorized(FeatureID.AdministratorOf), + this.authorizationService.isAuthorized(FeatureID.CanSubmit), + this.authorizationService.isAuthorized(FeatureID.CanEditItem), + ]).subscribe(([isCollectionAdmin, isCommunityAdmin, isSiteAdmin, canSubmit, canEditItem]) => { + const newSubMenuList = [ { id: 'new_community', parentID: 'new', @@ -201,7 +202,7 @@ export class MenuResolver implements Resolve { id: 'new_item', parentID: 'new', active: false, - visible: true, + visible: canSubmit, model: { type: MenuItemType.ONCLICK, text: 'menu.section.new_item', @@ -221,30 +222,8 @@ export class MenuResolver implements Resolve { link: '/processes/new' } as LinkMenuItemModel, }, - { - id: 'new', - active: false, - visible: true, - model: { - type: MenuItemType.TEXT, - text: 'menu.section.new' - } as TextMenuItemModel, - icon: 'plus', - index: 0 - }, - // TODO: enable this menu item once the feature has been implemented - // { - // id: 'new_item_version', - // parentID: 'new', - // active: false, - // visible: true, - // model: { - // type: MenuItemType.LINK, - // text: 'menu.section.new_item_version', - // link: '' - // } as LinkMenuItemModel, - // }, - + ]; + const editSubMenuList = [ /* Edit */ { id: 'edit_community', @@ -276,7 +255,7 @@ export class MenuResolver implements Resolve { id: 'edit_item', parentID: 'edit', active: false, - visible: true, + visible: canEditItem, model: { type: MenuItemType.ONCLICK, text: 'menu.section.edit_item', @@ -285,17 +264,47 @@ export class MenuResolver implements Resolve { } } as OnClickMenuItemModel, }, - { - id: 'edit', - active: false, - visible: true, - model: { - type: MenuItemType.TEXT, - text: 'menu.section.edit' - } as TextMenuItemModel, - icon: 'pencil-alt', - index: 1 - }, + ]; + const newSubMenu = { + id: 'new', + active: false, + visible: newSubMenuList.map(subMenu => subMenu.visible).reduce((x,y) => x || y), + model: { + type: MenuItemType.TEXT, + text: 'menu.section.new' + } as TextMenuItemModel, + icon: 'plus', + index: 0 + } + const editSubMenu = { + id: 'edit', + active: false, + visible: editSubMenuList.map(subMenu => subMenu.visible).reduce((x,y) => x || y), + model: { + type: MenuItemType.TEXT, + text: 'menu.section.edit' + } as TextMenuItemModel, + icon: 'pencil-alt', + index: 1 + } + + const menuList = [ + ...newSubMenuList, + newSubMenu, + ...editSubMenuList, + editSubMenu, + // TODO: enable this menu item once the feature has been implemented + // { + // id: 'new_item_version', + // parentID: 'new', + // active: false, + // visible: true, + // model: { + // type: MenuItemType.LINK, + // text: 'menu.section.new_item_version', + // link: '' + // } as LinkMenuItemModel, + // }, /* Statistics */ // TODO: enable this menu item once the feature has been implemented diff --git a/src/app/root/root.component.ts b/src/app/root/root.component.ts index 472ba440c9..c265cec034 100644 --- a/src/app/root/root.component.ts +++ b/src/app/root/root.component.ts @@ -61,7 +61,7 @@ export class RootComponent implements OnInit { } ngOnInit() { - this.sidebarVisible = this.menuService.isMenuVisible(MenuID.ADMIN); + this.sidebarVisible = this.menuService.isMenuVisibleWithVisibleSections(MenuID.ADMIN); this.collapsedSidebarWidth = this.cssService.getVariable('collapsedSidebarWidth'); this.totalSidebarWidth = this.cssService.getVariable('totalSidebarWidth'); diff --git a/src/app/shared/menu/menu.service.ts b/src/app/shared/menu/menu.service.ts index f44ddea649..ab513fbbdf 100644 --- a/src/app/shared/menu/menu.service.ts +++ b/src/app/shared/menu/menu.service.ts @@ -181,6 +181,18 @@ export class MenuService { ); } + /** + * Check if a given menu is visible and has visible sections + * @param {MenuID} menuID The ID of the menu that is to be checked + * @returns {Observable} Emits true if the given menu is + * visible and has visible sections, emits false when it's hidden + */ + isMenuVisibleWithVisibleSections(menuID: MenuID): Observable { + return observableCombineLatest([this.isMenuVisible(menuID), this.menuHasVisibleSections(menuID)]).pipe( + map(([menuVisible, visibleSections]) => menuVisible && visibleSections) + ); + } + /** * Check if a given menu is visible * @param {MenuID} menuID The ID of the menu that is to be checked @@ -192,6 +204,19 @@ export class MenuService { ); } + /** + * Check if a menu has at least one section that is visible. + * @param {MenuID} menuID The ID of the menu that is to be checked + * @returns {Observable} Emits true if the given menu has visible sections, emits false otherwise + */ + menuHasVisibleSections(menuID: MenuID): Observable { + return this.getMenu(menuID).pipe( + map((state: MenuState) => hasValue(state) + ? Object.values(state.sections).map(section => section.visible).reduce((x,y) => x || y, false) + : undefined) + ); + } + /** * Expands a given menu * @param {MenuID} menuID The ID of the menu From fa87b1d94e053082af61feddf225c267e00d94f4 Mon Sep 17 00:00:00 2001 From: Koen Pauwels Date: Tue, 6 Dec 2022 14:33:04 +0100 Subject: [PATCH 3/8] 97183 MenuResolver tests for regular user --- src/app/menu.resolver.spec.ts | 154 +++++++++++++++----- src/app/shared/testing/menu-service.stub.ts | 4 + 2 files changed, 122 insertions(+), 36 deletions(-) diff --git a/src/app/menu.resolver.spec.ts b/src/app/menu.resolver.spec.ts index 4fd44efe66..afe053d1d4 100644 --- a/src/app/menu.resolver.spec.ts +++ b/src/app/menu.resolver.spec.ts @@ -19,6 +19,7 @@ import { cold } from 'jasmine-marbles'; import createSpy = jasmine.createSpy; import { createSuccessfulRemoteDataObject$ } from './shared/remote-data.utils'; import { createPaginatedList } from './shared/testing/utils.test'; +import { Feature } from './core/shared/feature.model'; const BOOLEAN = { t: true, f: false }; const MENU_STATE = { @@ -142,29 +143,7 @@ describe('MenuResolver', () => { }); describe('createAdminMenu$', () => { - it('should retrieve the menu by ID return an Observable that emits true as soon as it is created', () => { - (menuService as any).getMenu.and.returnValue(cold('--u--m', { - u: undefined, - m: MENU_STATE, - })); - - expect(resolver.createAdminMenu$()).toBeObservable(cold('-----(t|)', BOOLEAN)); - expect(menuService.getMenu).toHaveBeenCalledOnceWith(MenuID.ADMIN); - }); - - describe('for regular user', () => { - beforeEach(() => { - authorizationService.isAuthorized = createSpy('isAuthorized').and.callFake(() => { - return observableOf(false); - }); - }); - - beforeEach((done) => { - resolver.createAdminMenu$().subscribe((_) => { - done(); - }); - }); - + const dontShowAdminSections = () => { it('should not show site admin section', () => { expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ id: 'admin_search', visible: false, @@ -183,19 +162,6 @@ describe('MenuResolver', () => { })); }); - it('should not show edit_community', () => { - expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ - id: 'edit_community', visible: false, - })); - - }); - - it('should not show edit_collection', () => { - expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ - id: 'edit_collection', visible: false, - })); - }); - it('should not show access control section', () => { expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ id: 'access_control', visible: false, @@ -222,6 +188,122 @@ describe('MenuResolver', () => { id: 'export', visible: true, })); }); + }; + + const dontShowNewSection = () => { + it('should not show the "New" section', () => { + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'new_community', visible: false, + })); + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'new_collection', visible: false, + })); + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'new_item', visible: false, + })); + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'new', visible: false, + })); + }); + }; + + const dontShowEditSection = () => { + it('should not show the "Edit" section', () => { + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'edit_community', visible: false, + })); + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'edit_collection', visible: false, + })); + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'edit_item', visible: false, + })); + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'edit', visible: false, + })); + }); + }; + + it('should retrieve the menu by ID return an Observable that emits true as soon as it is created', () => { + (menuService as any).getMenu.and.returnValue(cold('--u--m', { + u: undefined, + m: MENU_STATE, + })); + + expect(resolver.createAdminMenu$()).toBeObservable(cold('-----(t|)', BOOLEAN)); + expect(menuService.getMenu).toHaveBeenCalledOnceWith(MenuID.ADMIN); + }); + + describe('for regular user', () => { + beforeEach(() => { + authorizationService.isAuthorized = createSpy('isAuthorized').and.callFake((featureID) => { + return observableOf(false); + }); + }); + + beforeEach((done) => { + resolver.createAdminMenu$().subscribe((_) => { + done(); + }); + }); + + dontShowAdminSections(); + dontShowNewSection(); + dontShowEditSection(); + }); + + describe('regular user who can submit', () => { + beforeEach(() => { + authorizationService.isAuthorized = createSpy('isAuthorized') + .and.callFake((featureID: FeatureID) => { + return observableOf(featureID === FeatureID.CanSubmit); + }); + }); + + beforeEach((done) => { + resolver.createAdminMenu$().subscribe((_) => { + done(); + }); + }); + + it('should show "New Item" section', () => { + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'new_item', visible: true, + })); + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'new', visible: true, + })); + }); + + dontShowAdminSections(); + dontShowEditSection(); + }); + + describe('regular user who can edit items', () => { + beforeEach(() => { + authorizationService.isAuthorized = createSpy('isAuthorized') + .and.callFake((featureID: FeatureID) => { + return observableOf(featureID === FeatureID.CanEditItem); + }); + }); + + beforeEach((done) => { + resolver.createAdminMenu$().subscribe((_) => { + done(); + }); + }); + + it('should show "Edit Item" section', () => { + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'edit_item', visible: true, + })); + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'edit', visible: true, + })); + }); + + dontShowAdminSections(); + dontShowNewSection(); }); describe('for site admin', () => { diff --git a/src/app/shared/testing/menu-service.stub.ts b/src/app/shared/testing/menu-service.stub.ts index 926232bad0..71ee777157 100644 --- a/src/app/shared/testing/menu-service.stub.ts +++ b/src/app/shared/testing/menu-service.stub.ts @@ -66,6 +66,10 @@ export class MenuServiceStub { return observableOf(true); } + isMenuVisibleWithVisibleSections(id: MenuID): Observable { + return observableOf(true); + } + isMenuCollapsed(id: MenuID): Observable { return observableOf(false); } From 4f20d200cac0c117ef3f975adf7daee43c922227 Mon Sep 17 00:00:00 2001 From: Koen Pauwels Date: Tue, 6 Dec 2022 15:45:46 +0100 Subject: [PATCH 4/8] 97183 Tests for MenuService.isMenuVisibleWithVisibleSections --- src/app/shared/menu/menu.service.spec.ts | 78 ++++++++++++++++++++++++ src/app/shared/menu/menu.service.ts | 8 ++- 2 files changed, 83 insertions(+), 3 deletions(-) diff --git a/src/app/shared/menu/menu.service.spec.ts b/src/app/shared/menu/menu.service.spec.ts index 7a6b304ec1..26f32b64ef 100644 --- a/src/app/shared/menu/menu.service.spec.ts +++ b/src/app/shared/menu/menu.service.spec.ts @@ -243,6 +243,84 @@ describe('MenuService', () => { }); }); + describe('isMenuVisibleWithVisibleSections', () => { + it('should return false when the menu is empty', () => { + const fakeMenu = { + id: MenuID.ADMIN, + collapsed: false, + visible: true, + sections: {}, + previewCollapsed: false, + sectionToSubsectionIndex: {} + } as any; + spyOn(service, 'getMenu').and.returnValue(observableOf(fakeMenu)); + + const result = service.isMenuVisibleWithVisibleSections(MenuID.ADMIN); + const expected = cold('(b|)', { + b: false + }); + + expect(result).toBeObservable(expected); + }); + it('should return false when no top-level sections are visible', () => { + const noTopLevelVisibleSections = { + section: {id: 's1', visible: false}, + section_2: {id: 's2', visible: false}, + section_3: {id: 's3', visible: false}, + section_4: {id: 's1_1', visible: true, parentID: 's1'}, + section_5: {id: 's2_1', visible: true, parentID: 's2'}, + } + const fakeMenu = { + id: MenuID.ADMIN, + collapsed: false, + visible: true, + sections: noTopLevelVisibleSections, + previewCollapsed: false, + sectionToSubsectionIndex: { + 'section': ['section_4'], + 'section_2': ['section_5'], + } + } as any; + spyOn(service, 'getMenu').and.returnValue(observableOf(fakeMenu)); + + const result = service.isMenuVisibleWithVisibleSections(MenuID.ADMIN); + const expected = cold('(b|)', { + b: false + }); + + expect(result).toBeObservable(expected); + }); + + it('should return true when any top-level section is visible', () => { + const noTopLevelVisibleSections = { + section: {id: 's1', visible: false}, + section_2: {id: 's2', visible: true}, + section_3: {id: 's3', visible: false}, + section_4: {id: 's1_1', visible: true, parentID: 's1'}, + section_5: {id: 's2_1', visible: true, parentID: 's2'}, + } + const fakeMenu = { + id: MenuID.ADMIN, + collapsed: false, + visible: true, + sections: noTopLevelVisibleSections, + previewCollapsed: false, + sectionToSubsectionIndex: { + 'section': ['section_4'], + 'section_2': ['section_5'], + } + } as any; + spyOn(service, 'getMenu').and.returnValue(observableOf(fakeMenu)); + + const result = service.isMenuVisibleWithVisibleSections(MenuID.ADMIN); + const expected = cold('(b|)', { + b: true + }); + + expect(result).toBeObservable(expected); + }); + }); + describe('isMenuVisible', () => { beforeEach(() => { spyOn(service, 'getMenu').and.returnValue(observableOf(fakeMenu)); diff --git a/src/app/shared/menu/menu.service.ts b/src/app/shared/menu/menu.service.ts index ab513fbbdf..8e2cb8a894 100644 --- a/src/app/shared/menu/menu.service.ts +++ b/src/app/shared/menu/menu.service.ts @@ -182,7 +182,7 @@ export class MenuService { } /** - * Check if a given menu is visible and has visible sections + * Check if a given menu is visible and has visible top-level (!) sections * @param {MenuID} menuID The ID of the menu that is to be checked * @returns {Observable} Emits true if the given menu is * visible and has visible sections, emits false when it's hidden @@ -205,14 +205,16 @@ export class MenuService { } /** - * Check if a menu has at least one section that is visible. + * Check if a menu has at least one top-level (!) section that is visible. * @param {MenuID} menuID The ID of the menu that is to be checked * @returns {Observable} Emits true if the given menu has visible sections, emits false otherwise */ menuHasVisibleSections(menuID: MenuID): Observable { return this.getMenu(menuID).pipe( map((state: MenuState) => hasValue(state) - ? Object.values(state.sections).map(section => section.visible).reduce((x,y) => x || y, false) + ? Object.values(state.sections) + .map(section => section.visible && section.parentID === undefined) + .reduce((x,y) => x || y, false) : undefined) ); } From 92c7922fbea17a5656f1a31240340223d81ffaed Mon Sep 17 00:00:00 2001 From: Koen Pauwels Date: Tue, 6 Dec 2022 15:59:01 +0100 Subject: [PATCH 5/8] 97183 Fixed linting issues --- src/app/menu.resolver.ts | 4 ++-- src/app/shared/menu/menu.service.spec.ts | 16 ++++++++-------- src/app/shared/menu/menu.service.ts | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/app/menu.resolver.ts b/src/app/menu.resolver.ts index 19413ab4be..933014c93a 100644 --- a/src/app/menu.resolver.ts +++ b/src/app/menu.resolver.ts @@ -275,7 +275,7 @@ export class MenuResolver implements Resolve { } as TextMenuItemModel, icon: 'plus', index: 0 - } + }; const editSubMenu = { id: 'edit', active: false, @@ -286,7 +286,7 @@ export class MenuResolver implements Resolve { } as TextMenuItemModel, icon: 'pencil-alt', index: 1 - } + }; const menuList = [ ...newSubMenuList, diff --git a/src/app/shared/menu/menu.service.spec.ts b/src/app/shared/menu/menu.service.spec.ts index 26f32b64ef..589b5ebf6b 100644 --- a/src/app/shared/menu/menu.service.spec.ts +++ b/src/app/shared/menu/menu.service.spec.ts @@ -245,7 +245,7 @@ describe('MenuService', () => { describe('isMenuVisibleWithVisibleSections', () => { it('should return false when the menu is empty', () => { - const fakeMenu = { + const testMenu = { id: MenuID.ADMIN, collapsed: false, visible: true, @@ -253,7 +253,7 @@ describe('MenuService', () => { previewCollapsed: false, sectionToSubsectionIndex: {} } as any; - spyOn(service, 'getMenu').and.returnValue(observableOf(fakeMenu)); + spyOn(service, 'getMenu').and.returnValue(observableOf(testMenu)); const result = service.isMenuVisibleWithVisibleSections(MenuID.ADMIN); const expected = cold('(b|)', { @@ -269,8 +269,8 @@ describe('MenuService', () => { section_3: {id: 's3', visible: false}, section_4: {id: 's1_1', visible: true, parentID: 's1'}, section_5: {id: 's2_1', visible: true, parentID: 's2'}, - } - const fakeMenu = { + }; + const testMenu = { id: MenuID.ADMIN, collapsed: false, visible: true, @@ -281,7 +281,7 @@ describe('MenuService', () => { 'section_2': ['section_5'], } } as any; - spyOn(service, 'getMenu').and.returnValue(observableOf(fakeMenu)); + spyOn(service, 'getMenu').and.returnValue(observableOf(testMenu)); const result = service.isMenuVisibleWithVisibleSections(MenuID.ADMIN); const expected = cold('(b|)', { @@ -298,8 +298,8 @@ describe('MenuService', () => { section_3: {id: 's3', visible: false}, section_4: {id: 's1_1', visible: true, parentID: 's1'}, section_5: {id: 's2_1', visible: true, parentID: 's2'}, - } - const fakeMenu = { + }; + const testMenu = { id: MenuID.ADMIN, collapsed: false, visible: true, @@ -310,7 +310,7 @@ describe('MenuService', () => { 'section_2': ['section_5'], } } as any; - spyOn(service, 'getMenu').and.returnValue(observableOf(fakeMenu)); + spyOn(service, 'getMenu').and.returnValue(observableOf(testMenu)); const result = service.isMenuVisibleWithVisibleSections(MenuID.ADMIN); const expected = cold('(b|)', { diff --git a/src/app/shared/menu/menu.service.ts b/src/app/shared/menu/menu.service.ts index 8e2cb8a894..eeb6c0f2c2 100644 --- a/src/app/shared/menu/menu.service.ts +++ b/src/app/shared/menu/menu.service.ts @@ -208,7 +208,7 @@ export class MenuService { * Check if a menu has at least one top-level (!) section that is visible. * @param {MenuID} menuID The ID of the menu that is to be checked * @returns {Observable} Emits true if the given menu has visible sections, emits false otherwise - */ + */ menuHasVisibleSections(menuID: MenuID): Observable { return this.getMenu(menuID).pipe( map((state: MenuState) => hasValue(state) From 742b68b7060fd7aebc4acf25aba9a4c5d0d57dcd Mon Sep 17 00:00:00 2001 From: Koen Pauwels Date: Fri, 9 Dec 2022 14:52:41 +0100 Subject: [PATCH 6/8] 97183 Minor fixes (code review) --- src/app/menu.resolver.spec.ts | 6 ++++++ src/app/menu.resolver.ts | 6 +++--- src/app/shared/menu/menu.service.ts | 3 +-- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/app/menu.resolver.spec.ts b/src/app/menu.resolver.spec.ts index afe053d1d4..eb49fe1a11 100644 --- a/src/app/menu.resolver.spec.ts +++ b/src/app/menu.resolver.spec.ts @@ -319,6 +319,12 @@ describe('MenuResolver', () => { }); }); + it('should show new_process', () => { + expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ + id: 'new_process', visible: true, + })) + }); + it('should contain site admin section', () => { expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ id: 'admin_search', visible: true, diff --git a/src/app/menu.resolver.ts b/src/app/menu.resolver.ts index 933014c93a..eb13e3ec8b 100644 --- a/src/app/menu.resolver.ts +++ b/src/app/menu.resolver.ts @@ -215,7 +215,7 @@ export class MenuResolver implements Resolve { id: 'new_process', parentID: 'new', active: false, - visible: isCollectionAdmin, + visible: isSiteAdmin, model: { type: MenuItemType.LINK, text: 'menu.section.new_process', @@ -268,7 +268,7 @@ export class MenuResolver implements Resolve { const newSubMenu = { id: 'new', active: false, - visible: newSubMenuList.map(subMenu => subMenu.visible).reduce((x,y) => x || y), + visible: newSubMenuList.some(subMenu => subMenu.visible), model: { type: MenuItemType.TEXT, text: 'menu.section.new' @@ -279,7 +279,7 @@ export class MenuResolver implements Resolve { const editSubMenu = { id: 'edit', active: false, - visible: editSubMenuList.map(subMenu => subMenu.visible).reduce((x,y) => x || y), + visible: editSubMenuList.some(subMenu => subMenu.visible), model: { type: MenuItemType.TEXT, text: 'menu.section.edit' diff --git a/src/app/shared/menu/menu.service.ts b/src/app/shared/menu/menu.service.ts index eeb6c0f2c2..087145ae82 100644 --- a/src/app/shared/menu/menu.service.ts +++ b/src/app/shared/menu/menu.service.ts @@ -213,8 +213,7 @@ export class MenuService { return this.getMenu(menuID).pipe( map((state: MenuState) => hasValue(state) ? Object.values(state.sections) - .map(section => section.visible && section.parentID === undefined) - .reduce((x,y) => x || y, false) + .some(section => section.visible && section.parentID === undefined) : undefined) ); } From 69bd324da52c014148f49947e1a91606556c0f7f Mon Sep 17 00:00:00 2001 From: Koen Pauwels Date: Mon, 12 Dec 2022 10:01:04 +0100 Subject: [PATCH 7/8] 97183 Fix linter issue --- src/app/menu.resolver.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/app/menu.resolver.spec.ts b/src/app/menu.resolver.spec.ts index eb49fe1a11..c647178c88 100644 --- a/src/app/menu.resolver.spec.ts +++ b/src/app/menu.resolver.spec.ts @@ -322,7 +322,7 @@ describe('MenuResolver', () => { it('should show new_process', () => { expect(menuService.addSection).toHaveBeenCalledWith(MenuID.ADMIN, jasmine.objectContaining({ id: 'new_process', visible: true, - })) + })); }); it('should contain site admin section', () => { From e31781b5d054961aa86d564430ba3731b7913eb6 Mon Sep 17 00:00:00 2001 From: Koen Pauwels Date: Mon, 12 Dec 2022 10:07:00 +0100 Subject: [PATCH 8/8] 97183 Fix linter issue, again --- src/app/menu.resolver.spec.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/app/menu.resolver.spec.ts b/src/app/menu.resolver.spec.ts index c647178c88..eef5c2d5af 100644 --- a/src/app/menu.resolver.spec.ts +++ b/src/app/menu.resolver.spec.ts @@ -19,7 +19,6 @@ import { cold } from 'jasmine-marbles'; import createSpy = jasmine.createSpy; import { createSuccessfulRemoteDataObject$ } from './shared/remote-data.utils'; import { createPaginatedList } from './shared/testing/utils.test'; -import { Feature } from './core/shared/feature.model'; const BOOLEAN = { t: true, f: false }; const MENU_STATE = {