Address review comments

mozilla · Oct 24, 2019 · 68c9039 · 68c9039
1 parent 1fe60ff
commit 68c9039
Show file tree

Hide file tree

Showing 15 changed files with 115 additions and 146 deletions.
diff --git a/src/amo/components/ErrorPage/NotAuthorized/index.js b/src/amo/components/ErrorPage/NotAuthorized/index.js
@@ -33,7 +33,7 @@ export class NotAuthorizedBase extends React.Component<Props> {
     /* eslint-disable react/no-danger */
     return (
       <NestedStatus code={401}>
-        <Page>
+        <Page showWrongPlatformWarning={false}>
           <Card
             className="ErrorPage NotAuthorized"
             header={i18n.gettext('Not Authorized')}

diff --git a/src/amo/components/ErrorPage/NotFound/index.js b/src/amo/components/ErrorPage/NotFound/index.js
@@ -64,7 +64,7 @@ export class NotFoundBase extends React.Component<InternalProps> {
 
     return (
       <NestedStatus code={404}>
-        <Page>
+        <Page showWrongPlatformWarning={false}>
           <Card
             className="ErrorPage NotFound"
             header={i18n.gettext('Oops! We can’t find that page')}

diff --git a/src/amo/components/ErrorPage/ServerError/index.js b/src/amo/components/ErrorPage/ServerError/index.js
@@ -28,7 +28,7 @@ export class ServerErrorBase extends React.Component {
     /* eslint-disable react/no-danger */
     return (
       <NestedStatus code={500}>
-        <Page>
+        <Page showWrongPlatformWarning={false}>
           <Card
             className="ErrorPage ServerError"
             header={i18n.gettext('Server Error')}

diff --git a/src/amo/components/Page/index.js b/src/amo/components/Page/index.js
@@ -63,13 +63,7 @@ export const PageBase = ({
           (!isHomePage ||
             !enableFeatureHeroRecommendation ||
             clientApp === CLIENT_APP_ANDROID) && <AppBanner />}
-          {// Exclude the WrongPlatformWarning from the home page if it will be
-          // included via HeroRecommendation or if showWrongPlatformWarning is
-          // false, but include it on the Android home page.
-          (!isHomePage ||
-            !enableFeatureHeroRecommendation ||
-            clientApp === CLIENT_APP_ANDROID) &&
-            showWrongPlatformWarning && <WrongPlatformWarning />}
+          {showWrongPlatformWarning && <WrongPlatformWarning />}
           {children}
         </div>
       </div>

diff --git a/src/amo/components/Routes/index.js b/src/amo/components/Routes/index.js
@@ -192,7 +192,7 @@ const Routes = ({ _config = config }: Props = {}) => (
       exact
       path="/:lang/:application/simulate-async-error/"
       component={() => (
-        <Page>
+        <Page showWrongPlatformWarning={false}>
           <SimulateAsyncError />
         </Page>
       )}
@@ -201,7 +201,7 @@ const Routes = ({ _config = config }: Props = {}) => (
       exact
       path="/:lang/:application/simulate-sync-error/"
       component={() => (
-        <Page>
+        <Page showWrongPlatformWarning={false}>
           <SimulateSyncError />
         </Page>
       )}
@@ -210,7 +210,7 @@ const Routes = ({ _config = config }: Props = {}) => (
       exact
       path="/:lang/:application/simulate-client-error/"
       component={() => (
-        <Page>
+        <Page showWrongPlatformWarning={false}>
           <SimulateClientError />
         </Page>
       )}

diff --git a/src/amo/components/StaticPage/index.js b/src/amo/components/StaticPage/index.js
@@ -19,7 +19,7 @@ const StaticPage = (props: Props) => {
   const { title, metaDescription, children } = props;
 
   return (
-    <Page>
+    <Page showWrongPlatformWarning={false}>
       <Card className="StaticPage" header={title}>
         <Helmet>
           <title>{title}</title>

diff --git a/src/amo/components/WrongPlatformWarning/index.js b/src/amo/components/WrongPlatformWarning/index.js
@@ -19,7 +19,8 @@ import './styles.scss';
 
 type Props = {|
   className?: string,
-  forAddonDetailPage?: boolean,
+  fixAndroidLinkMessage?: string,
+  fixFirefoxLinkMessage?: string,
 |};
 
 type InternalProps = {|
@@ -34,15 +35,13 @@ type InternalProps = {|
 export class WrongPlatformWarningBase extends React.Component<InternalProps> {
   static defaultProps = {
     _correctedLocationForPlatform: correctedLocationForPlatform,
-    forAddonDetailPage: false,
   };
 
   render() {
     const {
       _correctedLocationForPlatform,
       className,
       clientApp,
-      forAddonDetailPage,
       i18n,
       location,
       userAgentInfo,
@@ -58,43 +57,24 @@ export class WrongPlatformWarningBase extends React.Component<InternalProps> {
       return null;
     }
 
-    let message;
+    const fixAndroidLinkMessage =
+      this.props.fixAndroidLinkMessage ||
+      i18n.gettext(
+        `To find add-ons compatible with Firefox on Android,
+               <a href="%(newLocation)s">visit our mobile site</a>.`,
+      );
 
-    if (forAddonDetailPage) {
-      message =
-        clientApp === CLIENT_APP_ANDROID
-          ? i18n.sprintf(
-              i18n.gettext(
-                `This add-on is not compatible with this platform.
-               <a href="%(newLocation)s">Browse add-ons for Firefox on desktop</a>.`,
-              ),
-              { newLocation },
-            )
-          : i18n.sprintf(
-              i18n.gettext(
-                `This add-on is not compatible with this platform.
-               <a href="%(newLocation)s">Browse add-ons for Firefox on Android</a>.`,
-              ),
-              { newLocation },
-            );
-    } else {
-      message =
-        clientApp === CLIENT_APP_ANDROID
-          ? i18n.sprintf(
-              i18n.gettext(
-                `To find add-ons compatible with Firefox on desktop,
+    const fixFirefoxLinkMessage =
+      this.props.fixFirefoxLinkMessage ||
+      i18n.gettext(
+        `To find add-ons compatible with Firefox on desktop,
                <a href="%(newLocation)s">visit our desktop site</a>.`,
-              ),
-              { newLocation },
-            )
-          : i18n.sprintf(
-              i18n.gettext(
-                `To find add-ons compatible with Firefox on Android,
-               <a href="%(newLocation)s">visit our mobile site</a>.`,
-              ),
-              { newLocation },
-            );
-    }
+      );
+
+    const message =
+      clientApp === CLIENT_APP_ANDROID
+        ? i18n.sprintf(fixFirefoxLinkMessage, { newLocation })
+        : i18n.sprintf(fixAndroidLinkMessage, { newLocation });
 
     return (
       <div className={makeClassName('WrongPlatformWarning', className)}>

diff --git a/src/amo/pages/Addon/index.js b/src/amo/pages/Addon/index.js
@@ -482,7 +482,17 @@ export class AddonBase extends React.Component {
                   {i18n.gettext('Extension Metadata')}
                 </h2>
               </header>
-              <WrongPlatformWarning forAddonDetailPage />
+              <WrongPlatformWarning
+                className="Addon-WrongPlatformWarning"
+                fixAndroidLinkMessage={i18n.gettext(
+                  `This add-on is not compatible with this platform.
+                    <a href="%(newLocation)s">Browse add-ons for Firefox on Android</a>.`,
+                )}
+                fixFirefoxLinkMessage={i18n.gettext(
+                  `This add-on is not compatible with this platform.
+                    <a href="%(newLocation)s">Browse add-ons for Firefox on desktop</a>.`,
+                )}
+              />
               {addon && <InstallWarning addon={addon} />}
             </Card>
 

diff --git a/src/amo/pages/Addon/styles.scss b/src/amo/pages/Addon/styles.scss
@@ -178,7 +178,7 @@
   // }
 }
 
-.Addon .WrongPlatformWarning {
+.Addon-WrongPlatformWarning {
   margin: 12px 0 0 0;
   padding-left: 0;
   padding-right: 0;

diff --git a/src/amo/pages/Home/index.js b/src/amo/pages/Home/index.js
@@ -228,8 +228,12 @@ export class HomeBase extends React.Component {
       return null;
     };
 
+    const showHero =
+      _config.get('enableFeatureHeroRecommendation') &&
+      clientApp !== CLIENT_APP_ANDROID;
+
     return (
-      <Page isHomePage>
+      <Page isHomePage showWrongPlatformWarning={!showHero}>
         <div className="Home">
           <HeadMetaTags
             description={i18n.gettext(`Download Firefox extensions and themes.

diff --git a/src/core/constants.js b/src/core/constants.js
@@ -309,5 +309,3 @@ export const LTR = 'ltr';
 
 export const AMO_REQUEST_ID_HEADER = 'amo-request-id';
 export const DISCO_TAAR_CLIENT_ID_HEADER = 'moz-client-id';
-
-export const USER_AGENT_BROWSER_FIREFOX = 'Firefox';
diff --git a/src/core/reducers/api.js b/src/core/reducers/api.js
@@ -19,6 +19,7 @@ import type {
 } from 'core/actions/index';
 import type { Exact } from 'core/types/util';
 
+export const USER_AGENT_BROWSER_FIREFOX = 'Firefox';
 export const USER_AGENT_OS_ANDROID: 'Android' = 'Android';
 export const USER_AGENT_OS_BSD_DRAGONFLY: 'DragonFly' = 'DragonFly';
 export const USER_AGENT_OS_BSD_FREEBSD: 'FreeBSD' = 'FreeBSD';

diff --git a/src/core/utils/compatibility.js b/src/core/utils/compatibility.js
@@ -4,7 +4,11 @@ import { oneLine } from 'common-tags';
 import invariant from 'invariant';
 import mozCompare from 'mozilla-version-comparator';
 
-import { USER_AGENT_OS_ANDROID } from 'core/reducers/api';
+import {
+  USER_AGENT_BROWSER_FIREFOX,
+  USER_AGENT_OS_ANDROID,
+  USER_AGENT_OS_IOS,
+} from 'core/reducers/api';
 import {
   ADDON_TYPE_EXTENSION,
   ADDON_TYPE_OPENSEARCH,
@@ -18,7 +22,6 @@ import {
   INCOMPATIBLE_OVER_MAX_VERSION,
   INCOMPATIBLE_UNDER_MIN_VERSION,
   INCOMPATIBLE_UNSUPPORTED_PLATFORM,
-  USER_AGENT_BROWSER_FIREFOX,
 } from 'core/constants';
 import { findInstallURL } from 'core/installAddon';
 import log from 'core/logger';
@@ -317,29 +320,34 @@ export const correctedLocationForPlatform = ({
 
   const { browser, os } = userAgentInfo;
 
-  let newLocation = null;
+  let currentClientApp;
+  let newClientApp;
+
+  if (os.name === USER_AGENT_OS_IOS) {
+    return null;
+  }
 
   if (
     os.name === USER_AGENT_OS_ANDROID &&
     browser.name === USER_AGENT_BROWSER_FIREFOX &&
     clientApp === CLIENT_APP_FIREFOX
   ) {
-    newLocation = location.pathname.replace(
-      CLIENT_APP_FIREFOX,
-      CLIENT_APP_ANDROID,
-    );
+    currentClientApp = CLIENT_APP_FIREFOX;
+    newClientApp = CLIENT_APP_ANDROID;
   }
 
   if (
     os.name !== USER_AGENT_OS_ANDROID &&
     browser.name === USER_AGENT_BROWSER_FIREFOX &&
     clientApp === CLIENT_APP_ANDROID
   ) {
-    newLocation = location.pathname.replace(
-      CLIENT_APP_ANDROID,
-      CLIENT_APP_FIREFOX,
-    );
+    currentClientApp = CLIENT_APP_ANDROID;
+    newClientApp = CLIENT_APP_FIREFOX;
   }
 
-  return newLocation && `${newLocation}${location.search}`;
+  return currentClientApp && newClientApp
+    ? `${location.pathname.replace(currentClientApp, newClientApp)}${
+        location.search
+      }`
+    : null;
 };
diff --git a/tests/unit/amo/components/TestWrongPlatformWarning.js b/tests/unit/amo/components/TestWrongPlatformWarning.js
@@ -85,50 +85,28 @@ describe(__filename, () => {
     });
   });
 
-  // This is the test config for the four possible states of the message, the format is:
-  // [clientApp, forAddonDetailPage, [array of expected message text]
-  const messageTestData = [
+  it.each([
     [
       CLIENT_APP_ANDROID,
-      false,
       [
         'To find add-ons compatible with Firefox on desktop',
         'visit our desktop site',
       ],
     ],
     [
       CLIENT_APP_FIREFOX,
-      false,
       [
         'To find add-ons compatible with Firefox on Android',
         'visit our mobile site',
       ],
     ],
-    [
-      CLIENT_APP_ANDROID,
-      true,
-      [
-        'This add-on is not compatible with this platform',
-        'Browse add-ons for Firefox on desktop',
-      ],
-    ],
-    [
-      CLIENT_APP_FIREFOX,
-      true,
-      [
-        'This add-on is not compatible with this platform',
-        'Browse add-ons for Firefox on Android',
-      ],
-    ],
-  ];
-
-  it.each(messageTestData)(
-    'generates the expected message when clientApp is %s and forAddonDetailPage is %s',
-    (clientApp, forAddonDetailPage, expectedText) => {
+  ])(
+    'generates the expected message when clientApp is %s',
+    (clientApp, expectedText) => {
       const newLocation = '/some/location/';
       _correctedLocationForPlatform.returns(newLocation);
       _dispatchClientMetadata({ clientApp });
-      const root = render({ forAddonDetailPage });
+      const root = render();
 
       for (const text of expectedText) {
         expect(root.find('.WrongPlatformWarning-message').html()).toContain(
@@ -141,13 +119,23 @@ describe(__filename, () => {
     },
   );
 
-  it('defaults forAddonDetailPage to `false`', () => {
-    _correctedLocationForPlatform.returns('/some/location/');
-    _dispatchClientMetadata({ clientApp: CLIENT_APP_ANDROID });
-    const root = render();
+  it.each([CLIENT_APP_ANDROID, CLIENT_APP_FIREFOX])(
+    'can display a custom message when clientApp is %s',
+    (clientApp) => {
+      const newLocation = '/some/location/';
+      _correctedLocationForPlatform.returns(newLocation);
+      _dispatchClientMetadata({ clientApp });
+      const fixAndroidLinkMessage =
+        'A message to show when clientApp is firefox';
+      const fixFirefoxLinkMessage =
+        'A message to show when clientApp is android';
+      const root = render({ fixAndroidLinkMessage, fixFirefoxLinkMessage });
 
-    expect(root.find('.WrongPlatformWarning-message').html()).toContain(
-      'To find add-ons compatible with Firefox on desktop',
-    );
-  });
+      expect(root.find('.WrongPlatformWarning-message').html()).toContain(
+        clientApp === CLIENT_APP_ANDROID
+          ? fixFirefoxLinkMessage
+          : fixAndroidLinkMessage,
+      );
+    },
+  );
 });
Original file line number	Diff line number	Diff line change
Expand Up		@@ -309,5 +309,3 @@ export const LTR = 'ltr';

		export const AMO_REQUEST_ID_HEADER = 'amo-request-id';
		export const DISCO_TAAR_CLIENT_ID_HEADER = 'moz-client-id';

		export const USER_AGENT_BROWSER_FIREFOX = 'Firefox';