feat: Add a link to the stats dashboard for add-on owners #3203

tofumatt · 2017-09-22T01:52:05Z

Links to the stats dashboard from the user count on the AddonMeta section and also adds a link in the dashboard.

In order to see this locally you'll be signed in and visit an add-on you own.

Screenshots

codecov-io · 2017-09-22T01:52:09Z

Codecov Report

Merging #3203 into master will increase coverage by 0.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#3203      +/-   ##
==========================================
+ Coverage   95.41%   95.43%   +0.01%     
==========================================
  Files         160      160              
  Lines        3013     3021       +8     
  Branches      594      595       +1     
==========================================
+ Hits         2875     2883       +8     
  Misses        119      119              
  Partials       19       19

Impacted Files	Coverage Δ
src/core/utils.js	`100% <100%> (ø)`	⬆️
src/amo/components/AddonMoreInfo/index.js	`100% <100%> (ø)`	⬆️
src/amo/components/AddonMeta.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d706af...d558fef. Read the comment docs.

tofumatt · 2017-09-22T10:24:47Z

Failing test is mozilla/addons#10799 again.

kumar303

Looks good. Adding createInternalAddon() is my only major change request -- the rest is cleanup.

kumar303 · 2017-09-25T13:28:26Z

src/amo/components/AddonMeta.js

@@ -1,22 +1,28 @@
 import React from 'react';


This needs /* @flow */ if you're going to use it for prop types.

kumar303 · 2017-09-25T13:31:34Z

src/amo/components/AddonMeta.js

@@ -66,6 +79,13 @@ export class AddonMetaBase extends React.Component {
  }
 }

+export const mapStateToProps = (state) => {
+  return {
+    userId: state.user ? state.user.id : null,


Due to the beauty of Redux, state.user will never be falsy. You can just do userId: state.user.id -- id will be null when the user is not signed in.

kumar303 · 2017-09-25T13:33:43Z

src/amo/components/AddonMoreInfo/index.js

@@ -164,6 +180,13 @@ export class AddonMoreInfoBase extends React.Component {
  }
 }

+export const mapStateToProps = (state) => {
+  return {
+    userId: state.user ? state.user.id : null,


same thing here

kumar303 · 2017-09-25T13:38:51Z

tests/unit/amo/components/TestAddonMeta.js


 describe('<AddonMeta>', () => {
+  function render({
+    addon = fakeAddon,


We should get into the habit of doing addon = createInternalAddon(fakeAddon) because the internal addon object is what this component will receive. You can import it from https://github.com/mozilla/addons-frontend/blob/master/src/core/reducers/addons.js#L83

Should fakeAddon just get wrapped in createInternalAddon()? I noticed you doing that in another test file a lot and it seems like boilerplate. Maybe we want a helper like fakeAddon({ ...extraProps }) instead of the object we manipulate now.

kumar303 · 2017-09-25T13:39:16Z

tests/unit/amo/components/TestAddonMeta.js

@@ -53,6 +65,31 @@ describe('<AddonMeta>', () => {
      });
      expect(getUserCount(root)).toMatch(/^1\.000/);
    });
+
+    it('links to stats if add-on author is viewing the page', () => {
+      const addon = {


this should also be const addon = createInternalAddon({ ...fakeAddon, ... })

kumar303 · 2017-09-25T13:47:37Z

tests/unit/amo/components/TestAddonMoreInfo.js

@@ -182,4 +187,29 @@ describe(__filename, () => {
    expect(root.find('.AddonMoreInfo-database-id-content'))
      .toHaveText('9001');
  });
+
+  it('links to stats if add-on author is viewing the page', () => {
+    const addon = {


This should be addon = createInternalAddon({ ...fakeAddon, slug: ... }).

Also, while you're in there could you fix the render() function?

function render(props) { return shallowUntilTarget( <AddonMoreInfo addon={createInternalAddon(fakeAddon)} i18n={getFakeI18nInst()} store={store} {...props} />, AddonMoreInfoBase ); }

kumar303 · 2017-09-25T13:48:28Z

tests/unit/amo/components/TestAddonMoreInfo.js

+      slug: 'coolio',
+      authors: [
+        {
+          id: 11,


same comment here about extending from fakeAddon.authors[0]

kumar303 · 2017-09-25T13:48:43Z

tests/unit/amo/components/TestAddonMoreInfo.js

+    };
+    const root = render({
+      addon,
+      store: dispatchSignInActions({ userId: 11 }).store,


same comment here about referencing a variable for userId.

kumar303 · 2017-09-25T13:53:34Z

tests/unit/core/test_utils.js

+  });
+
+  it('returns false when addon is not set', () => {
+    expect(isAddonAuthor({ addon: null, userId: 5838591 })).toEqual(false);


Since the code relies on falsyness (not just null) then I think you need another test for the addon: undefined case.

Ideally the function should throw if addon or userId is undefined (since it could have been a calling mistake) but I think React might actually set addon to undefined in some cases because JavaScript is awful.

kumar303 · 2017-09-25T13:56:31Z

tests/unit/core/test_utils.js

+  });
+
+  it('returns false if add-on has no authors', () => {
+    const partialAddon = { ...addon, authors: [] };


I don't see authors: null covered which is in the if branch. However, I don't know if that's very realistic.

It will work, so I added a test case. I'm not sure either but good to test it to make sure.

willdurand

@kumar303 reviewed this PR in the meantime, so I only leave comments :)

willdurand · 2017-09-25T13:34:27Z

src/amo/components/AddonMeta.js

+type PropTypes = {
+  addon: AddonType | null,
+  i18n: Object,
+  userId: number | null,


Not sure we have discussed this yet (at least not with me), but Flow users tend to use ?number to mark a nullable/maybe value: https://flow.org/en/docs/types/maybe/.

Also I would go for strict typing if possible.

I guess it's up to the design of the component. If it were userId? then undefined would be allowed. I think allowing undefined is risky and we should avoid it whenever possible. I like using null values to indicate that the property value is still loading.

willdurand · 2017-09-25T13:35:52Z

src/amo/components/AddonMoreInfo/index.js

-    addon: PropTypes.object.isRequired,
-    i18n: PropTypes.object.isRequired,
-  }
+  props: PropTypes;


willdurand · 2017-09-25T13:39:44Z

src/amo/components/AddonMeta.js

@@ -49,7 +55,14 @@ export class AddonMetaBase extends React.Component {
        <div className="AddonMeta-item AddonMeta-users">
          <h3 className="visually-hidden">{i18n.gettext('Used by')}</h3>
          <p className="AddonMeta-text AddonMeta-user-count">
-            {userCount}
+            {addon && isAddonAuthor({ addon, userId }) ? (


There is probably no need for addon && because it is checked in the isAddonAuthor() function.

Annoyingly, flow can't understand I won't access that property unless isAddonAuthor is true (which it never would be if addon were null), so I left this in.

willdurand · 2017-09-25T13:40:28Z

tests/unit/amo/components/TestAddonMeta.js

-    ...customProps,
-  };
-  return shallow(<AddonMetaBase {...props} />);
-}

 describe('<AddonMeta>', () => {


You could update the describe name to use __filename if you wish.

willdurand · 2017-09-25T13:42:10Z

tests/unit/amo/components/TestAddonMeta.js

+import Link from 'amo/components/Link';
+import {
+  dispatchSignInActions,
+  dispatchClientMetadata,


This should be imported first.

NIIIIIICE catch 👍 🤣

willdurand · 2017-09-25T13:48:31Z

tests/unit/amo/components/TestAddonMeta.js

@@ -53,6 +65,31 @@ describe('<AddonMeta>', () => {
      });
      expect(getUserCount(root)).toMatch(/^1\.000/);
    });
+
+    it('links to stats if add-on author is viewing the page', () => {


Do we have the opposite test case somewhere?

Good catch, we didn't. I added one.

willdurand · 2017-09-25T13:49:11Z

tests/unit/amo/components/TestAddonMoreInfo.js

+import Link from 'amo/components/Link';
+import {
+  dispatchSignInActions,
+  dispatchClientMetadata,


This should be imported first.

willdurand · 2017-09-25T13:53:58Z

tests/unit/amo/components/TestAddonMeta.js

+    it('links to stats if add-on author is viewing the page', () => {
+      const addon = {
+        ...fakeAddon,
+        slug: 'coolio',


willdurand · 2017-09-25T13:55:05Z

tests/unit/core/test_utils.js

+    expect(isAddonAuthor({ addon, userId: 5838591 })).toEqual(true);
+  });
+
+  it('returns true when userId is not in add-on author object', () => {


it returns false

willdurand · 2017-09-25T13:55:52Z

tests/unit/core/test_utils.js

+    expect(isAddonAuthor({ addon, userId: 5838592 })).toEqual(false);
+  });
+
+  it('returns false when addon is not set', () => {


when addon is null

"not set" could be tested with addon: undefined no?

tofumatt · 2017-09-26T01:33:38Z

Thanks for the reviews, both. Ready for another r?

tofumatt · 2017-09-26T16:03:45Z

This is all rebased and ready for a final r? before the tag if possible 😄

kumar303

The design of renderDefinitions() is that it's a dumb method. You made it a little too smart :) You pass it definitions to render and that's it. This design allows the loading state to work correctly.

I think it will be a little confusing if we land your changes but I rewrote it for you with more dumb. Let me know what you think. This patch could also be applied as a follow-up on master, your call.

r+ with this patch.

diff --git a/src/amo/components/AddonMoreInfo/index.js b/src/amo/components/AddonMoreInfo/index.js
index d2a47292..0e254c21 100644
--- a/src/amo/components/AddonMoreInfo/index.js
+++ b/src/amo/components/AddonMoreInfo/index.js
@@ -24,7 +24,7 @@ export class AddonMoreInfoBase extends React.Component {
   props: PropTypes;
 
   listContent() {
-    const { addon, i18n } = this.props;
+    const { addon, i18n, userId } = this.props;
 
     if (!addon) {
       return this.renderDefinitions({
@@ -109,6 +109,14 @@ export class AddonMoreInfoBase extends React.Component {
           {i18n.gettext('See all beta versions')}
         </a>
       ) : null,
+      statsLink: addon && isAddonAuthor({ addon, userId }) ? (
+        <a
+          className="AddonMoreInfo-stats-link"
+          href={`/addon/${addon.slug}/statistics/`}
+        >
+          {i18n.gettext('Visit stats dashboard')}
+        </a>
+      ) : null,
     });
   }
 
@@ -123,8 +131,9 @@ export class AddonMoreInfoBase extends React.Component {
     versionHistoryLink,
     betaVersionsLink = null,
     addonId,
+    statsLink = null,
   }: Object) {
-    const { addon, i18n, userId } = this.props;
+    const { i18n } = this.props;
     return (
       <dl className="AddonMoreInfo-contents">
         {homepage || supportUrl ? (
@@ -176,18 +185,12 @@ export class AddonMoreInfoBase extends React.Component {
           </dt>
         ) : null}
         {betaVersionsLink ? <dd>{betaVersionsLink}</dd> : null}
-        {addon && isAddonAuthor({ addon, userId }) ? (
+        {statsLink ? (
           <dt className="AddonMoreInfo-stats-title">
             {i18n.gettext('Usage Statistics')}
           </dt>
         ) : null}
-        {addon && isAddonAuthor({ addon, userId }) ? (
-          <dd className="AddonMoreInfo-stats-content">
-            <Link href={`/addon/${addon.slug}/statistics/`}>
-              {i18n.gettext('Visit stats dashboard')}
-            </Link>
-          </dd>
-        ) : null}
+        {statsLink ? <dd>{statsLink}</dd> : null}
         <dt
           className="AddonMoreInfo-database-id-title"
           title={i18n.gettext(`This ID is useful for debugging and
diff --git a/tests/unit/amo/components/TestAddonMoreInfo.js b/tests/unit/amo/components/TestAddonMoreInfo.js
index 7ada6cd1..10be6da7 100644
--- a/tests/unit/amo/components/TestAddonMoreInfo.js
+++ b/tests/unit/amo/components/TestAddonMoreInfo.js
@@ -4,7 +4,6 @@ import React from 'react';
 import AddonMoreInfo, {
   AddonMoreInfoBase,
 } from 'amo/components/AddonMoreInfo';
-import Link from 'amo/components/Link';
 import { createInternalAddon } from 'core/reducers/addons';
 import {
   dispatchClientMetadata,
@@ -224,7 +223,7 @@ describe(__filename, () => {
       store: dispatchSignInActions({ userId: 5 }).store,
     });
 
-    const statsLink = root.find('.AddonMoreInfo-stats-content').find(Link);
+    const statsLink = root.find('.AddonMoreInfo-stats-link');
     expect(statsLink).toHaveLength(0);
   });
 
@@ -249,7 +248,7 @@ describe(__filename, () => {
       store: dispatchSignInActions({ userId: authorUserId }).store,
     });
 
-    const statsLink = root.find('.AddonMoreInfo-stats-content').find(Link);
+    const statsLink = root.find('.AddonMoreInfo-stats-link');
     expect(statsLink).toHaveLength(1);
     expect(statsLink).toHaveProp('children', 'Visit stats dashboard');
     expect(statsLink).toHaveProp('href', '/addon/coolio/statistics/');

kumar303 · 2017-09-26T17:02:54Z

Oh, also, in my patch I changed Link to a because it will link to an external site (the legacy one), correct? That's how the other definitions were being rendered.

fixes #2751

tofumatt requested a review from kumar303 September 22, 2017 01:52

tofumatt force-pushed the stats-link-2751 branch from 29d0606 to e3f014c Compare September 22, 2017 01:52

willdurand requested review from willdurand and removed request for kumar303 September 25, 2017 13:33

kumar303 suggested changes Sep 25, 2017

View reviewed changes

willdurand reviewed Sep 25, 2017

View reviewed changes

tofumatt mentioned this pull request May 5, 2024

Make fakeAddon a helper using createInternalAddon, not an object mozilla/addons#10819

Closed

tofumatt force-pushed the stats-link-2751 branch from e3f014c to 9247b29 Compare September 26, 2017 01:32

tofumatt requested a review from kumar303 September 26, 2017 01:33

tofumatt force-pushed the stats-link-2751 branch 2 times, most recently from 70fdffd to ab82c33 Compare September 26, 2017 15:47

kumar303 suggested changes Sep 26, 2017

View reviewed changes

tofumatt force-pushed the stats-link-2751 branch from ab82c33 to b1fe89b Compare September 26, 2017 18:39

tofumatt added 2 commits September 26, 2017 19:58

fix: Do not increase link text size

a76f9dc

feat: Add link to stats for add-on developer

7c13be3

fixes #2751

tofumatt force-pushed the stats-link-2751 branch from b1fe89b to 7c13be3 Compare September 26, 2017 18:58

chore: Convert <a> tags to <Link> components

d558fef

tofumatt merged commit fa871c7 into master Sep 26, 2017

tofumatt deleted the stats-link-2751 branch September 26, 2017 20:31

feat: Add a link to the stats dashboard for add-on owners #3203

feat: Add a link to the stats dashboard for add-on owners #3203

Conversation

tofumatt commented Sep 22, 2017

Screenshots

codecov-io commented Sep 22, 2017 • edited

Codecov Report

tofumatt commented Sep 22, 2017

kumar303 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willdurand left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented Sep 26, 2017

tofumatt commented Sep 26, 2017

kumar303 left a comment

Choose a reason for hiding this comment

kumar303 commented Sep 26, 2017

codecov-io commented Sep 22, 2017 •

edited

willdurand left a comment •

edited