Add actions and reducers for #1285 and #1286 #1439

tofumatt · 2016-11-29T20:50:20Z

This is half the size of the other PR (#1438)–it implements just the actions/reducers needed to deal with featured, highly rated, and popular add-ons (which will be on the extensions and themes homepage).

tofumatt · 2016-11-29T20:50:47Z

I'll make another PR (or rebase #1438 onto this) once this is reviewed.

kumar303

I am about to hop in another meeting but I wanted to submit this partial review maybe to get some thoughts from you. I think you can combine the actions and reducers for featured, highlyRated, and popular all into one. What do you think? Did you try that? Details below...

kumar303 · 2016-12-01T19:40:08Z

src/amo/actions/featured.js

+} from 'core/constants';
+
+
+export function featuredGet({ addonType }) {


I find it makes the most sense to read action functions as verbs. Perhaps getFeatured ?

kumar303 · 2016-12-01T19:40:15Z

src/amo/actions/featured.js

+  };
+}
+
+export function featuredLoad({ entities, result, addonType }) {


featuredLoaded ?

To keep it consistent (all actions as verbs) we'd want loadFeatured, which I'll move to; I've filed #1452 to discuss it.

kumar303 · 2016-12-01T19:40:25Z

src/amo/actions/featured.js

+  };
+}
+
+export function featuredFail({ addonType }) {


featuredFailed?

kumar303 · 2016-12-01T19:55:42Z

src/amo/reducers/highlyRated.js

+
+const initialState = {
+  count: 0,
+  filters: {},


why do you need to track filters in state for highlyRated and popular add-on data? It looks like (in your other patch) that the filters are only used to make the API call. The API call populates the data in state so why do you need to save the filters? You should already have the filtered results.

kumar303 · 2016-12-01T19:58:46Z

src/amo/reducers/featured.js

+const initialState = {
+  count: 0,
+  loading: false,
+  results: [],


I see three separate reducers: featured, highlyRated, and popular but they all look nearly identical. I also see a lot of repetition in all the code and tests that cover their logic. What if you combined them all into one reducer and just tracked the "buckets" of add-on results in separate keys? more like:

const initialState = { featured: { count: 0, loading: false, results: [], }, highlyRated: { count: 0, loading: false, results: [], }, popular: { count: 0, loading: false, results: [], }, };

That actually works for me, but I worried about too tightly coupling things that could maybe be used separately. I think you're right though, and one reducer could be used. I think I got a bit tripped up using Promise.all() and assigning things to the right state but maybe I need to give it another go.

I'll try combining them all and making the tweaks suggested and see how it goes. Thanks 👍

tofumatt · 2016-12-02T19:43:35Z

src/amo/actions/featured.js

+} from 'core/constants';
+
+
+export function featuredGet({ addonType }) {


I agree but we haven't been doing that in the past. I'm okay moving toward it; it makes them harder to scan through in a file (here they all stand out as featured[ACTION] but I'll move to it here and file an issue to consolidate them.

tofumatt · 2016-12-03T02:49:02Z

You're right about combing them as this is now about a third of the code it was before. I'll need to do a bit of extra to merge all three responses but this is worth it. 👍

tofumatt · 2016-12-14T16:30:10Z

This is ready for another r? btw

kumar303 · 2016-12-14T23:29:51Z

Just a thought: actions are the interface into data storage. I feel like all reducer tests should be executed with actions. Thus, a reducer test shouldn't ever need to begin with raw state objects. I used this pattern here and it felt right: https://github.com/mozilla/addons-frontend/pull/1418/files#diff-c696d213dc19550ac8bc5c0fac172451R21

Maybe this is just something we should think about more. Just thinking out loud ;)

kumar303

This approach looks a lot better, thanks. I asked for some small changes and had a few questions.

kumar303 · 2016-12-14T23:04:04Z

src/amo/reducers/landing.js

+  let newState;
+  switch (action.type) {
+    case LANDING_GET:
+      return { ...state, ...payload, loading: true };


I would rather see addonType: payload.addonType instead of ...payload because it's not super clear otherwise. Should initialState have a default value for addonType? Maybe that would be more clear.

I don't think there should be a default value set but I've set one to null so the data structure is clearer. And I switched to addonType: payload.addonType; you're right the explicitness is better 👍

kumar303 · 2016-12-14T23:07:12Z

src/amo/actions/landing.js

+}
+
+export function loadLanding(
+  { addonType, entities, featured, highlyRated, popular }


The reducer is going to throw reference errors if any one of featured, highlyRated, or popular is missing. If I'm correct about that, you should raise an error here if any one of those payload keys is falsey.

kumar303 · 2016-12-14T23:14:06Z

src/amo/reducers/landing.js

+
+export default function landing(state = initialState, action) {
+  const { payload } = action;
+  let newState;


I find that it's a little cleaner to only define this in the exact switch case that you need it for. You might have tried that and got a lint error because you have to use brackets to declare a new scope:

switch (action.type) { case LANDING_LOADED: { const newState = { ...state, loading: false }; newState[key] = ... return newState; } }

Ah, neat. Yeah I was getting scope errors and didn't know you could force a new scope like that 👍

kumar303 · 2016-12-14T23:16:02Z

src/amo/reducers/landing.js

+
+      return newState;
+    case LANDING_FAILED:
+      return initialState;


This would not preserve the previously declared addonType. Is that important?

I don't know if it would be used but it should; I've changed it.

kumar303 · 2016-12-14T23:20:59Z

src/amo/reducers/landing.js

+} from 'core/constants';
+
+
+export const initialState = {


is entities used? I see that some actions are setting it and some tests are checking for it. If so, a default value (even if it's null) would help me understand the data structure better. Is entities really necessary though if we already have results arrays?

We use entities in the LOADED reducer to create our results object, yeah. But we don't use it/access it outside the reducer/in the components.

aha, I see it now on this line: https://github.com/mozilla/addons-frontend/pull/1439/files#diff-cba04c1c93442d633523fc29d40f97e0R26

kumar303 · 2016-12-15T20:51:48Z

src/amo/reducers/landing.js

+      ['featured', 'highlyRated', 'popular'].forEach((key) => {
+        newState[key] = {
+          count: payload[key].result.count,
+          results: payload[key].result.results.map((slug) => payload.entities.addons[slug]),


shouldn't payload.entities be payload[key].entities ? Otherwise results for other keys will get erased.

Indeed! This was a bug when I separated out the reducers, it was payload[key].entities originally. Thanks!

tofumatt · 2016-12-15T23:25:45Z

Okay, I think I've addressed all the comments. Ready for another r?

kumar303

r+wc

Looks good, thanks for the fixups. I suggested some changes but they aren't crucial to getting this code to work.

kumar303 · 2016-12-15T23:46:56Z

src/amo/actions/landing.js

+}
+
+export function loadLanding(
+  { addonType, entities, featured, highlyRated, popular }


Now that I understand this better, I think you can remove entities, right?

kumar303 · 2016-12-15T23:51:50Z

src/amo/actions/landing.js

+} from 'core/constants';
+
+
+export function getLanding({ addonType }) {


I think it would be nice to throw an error here if addonType is falsey

kumar303 · 2016-12-15T23:53:21Z

src/amo/actions/landing.js

+}
+
+export function loadLanding(
+  { addonType, entities, featured, highlyRated, popular }


It would be nice to do some payload checking here:

make sure addonType is set

make sure at least one of the featured, highlyRated, or popular keys are set

check for result, and entities within a set key

I don't know if that's overkill but it seems like it might help prevent typo'd calls.

I want to land this soon: https://github.com/mozilla/addons-frontend/issues/1461

I'd like to land #1461 but I think it's overkill for this particular method, especially as we don't tend to do it elsewhere IIRC.

kumar303 · 2016-12-15T23:54:36Z

tests/client/amo/actions/test_landing.js

+    popular: sinon.stub(),
+  };
+  const action = actions.loadLanding({
+    addonType: 'theme', entities, ...response });


I don't think entities is needed here

kumar303 · 2016-12-15T23:58:16Z

tests/client/amo/actions/test_landing.js

+
+  it('sets the payload', () => {
+    assert.deepEqual(action.payload, {
+      addonType: 'theme', entities, ...response });


again, entities can be removed.

This is an example of where I think you'll get better tests by following a pattern like: set data using an action -> make assertions about how the reducer stored the data. Instead, what you're doing is making assertions about the action payload which isn't so helpful.

kumar303 · 2016-12-16T00:00:59Z

tests/client/amo/reducers/testLanding.js

+        addonType, featured, highlyRated, loading, popular,
+      } = landing(initialState, {
+        type: 'LANDING_GET',
+        payload: { addonType: 'theme' },


I think this would be a better test if you passed actions.loadLanding() directly to landing().

kumar303 · 2016-12-16T00:01:38Z

tests/client/amo/reducers/testLanding.js

+      };
+      const { featured, highlyRated, popular } = landing(initialData, {
+        type: 'LANDING_LOADED',
+        payload: {


again, I think passing actions.loadActions() directly would make this a more robust test

tofumatt assigned kumar303 Nov 29, 2016

tofumatt added the pull request ready label Nov 29, 2016

kumar303 suggested changes Dec 1, 2016

View reviewed changes

tofumatt commented Dec 2, 2016

View reviewed changes

tofumatt force-pushed the extensions-actions-1286 branch 3 times, most recently from e24c518 to 82dfb84 Compare December 3, 2016 02:41

tofumatt force-pushed the extensions-actions-1286 branch 3 times, most recently from 3f29911 to 49aab63 Compare December 3, 2016 11:57

tofumatt mentioned this pull request Dec 14, 2016

Extensions page 1286 #1438

Closed

tofumatt force-pushed the extensions-actions-1286 branch from 49aab63 to 2e9ea2d Compare December 14, 2016 16:30

kumar303 suggested changes Dec 14, 2016

View reviewed changes

kumar303 reviewed Dec 15, 2016

View reviewed changes

tofumatt force-pushed the extensions-actions-1286 branch from 2e9ea2d to deddf28 Compare December 15, 2016 23:25

kumar303 approved these changes Dec 16, 2016

View reviewed changes

Add actions and reducers for #1285 and #1286

a151708

tofumatt force-pushed the extensions-actions-1286 branch from deddf28 to a151708 Compare December 16, 2016 13:21

tofumatt merged commit e0d6f35 into master Dec 16, 2016

tofumatt deleted the extensions-actions-1286 branch December 16, 2016 13:35

tofumatt removed the pull request ready label Dec 16, 2016

		} from 'core/constants';


		export function featuredGet({ addonType }) {

		} from 'core/constants';


		export function getLanding({ addonType }) {

Add actions and reducers for #1285 and #1286 #1439

Add actions and reducers for #1285 and #1286 #1439

Conversation

tofumatt commented Nov 29, 2016

tofumatt commented Nov 29, 2016 • edited Loading

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

tofumatt commented Dec 3, 2016

tofumatt commented Dec 14, 2016

kumar303 commented Dec 14, 2016

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

tofumatt commented Dec 15, 2016

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

tofumatt commented Nov 29, 2016 •

edited

Loading