I4852-fetch logic adjustment on addons by authors card #4992

rebmullin · 2018-05-14T16:23:25Z

fixes #4852 - fixes item count on addonsByAuthorCard component when slug is updated

b/c the component was remounting the logic in componentWillReceiveProps was never different so the refetching logic wasn't hitting here. I adjusted logic to (re)fetch always on mount

codecov-io · 2018-05-14T16:32:18Z

Codecov Report

Merging #4992 into master will decrease coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4992      +/-   ##
==========================================
- Coverage   96.62%   96.62%   -0.01%     
==========================================
  Files         213      213              
  Lines        5007     5006       -1     
  Branches     1002     1001       -1     
==========================================
- Hits         4838     4837       -1     
  Misses        151      151              
  Partials       18       18

Impacted Files	Coverage Δ
src/amo/components/AddonsByAuthorsCard/index.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 675803e...9ea3da9. Read the comment docs.

bobsilverberg · 2018-05-15T15:19:35Z

src/amo/components/AddonsByAuthorsCard/index.js

-    }
-  }
-
-  componentWillReceiveProps({


I haven't run the code yet, but at first glance this doesn't look right. You've removed a bunch of code, and the associated tests, which suggests to me that those use cases are no longer supported. Why is that ok? Have you tested the cases for which you've removed code to make sure they still work?

Please let me know your response to this and then I'll proceed with the rest of the review.

Hey @bobsilverberg - sorry I wasn’t too clear above. Basically tho this component (AddonsByAuthorsCard) gets remounted so when it runs through componentWillReceiveProps, the nextProps are never different from the this.props. This means the fetch logic located within this conditional - which compares old and new props - never gets reached. This is why it wasn't refetching the data..

I see the code was extracted out recently from Addons (code update) and checking old vs newProps worked there but again b/c of remounting it doesn't when moved into its own component.

It’s a big component but I can look into why Addons is remounting this component. I know I can prevent with shouldComponentMount but not sure if will give us what we need either (or its ramifications here). Anyway maybe we can discuss more tomorrow on a call to sort this out and make sure I move forward with the best approach? :)

Regarding tests...I thought I was removing the tests on this check and moving into the mount test. I always keep yarn test running to make sure nothing I didn’t break anything altho maybe it sounds like maybe I need to do more..

So I don't think we can do this because if you use the browser "back" button between different addon detail pages, componentWillReceiveProps() will definitely be called and we should be using it.

Ahh..that's something i didn't see/test. thanks for catching that. I will add that back in here and return those tests

willdurand · 2018-05-17T12:11:53Z

src/amo/components/AddonsByAuthorsCard/index.js

-    }
-  }
-
-  componentWillReceiveProps({


So I don't think we can do this because if you use the browser "back" button between different addon detail pages, componentWillReceiveProps() will definitely be called and we should be using it.

willdurand · 2018-05-17T12:12:24Z

src/amo/components/AddonsByAuthorsCard/index.js

@@ -55,43 +54,13 @@ export class AddonsByAuthorsCardBase extends React.Component<Props> {
  }

  componentWillMount() {


I think it would maybe make sense to always call dispatchFetchAddonsByAuthors() in componentWillMount().

rebmullin · 2018-05-17T17:40:59Z

hey @willdurand - thanks for the feedback. I've made some updates. Can you please take another look?

willdurand

I guess the change is correct but the test case needs to be updated.

willdurand · 2018-05-17T21:03:14Z

tests/unit/amo/components/TestAddonsByAuthorsCard.js

@@ -232,6 +232,28 @@ describe(__filename, () => {
    }));
  });

+  it('dispatches fetchAddonsByAuthors on mount', () => {


So I think we already have such a test case. What we would like to have now is a test case that fails when you revert your change in src/amo/components/AddonsByAuthorsCard/index.js, i.e. trying to render the component with a list of add-ons.

thanks for feedback @willdurand! I just need a little bit of clarification : ) Initially it was only fetching on mount if there were NO add-ons. So I am thinking maybe the following test should be removed:

it('should dispatch a fetch action if no add-ons found', () => { const { store } = dispatchClientMetadata(); const dispatchSpy = sinon.spy(store, 'dispatch'); const errorHandler = createStubErrorHandler(); render({ addonType: ADDON_TYPE_EXTENSION, authorUsernames: ['test2'], errorHandler, store, }); sinon.assert.calledWith(dispatchSpy, fetchAddonsByAuthors({ addonType: ADDON_TYPE_EXTENSION, authorUsernames: ['test2'], errorHandlerId: errorHandler.id, })); });

altho it wasn't failing it's just not need now b/c regardless of amount of add-ons it's going to do this fetch on mount - which it does on initial load and when you move around on client side (without back and forward buttons)...so it does this not only when there are no add-ons.

When i revert my changes (for testing purposes), i don't see anything failing via tests. But i guess i should add a test to make sure one of the items doesn't have a matching slug to the current slug and make sure that all is correct. Is that what you mean?

Right, this test case should not be kept or maybe it should be updated to "should always fetch addons by authors". That would also cover your change, because if you revert your change in the component, this test will fail.

hmm it;s not failing as expected here when i revert the component so i need to look into this a bit more :/

willdurand

r+wc

Thanks!

willdurand · 2018-05-22T14:34:20Z

tests/unit/amo/components/TestAddonsByAuthorsCard.js

@@ -213,7 +213,7 @@ describe(__filename, () => {
    expect(root).toHaveClassName('AddonsByAuthorsCard');
  });

-  it('should dispatch a fetch action if no add-ons found', () => {
+  it('should always fetch addons by authors', () => {


I think we should have a comment to not forget about why we want this (with a link to the issue).

willdurand · 2018-05-22T14:34:41Z

tests/unit/amo/components/TestAddonsByAuthorsCard.js

@@ -225,6 +225,7 @@ describe(__filename, () => {
      store,
    });

+    sinon.assert.callCount(dispatchSpy, 1);


I think you dont need this since you have calledWith below.

Is this PR till a WIP?

rebmullin · 2018-05-22T16:40:46Z

i * think * it should be ok now : )

willdurand

r+wc

Looks good to me, the comment could be shorter though.

willdurand · 2018-05-22T17:46:03Z

tests/unit/amo/components/TestAddonsByAuthorsCard.js

+  // what happens when it doesn't go back to server in some scenarios - we
+  // we want to always make sure to do a fetch on mount to make sure
+  // we have the latest addons list.
+  // REF: https://github.com/mozilla/addons-frontend/issues/4852


I am not sure I really understand that comment to be honest. Could you make it shorter and focused on the issue we want to solve?

In addition, we often use See: to link to an issue.

lol.. let me try to shorten this : )

rebmullin · 2018-05-22T19:03:34Z

hey @willdurand - i modified the comment a bit. Do you mind taking another look to make sure it's better/okay now?

willdurand · 2018-05-22T19:24:42Z

@rebmullin 🚢 it

rebmullin requested review from bobsilverberg and willdurand May 14, 2018 16:23

rebmullin changed the title ~~adjusting dispatch for addons by authors card~~ I4852-adjusting dispatch for addons by authors card May 14, 2018

rebmullin changed the title ~~I4852-adjusting dispatch for addons by authors card~~ I4852-fetch logic a bit on addons by authors card May 14, 2018

rebmullin force-pushed the I4852-addoncards-on-slug-change branch from 5d44fc4 to c8bfdc4 Compare May 15, 2018 01:21

bobsilverberg self-assigned this May 15, 2018

bobsilverberg reviewed May 15, 2018

View reviewed changes

rebmullin changed the title ~~I4852-fetch logic a bit on addons by authors card~~ I4852-fetch logic adjustment on addons by authors card May 15, 2018

willdurand assigned willdurand and unassigned bobsilverberg May 16, 2018

willdurand requested changes May 17, 2018

View reviewed changes

rebmullin force-pushed the I4852-addoncards-on-slug-change branch 3 times, most recently from b267234 to f51c8f5 Compare May 17, 2018 17:10

rebmullin requested a review from willdurand May 17, 2018 17:41

willdurand requested changes May 17, 2018

View reviewed changes

rebmullin changed the title ~~I4852-fetch logic adjustment on addons by authors card~~ [WIP] I4852-fetch logic adjustment on addons by authors card May 22, 2018

willdurand previously approved these changes May 22, 2018

View reviewed changes

rebmullin changed the title ~~[WIP] I4852-fetch logic adjustment on addons by authors card~~ I4852-fetch logic adjustment on addons by authors card May 22, 2018

rebmullin added 5 commits May 22, 2018 13:12

adjusting dispatch for addons by authors card

51e727a

updating per comments

c733196

unremoving tests :/

61edec9

updating per feedback

65771d4

updating per comments

b2769f2

rebmullin force-pushed the I4852-addoncards-on-slug-change branch from 268f7bf to b2769f2 Compare May 22, 2018 17:20

reordering to min diff chgs

6f575c0

willdurand approved these changes May 22, 2018

View reviewed changes

updating comment per pr reqst

9ea3da9

rebmullin merged commit 2f74424 into mozilla:master May 22, 2018

rebmullin deleted the I4852-addoncards-on-slug-change branch June 21, 2018 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I4852-fetch logic adjustment on addons by authors card #4992

I4852-fetch logic adjustment on addons by authors card #4992

rebmullin commented May 14, 2018 •

edited

codecov-io commented May 14, 2018 •

edited

bobsilverberg May 15, 2018

rebmullin May 15, 2018 •

edited

willdurand May 17, 2018

rebmullin May 17, 2018

willdurand May 17, 2018

willdurand May 17, 2018

rebmullin commented May 17, 2018

willdurand left a comment

willdurand May 17, 2018

rebmullin May 18, 2018

willdurand May 18, 2018

rebmullin May 22, 2018

willdurand left a comment

willdurand May 22, 2018

willdurand May 22, 2018

rebmullin commented May 22, 2018

willdurand left a comment

willdurand May 22, 2018

rebmullin May 22, 2018

rebmullin commented May 22, 2018

willdurand commented May 22, 2018

		@@ -55,43 +54,13 @@ export class AddonsByAuthorsCardBase extends React.Component<Props> {
		}

		componentWillMount() {

I4852-fetch logic adjustment on addons by authors card #4992

I4852-fetch logic adjustment on addons by authors card #4992

Conversation

rebmullin commented May 14, 2018 • edited

codecov-io commented May 14, 2018 • edited

Codecov Report

Choose a reason for hiding this comment

rebmullin May 15, 2018 • 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

rebmullin commented May 17, 2018

willdurand 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

willdurand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebmullin commented May 22, 2018

willdurand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rebmullin commented May 22, 2018

willdurand commented May 22, 2018

rebmullin commented May 14, 2018 •

edited

codecov-io commented May 14, 2018 •

edited

rebmullin May 15, 2018 •

edited