From 01c1d44e659b5940db8d8e7765bcd404902a8e82 Mon Sep 17 00:00:00 2001
From: Julien Maury <6878027+jmau111@users.noreply.github.com>
Date: Thu, 14 Apr 2022 02:48:55 -0700
Subject: [PATCH] Fix duplicate IDs on homepage (#17092)

* fix duplicate ID main nav

* fix duplicate ID other nav headings

* fix duplicate ID reading list count

* fix duplicate attribute class

* missing case readingCountList on feed

* missing dot in query selector

* Revert "fix duplicate ID other nav headings"

This reverts commit 4f495962c47e08f0f6c41e33e897b2db176432f5.

* use generic class 'other-nav' for tests

* pass a variable 'context' to the partial nav

* pass var 'context' to partial

* fix unwanted typo in partial sidebar

* Update app/views/admin/navigation_links/index.html.erb

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

* Update app/assets/javascripts/initializers/initializeReadingListIcons.js

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

* Update app/assets/javascripts/initializers/initializeReadingListIcons.js

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>

* js-reading-list-count renaming

* use data-testid instead of generic classes for tests

* fix typo in specs

Co-authored-by: Suzanne Aitchison <suzanne@forem.com>
---
 .../initializeReadingListIcons.js             | 44 +++++++++++--------
 .../admin/navigation_links/index.html.erb     |  4 +-
 app/views/articles/_sidebar.html.erb          |  2 +-
 app/views/layouts/_main_nav.html.erb          |  6 +--
 app/views/layouts/_sidebar_nav_link.html.erb  |  2 +-
 app/views/shared/_hamburger.html.erb          |  2 +-
 .../homepage/user_visits_homepage_spec.rb     | 24 +++++-----
 7 files changed, 46 insertions(+), 38 deletions(-)

diff --git a/app/assets/javascripts/initializers/initializeReadingListIcons.js b/app/assets/javascripts/initializers/initializeReadingListIcons.js
index bf76fbd17..f89e2d041 100644
--- a/app/assets/javascripts/initializers/initializeReadingListIcons.js
+++ b/app/assets/javascripts/initializers/initializeReadingListIcons.js
@@ -33,14 +33,17 @@ function highlightButton(button) {
 }
 
 function addReadingListCountToHomePage() {
-  var user = userData();
-  var readingListCount;
-  if (user && document.getElementById('reading-list-count')) {
-    readingListCount =
-      user.reading_list_ids.length > 0 ? user.reading_list_ids.length : '';
-    document.getElementById('reading-list-count').innerHTML = readingListCount;
-    document.getElementById('reading-list-count').dataset.count =
-      user.reading_list_ids.length;
+  const user = userData();
+  const readingListContainers = document.querySelectorAll(
+    '.js-reading-list-count',
+  );
+  if (user && readingListContainers) {
+    readingListContainers.forEach(function (e) {
+      const readingListCount =
+        user.reading_list_ids.length > 0 ? user.reading_list_ids.length : '';
+      e.innerHTML = readingListCount;
+      e.dataset.count = user.reading_list_ids.length;
+    });
   }
 }
 
@@ -83,17 +86,22 @@ function renderButtonState(button, json) {
 }
 
 function renderNewSidebarCount(button, json) {
-  var newCount;
-  var count = document.getElementById('reading-list-count').dataset.count;
-  count = parseInt(count, 10);
-  if (json.result === 'create') {
-    newCount = count + 1;
-  } else if (count !== 0) {
-    newCount = count - 1;
+  const readingListContainers = document.querySelectorAll(
+    '.js-reading-list-count',
+  );
+  if (readingListContainers) {
+    readingListContainers.forEach(function (e) {
+      const count = parseInt(e.dataset.count, 10);
+      let newCount;
+      if (json.result === 'create') {
+        newCount = count + 1;
+      } else if (count !== 0) {
+        newCount = count - 1;
+      }
+      e.dataset.count = newCount;
+      e.innerHTML = newCount > 0 ? newCount : '';
+    });
   }
-  document.getElementById('reading-list-count').dataset.count = newCount;
-  document.getElementById('reading-list-count').innerHTML =
-    newCount > 0 ? newCount : '';
 }
 
 function buttonFormData(button) {
diff --git a/app/views/admin/navigation_links/index.html.erb b/app/views/admin/navigation_links/index.html.erb
index 749e1b81e..b0020055a 100644
--- a/app/views/admin/navigation_links/index.html.erb
+++ b/app/views/admin/navigation_links/index.html.erb
@@ -17,7 +17,7 @@
               <div class="crayons-icon crayons-icon--default">
                   <%= link[:icon].html_safe %>
               </div>
-              <%= link[:name] %><span id="reading-list-count" class="c-indicator"></span>
+              <%= link[:name] %><span class="js-reading-list-count c-indicator"></span>
             </a>
           </div>
 
@@ -45,7 +45,7 @@
               <div class="crayons-icon crayons-icon--default">
                   <%= link[:icon].html_safe %>
               </div>
-              <%= link[:name] %><span id="reading-list-count" class="c-indicator"></span>
+              <%= link[:name] %><span class="js-reading-list-count c-indicator"></span>
             </a>
           </div>
 
diff --git a/app/views/articles/_sidebar.html.erb b/app/views/articles/_sidebar.html.erb
index 797bf8d35..0f13bc2e1 100644
--- a/app/views/articles/_sidebar.html.erb
+++ b/app/views/articles/_sidebar.html.erb
@@ -2,7 +2,7 @@
   <aside class="side-bar" aria-label="Primary sidebar">
     <% cache(release_adjusted_cache_key("main-sidebar-nav--#{user_signed_in?}"), expires_in: 15.minutes) do %>
       <%= render "shared/auth_widget" unless user_signed_in? %>
-      <%= render "layouts/main_nav" %>
+      <%= render partial: "layouts/main_nav", locals: { context: "sidebar" } %>
       <%= render "layouts/sidebar_tags" %>
       <% @sponsorships = Sponsorship.gold.live.includes(:organization).order(featured_number: :asc) %>
       <%# the pattern .present?/.each has the advantage of issuing only a single SQL query to load objects in memory %>
diff --git a/app/views/layouts/_main_nav.html.erb b/app/views/layouts/_main_nav.html.erb
index 350fc34b6..d18345efb 100644
--- a/app/views/layouts/_main_nav.html.erb
+++ b/app/views/layouts/_main_nav.html.erb
@@ -1,7 +1,7 @@
 <% default_nav_links = NavigationLink.default_section.ordered.to_a %>
 <% other_nav_links = NavigationLink.other_section.ordered.to_a %>
 
-<nav class="mb-4 <% unless user_signed_in? %>mt-4<% end %>" id="main-navigation" aria-label="<%= community_name %>">
+<nav class="mb-4 <% unless user_signed_in? %>mt-4<% end %>" data-testid="main-nav" aria-label="<%= community_name %>">
   <ul class="default-navigation-links sidebar-navigation-links spec-sidebar-navigation-links">
     <% default_nav_links.each do |link| %>
       <%= render "layouts/sidebar_nav_link", link: link %>
@@ -13,8 +13,8 @@
     visible based on the current state (see
     ApplicationHelper#display_navigation_link? for details).  %>
 <% if other_nav_links.any? %>
-  <nav class="mb-4" aria-labelledby="other-nav-heading">
-    <h2 id="other-nav-heading" class="crayons-subtitle-3 py-2 pl-3">
+  <nav class="mb-4" data-testid="other-nav" aria-labelledby="other-nav-heading-<%= context %>">
+    <h2 id="other-nav-heading-<%= context %>" class="crayons-subtitle-3 py-2 pl-3">
       <%= t("views.main.nav.other") %>
     </h2>
     <ul class="other-navigation-links sidebar-navigation-links spec-sidebar-navigation-links">
diff --git a/app/views/layouts/_sidebar_nav_link.html.erb b/app/views/layouts/_sidebar_nav_link.html.erb
index 76ba8c72a..e54d44e2c 100644
--- a/app/views/layouts/_sidebar_nav_link.html.erb
+++ b/app/views/layouts/_sidebar_nav_link.html.erb
@@ -6,7 +6,7 @@
       </span>
       <%= t("views.main.nav_name.#{link.name}", default: link.name) %>
       <% if link.url.include?("readinglist") %>
-        <span id="reading-list-count" class="c-indicator ml-2 self-center"></span>
+        <span class="js-reading-list-count c-indicator ml-2 self-center"></span>
       <% end %>
     </a>
   </li>
diff --git a/app/views/shared/_hamburger.html.erb b/app/views/shared/_hamburger.html.erb
index 1149f4310..784609b71 100644
--- a/app/views/shared/_hamburger.html.erb
+++ b/app/views/shared/_hamburger.html.erb
@@ -10,7 +10,7 @@
 
     <div class="p-2">
       <%= render partial: "shared/auth_widget" unless user_signed_in? %>
-      <%= render partial: "layouts/main_nav" %>
+      <%= render partial: "layouts/main_nav", locals: { context: "hamburger" } %>
     </div>
   </div>
   <div class="hamburger__overlay js-hamburger-trigger"></div>
diff --git a/spec/system/homepage/user_visits_homepage_spec.rb b/spec/system/homepage/user_visits_homepage_spec.rb
index b13bf3f0a..fdc7d1ec9 100644
--- a/spec/system/homepage/user_visits_homepage_spec.rb
+++ b/spec/system/homepage/user_visits_homepage_spec.rb
@@ -62,28 +62,28 @@ RSpec.describe "User visits a homepage", type: :system do
       end
 
       it "shows expected number of links when signed out" do
-        within("#main-navigation", match: :first) do
+        within("nav[data-testid='main-nav']", match: :first) do
           expect(page).to have_selector(".sidebar-navigation-link", count: 1)
         end
 
-        within("nav[aria-labelledby='other-nav-heading']", match: :first) do
+        within("nav[data-testid='other-nav']", match: :first) do
           expect(page).to have_selector(".sidebar-navigation-link", count: 2)
         end
       end
 
       it "shows the Other section when other nav links exist" do
-        within("nav[aria-labelledby='other-nav-heading']", match: :first) do
+        within("nav[data-testid='other-nav']", match: :first) do
           expect(page).to have_selector(".other-navigation-links")
         end
 
         NavigationLink.other_section.destroy_all
         visit "/"
 
-        expect(page).not_to have_selector("nav[aria-labelledby='other-nav-heading']")
+        expect(page).not_to have_selector("nav[data-testid='other-nav']")
       end
 
       it "hides link when display_only_when_signed_in is true" do
-        within("#main-navigation", match: :first) do
+        within("nav[data-testid='main-nav']", match: :first) do
           expect(page).to have_selector(".default-navigation-links .sidebar-navigation-link", count: 1)
         end
       end
@@ -96,7 +96,7 @@ RSpec.describe "User visits a homepage", type: :system do
                position: 3)
         visit "/"
 
-        within("#main-navigation", match: :first) do
+        within("nav[data-testid='main-nav']", match: :first) do
           expect(page).to have_selector(".default-navigation-links li:nth-child(1)", text: "Shop")
           expect(page).to have_selector(".default-navigation-links li:nth-child(2)", text: "Mock")
         end
@@ -191,35 +191,35 @@ RSpec.describe "User visits a homepage", type: :system do
       end
 
       it "shows the correct navigation_links" do
-        within("#main-navigation", match: :first) do
+        within("nav[data-testid='main-nav']", match: :first) do
           expect(page).to have_text(navigation_link_1.name)
           expect(page).to have_text(navigation_link_3.name)
         end
 
-        within("nav[aria-labelledby='other-nav-heading']", match: :first) do
+        within("nav[data-testid='other-nav']", match: :first) do
           expect(page).to have_text(navigation_link_2.name)
         end
       end
 
       it "shows the correct urls" do
-        within("#main-navigation", match: :first) do
+        within("nav[data-testid='main-nav']", match: :first) do
           expect(page).to have_link(href: navigation_link_1.url)
           expect(page).to have_link(href: navigation_link_3.url)
         end
 
-        within("nav[aria-labelledby='other-nav-heading']", match: :first) do
+        within("nav[data-testid='other-nav']", match: :first) do
           expect(page).to have_link(href: navigation_link_2.url)
         end
       end
 
       it "shows expected # of links when signed in" do
-        within("#main-navigation", match: :first) do
+        within("nav[data-testid='main-nav']", match: :first) do
           expect(page).to have_selector(".sidebar-navigation-link", count: 2) # it's count: 1 when signed out
         end
       end
 
       it "shows link when display_only_when_signed_in is true" do
-        within("#main-navigation", match: :first) do
+        within("nav[data-testid='main-nav']", match: :first) do
           expect(page).to have_selector(".default-navigation-links li:nth-child(2)", text: "Beauty")
         end
       end