Use internal addon representation and themeData (#3320)

willdurand · web-flow · commit 3d5f3d15c8a1 · 2017-10-03T20:39:35.000+02:00
diff --git a/src/amo/components/SearchResult/index.js b/src/amo/components/SearchResult/index.js
@@ -41,9 +41,9 @@ export class SearchResultBase extends React.Component {
 
     // Fall-back to default icon if invalid icon url.
     const iconURL = getAddonIconUrl(addon);
-    const themeURL = (addon && addon.theme_data &&
-      isAllowedOrigin(addon.theme_data.previewURL)) ?
-      addon.theme_data.previewURL : null;
+    const themeURL = (addon && addon.themeData &&
+      isAllowedOrigin(addon.themeData.previewURL)) ?
+      addon.themeData.previewURL : null;
     const imageURL = isTheme ? themeURL : iconURL;
 
     // Sets classes to handle fallback if theme preview is not available.
diff --git a/src/amo/reducers/addonsByAuthors.js b/src/amo/reducers/addonsByAuthors.js
@@ -1,5 +1,6 @@
 /* @flow */
-import type { AddonType } from 'core/types/addons';
+import type { AddonType, ExternalAddonType } from 'core/types/addons';
+import { createInternalAddon } from 'core/reducers/addons';
 
 
 type State = {
@@ -67,7 +68,7 @@ export const fetchOtherAddonsByAuthors = (
 
 type LoadOtherAddonsByAuthorsParams = {|
   slug: string,
-  addons: Array<AddonType>,
+  addons: Array<ExternalAddonType>,
 |};
 
 type LoadOtherAddonsByAuthorsAction = {|
@@ -118,7 +119,8 @@ const reducer = (
             // This ensures we do not display the main add-on in the list of
             // "add-ons by these authors".
             .filter((addon) => addon.slug !== action.payload.slug)
-            .slice(0, OTHER_ADDONS_BY_AUTHORS_PAGE_SIZE),
+            .slice(0, OTHER_ADDONS_BY_AUTHORS_PAGE_SIZE)
+            .map((addon) => createInternalAddon(addon)),
         },
       };
     default:
diff --git a/src/amo/reducers/landing.js b/src/amo/reducers/landing.js
@@ -2,6 +2,7 @@ import {
   LANDING_GET,
   LANDING_LOADED,
 } from 'core/constants';
+import { createInternalAddon } from 'core/reducers/addons';
 
 
 export const initialState = {
@@ -31,7 +32,7 @@ export default function landing(state = initialState, action) {
           newState[key] = {
             count: payload[key].result.count,
             results: payload[key].result.results.map((slug) => (
-              payload[key].entities.addons[slug]
+              createInternalAddon(payload[key].entities.addons[slug])
             )),
           };
         }
diff --git a/src/core/reducers/addons.js b/src/core/reducers/addons.js
@@ -4,7 +4,11 @@ import { oneLine } from 'common-tags';
 import { ADDON_TYPE_THEME } from 'core/constants';
 import type { ErrorHandlerType } from 'core/errorHandler';
 import log from 'core/logger';
-import type { AddonType, ExternalAddonType } from 'core/types/addons';
+import type {
+  AddonType,
+  ExternalAddonType,
+  ThemeData,
+} from 'core/types/addons';
 
 
 export const LOAD_ADDONS = 'LOAD_ADDONS';
@@ -80,6 +84,42 @@ export function removeUndefinedProps(object: Object): Object {
   return newObject;
 }
 
+export function createInternalThemeData(
+  apiAddon: ExternalAddonType
+): ThemeData | null {
+  if (!apiAddon.theme_data) {
+    return null;
+  }
+
+  return {
+    accentcolor: apiAddon.theme_data.accentcolor,
+    author: apiAddon.theme_data.author,
+    category: apiAddon.theme_data.category,
+
+    // TODO: Set this back to apiAddon.theme_data.description after
+    // https://github.com/mozilla/addons-frontend/issues/1416 is fixed.
+    // theme_data will contain `description: 'None'` when the description is
+    // actually `null` and we don't want to set that on the addon itself so we
+    // reset it in case it's been overwritten.
+    //
+    // See also https://github.com/mozilla/addons-server/issues/5650.
+    description: apiAddon.description,
+
+    detailURL: apiAddon.theme_data.detailURL,
+    footer: apiAddon.theme_data.footer,
+    footerURL: apiAddon.theme_data.footerURL,
+    header: apiAddon.theme_data.header,
+    headerURL: apiAddon.theme_data.headerURL,
+    iconURL: apiAddon.theme_data.iconURL,
+    id: apiAddon.theme_data.id,
+    name: apiAddon.theme_data.name,
+    previewURL: apiAddon.theme_data.previewURL,
+    textcolor: apiAddon.theme_data.textcolor,
+    updateURL: apiAddon.theme_data.updateURL,
+    version: apiAddon.theme_data.version,
+  };
+}
+
 export function createInternalAddon(
   apiAddon: ExternalAddonType
 ): AddonType {
@@ -138,41 +178,21 @@ export function createInternalAddon(
   };
 
   if (addon.type === ADDON_TYPE_THEME && apiAddon.theme_data) {
-    // This merges theme_data into the addon.
-    // TODO: Let's stop doing that because it's confusing. Lots of
-    // deep button / install code will need to be fixed.
-    addon = {
-      ...addon,
-      ...removeUndefinedProps({
-        accentcolor: apiAddon.theme_data.accentcolor,
-        author: apiAddon.theme_data.author,
-        category: apiAddon.theme_data.category,
-
-        // TODO: Set this back to apiAddon.theme_data.description after
-        // https://github.com/mozilla/addons-frontend/issues/1416
-        // is fixed.
-        // theme_data will contain `description: 'None'` when the
-        // description
-        // is actually `null` and we don't want to set that on the addon
-        // itself so we reset it in case it's been overwritten.
-        //
-        // See also https://github.com/mozilla/addons-server/issues/5650.
-        description: apiAddon.description,
-
-        detailURL: apiAddon.theme_data.detailURL,
-        footer: apiAddon.theme_data.footer,
-        footerURL: apiAddon.theme_data.footerURL,
-        header: apiAddon.theme_data.header,
-        headerURL: apiAddon.theme_data.headerURL,
-        iconURL: apiAddon.theme_data.iconURL,
-        id: apiAddon.theme_data.id,
-        name: apiAddon.theme_data.name,
-        previewURL: apiAddon.theme_data.previewURL,
-        textcolor: apiAddon.theme_data.textcolor,
-        updateURL: apiAddon.theme_data.updateURL,
-        version: apiAddon.theme_data.version,
-      }),
-    };
+    const themeData = createInternalThemeData(apiAddon);
+
+    if (themeData !== null) {
+      // This merges theme_data into the addon.
+      //
+      // TODO: Let's stop doing that because it's confusing. Lots of deep
+      // button/install code will need to be fixed.
+      //
+      // Use addon.themeData[themeProp] instead of addon[themeProp].
+      addon = {
+        ...addon,
+        ...removeUndefinedProps(themeData),
+        themeData,
+      };
+    }
   }
 
   if (apiAddon.current_version && apiAddon.current_version.files.length > 0) {
diff --git a/src/core/reducers/search.js b/src/core/reducers/search.js
@@ -2,6 +2,7 @@ import {
   SEARCH_STARTED,
   SEARCH_LOADED,
 } from 'core/constants';
+import { createInternalAddon } from 'core/reducers/addons';
 
 export const initialState = {
   count: 0,
@@ -26,7 +27,7 @@ export default function search(state = initialState, action) {
         count: payload.result.count,
         loading: false,
         results: payload.result.results.map((slug) => (
-          payload.entities.addons[slug]
+          createInternalAddon(payload.entities.addons[slug])
         )),
       };
     default:
diff --git a/src/core/types/addons.js b/src/core/types/addons.js
@@ -51,7 +51,7 @@ export type AddonAuthorType = {|
   username: string,
 |};
 
-type ThemeData = {|
+export type ThemeData = {|
   accentcolor?: string,
   author?: string,
   category?: string,
@@ -141,4 +141,5 @@ export type AddonType = {
     windows: ?string,
   },
   isRestartRequired: boolean,
+  themeData?: ThemeData,
 };
diff --git a/tests/unit/amo/components/TestAddon.js b/tests/unit/amo/components/TestAddon.js
@@ -1104,7 +1104,9 @@ describe(__filename, () => {
       const root = renderMoreAddons({ addon, addonsByAuthors });
 
       expect(root).not.toHaveClassName('.AddonDescription-more-addons--theme');
-      expect(root).toHaveProp('addons', addonsByAuthors);
+      expect(root).toHaveProp('addons', addonsByAuthors.map((addonByAuthor) => (
+        createInternalAddon(addonByAuthor)
+      )));
     });
 
     it('indicates when other add-ons are themes', () => {
@@ -1118,7 +1120,9 @@ describe(__filename, () => {
       const root = renderMoreAddons({ addon, addonsByAuthors });
 
       expect(root).toHaveClassName('.AddonDescription-more-addons--theme');
-      expect(root).toHaveProp('addons', addonsByAuthors);
+      expect(root).toHaveProp('addons', addonsByAuthors.map((addonByAuthor) => (
+        createInternalAddon(addonByAuthor)
+      )));
     });
 
     it('adds a CSS class to the main component when there are add-ons', () => {
diff --git a/tests/unit/amo/components/TestSearchResult.js b/tests/unit/amo/components/TestSearchResult.js
@@ -2,6 +2,7 @@ import { shallow } from 'enzyme';
 import React from 'react';
 
 import { SearchResultBase } from 'amo/components/SearchResult';
+import { createInternalAddon } from 'core/reducers/addons';
 import { fakeAddon } from 'tests/unit/amo/helpers';
 import { getFakeI18nInst } from 'tests/unit/helpers';
 import { ADDON_TYPE_THEME } from 'core/constants';
@@ -10,7 +11,7 @@ import Rating from 'ui/components/Rating';
 
 
 describe(__filename, () => {
-  const baseAddon = {
+  const baseAddon = createInternalAddon({
     ...fakeAddon,
     authors: [
       { name: 'A funky déveloper' },
@@ -19,7 +20,7 @@ describe(__filename, () => {
     average_daily_users: 5253,
     name: 'A search result',
     slug: 'a-search-result',
-  };
+  });
 
   function render({ addon = baseAddon, lang = 'en-GB', ...props } = {}) {
     return shallow(
@@ -46,7 +47,9 @@ describe(__filename, () => {
   });
 
   it('ignores an empty author list', () => {
-    const root = render({ addon: { ...fakeAddon, authors: undefined } });
+    const root = render({
+      addon: createInternalAddon({ ...fakeAddon, authors: undefined }),
+    });
 
     expect(root).not.toHaveClassName('SearchResult-author');
   });
@@ -73,7 +76,12 @@ describe(__filename, () => {
   });
 
   it('renders the user count as singular', () => {
-    const root = render({ addon: { ...fakeAddon, average_daily_users: 1 } });
+    const root = render({
+      addon: createInternalAddon({
+        ...fakeAddon,
+        average_daily_users: 1,
+      }),
+    });
 
     expect(root.find('.SearchResult-users')).toIncludeText('1 user');
   });
@@ -104,7 +112,7 @@ describe(__filename, () => {
   });
 
   it('displays a placeholder if the icon is malformed', () => {
-    const addon = { ...fakeAddon, icon_url: 'whatevs' };
+    const addon = createInternalAddon({ ...fakeAddon, icon_url: 'whatevs' });
     const root = render({ addon });
 
     // image `require` calls in jest return the filename
@@ -113,20 +121,23 @@ describe(__filename, () => {
   });
 
   it('adds a theme-specific class', () => {
-    const addon = {
+    const addon = createInternalAddon({
       ...fakeAddon,
       type: ADDON_TYPE_THEME,
       theme_data: {
         previewURL: 'https://addons.cdn.mozilla.net/user-media/addons/334902/preview_large.jpg?1313374873',
       },
-    };
+    });
     const root = render({ addon });
 
     expect(root).toHaveClassName('SearchResult--theme');
   });
 
   it('displays a message if the theme preview image is unavailable', () => {
-    const addon = { ...fakeAddon, type: ADDON_TYPE_THEME };
+    const addon = createInternalAddon({
+      ...fakeAddon,
+      type: ADDON_TYPE_THEME,
+    });
     const root = render({ addon });
 
     expect(root.find('.SearchResult-notheme'))
diff --git a/tests/unit/amo/reducers/testLanding.js b/tests/unit/amo/reducers/testLanding.js
@@ -1,10 +1,11 @@
 import { getLanding } from 'amo/actions/landing';
 import landing, { initialState } from 'amo/reducers/landing';
 import { ADDON_TYPE_THEME } from 'core/constants';
+import { createInternalAddon } from 'core/reducers/addons';
 import { fakeAddon } from 'tests/unit/amo/helpers';
 
 
-describe('landing reducer', () => {
+describe(__filename, () => {
   it('defaults to not loading', () => {
     const { loading } = landing(undefined, { type: 'unrelated' });
 
@@ -84,9 +85,9 @@ describe('landing reducer', () => {
     it('sets the results', () => {
       const entities = {
         addons: {
-          bar: { slug: 'bar' },
-          foo: { slug: 'foo' },
-          food: { slug: 'food' },
+          bar: { ...fakeAddon, slug: 'bar' },
+          foo: { ...fakeAddon, slug: 'foo' },
+          food: { ...fakeAddon, slug: 'food' },
         },
       };
       const state = landing(initialState, {
@@ -103,7 +104,10 @@ describe('landing reducer', () => {
       });
       expect(state.featured.count).toEqual(2);
       expect(state.featured.results)
-        .toEqual([{ slug: 'foo' }, { slug: 'food' }]);
+        .toEqual([
+          createInternalAddon({ ...fakeAddon, slug: 'foo' }),
+          createInternalAddon({ ...fakeAddon, slug: 'food' }),
+        ]);
       expect(state.highlyRated).toEqual({ count: 0, results: [] });
       expect(state.popular).toEqual({ count: 0, results: [] });
       expect(state.resultsLoaded).toEqual(true);
@@ -112,9 +116,9 @@ describe('landing reducer', () => {
     it('does not set null keys', () => {
       const entities = {
         addons: {
-          bar: { slug: 'bar' },
-          foo: { slug: 'foo' },
-          food: { slug: 'food' },
+          bar: { ...fakeAddon, slug: 'bar' },
+          foo: { ...fakeAddon, slug: 'foo' },
+          food: { ...fakeAddon, slug: 'food' },
         },
       };
       const { highlyRated } = landing({
diff --git a/tests/unit/amo/reducers/test_addonsByAuthors.js b/tests/unit/amo/reducers/test_addonsByAuthors.js
@@ -5,6 +5,7 @@ import reducer, {
   initialState,
   loadOtherAddonsByAuthors,
 } from 'amo/reducers/addonsByAuthors';
+import { createInternalAddon } from 'core/reducers/addons';
 import { fakeAddon } from 'tests/unit/amo/helpers';
 
 
@@ -42,7 +43,7 @@ describe(__filename, () => {
         addons: [fakeAddon],
       }));
       expect(state.byAddonSlug).toEqual({
-        'addon-slug': [fakeAddon],
+        'addon-slug': [createInternalAddon(fakeAddon)],
       });
     });
 
@@ -75,7 +76,7 @@ describe(__filename, () => {
         slug,
       }));
       expect(previousState.byAddonSlug)
-        .toEqual({ 'addon-slug': [fakeAddon] });
+        .toEqual({ 'addon-slug': [createInternalAddon(fakeAddon)] });
 
       const state = reducer(previousState, fetchOtherAddonsByAuthors({
         authors: ['author1'],
diff --git a/tests/unit/core/reducers/test_addons.js b/tests/unit/core/reducers/test_addons.js
diff --git a/tests/unit/core/reducers/test_search.js b/tests/unit/core/reducers/test_search.js
diff --git a/tests/unit/disco/containers/TestDiscoPane.js b/tests/unit/disco/containers/TestDiscoPane.js
