Fix intermittently broken reaction images on home feed (#19779)

* move reaction icons to base layout * fix reactions count specs?? * also fix article specs
2023-07-27 19:48:07 +01:00 · 2023-07-27 19:48:07 +01:00 · 9754d14182
commit 9754d14182
parent 7be865b295
16 changed files with 132 additions and 27 deletions
--- a/app/javascript/utilities/i18n_support.js
+++ b/app/javascript/utilities/i18n_support.js
--- a/app/javascript/support/reaction_images.js
+++ b/app/javascript/support/reaction_images.js
@ -0,0 +1,22 @@
+const fs = require('fs');
+const yaml = require('js-yaml');
+
+const locale = './config/reactions.yml';
+const data = fs.readFileSync(locale, 'utf8');
+const yamlData = yaml.load(data);
+
+const publicReactionIcons = Object.keys(yamlData)
+  .filter(
+    (slug) =>
+      yamlData[slug].privileged !== true && yamlData[slug].published !== false,
+  )
+  .map((slug) => {
+    const { name, icon, position } = yamlData[slug];
+
+    return `<img data-name="${name}" data-position="${position}" data-slug="${slug}" src="/assets/${icon}.svg" width="18" height="18" />`;
+  })
+  .join('');
+
+export function reactionImagesSupport() {
+  document.body.innerHTML += `<div id="reaction-category-resources" class="hidden" aria-hidden="true">${publicReactionIcons}</div>`;
+}
--- a/app/javascript/articles/tests/Article.test.jsx
+++ b/app/javascript/articles/tests/Article.test.jsx
@ -4,6 +4,7 @@ import { render } from '@testing-library/preact';
 import { axe } from 'jest-axe';
 import '@testing-library/jest-dom';
 import { Article } from '..';
+import { reactionImagesSupport } from '../../__support__/reaction_images';
 import { locale } from '../../utilities/locale';
 import {
  article,
@ -27,6 +28,10 @@ const commonProps = {
 };

 describe('<Article /> component', () => {
+  beforeAll(() => {
+    reactionImagesSupport();
+  });
+
  it('should have no a11y violations for a standard article', async () => {
    const { container } = render(
      <Article
--- a/app/javascript/articles/tests/snapshots/Article.test.jsx.snap
+++ b/app/javascript/articles/tests/snapshots/Article.test.jsx.snap
@ -4,6 +4,52 @@ exports[`<Article /> component should render a rich feed 1`] = `
 Object {
  "asFragment": [Function],
  "baseElement": <body>
+    <div
+      aria-hidden="true"
+      class="hidden"
+      id="reaction-category-resources"
+    >
+      <img
+        data-name="Like"
+        data-position="1"
+        data-slug="like"
+        height="18"
+        src="/assets/sparkle-heart.svg"
+        width="18"
+      />
+      <img
+        data-name="Unicorn"
+        data-position="2"
+        data-slug="unicorn"
+        height="18"
+        src="/assets/multi-unicorn.svg"
+        width="18"
+      />
+      <img
+        data-name="Exploding Head"
+        data-position="3"
+        data-slug="exploding_head"
+        height="18"
+        src="/assets/exploding-head.svg"
+        width="18"
+      />
+      <img
+        data-name="Raised Hands"
+        data-position="4"
+        data-slug="raised_hands"
+        height="18"
+        src="/assets/raised-hands.svg"
+        width="18"
+      />
+      <img
+        data-name="Fire"
+        data-position="5"
+        data-slug="fire"
+        height="18"
+        src="/assets/fire.svg"
+        width="18"
+      />
+    </div>
    <div>
      <article
        class="crayons-story cursor-pointer crayons-story--featured"
--- a/app/javascript/articles/components/ReactionsCount.jsx
+++ b/app/javascript/articles/components/ReactionsCount.jsx
@ -21,9 +21,9 @@ export const ReactionsCount = ({ article }) => {
    reversable.reverse();

    const icons = reversable.map((category) => {
-      const path = reactionIcons?.querySelector(
+      const path = reactionIcons.querySelector(
        `img[data-slug=${category.slug}]`,
-      )?.src;
+      ).src;
      const alt = category.name;
      return (
        <span className="crayons_icon_container" key={category.slug}>
@ -33,7 +33,11 @@ export const ReactionsCount = ({ article }) => {
    });

    return (
-      <span className="multiple_reactions_icons_container" dir="rtl">
+      <span
+        className="multiple_reactions_icons_container"
+        dir="rtl"
+        data-testid="multiple-reactions-icons-container"
+      >
        {icons}
      </span>
    );
--- a/app/javascript/articles/components/tests/ReactionsCount.test.jsx
+++ b/app/javascript/articles/components/tests/ReactionsCount.test.jsx
@ -1,6 +1,8 @@
 import { h } from 'preact';
-import { render } from '@testing-library/preact';
+import { render, within } from '@testing-library/preact';
 import '@testing-library/jest-dom';
+import { i18nSupport } from '../../../__support__/i18n';
+import { reactionImagesSupport } from '../../../__support__/reaction_images';
 import { ReactionsCount } from '..';
 import {
  articleWithReactions,
@ -9,12 +11,17 @@ import {
 } from '../../__tests__/utilities/articleUtilities.js';

 describe('<ReactionsCount /> component', () => {
+  beforeAll(() => {
+    i18nSupport();
+    reactionImagesSupport();
+  });
+
  it('should not display reactions data when there are no reactions', async () => {
    const { queryByText } = render(
      <ReactionsCount article={articleWithoutReactions} />,
    );

-    expect(queryByText('0 reactions')).toBeNull();
+    expect(queryByText(/0 reactions/i)).not.toExist();
  });

  it('should display reaction count when there are exactly one reaction', async () => {
@ -22,26 +29,40 @@ describe('<ReactionsCount /> component', () => {
      <ReactionsCount article={articleWithOneReaction} />,
    );

-    expect(queryByText('1 reaction')).toBeNull();
+    expect(queryByText(/1 reaction/i)).toExist();
  });

  it('should display reactions data when there are reactions', async () => {
-    const { findByText } = render(
+    const { queryByText } = render(
      <ReactionsCount article={articleWithReactions} />,
    );

-    expect(findByText('232 reactions')).toBeDefined();
+    expect(queryByText(/232 reactions/i)).toExist();
  });

  it('should display multiple reactions when there are reactions', async () => {
-    const { findByText } = render(
+    const { getByTestId } = render(
      <ReactionsCount article={articleWithReactions} />,
    );

-    expect(
-      findByText(
-        '<span class="multiple_reactions_icons_container" dir="rtl"><span class="crayons_icon_container"><img src="/assets/like.svg" alt="Like" width="18" height="18"></span>',
-      ),
-    ).toBeDefined();
+    const container = getByTestId('multiple-reactions-icons-container');
+    const { queryByAltText } = within(container);
+
+    // `articleWithReactions` has all reactions except exploding head
+    // also, we are not using `toHaveAttribute` directly because Jest inserts a
+    // base URL and we are only interested in the path
+    expect(queryByAltText('Like').getAttribute('src')).toContain(
+      '/assets/sparkle-heart.svg',
+    );
+    expect(queryByAltText('Unicorn').getAttribute('src')).toContain(
+      '/assets/multi-unicorn.svg',
+    );
+    expect(queryByAltText('Fire').getAttribute('src')).toContain(
+      '/assets/fire.svg',
+    );
+    expect(queryByAltText('Raised Hands').getAttribute('src')).toContain(
+      '/assets/raised-hands.svg',
+    );
+    expect(queryByAltText('Exploding Head')).not.toExist();
  });
 });
--- a/app/javascript/onboarding/components/tests/FollowUsers.test.jsx
+++ b/app/javascript/onboarding/components/tests/FollowUsers.test.jsx
@ -4,7 +4,7 @@ import fetch from 'jest-fetch-mock';
 import '@testing-library/jest-dom';

 import { axe } from 'jest-axe';
-import { i18nSupport } from '../../../utilities/i18n_support';
+import { i18nSupport } from '../../../__support__/i18n';
 import { FollowUsers } from '../FollowUsers';

 global.fetch = fetch;
--- a/app/views/articles/index.html.erb
+++ b/app/views/articles/index.html.erb
@ -89,8 +89,6 @@
        </nav>
      </header>

-      <%= render "articles/reaction_category_resources" %>
-
      <% if user_signed_in? %>
        <div id="homepage-feed" style="min-height: 90vh"></div>
      <% else %>
--- a/app/views/collections/show.html.erb
+++ b/app/views/collections/show.html.erb
@ -11,8 +11,6 @@
    <%= link_to t("views.series.list.back", user: @user.name), user_series_path(@user.username) %>
  </header>

-  <%= render "articles/reaction_category_resources" %>
-
  <div class="articles-list" id="articles-list">
    <div class="substories" id="substories">
      <% if @articles.present? %>
--- a/app/views/layouts/application.html.erb
+++ b/app/views/layouts/application.html.erb
@ -110,6 +110,7 @@
    <%= render "layouts/signup_modal" unless user_signed_in? %>
  <% end %>
  <div id="i18n-translations" data-translations="<%= { I18n.locale.to_sym => { core: I18n::JS.translations[I18n.locale.to_sym][:core] } }.to_json %>"></div>
+  <%= render "articles/reaction_category_resources" %>
  </body>
  </html>
 <% end %>
--- a/app/views/organizations/_main_feed.html.erb
+++ b/app/views/organizations/_main_feed.html.erb
@ -1,5 +1,3 @@
-<%= render "articles/reaction_category_resources" %>
-
 <div id="organization-article-index"></div>
 <%= render partial: "articles/single_story", collection: @stories, as: :story, locals: { featured: false } %>
 <% if @stories.size > 1 %>
--- a/app/views/stories/articles_search/index.html.erb
+++ b/app/views/stories/articles_search/index.html.erb
@ -51,8 +51,6 @@
      <%= render "stories/articles_search/nav_menu" %>
    </div>

-    <%= render "articles/reaction_category_resources" %>
-
    <div class="articles-list crayons-layout__content" id="articles-list" data-follow-button-container="true">
      <div id="banner-section"></div>
      <div class="substories" id="substories">
--- a/app/views/stories/tagged_articles/_main_feed.html.erb
+++ b/app/views/stories/tagged_articles/_main_feed.html.erb
@ -1,5 +1,3 @@
-<%= render "articles/reaction_category_resources" %>
-
 <%= render partial: "articles/single_story", collection: @stories, as: :story, locals: { featured: false } %>
 <% if @stories.size > 1 %>
  <div class="placeholder-div"></div>
--- a/app/views/users/_main_feed.html.erb
+++ b/app/views/users/_main_feed.html.erb
@ -1,5 +1,3 @@
-<%= render "articles/reaction_category_resources" %>
-
 <% if @pinned_stories.present? %>
  <div class="crayons-card mb-2 border-2 border-solid border-accent-brand">
    <header>
--- a/config/reactions.yml
+++ b/config/reactions.yml
@ -9,12 +9,15 @@ unicorn:
  icon: multi-unicorn
 exploding_head:
  position: 3
+  name: Exploding Head
  icon: exploding-head
 raised_hands:
  position: 4
+  name: Raised Hands
  icon: raised-hands
 fire:
  position: 5
+  name: Fire
  icon: fire
 readinglist:
  position: 5
--- a/testSetup.js
+++ b/testSetup.js
@ -27,6 +27,9 @@ expect.extend({
   * `undefined`, which makes it unsuitable for testing if a value exists as the
   * value might be `null` (which would pass a `toBeDefined` check).
   *
+   * Additionally this matcher will fail if its subject is a Promise, to remind
+   * the caller to await their Promises.
+   *
   * @param {any} subject The subject of the matcher
   * @returns
   */
@ -38,6 +41,18 @@ expect.extend({
      };
    }

+    // This only works with JS native Promises, but that's what Jest and Testing
+    // Library produce, so that's a reasonable tradeoff
+    if (subject instanceof Promise) {
+      return {
+        pass: false,
+        message: () =>
+          `Received a Promise (${this.utils.printReceived(
+            subject,
+          )}) instead of a value`,
+      };
+    }
+
    return {
      pass: true,
      message: () =>