From 2828e00c4ed2ea26a26e9145cc27891e317d2a70 Mon Sep 17 00:00:00 2001 From: tuutti Date: Thu, 1 Jun 2023 08:30:36 +0300 Subject: [PATCH 01/14] UHF-8522: Check entity view access for each menu link --- helfi_navigation.services.yml | 2 +- src/Menu/MenuTreeBuilder.php | 89 ++++++++++++++++++++++---------- src/Menu/MenuTreeManipulator.php | 13 ----- 3 files changed, 62 insertions(+), 42 deletions(-) delete mode 100644 src/Menu/MenuTreeManipulator.php diff --git a/helfi_navigation.services.yml b/helfi_navigation.services.yml index 3c0591a..9449a27 100644 --- a/helfi_navigation.services.yml +++ b/helfi_navigation.services.yml @@ -34,7 +34,7 @@ services: helfi_navigation.anonymous_user: class: Drupal\Core\Session\AnonymousUserSession helfi_navigation.menu_tree_manipulators: - class: Drupal\helfi_navigation\Menu\MenuTreeManipulator + class: Drupal\Core\Menu\DefaultMenuTreeManipulator arguments: - '@access_manager' - '@helfi_navigation.anonymous_user' diff --git a/src/Menu/MenuTreeBuilder.php b/src/Menu/MenuTreeBuilder.php index 0c07c7a..b505800 100644 --- a/src/Menu/MenuTreeBuilder.php +++ b/src/Menu/MenuTreeBuilder.php @@ -4,12 +4,15 @@ namespace Drupal\helfi_navigation\Menu; +use Drupal\Core\Access\AccessResult; use Drupal\Core\Access\AccessResultInterface; use Drupal\Core\Entity\EntityTypeManagerInterface; use Drupal\Core\Menu\MenuLinkInterface; use Drupal\Core\Menu\MenuLinkManagerInterface; +use Drupal\Core\Menu\MenuLinkTreeElement; use Drupal\Core\Menu\MenuLinkTreeInterface; use Drupal\Core\Menu\MenuTreeParameters; +use Drupal\Core\Session\AnonymousUserSession; use Drupal\helfi_api_base\Link\InternalDomainResolver; use Drupal\helfi_navigation\Event\MenuTreeBuilderLink; use Drupal\menu_link_content\MenuLinkContentInterface; @@ -35,11 +38,11 @@ final class MenuTreeBuilder { * The event dispatcher. */ public function __construct( - private EntityTypeManagerInterface $entityTypeManager, - private InternalDomainResolver $domainResolver, - private MenuLinkTreeInterface $menuTree, - private MenuLinkManagerInterface $menuLinkManager, - private EventDispatcherInterface $eventDispatcher + private readonly EntityTypeManagerInterface $entityTypeManager, + private readonly InternalDomainResolver $domainResolver, + private readonly MenuLinkTreeInterface $menuTree, + private readonly MenuLinkManagerInterface $menuLinkManager, + private readonly EventDispatcherInterface $eventDispatcher ) { } @@ -122,26 +125,13 @@ private function transform(array $menuItems, string $langcode, string $rootId = foreach ($menuItems as $element) { /** @var \Drupal\menu_link_content\MenuLinkContentInterface $link */ // @todo Do we want to show links other than MenuLinkContent? - if (!$link = $this->getEntity($element->link)) { + if (!$link = $this->getEntity($element->link, $langcode)) { continue; } + $this->evaluateEntityAccess($element); - // Handle only menu links with translations. - if ( - !$link->hasTranslation($langcode) || - !$link->isTranslatable() - ) { - continue; - } - - /** @var \Drupal\menu_link_content\MenuLinkContentInterface $link */ - $link = $link->getTranslation($langcode); - - // Only show accessible links (and published). - if ( - !$link->get('content_translation_status')->value || - ($element->access instanceof AccessResultInterface && !$element->access->isAllowed()) - ) { + // Only show accessible links. + if ($element->access instanceof AccessResultInterface && !$element->access->isAllowed()) { continue; } @@ -237,25 +227,68 @@ private function processItem(MenuTreeBuilderLink $link) : array { * * @param \Drupal\Core\Menu\MenuLinkInterface $link * The menu link. + * @param string $langcode + * The langcode. * * @return \Drupal\menu_link_content\MenuLinkContentInterface|null - * NULL if entity not found and - * a MenuLinkContentInterface if found. + * NULL if a valid entity is not found and a MenuLinkContentInterface + * if found. * * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException */ - private function getEntity(MenuLinkInterface $link): ? MenuLinkContentInterface { - // MenuLinkContent::getEntity() has protected visibility and cannot be used - // to directly fetch the entity. + private function getEntity(MenuLinkInterface $link, string $langcode): ? MenuLinkContentInterface { $metadata = $link->getMetaData(); if (empty($metadata['entity_id'])) { return NULL; } - return $this->entityTypeManager + /** @var \Drupal\menu_link_content\Entity\MenuLinkContent $entity */ + $entity = $this->entityTypeManager ->getStorage('menu_link_content') ->load($metadata['entity_id']); + + if (!$entity || !$entity->isTranslatable() || !$entity->hasTranslation($langcode)) { + return NULL; + } + $entity = $entity->getTranslation($langcode); + + if (!$entity->hasField('content_translation_status') || !$entity->get('content_translation_status')->value) { + return NULL; + } + + return $entity; + + } + + /** + * Evaluates the access for link's target. + * + * @param \Drupal\Core\Menu\MenuLinkTreeElement $element + * The element to check entity access for. + */ + private function evaluateEntityAccess(MenuLinkTreeElement $element) : void { + // Attempt to fetch the entity type and id from link's route parameters. + // The route parameters should be an array containing entity type => id + // like: ['node' => '1']. + $routeParameters = $element->link->getRouteParameters(); + $entityType = key($routeParameters); + + if (!$this->entityTypeManager->hasDefinition($entityType)) { + return; + } + $storage = $this->entityTypeManager + ->getStorage($entityType); + + if (!$entity = $storage->load($routeParameters[$entityType])) { + return; + } + + // Disallow access if the anonymous user has no access to + // the target entity. + if (!$entity->access('view', new AnonymousUserSession())) { + $element->access = AccessResult::neutral(); + } } } diff --git a/src/Menu/MenuTreeManipulator.php b/src/Menu/MenuTreeManipulator.php deleted file mode 100644 index e462cca..0000000 --- a/src/Menu/MenuTreeManipulator.php +++ /dev/null @@ -1,13 +0,0 @@ - Date: Thu, 1 Jun 2023 08:35:09 +0300 Subject: [PATCH 02/14] UHF-8522: Fixed class name --- helfi_navigation.services.yml | 2 +- src/Menu/MenuTreeBuilder.php | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/helfi_navigation.services.yml b/helfi_navigation.services.yml index 9449a27..3450787 100644 --- a/helfi_navigation.services.yml +++ b/helfi_navigation.services.yml @@ -34,7 +34,7 @@ services: helfi_navigation.anonymous_user: class: Drupal\Core\Session\AnonymousUserSession helfi_navigation.menu_tree_manipulators: - class: Drupal\Core\Menu\DefaultMenuTreeManipulator + class: Drupal\Core\Menu\DefaultMenuLinkTreeManipulators arguments: - '@access_manager' - '@helfi_navigation.anonymous_user' diff --git a/src/Menu/MenuTreeBuilder.php b/src/Menu/MenuTreeBuilder.php index b505800..38b4662 100644 --- a/src/Menu/MenuTreeBuilder.php +++ b/src/Menu/MenuTreeBuilder.php @@ -223,7 +223,7 @@ private function processItem(MenuTreeBuilderLink $link) : array { } /** - * Load entity with given menu link. + * Load MenuLinkContent entity for given menu link. * * @param \Drupal\Core\Menu\MenuLinkInterface $link * The menu link. @@ -253,7 +253,8 @@ private function getEntity(MenuLinkInterface $link, string $langcode): ? MenuLin } $entity = $entity->getTranslation($langcode); - if (!$entity->hasField('content_translation_status') || !$entity->get('content_translation_status')->value) { + // Skip unpublished translations. + if (!$entity->get('content_translation_status')->value) { return NULL; } From f0e3acab75145e146b6e32ad1ee187f62230bc9e Mon Sep 17 00:00:00 2001 From: tuutti Date: Thu, 1 Jun 2023 09:27:12 +0300 Subject: [PATCH 03/14] UHF-8522: Fixed comment --- src/EventSubscriber/RedirectEventSubscriber.php | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/EventSubscriber/RedirectEventSubscriber.php b/src/EventSubscriber/RedirectEventSubscriber.php index 264e5e9..0262442 100644 --- a/src/EventSubscriber/RedirectEventSubscriber.php +++ b/src/EventSubscriber/RedirectEventSubscriber.php @@ -24,8 +24,8 @@ final class RedirectEventSubscriber implements EventSubscriberInterface { * The config factory service. */ public function __construct( - private RedirectRepository $repository, - private ConfigFactoryInterface $configFactory, + private readonly RedirectRepository $repository, + private readonly ConfigFactoryInterface $configFactory, ) { } @@ -35,7 +35,10 @@ public function __construct( * Gets all available redirects for given link and updates the URL * to use the redirect destination. * - * This is required by javascript navigation to build the active trail. + * Mobile navigation uses path to figure out the active trail, something + * like "if (path in menu link === current url path in browser)". Override + * the menu link path to match the path where user actually lands after + * clicking the link. * * @param \Drupal\helfi_navigation\Event\MenuTreeBuilderLink $event * The event to respond to. From 6f19a561d4712af43b829a9c9c63fdf4b7af3dcd Mon Sep 17 00:00:00 2001 From: tuutti Date: Thu, 1 Jun 2023 10:26:47 +0300 Subject: [PATCH 04/14] UHF-8522: Use account switcher to set anonymous user rather than trying to manually set it all around the places, removed unused services --- helfi_navigation.services.yml | 9 +------- src/MainMenuManager.php | 22 +++++++++++++------ src/Menu/MenuTreeBuilder.php | 13 +++++------ src/Plugin/rest/resource/GlobalMobileMenu.php | 2 +- tests/src/Kernel/MenuTreeBuilderTest.php | 12 ++++++++++ tests/src/Kernel/MenuTreeBuilderTestBase.php | 1 + tests/src/Traits/MenuLinkTrait.php | 8 +++++++ 7 files changed, 44 insertions(+), 23 deletions(-) diff --git a/helfi_navigation.services.yml b/helfi_navigation.services.yml index 3450787..afe538f 100644 --- a/helfi_navigation.services.yml +++ b/helfi_navigation.services.yml @@ -23,6 +23,7 @@ services: - '@config.factory' - '@helfi_navigation.api_manager' - '@helfi_navigation.menu_tree_builder' + - '@account_switcher' helfi_navigation.menu_tree_builder: class: Drupal\helfi_navigation\Menu\MenuTreeBuilder arguments: @@ -31,14 +32,6 @@ services: - '@menu.link_tree' - '@plugin.manager.menu.link' - '@event_dispatcher' - helfi_navigation.anonymous_user: - class: Drupal\Core\Session\AnonymousUserSession - helfi_navigation.menu_tree_manipulators: - class: Drupal\Core\Menu\DefaultMenuLinkTreeManipulators - arguments: - - '@access_manager' - - '@helfi_navigation.anonymous_user' - - '@entity_type.manager' helfi_navigation.cache_warmer: class: Drupal\helfi_navigation\CacheWarmer arguments: diff --git a/src/MainMenuManager.php b/src/MainMenuManager.php index 46b2e33..5e25cde 100644 --- a/src/MainMenuManager.php +++ b/src/MainMenuManager.php @@ -5,7 +5,8 @@ namespace Drupal\helfi_navigation; use Drupal\Core\Config\ConfigFactoryInterface; -use Drupal\Core\Language\LanguageInterface; +use Drupal\Core\Session\AccountSwitcherInterface; +use Drupal\Core\Session\AnonymousUserSession; use Drupal\Core\Url; use Drupal\helfi_navigation\Menu\MenuTreeBuilder; use Drupal\language\ConfigurableLanguageManagerInterface; @@ -26,12 +27,15 @@ class MainMenuManager { * The api manager. * @param \Drupal\helfi_navigation\Menu\MenuTreeBuilder $menuTreeBuilder * The menu tree builder. + * @param \Drupal\Core\Session\AccountSwitcherInterface $accountSwitcher + * The account switcher service. */ public function __construct( - private ConfigurableLanguageManagerInterface $languageManager, - private ConfigFactoryInterface $config, - private ApiManager $apiManager, - private MenuTreeBuilder $menuTreeBuilder, + private readonly ConfigurableLanguageManagerInterface $languageManager, + private readonly ConfigFactoryInterface $config, + private readonly ApiManager $apiManager, + private readonly MenuTreeBuilder $menuTreeBuilder, + private readonly AccountSwitcherInterface $accountSwitcher, ) { } @@ -46,6 +50,9 @@ public function __construct( * @throws \GuzzleHttp\Exception\GuzzleException */ public function sync(string $langcode): bool { + // Sync menu as an anonymous user to make sure no sensitive + // links are synced. + $this->accountSwitcher->switchTo(new AnonymousUserSession()); $response = $this->apiManager->update( $langcode, [ @@ -54,6 +61,8 @@ public function sync(string $langcode): bool { 'menu_tree' => $this->build($langcode), ] ); + $this->accountSwitcher->switchBack(); + if (!isset($response->data->status)) { throw new \InvalidArgumentException('Failed to parse entity published state.'); } @@ -96,8 +105,7 @@ public function getSiteName(string $langcode) : string { * @return mixed * Menu tree. */ - public function build(string $langcode = NULL): array { - $langcode = $langcode ?: $this->languageManager->getCurrentLanguage(LanguageInterface::TYPE_INTERFACE)->getId(); + public function build(string $langcode): array { $siteName = $this->getSiteName($langcode); $instanceUri = Url::fromRoute('', options: [ diff --git a/src/Menu/MenuTreeBuilder.php b/src/Menu/MenuTreeBuilder.php index 38b4662..3f5e31f 100644 --- a/src/Menu/MenuTreeBuilder.php +++ b/src/Menu/MenuTreeBuilder.php @@ -12,7 +12,6 @@ use Drupal\Core\Menu\MenuLinkTreeElement; use Drupal\Core\Menu\MenuLinkTreeInterface; use Drupal\Core\Menu\MenuTreeParameters; -use Drupal\Core\Session\AnonymousUserSession; use Drupal\helfi_api_base\Link\InternalDomainResolver; use Drupal\helfi_navigation\Event\MenuTreeBuilderLink; use Drupal\menu_link_content\MenuLinkContentInterface; @@ -79,7 +78,8 @@ public function build( $tree = $this->menuTree->transform($tree, [ // Sync menu links accessible to anonymous users and sort them // the same way core does. - ['callable' => 'helfi_navigation.menu_tree_manipulators:checkAccess'], + ['callable' => 'menu.default_tree_manipulators:checkNodeAccess'], + ['callable' => 'menu.default_tree_manipulators:checkAccess'], ['callable' => 'menu.default_tree_manipulators:generateIndexAndSort'], ]); @@ -284,11 +284,10 @@ private function evaluateEntityAccess(MenuLinkTreeElement $element) : void { if (!$entity = $storage->load($routeParameters[$entityType])) { return; } - - // Disallow access if the anonymous user has no access to - // the target entity. - if (!$entity->access('view', new AnonymousUserSession())) { - $element->access = AccessResult::neutral(); + // Disallow access if user has no access to the target entity. + if (!$entity->access('view')) { + $element->access = AccessResult::neutral() + ->addCacheableDependency($entity); } } diff --git a/src/Plugin/rest/resource/GlobalMobileMenu.php b/src/Plugin/rest/resource/GlobalMobileMenu.php index 346a5a0..366095b 100644 --- a/src/Plugin/rest/resource/GlobalMobileMenu.php +++ b/src/Plugin/rest/resource/GlobalMobileMenu.php @@ -109,7 +109,7 @@ public function get(): ResourceResponse { $project_name = $environment->getId(); // Create menu tree and add data to the local menu. - $menuTree = $this->menuManager->build(); + $menuTree = $this->menuManager->build($langcode); $menuTree['is_injected'] = TRUE; // Commented lines are present in the api request, diff --git a/tests/src/Kernel/MenuTreeBuilderTest.php b/tests/src/Kernel/MenuTreeBuilderTest.php index 6e92e02..0d88f63 100644 --- a/tests/src/Kernel/MenuTreeBuilderTest.php +++ b/tests/src/Kernel/MenuTreeBuilderTest.php @@ -127,6 +127,18 @@ public function testBuildMenuTree() : void { ->save(); $tree = $this->getMenuTree('fi'); $this->assertCount(1, $tree['sub_tree']); + + // Unpublish all nodes and make sure they are not visible anymore. + foreach ($this->nodes as $node) { + $node->setUnpublished() + ->save(); + } + // Rebuild the container to empty static entity cache. + $this->container->get('kernel')->rebuildContainer(); + + // Make sure no links are visible after the node was unpublished. + $tree = $this->getMenuTree('fi'); + $this->assertCount(0, $tree['sub_tree']); } } diff --git a/tests/src/Kernel/MenuTreeBuilderTestBase.php b/tests/src/Kernel/MenuTreeBuilderTestBase.php index 4d74607..857f076 100644 --- a/tests/src/Kernel/MenuTreeBuilderTestBase.php +++ b/tests/src/Kernel/MenuTreeBuilderTestBase.php @@ -30,6 +30,7 @@ protected function setUp(): void { parent::setUp(); $this->installEntitySchema('node'); + $this->installSchema('node', ['node_access']); // We have a dependency to anonymous user when checking menu permissions // and might run into 'entity:user' context is required error when trying diff --git a/tests/src/Traits/MenuLinkTrait.php b/tests/src/Traits/MenuLinkTrait.php index 69d719f..7699fba 100644 --- a/tests/src/Traits/MenuLinkTrait.php +++ b/tests/src/Traits/MenuLinkTrait.php @@ -15,6 +15,13 @@ */ trait MenuLinkTrait { + /** + * An array of nodes. + * + * @var \Drupal\node\NodeInterface[] + */ + protected array $nodes = []; + /** * Creates a new test node. * @@ -31,6 +38,7 @@ protected function createNodeWithAlias() : NodeInterface { 'path' => ['alias' => '/test-node-page', 'langcode' => 'en'], ]); $node->save(); + $this->nodes[] = $node; return $node; } From 46d828d614911c7f4c6ff514aa9920f7c8352c47 Mon Sep 17 00:00:00 2001 From: tuutti Date: Thu, 1 Jun 2023 10:27:34 +0300 Subject: [PATCH 05/14] UHF-8522: Removed unused variable --- helfi_navigation.module | 4 ---- 1 file changed, 4 deletions(-) diff --git a/helfi_navigation.module b/helfi_navigation.module index 1e05453..9a3b793 100644 --- a/helfi_navigation.module +++ b/helfi_navigation.module @@ -156,10 +156,6 @@ function helfi_navigation_cron() : void { * Implements hook_page_attachments(). */ function helfi_navigation_page_attachments(array &$attachments) : void { - $langcode = Drupal::languageManager() - ->getCurrentLanguage(LanguageInterface::TYPE_CONTENT) - ->getId(); - /** @var \Drupal\helfi_api_base\Language\DefaultLanguageResolver $defaultLanguageResolver */ $defaultLanguageResolver = Drupal::service('helfi_api_base.default_language_resolver'); From 041533f803da40919269d7d2525a953beabc9adf Mon Sep 17 00:00:00 2001 From: tuutti Date: Thu, 1 Jun 2023 10:30:02 +0300 Subject: [PATCH 06/14] UHF-8522: phpcs fixes --- helfi_navigation.module | 1 - 1 file changed, 1 deletion(-) diff --git a/helfi_navigation.module b/helfi_navigation.module index 9a3b793..988d3a9 100644 --- a/helfi_navigation.module +++ b/helfi_navigation.module @@ -10,7 +10,6 @@ declare(strict_types = 1); use Drupal\Core\Entity\EntityTypeInterface; use Drupal\Core\Field\BaseFieldDefinition; use Drupal\Core\Form\FormStateInterface; -use Drupal\Core\Language\LanguageInterface; use Drupal\Core\StringTranslation\TranslatableMarkup; use Drupal\menu_link_content\Entity\MenuLinkContent; use Drupal\menu_link_content\MenuLinkContentInterface; From 11ff86bef0b09223ee62cb719d6429ac76bb9ca6 Mon Sep 17 00:00:00 2001 From: tuutti Date: Thu, 1 Jun 2023 10:54:08 +0300 Subject: [PATCH 07/14] UHF-8522: Added comment --- src/Menu/MenuTreeBuilder.php | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Menu/MenuTreeBuilder.php b/src/Menu/MenuTreeBuilder.php index 3f5e31f..faeb81c 100644 --- a/src/Menu/MenuTreeBuilder.php +++ b/src/Menu/MenuTreeBuilder.php @@ -124,7 +124,6 @@ private function transform(array $menuItems, string $langcode, string $rootId = foreach ($menuItems as $element) { /** @var \Drupal\menu_link_content\MenuLinkContentInterface $link */ - // @todo Do we want to show links other than MenuLinkContent? if (!$link = $this->getEntity($element->link, $langcode)) { continue; } @@ -248,6 +247,7 @@ private function getEntity(MenuLinkInterface $link, string $langcode): ? MenuLin ->getStorage('menu_link_content') ->load($metadata['entity_id']); + // Skip untranslatable/untranslated menu links. if (!$entity || !$entity->isTranslatable() || !$entity->hasTranslation($langcode)) { return NULL; } @@ -284,8 +284,10 @@ private function evaluateEntityAccess(MenuLinkTreeElement $element) : void { if (!$entity = $storage->load($routeParameters[$entityType])) { return; } - // Disallow access if user has no access to the target entity. if (!$entity->access('view')) { + // Disallow access if user has no view access to the target entity. + // This updates the existing access result and will be evaluated in + // ::transform(). $element->access = AccessResult::neutral() ->addCacheableDependency($entity); } From 90c30c88280320b4d908ad58b154970cd3808469 Mon Sep 17 00:00:00 2001 From: tuutti Date: Fri, 2 Jun 2023 06:16:32 +0300 Subject: [PATCH 08/14] UHF-8522: Added test for external menu tree builder --- src/ExternalMenuTreeBuilder.php | 4 +- .../Kernel/ExternalMenuTreeBuilderTest.php | 92 +++++++++++++++++++ tests/src/Traits/MenuLinkTrait.php | 11 +++ tests/src/Unit/MenuTreeBuilderTest.php | 3 + 4 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 tests/src/Kernel/ExternalMenuTreeBuilderTest.php diff --git a/src/ExternalMenuTreeBuilder.php b/src/ExternalMenuTreeBuilder.php index aaff972..708356c 100644 --- a/src/ExternalMenuTreeBuilder.php +++ b/src/ExternalMenuTreeBuilder.php @@ -32,8 +32,8 @@ final class ExternalMenuTreeBuilder { * The request stack. */ public function __construct( - private InternalDomainResolver $domainResolver, - private RequestStack $requestStack, + private readonly InternalDomainResolver $domainResolver, + private readonly RequestStack $requestStack, ) { } diff --git a/tests/src/Kernel/ExternalMenuTreeBuilderTest.php b/tests/src/Kernel/ExternalMenuTreeBuilderTest.php new file mode 100644 index 0000000..ee8f85e --- /dev/null +++ b/tests/src/Kernel/ExternalMenuTreeBuilderTest.php @@ -0,0 +1,92 @@ +createLinks(); + + $options = [ + 'level' => 0, + 'menu_type' => 'main', + 'expand_all_items' => FALSE, + 'max_depth' => 10, + ]; + $tree = $this->getMenuTree('en'); + // Convert to stdClass. + $tree = json_decode(json_encode($tree)); + + $externalMenuTreeBuilder = new ExternalMenuTreeBuilder( + $this->container->get('helfi_api_base.internal_domain_resolver'), + $requestStack->reveal(), + ); + return $externalMenuTreeBuilder->build($tree->sub_tree, $options); + } + + /** + * Make sure trying to render menu tree without active request will fail. + */ + public function testInvalidRequestException() : void { + $this->expectException(\LogicException::class); + + $requestStack = $this->prophesize(RequestStack::class); + $requestStack->getCurrentRequest()->willReturn(NULL); + $this->getExternalMenuTree($requestStack); + } + + /** + * Tests external menu tree build. + * + * @covers ::__construct + * @covers ::build + * @covers ::transform + * @covers ::updateActiveTrail + * @covers ::createLink + * @covers ::inActiveTrail + */ + public function testBuild() : void { + $request = $this->prophesize(Request::class); + $request->getUri()->willReturn('http://localhost/test'); + $requestStack = $this->prophesize(RequestStack::class); + $requestStack->getCurrentRequest()->willReturn($request->reveal()); + + $externalTree = $this->getExternalMenuTree($requestStack); + // Make sure parent is marked in active trail as well when the child item + // is the currently active page. + $this->assertTrue($externalTree[0]['in_active_trail']); + $this->assertFalse($externalTree[0]['is_currentPage']); + $this->assertFalse($externalTree[0]['external']); + $this->assertTrue($externalTree[0]['below'][0]['in_active_trail']); + $this->assertTrue($externalTree[0]['below'][0]['is_currentPage']); + + // Make sure links get marked as external correctly. + $this->assertTrue($externalTree[1]['below'][0]['below'][0]['external']); + } + +} diff --git a/tests/src/Traits/MenuLinkTrait.php b/tests/src/Traits/MenuLinkTrait.php index 7699fba..93b03fa 100644 --- a/tests/src/Traits/MenuLinkTrait.php +++ b/tests/src/Traits/MenuLinkTrait.php @@ -80,6 +80,8 @@ protected function createLinks() : array { 'langcode' => 'en', 'link' => ['uri' => 'internal:/admin/content'], 'expanded' => TRUE, + 'weight' => 0, + 'description' => 'Link 1', ], [ 'uuid' => 'de6409aa-c620-4327-90f4-127176f209b2', @@ -87,6 +89,7 @@ protected function createLinks() : array { 'title' => 'Link 1 depth 1', 'langcode' => 'en', 'link' => ['uri' => 'internal:/test'], + 'weight' => 0, ], [ 'uuid' => '79736feb-77b7-4981-821b-90f02814e5b5', @@ -94,18 +97,21 @@ protected function createLinks() : array { 'langcode' => 'en', 'enabled' => FALSE, 'link' => ['uri' => 'internal:/test'], + 'weight' => 1, ], [ 'uuid' => '520df961-36dc-4fd4-8cff-3d3acaa9f0a8', 'langcode' => 'fi', 'title' => 'Link 3', 'link' => ['uri' => 'entity:node/1'], + 'weight' => 2, ], [ 'uuid' => '3fd92b84-6b9c-4970-934e-6b4468b618c0', 'langcode' => 'en', 'title' => 'Link 4', 'link' => ['uri' => 'entity:node/1'], + 'weight' => 3, ], [ 'uuid' => 'b108dc3c-3a22-455f-90a7-238331bc2bfe', @@ -113,6 +119,7 @@ protected function createLinks() : array { 'title' => 'Link 4 - Child 1', 'parent' => 'menu_link_content:3fd92b84-6b9c-4970-934e-6b4468b618c0', 'link' => ['uri' => 'internal:/test'], + 'weight' => 0, ], [ 'uuid' => '64a5a6d1-ffce-481b-b321-260d9cf66ad9', @@ -120,6 +127,7 @@ protected function createLinks() : array { 'title' => 'Link 5 internal', 'link' => ['https://www.hel.fi/helsinki/test'], 'lang_attribute' => 'en-US', + 'weight' => 4, ], [ 'uuid' => '0d8a1366-4fcd-4dbc-bb75-854dedf28a1b', @@ -127,6 +135,7 @@ protected function createLinks() : array { 'title' => 'Link 5 internal - Child 1', 'parent' => 'menu_link_content:64a5a6d1-ffce-481b-b321-260d9cf66ad9', 'link' => ['https://www.hel.fi/helsinki/2'], + 'weight' => 0, ], [ 'uuid' => '0b10ba16-e2d5-4251-ac37-8ed27a02ff1f', @@ -134,6 +143,7 @@ protected function createLinks() : array { 'title' => 'Link 5 internal - Child 1 - Child 1', 'parent' => 'menu_link_content:0d8a1366-4fcd-4dbc-bb75-854dedf28a1b', 'link' => ['tel:+358040123'], + 'weight' => 0, ], [ 'uuid' => '263464a3-bd76-4a4e-a77f-03ab4960ff1e', @@ -141,6 +151,7 @@ protected function createLinks() : array { 'title' => 'Link 6', 'link' => ['uri' => 'route:'], 'lang_attribute' => 'en-GB', + 'weight' => 5, ], ]; diff --git a/tests/src/Unit/MenuTreeBuilderTest.php b/tests/src/Unit/MenuTreeBuilderTest.php index 71186fc..b85ae85 100644 --- a/tests/src/Unit/MenuTreeBuilderTest.php +++ b/tests/src/Unit/MenuTreeBuilderTest.php @@ -11,6 +11,7 @@ use Drupal\helfi_navigation\Menu\MenuTreeBuilder; use Drupal\Tests\UnitTestCase; use Prophecy\Argument; +use Prophecy\PhpUnit\ProphecyTrait; use Symfony\Component\EventDispatcher\EventDispatcherInterface; /** @@ -19,6 +20,8 @@ */ class MenuTreeBuilderTest extends UnitTestCase { + use ProphecyTrait; + /** * Tests root element validation. * From 50c44bb693f6fc53f8616deda2f3abf7474b2a29 Mon Sep 17 00:00:00 2001 From: tuutti Date: Fri, 2 Jun 2023 06:28:44 +0300 Subject: [PATCH 09/14] UHF-8522: Always check if item is external, removed unused description since the original API does not have the field --- src/ExternalMenuTreeBuilder.php | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/ExternalMenuTreeBuilder.php b/src/ExternalMenuTreeBuilder.php index 708356c..1208b43 100644 --- a/src/ExternalMenuTreeBuilder.php +++ b/src/ExternalMenuTreeBuilder.php @@ -145,14 +145,7 @@ private function createLink( if (!isset($item->parentId)) { $item->parentId = NULL; } - - if (!isset($item->external)) { - $item->external = $this->domainResolver->isExternal($item->url); - } - - if (isset($item->description)) { - $link_definition['description'] = $item->description; - } + $item->external = $this->domainResolver->isExternal($item->url); if (isset($item->weight)) { $link_definition['weight'] = $item->weight; From 5346ae1b5e90ec70b7014aa9cde5f8a41a67372c Mon Sep 17 00:00:00 2001 From: tuutti Date: Fri, 2 Jun 2023 09:27:43 +0300 Subject: [PATCH 10/14] UHF-8522: Added test for GlobalMobileMenu endpoint --- src/ApiManager.php | 38 ++++-- src/MainMenuManager.php | 2 + src/Plugin/rest/resource/GlobalMobileMenu.php | 79 +++++++----- tests/src/Functional/MenuBlockTest.php | 10 +- .../FunctionalJavascript/JsSettingsTest.php | 10 +- tests/src/Kernel/CacheWarmerTest.php | 10 +- .../rest/resource/GlobalMobileMenuTest.php | 120 ++++++++++++++++++ 7 files changed, 208 insertions(+), 61 deletions(-) create mode 100644 tests/src/Kernel/Plugin/rest/resource/GlobalMobileMenuTest.php diff --git a/src/ApiManager.php b/src/ApiManager.php index f5ab11d..3a41f9d 100644 --- a/src/ApiManager.php +++ b/src/ApiManager.php @@ -34,7 +34,7 @@ class ApiManager { * * @var null|string */ - private ?string $authorization; + private ?string $authorization = NULL; /** * The previous exception. @@ -67,14 +67,13 @@ class ApiManager { * The config factory. */ public function __construct( - private TimeInterface $time, - private CacheBackendInterface $cache, - private ClientInterface $httpClient, - private EnvironmentResolverInterface $environmentResolver, - private LoggerInterface $logger, - ConfigFactoryInterface $configFactory + private readonly TimeInterface $time, + private readonly CacheBackendInterface $cache, + private readonly ClientInterface $httpClient, + private readonly EnvironmentResolverInterface $environmentResolver, + private readonly LoggerInterface $logger, + private readonly ConfigFactoryInterface $configFactory ) { - $this->authorization = $configFactory->get('helfi_navigation.api')->get('key'); } /** @@ -186,14 +185,14 @@ public function get( * @throws \GuzzleHttp\Exception\GuzzleException */ public function update(string $langcode, array $data) : ApiResponse { - if (!$this->authorization) { + if (!$this->hasAuthorization()) { throw new ConfigException('Missing "helfi_navigation.api" key setting.'); } $endpoint = sprintf('%s/%s', static::GLOBAL_MENU_ENDPOINT, $this->environmentResolver->getActiveEnvironment()->getId()); return $this->makeRequest('POST', $endpoint, $langcode, [ 'json' => $data, - 'headers' => ['Authorization' => sprintf('Basic %s', $this->authorization)], + 'headers' => ['Authorization' => sprintf('Basic %s', $this->getAuthorization())], ]); } @@ -261,8 +260,23 @@ public function getUrl(string $type, string $langcode, array $options = []) : st * @return bool * Is the system authorized to use secured endpoints. */ - public function isAuthorized(): bool { - return (bool) $this->authorization; + public function hasAuthorization(): bool { + return (bool) $this->getAuthorization(); + } + + /** + * Gets the authorization. + * + * @return string|null + * The authorization token. + */ + public function getAuthorization() : ?string { + if (!$this->authorization) { + $this->authorization = $this->configFactory + ->get('helfi_navigation.api') + ->get('key'); + } + return $this->authorization; } /** diff --git a/src/MainMenuManager.php b/src/MainMenuManager.php index 5e25cde..c7acac0 100644 --- a/src/MainMenuManager.php +++ b/src/MainMenuManager.php @@ -104,6 +104,8 @@ public function getSiteName(string $langcode) : string { * * @return mixed * Menu tree. + * + * @throws \InvalidArgumentException */ public function build(string $langcode): array { $siteName = $this->getSiteName($langcode); diff --git a/src/Plugin/rest/resource/GlobalMobileMenu.php b/src/Plugin/rest/resource/GlobalMobileMenu.php index 366095b..03864ce 100644 --- a/src/Plugin/rest/resource/GlobalMobileMenu.php +++ b/src/Plugin/rest/resource/GlobalMobileMenu.php @@ -7,7 +7,7 @@ use Drupal\Core\Config\ConfigFactory; use Drupal\Core\Language\LanguageInterface; use Drupal\Core\Language\LanguageManagerInterface; -use Drupal\helfi_api_base\Environment\EnvironmentResolver; +use Drupal\helfi_api_base\Environment\EnvironmentResolverInterface; use Drupal\helfi_navigation\ApiManager; use Drupal\helfi_navigation\MainMenuManager; use Drupal\rest\Plugin\ResourceBase; @@ -32,51 +32,46 @@ final class GlobalMobileMenu extends ResourceBase { * * @var \Drupal\helfi_navigation\ApiManager */ - protected ApiManager $apiManager; + private readonly ApiManager $apiManager; /** * The language manager service.. * * @var \Drupal\Core\Language\LanguageManagerInterface */ - protected LanguageManagerInterface $languageManager; + private readonly LanguageManagerInterface $languageManager; /** * The config factory service. * * @var \Drupal\Core\Config\ConfigFactory */ - protected ConfigFactory $configFactory; + private readonly ConfigFactory $configFactory; /** * The environment resolver service. * - * @var \Drupal\helfi_api_base\Environment\EnvironmentResolver + * @var \Drupal\helfi_api_base\Environment\EnvironmentResolverInterface */ - protected EnvironmentResolver $environmentResolver; + private readonly EnvironmentResolverInterface $environmentResolver; /** * The menu manager service. * - * @var Drupal\helfi_navigation\MainMenuManager + * @var \Drupal\helfi_navigation\MainMenuManager */ - protected MainMenuManager $menuManager; + private readonly MainMenuManager $mainMenuManager; /** * {@inheritdoc} */ public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) : self { - $instance = parent::create( - $container, - $configuration, - $plugin_id, - $plugin_definition - ); + $instance = parent::create($container, $configuration, $plugin_id, $plugin_definition); $instance->configFactory = $container->get('config.factory'); $instance->languageManager = $container->get('language_manager'); $instance->apiManager = $container->get('helfi_navigation.api_manager'); $instance->environmentResolver = $container->get('helfi_api_base.environment_resolver'); - $instance->menuManager = $container->get('helfi_navigation.menu_manager'); + $instance->mainMenuManager = $container->get('helfi_navigation.menu_manager'); return $instance; } @@ -93,50 +88,66 @@ public function get(): ResourceResponse { ->getId(); try { + // Fetch the main menu from Etusivu instance. $apiResponse = $this->apiManager->get($langcode, 'main', []); } - catch (\Exception) { + catch (\Exception $e) { return new ResourceResponse([], 404); } // If authorization key is set, just return the menu without enrichment. - if ($this->apiManager->isAuthorized()) { - return new ResourceResponse($this->normalizeResponseData($apiResponse->data), 200); + // This is used to so instances that are not a part of the + // "global navigation" to show their own main menu in mobile + // navigation, namely Rekry. + // @see https://helsinkisolutionoffice.atlassian.net/browse/UHF-7607 + if ($this->apiManager->hasAuthorization()) { + return $this->toResourceResponse( + $this->normalizeResponseData($apiResponse->data) + ); } - $environment = $this->environmentResolver->getActiveEnvironment(); + $projectName = $this->environmentResolver + ->getActiveEnvironment() + ->getId(); $site_name = $this->configFactory->get('system.site')->get('name'); - $project_name = $environment->getId(); // Create menu tree and add data to the local menu. - $menuTree = $this->menuManager->build($langcode); + $menuTree = $this->mainMenuManager->build($langcode); + // This is used by Mobile navigation javascript to + // figure out if special handling is needed. $menuTree['is_injected'] = TRUE; - // Commented lines are present in the api request, - // most likely generated by the drupal api module. $site_data = [ - // 'content_translation_changed' => '', - // 'content_translation_created' => '', - // 'content_translation_created' => '', - // 'content_translation_source' => '', - // 'content_translation_uid' => '', - // 'metatag' => '', - // 'default_langcode' => [(object) ['value' => TRUE]], 'langcode' => [['value' => $langcode]], 'menu_tree' => [0 => $menuTree], 'name' => [['value' => $site_name]], - 'project' => [['value' => $project_name]], + 'project' => [['value' => $projectName]], 'status' => [['value' => TRUE]], 'uuid' => [['value' => $this->configFactory->get('system.site')->get('uuid')]], 'weight' => [['value' => 0]], ]; // Add local menu to the api response. - $apiResponse->data->{$project_name} = $site_data; + $apiResponse->data->{$projectName} = $site_data; - $response = new ResourceResponse($this->normalizeResponseData($apiResponse->data), 200); - $response->getCacheableMetadata()->addCacheTags(['config:system.menu.main']); + return $this->toResourceResponse( + $this->normalizeResponseData($apiResponse->data) + ); + } + /** + * Constructs a new resource response with required cache dependencies. + * + * @param array $data + * The response data. + * + * @return \Drupal\rest\ResourceResponse + * The resource response. + */ + private function toResourceResponse(array $data) : ResourceResponse { + $response = new ResourceResponse($data, 200); + $response->getCacheableMetadata() + ->addCacheTags(['config:system.menu.main']); return $response; } diff --git a/tests/src/Functional/MenuBlockTest.php b/tests/src/Functional/MenuBlockTest.php index d549c29..9589f2e 100644 --- a/tests/src/Functional/MenuBlockTest.php +++ b/tests/src/Functional/MenuBlockTest.php @@ -4,12 +4,13 @@ namespace Drupal\Tests\helfi_navigation\Functional; -use Drupal\helfi_api_base\Environment\EnvironmentResolver; +use Drupal\helfi_api_base\Environment\EnvironmentEnum; use Drupal\helfi_api_base\Environment\Project; use Drupal\language\Entity\ConfigurableLanguage; use Drupal\node\Entity\Node; use Drupal\node\Entity\NodeType; use Drupal\Tests\BrowserTestBase; +use Drupal\Tests\helfi_api_base\Traits\EnvironmentResolverTrait; /** * Tests menu blocks. @@ -18,6 +19,8 @@ */ class MenuBlockTest extends BrowserTestBase { + use EnvironmentResolverTrait; + /** * {@inheritdoc} */ @@ -59,10 +62,7 @@ public function setUp() : void { 'type' => 'page', ])->save(); - $config = $this->config('helfi_api_base.environment_resolver.settings'); - $config->set(EnvironmentResolver::ENVIRONMENT_NAME_KEY, 'local'); - $config->set(EnvironmentResolver::PROJECT_NAME_KEY, Project::ASUMINEN); - $config->save(); + $this->setActiveProject(Project::ASUMINEN, EnvironmentEnum::Local); _helfi_navigation_generate_blocks('stark', 'content', TRUE); } diff --git a/tests/src/FunctionalJavascript/JsSettingsTest.php b/tests/src/FunctionalJavascript/JsSettingsTest.php index 4e56d91..37c2a92 100644 --- a/tests/src/FunctionalJavascript/JsSettingsTest.php +++ b/tests/src/FunctionalJavascript/JsSettingsTest.php @@ -5,9 +5,10 @@ namespace Drupal\Tests\helfi_navigation\FunctionalJavascript; use Drupal\FunctionalJavascriptTests\WebDriverTestBase; -use Drupal\helfi_api_base\Environment\EnvironmentResolver; +use Drupal\helfi_api_base\Environment\EnvironmentEnum; use Drupal\helfi_api_base\Environment\Project; use Drupal\language\Entity\ConfigurableLanguage; +use Drupal\Tests\helfi_api_base\Traits\EnvironmentResolverTrait; /** * Tests drupal settings. @@ -16,6 +17,8 @@ */ class JsSettingsTest extends WebDriverTestBase { + use EnvironmentResolverTrait; + /** * {@inheritdoc} */ @@ -39,10 +42,7 @@ public function setUp() : void { foreach (['fi', 'sv'] as $langcode) { ConfigurableLanguage::createFromLangcode($langcode)->save(); } - $config = $this->config('helfi_api_base.environment_resolver.settings'); - $config->set(EnvironmentResolver::ENVIRONMENT_NAME_KEY, 'local'); - $config->set(EnvironmentResolver::PROJECT_NAME_KEY, Project::ASUMINEN); - $config->save(); + $this->setActiveProject(Project::ASUMINEN, EnvironmentEnum::Local); $this->config('language.negotiation') ->set('url.prefixes', ['en' => 'en', 'fi' => 'fi', 'sv' => 'sv']) diff --git a/tests/src/Kernel/CacheWarmerTest.php b/tests/src/Kernel/CacheWarmerTest.php index c536136..db130b6 100644 --- a/tests/src/Kernel/CacheWarmerTest.php +++ b/tests/src/Kernel/CacheWarmerTest.php @@ -6,9 +6,10 @@ use Drupal\Core\Cache\CacheTagsInvalidatorInterface; use Drupal\Core\TempStore\SharedTempStoreFactory; -use Drupal\helfi_api_base\Environment\EnvironmentResolver; +use Drupal\helfi_api_base\Environment\EnvironmentEnum; use Drupal\helfi_api_base\Environment\Project; use Drupal\helfi_navigation\CacheWarmer; +use Drupal\Tests\helfi_api_base\Traits\EnvironmentResolverTrait; use Prophecy\Argument; /** @@ -19,6 +20,8 @@ */ class CacheWarmerTest extends KernelTestBase { + use EnvironmentResolverTrait; + /** * The shared temp store factory. * @@ -33,10 +36,7 @@ protected function setUp(): void { parent::setUp(); $this->populateConfiguration('Test'); - $config = $this->config('helfi_api_base.environment_resolver.settings'); - $config->set(EnvironmentResolver::ENVIRONMENT_NAME_KEY, 'local'); - $config->set(EnvironmentResolver::PROJECT_NAME_KEY, Project::ASUMINEN); - $config->save(); + $this->setActiveProject(Project::ASUMINEN, EnvironmentEnum::Local); $this->sharedTempStore = $this->container->get('tempstore.shared'); } diff --git a/tests/src/Kernel/Plugin/rest/resource/GlobalMobileMenuTest.php b/tests/src/Kernel/Plugin/rest/resource/GlobalMobileMenuTest.php new file mode 100644 index 0000000..5012d40 --- /dev/null +++ b/tests/src/Kernel/Plugin/rest/resource/GlobalMobileMenuTest.php @@ -0,0 +1,120 @@ +installConfig('helfi_navigation'); + } + + /** + * Grants required permissions for anonymous user. + */ + private function grantRestfulPermissions() : void { + Role::load(RoleInterface::ANONYMOUS_ID) + ->grantPermission('restful get helfi_global_mobile_menu'); + } + + /** + * @covers ::__construct + * @covers ::create + * @covers ::get + */ + public function test403() : void { + // Make sure we get a 403 response when permissions are not set. + $request = $this->getMockedRequest('/api/v1/global-mobile-menu'); + $response = $this->processRequest($request); + + $this->assertEquals(HttpResponse::HTTP_FORBIDDEN, $response->getStatusCode()); + } + + /** + * @covers ::__construct + * @covers ::create + * @covers ::get + */ + public function test404() : void { + $this->grantRestfulPermissions(); + // Make sure a 404 response is sent when we fail to fetch mobile navigation. + $request = $this->getMockedRequest('/api/v1/global-mobile-menu'); + $response = $this->processRequest($request); + + $this->assertEquals(HttpResponse::HTTP_NOT_FOUND, $response->getStatusCode()); + } + + /** + * @covers ::get + * @covers ::__construct + * @covers ::normalizeResponseData + * @covers ::toResourceResponse + */ + public function testEndpointWithLocalModifications() : void { + $this->grantRestfulPermissions(); + $this->setActiveProject(Project::ASUMINEN, EnvironmentEnum::Test); + $this->config('system.site') + ->set('name', Project::ASUMINEN) + ->save(); + + $request = $this->getMockedRequest('/api/v1/global-mobile-menu'); + $response = $this->processRequest($request); + $array = json_decode($response->getContent(), TRUE); + + // Make sure 'is_injected' property is set, indicating that + // local navigation is appended into API response. + $this->assertTrue($array[Project::ASUMINEN]['menu_tree'][0]['is_injected']); + } + + /** + * @covers ::get + * @covers ::__construct + * @covers ::normalizeResponseData + * @covers ::toResourceResponse + */ + public function testEndpointPassthrough() : void { + $request = $this->getMockedRequest('/api/v1/global-mobile-menu'); + $response = $this->processRequest($request); + $array = json_decode($response->getContent(), TRUE); + + // Make sure Asuminen project has no navigation since + // we just return the API response from global menu + // endpoint without any modifications. + $this->assertArrayNotHasKey(Project::ASUMINEN, $array); + } + +} From a2f1090e09228816793e5c05146d9eec77bb6f50 Mon Sep 17 00:00:00 2001 From: tuutti Date: Fri, 2 Jun 2023 09:44:52 +0300 Subject: [PATCH 11/14] UHF-8522: Added test for menu queue --- src/MainMenuManager.php | 3 +- src/Plugin/QueueWorker/MenuQueue.php | 13 ++-- tests/src/Kernel/CacheWarmerTest.php | 2 + .../Unit/Plugin/QueueWorker/MenuQueueTest.php | 63 +++++++++++++++++++ 4 files changed, 73 insertions(+), 8 deletions(-) create mode 100644 tests/src/Unit/Plugin/QueueWorker/MenuQueueTest.php diff --git a/src/MainMenuManager.php b/src/MainMenuManager.php index c7acac0..9a0ee41 100644 --- a/src/MainMenuManager.php +++ b/src/MainMenuManager.php @@ -45,9 +45,8 @@ public function __construct( * @param string $langcode * The langcode. * - * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException - * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException * @throws \GuzzleHttp\Exception\GuzzleException + * @throws \InvalidArgumentException */ public function sync(string $langcode): bool { // Sync menu as an anonymous user to make sure no sensitive diff --git a/src/Plugin/QueueWorker/MenuQueue.php b/src/Plugin/QueueWorker/MenuQueue.php index ccbf8a6..f29be38 100644 --- a/src/Plugin/QueueWorker/MenuQueue.php +++ b/src/Plugin/QueueWorker/MenuQueue.php @@ -25,32 +25,33 @@ final class MenuQueue extends QueueWorkerBase implements ContainerFactoryPluginI * * @var \Drupal\helfi_navigation\MainMenuManager */ - private MainMenuManager $menuManager; + private MainMenuManager $mainMenuManager; /** * {@inheritdoc} */ public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) : self { $instance = new self($configuration, $plugin_id, $plugin_definition); - $instance->menuManager = $container->get('helfi_navigation.menu_manager'); + $instance->mainMenuManager = $container->get('helfi_navigation.menu_manager'); return $instance; } /** * Process queue item. * - * @param string $langcode + * @param string $data * Data of the processable language code. * * @throws \Exception * Throws exception if language code is not set. */ - public function processItem($langcode) { - if (!is_string($langcode)) { + public function processItem($data) : void { + // Data is a langcode, like 'fi' or 'en'. + if (!is_string($data)) { return; } try { - $this->menuManager->sync($langcode); + $this->mainMenuManager->sync($data); } catch (\Throwable) { // The failed sync will be logged by ApiManager. diff --git a/tests/src/Kernel/CacheWarmerTest.php b/tests/src/Kernel/CacheWarmerTest.php index db130b6..90ebb2a 100644 --- a/tests/src/Kernel/CacheWarmerTest.php +++ b/tests/src/Kernel/CacheWarmerTest.php @@ -11,6 +11,7 @@ use Drupal\helfi_navigation\CacheWarmer; use Drupal\Tests\helfi_api_base\Traits\EnvironmentResolverTrait; use Prophecy\Argument; +use Prophecy\PhpUnit\ProphecyTrait; /** * Tests cache warmer. @@ -21,6 +22,7 @@ class CacheWarmerTest extends KernelTestBase { use EnvironmentResolverTrait; + use ProphecyTrait; /** * The shared temp store factory. diff --git a/tests/src/Unit/Plugin/QueueWorker/MenuQueueTest.php b/tests/src/Unit/Plugin/QueueWorker/MenuQueueTest.php new file mode 100644 index 0000000..886e87c --- /dev/null +++ b/tests/src/Unit/Plugin/QueueWorker/MenuQueueTest.php @@ -0,0 +1,63 @@ +set('helfi_navigation.menu_manager', $menuManager->reveal()); + return MenuQueue::create($container, [], '', []); + } + + /** + * @covers ::create + * @covers ::processItem + */ + public function testInvalidData() : void { + // Make sure sync is not called for invalid data. + $menuManager = $this->prophesize(MainMenuManager::class); + $menuManager->sync(Argument::any()) + ->shouldNotBeCalled(); + $this->getSut($menuManager)->processItem(NULL); + } + + /** + * @covers ::create + * @covers ::processItem + */ + public function testQueueException() : void { + // Make sure queue doesn't die if ::sync() throws an exception. + $menuManager = $this->prophesize(MainMenuManager::class); + $menuManager->sync(Argument::any()) + ->shouldBeCalled() + ->willThrow(new \InvalidArgumentException()); + $this->getSut($menuManager)->processItem('fi'); + } + +} From 50b893c24b111f55740dbd512c439e9801d5e340 Mon Sep 17 00:00:00 2001 From: tuutti Date: Fri, 2 Jun 2023 09:54:00 +0300 Subject: [PATCH 12/14] UHF-8522: Make sure mock data is used --- src/ApiManager.php | 5 +++-- tests/src/Kernel/ExternalMenuTreeBuilderTest.php | 4 ---- .../src/Kernel/Plugin/rest/resource/GlobalMobileMenuTest.php | 2 +- tests/src/Unit/ApiManagerTest.php | 2 ++ 4 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/ApiManager.php b/src/ApiManager.php index 3a41f9d..df27b35 100644 --- a/src/ApiManager.php +++ b/src/ApiManager.php @@ -17,6 +17,7 @@ use GuzzleHttp\Exception\ConnectException; use GuzzleHttp\Exception\GuzzleException; use GuzzleHttp\Exception\TransferException; +use GuzzleHttp\Utils; use Psr\Log\LoggerInterface; /** @@ -319,7 +320,7 @@ private function makeRequest( } $response = $this->httpClient->request($method, $url, $options); - return new ApiResponse(\GuzzleHttp\json_decode($response->getBody()->getContents())); + return new ApiResponse(Utils::jsonDecode($response->getBody()->getContents())); } catch (\Exception $e) { if ($e instanceof GuzzleException) { @@ -346,7 +347,7 @@ private function makeRequest( sprintf('[%s]. Attempted to use mock data, but the mock file "%s" was not found for "%s" endpoint.', $e->getMessage(), basename($fileName), $endpoint) ); } - return new ApiResponse(\GuzzleHttp\json_decode(file_get_contents($fileName))); + return new ApiResponse(Utils::jsonDecode(file_get_contents($fileName))); } // Log the error and re-throw the exception. $this->logger->error('Request failed with error: ' . $e->getMessage()); diff --git a/tests/src/Kernel/ExternalMenuTreeBuilderTest.php b/tests/src/Kernel/ExternalMenuTreeBuilderTest.php index ee8f85e..bc6f6db 100644 --- a/tests/src/Kernel/ExternalMenuTreeBuilderTest.php +++ b/tests/src/Kernel/ExternalMenuTreeBuilderTest.php @@ -81,12 +81,8 @@ public function testBuild() : void { // is the currently active page. $this->assertTrue($externalTree[0]['in_active_trail']); $this->assertFalse($externalTree[0]['is_currentPage']); - $this->assertFalse($externalTree[0]['external']); $this->assertTrue($externalTree[0]['below'][0]['in_active_trail']); $this->assertTrue($externalTree[0]['below'][0]['is_currentPage']); - - // Make sure links get marked as external correctly. - $this->assertTrue($externalTree[1]['below'][0]['below'][0]['external']); } } diff --git a/tests/src/Kernel/Plugin/rest/resource/GlobalMobileMenuTest.php b/tests/src/Kernel/Plugin/rest/resource/GlobalMobileMenuTest.php index 5012d40..1387873 100644 --- a/tests/src/Kernel/Plugin/rest/resource/GlobalMobileMenuTest.php +++ b/tests/src/Kernel/Plugin/rest/resource/GlobalMobileMenuTest.php @@ -86,7 +86,7 @@ public function test404() : void { */ public function testEndpointWithLocalModifications() : void { $this->grantRestfulPermissions(); - $this->setActiveProject(Project::ASUMINEN, EnvironmentEnum::Test); + $this->setActiveProject(Project::ASUMINEN, EnvironmentEnum::Local); $this->config('system.site') ->set('name', Project::ASUMINEN) ->save(); diff --git a/tests/src/Unit/ApiManagerTest.php b/tests/src/Unit/ApiManagerTest.php index 6f7aa5b..55a3b55 100644 --- a/tests/src/Unit/ApiManagerTest.php +++ b/tests/src/Unit/ApiManagerTest.php @@ -139,6 +139,8 @@ private function getSut( * @covers ::makeRequest * @covers ::getDefaultRequestOptions * @covers ::getUrl + * @covers ::hasAuthorization + * @covers ::getAuthorization * @covers \Drupal\helfi_navigation\ApiResponse::__construct */ public function testUpdateMainMenu() : void { From b846d1bbf013c653a734966d1e7c37a566c7fe19 Mon Sep 17 00:00:00 2001 From: tuutti Date: Fri, 2 Jun 2023 10:01:38 +0300 Subject: [PATCH 13/14] UHF-8522: Test external menu link class --- .../Unit/Plugin/Menu/ExternalMenuLinkTest.php | 67 +++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 tests/src/Unit/Plugin/Menu/ExternalMenuLinkTest.php diff --git a/tests/src/Unit/Plugin/Menu/ExternalMenuLinkTest.php b/tests/src/Unit/Plugin/Menu/ExternalMenuLinkTest.php new file mode 100644 index 0000000..404d259 --- /dev/null +++ b/tests/src/Unit/Plugin/Menu/ExternalMenuLinkTest.php @@ -0,0 +1,67 @@ +assertNull($link->getDerivativeId()); + } + + /** + * @covers ::getTitle + */ + public function testGetTitle() : void { + $expectedTitle = 'some title'; + $link = new ExternalMenuLink([], '', ['title' => $expectedTitle]); + $this->assertEquals($expectedTitle, $link->getTitle()); + + // Make sure TranslatableMarkup is converted to string too. + $title = $this->getStringTranslationStub()->translate($expectedTitle); + + $link = new ExternalMenuLink([], '', ['title' => $title]); + $this->assertEquals($expectedTitle, $link->getTitle()); + } + + /** + * @covers ::getDescription + */ + public function testGetDescription() : void { + $expectedDescription = 'some description'; + $link = new ExternalMenuLink([], '', ['description' => $expectedDescription]); + $this->assertEquals($expectedDescription, $link->getDescription()); + + // Make sure TranslatableMarkup is converted to string too. + $title = $this->getStringTranslationStub()->translate($expectedDescription); + + $link = new ExternalMenuLink([], '', ['description' => $title]); + $this->assertEquals($expectedDescription, $link->getDescription()); + } + + /** + * @covers ::updateLink + */ + public function testUpdateLink() : void { + $link = new ExternalMenuLink([], '', ['title' => '123']); + $this->assertEquals('123', $link->getTitle()); + $link->updateLink(['title' => '321'], TRUE); + $this->assertEquals('321', $link->getTitle()); + } + +} From 1805ba22bb9f0e337908641c4600ef9775612fc1 Mon Sep 17 00:00:00 2001 From: tuutti Date: Fri, 2 Jun 2023 10:05:48 +0300 Subject: [PATCH 14/14] UHF-8522: Missing covers --- tests/src/Unit/Plugin/Menu/ExternalMenuLinkTest.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/src/Unit/Plugin/Menu/ExternalMenuLinkTest.php b/tests/src/Unit/Plugin/Menu/ExternalMenuLinkTest.php index 404d259..35df594 100644 --- a/tests/src/Unit/Plugin/Menu/ExternalMenuLinkTest.php +++ b/tests/src/Unit/Plugin/Menu/ExternalMenuLinkTest.php @@ -56,6 +56,7 @@ public function testGetDescription() : void { /** * @covers ::updateLink + * @covers ::getTitle */ public function testUpdateLink() : void { $link = new ExternalMenuLink([], '', ['title' => '123']);