Implement API, saga and reducer code to interact with TAAR Lite service via addons-server #4904

bobsilverberg · 2018-05-01T14:17:04Z

codecov-io · 2018-05-01T14:25:40Z

Codecov Report

Merging #4904 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#4904      +/-   ##
==========================================
+ Coverage   96.49%   96.52%   +0.03%     
==========================================
  Files         208      211       +3     
  Lines        4823     4869      +46     
  Branches      959      962       +3     
==========================================
+ Hits         4654     4700      +46     
  Misses        151      151              
  Partials       18       18

Impacted Files	Coverage Δ
src/amo/sagas/index.js	`100% <ø> (ø)`	⬆️
src/amo/store.js	`100% <ø> (ø)`	⬆️
src/amo/sagas/recommendations.js	`100% <100%> (ø)`
src/amo/reducers/recommendations.js	`100% <100%> (ø)`
src/amo/api/recommendations.js	`100% <100%> (ø)`
src/amo/utils.js	`100% <0%> (ø)`	⬆️
src/amo/components/Addon/index.js	`100% <0%> (ø)`	⬆️

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 2267785...f41d02c. Read the comment docs.

willdurand

Good job, I am blocking this PR because I would like your input on the recommandations state shape. I think we should not have undefined properties and always have the same shape (the Flow type for RecommendationsState looks perfect to me).

willdurand · 2018-05-03T10:49:10Z

src/amo/reducers/recommendations.js

+  | AbortFetchRecommendationsAction
+  | FetchRecommendationsAction
+  | LoadRecommendationsAction
+  ;


I think we put semi-colons on the last line, never alone.

I think I got that from https://github.com/mozilla/addons-frontend/blob/master/src/amo/reducers/collections.js#L724, but you're right, pretty much everywhere else we put it on the end of the lint.

ah yes, consistency is not our best strength 😅

willdurand · 2018-05-03T10:50:12Z

src/amo/reducers/recommendations.js

+        byGuid: {
+          ...state.byGuid,
+          [action.payload.guid]: {
+            loading: false,


According to the Flow def, we should have more properties here no?

willdurand · 2018-05-03T10:50:32Z

src/amo/reducers/recommendations.js

+        byGuid: {
+          ...state.byGuid,
+          [action.payload.guid]: {
+            loading: true,


willdurand · 2018-05-03T10:57:39Z

src/amo/sagas/recommendations.js

+      api: state.api, guid, recommended,
+    };
+    const recommendations = yield call(api.getRecommendations, params);
+    const { outcome } = recommendations;


if you want to destructure an object and have different variable names, you can do:

const { results: addons, fallback_reason: fallbackReason, outcome } = recommendations;

Nice, thanks!

willdurand · 2018-05-03T11:07:37Z

src/amo/reducers/recommendations.js

+export const abortFetchRecommendations = ({
+  guid,
+}: AbortFetchRecommendationsParams = {}): AbortFetchRecommendationsAction => {
+  return {


no invariant here?

willdurand · 2018-05-03T11:09:29Z

tests/unit/amo/api/test_recommendations.js

+import { createApiResponse } from 'tests/unit/helpers';
+import {
+  dispatchClientMetadata,
+} from 'tests/unit/amo/helpers';


nitpick: this could be one a single line.

willdurand · 2018-05-03T11:10:50Z

tests/unit/amo/reducers/test_recommendations.js

+    }));
+
+    expect(state.byGuid[guid].loading).toEqual(true);
+    expect(state.byGuid[guid].addons).toEqual(undefined);


According to the flow def, there is always an addon array that can be empty.

I updated that so it can be null, and updated this test.

willdurand · 2018-05-03T11:23:10Z

tests/unit/amo/reducers/test_recommendations.js

+    expect(state.byGuid[guid].addons).toEqual(undefined);
+  });
+
+


Extra blank line

willdurand · 2018-05-03T11:24:02Z

tests/unit/amo/reducers/test_recommendations.js

+
+    const loadedRecommendations = getRecommendationsByGuid({ guid, state });
+
+    expect(loadedRecommendations).not.toEqual(null);


I understand this line but it is (implicitly) covered by the line below no?

True. I'll remove it.

willdurand · 2018-05-03T11:27:12Z

tests/unit/amo/sagas/test_recommendations.js

+
+
+describe(__filename, () => {
+  let clientData;


Is this really necessary?

bobsilverberg · 2018-05-03T13:51:46Z

Thanks for the review @willdurand. I'm submitting my changes for you to have another look. This is currently failing the Flow check as it will need Flow 0.71, so ignore the CI failures for now. I won't land it until those are fixed.

willdurand · 2018-05-03T15:51:41Z

@bobsilverberg thanks, you can rebase now if you want.

…ce via addons-server

bobsilverberg · 2018-05-07T01:41:21Z

This is ready for another review, thanks @willdurand.

willdurand

r+wc

👍

willdurand · 2018-05-07T08:05:00Z

src/amo/reducers/recommendations.js

+
+export const abortFetchRecommendations = ({
+  guid,
+}: AbortFetchRecommendationsParams = {}): AbortFetchRecommendationsAction => {


I would not specify a default empty object here because guid is mandatory.

willdurand · 2018-05-07T08:20:02Z

src/amo/reducers/recommendations.js

+  errorHandlerId,
+  guid,
+  recommended,
+}: FetchRecommendationsParams = {}): FetchRecommendationsAction => {


Here too, I would not add the default empty object. It's not really clear to me why we have some actions with default empty objects and others without... I would be in favor of not doing this because it would raise a warning/error in development.

Agreed. That was code borrowed from another reducer and modified, but with required arguments there's no reason to default to {}.

willdurand · 2018-05-07T08:23:31Z

src/amo/store.js

@@ -51,6 +52,7 @@ export default function createStore({
      landing,
      languageTools,
      redirectTo,
+      recommendations,


Shouldn't this be before redirectTo?

willdurand · 2018-05-07T08:27:13Z

src/amo/reducers/recommendations.js

+  = 'LOAD_RECOMMENDATIONS';
+
+export type Recommendations = {|
+  addons: Array<ExternalAddonType> | null,


Because you use createInternalAddon() in the reducer, I suppose the type should be AddonType.

willdurand · 2018-05-07T08:28:04Z

src/amo/reducers/recommendations.js

+  outcome,
+}: LoadRecommendationsParams = {}): LoadRecommendationsAction => {
+  invariant(addons, 'addons is required');
+  invariant(fallbackReason, 'fallbackReason is required');


The API documentation says the fallback_reason is "nullable": http://addons-server.readthedocs.io/en/latest/topics/api/addons.html#recommendations

Ah, correct, good call!

willdurand · 2018-05-07T08:30:15Z

src/amo/reducers/recommendations.js

+  addons: Array<ExternalAddonType> | null,
+  fallbackReason: string | null,
+  loading: boolean,
+  outcome: string | null,


Do we want to specify the allowed values? The API documentation describes the following possible values:

outcome (string) – Outcome of the response returned. Will be either: recommended - responses from recommendation service; recommended_fallback - service timed out or returned empty results so we returned fallback; curated - recommended=False was requested so fallback returned.

Because the app doesn't care what the value is (i.e., all it does is record the value, but it never makes any decisions based on the value), I thought it might make sense to leave this open. Theoretically the API could change the strings, or add a string, and it shouldn't break anything.

However this does seem to go against our use of Flow, so I will update this to a type which allows specific strings, and I'll do the same for fallbackReason.

If you spend more than 5-10min trying to convince Flow, a string type will be fine :)

I didn't have to spend any time trying to convince Flow, so it's now a specific type. :-)

willdurand · 2018-05-07T08:30:54Z

src/amo/sagas/recommendations.js

+      outcome,
+    }));
+  } catch (error) {
+    log.warn(`Failed to fetch user collections: ${error}`);


s/user collections/recommendations

willdurand · 2018-05-07T08:35:01Z

src/amo/store.js

@@ -51,6 +52,7 @@ export default function createStore({
      landing,
      languageTools,
      redirectTo,
+      recommendations,


In addition, I noticed you have added the reducer here. Do you want to also add the saga in the rootSaga?

Yeah, I missed that, and added it in #4942, but it makes sense to add it in this patch. Thanks. 👍

bobsilverberg · 2018-05-07T12:44:03Z

Oops, I notice that I didn't use the new types for fallbackReason and outcome in LoadRecommendationsParams. I'll address that in PR #4942.

bobsilverberg requested a review from willdurand May 1, 2018 14:27

willdurand requested changes May 3, 2018

View reviewed changes

bobsilverberg requested a review from willdurand May 3, 2018 13:52

bobsilverberg added 2 commits May 3, 2018 12:21

Implement API, saga and reducer code to interact with TAAR Lite servi…

05526b2

…ce via addons-server

Address review comments

8b0d81c

bobsilverberg force-pushed the recommendations-api-4867 branch from 4a1bdcc to 8b0d81c Compare May 3, 2018 16:22

bobsilverberg mentioned this pull request May 7, 2018

Use taar for recommendations #4942

Merged

willdurand approved these changes May 7, 2018

View reviewed changes

Address review comments

f41d02c

bobsilverberg merged commit 1d66889 into mozilla:master May 7, 2018


		const loadedRecommendations = getRecommendationsByGuid({ guid, state });

		expect(loadedRecommendations).not.toEqual(null);

Implement API, saga and reducer code to interact with TAAR Lite service via addons-server #4904

Implement API, saga and reducer code to interact with TAAR Lite service via addons-server #4904

Conversation

bobsilverberg commented May 1, 2018

codecov-io commented May 1, 2018 • edited Loading

Codecov Report

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

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

bobsilverberg commented May 3, 2018

willdurand commented May 3, 2018

bobsilverberg commented May 7, 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

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

bobsilverberg commented May 7, 2018

codecov-io commented May 1, 2018 •

edited

Loading