Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/amo/api/recommendations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
/* @flow */
import invariant from 'invariant';

import { callApi } from 'core/api';
import type { ApiStateType } from 'core/reducers/api';
import type { PaginatedApiResponse } from 'core/types/api';
import type { ExternalAddonType } from 'core/types/addons';


export type GetRecommendationsParams = {|
api: ApiStateType,
guid: string,
recommended: boolean,
|};

export const getRecommendations = (
{ api, guid, recommended }: GetRecommendationsParams
): Promise<PaginatedApiResponse<ExternalAddonType>> => {
invariant(guid, 'A guid is required.');
invariant(typeof recommended === 'boolean', 'recommended is required');

return callApi({
auth: true,
endpoint: 'addons/recommendations/',
params: { guid, recommended },
state: api,
});
};
182 changes: 182 additions & 0 deletions src/amo/reducers/recommendations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
/* @flow */
import invariant from 'invariant';

import { createInternalAddon } from 'core/reducers/addons';
import type { ExternalAddonType } from 'core/types/addons';

export const ABORT_FETCH_RECOMMENDATIONS: 'ABORT_FETCH_RECOMMENDATIONS'
= 'ABORT_FETCH_RECOMMENDATIONS';
export const FETCH_RECOMMENDATIONS: 'FETCH_RECOMMENDATIONS'
= 'FETCH_RECOMMENDATIONS';
export const LOAD_RECOMMENDATIONS: 'LOAD_RECOMMENDATIONS'
= 'LOAD_RECOMMENDATIONS';

export type Recommendations = {|
addons: Array<ExternalAddonType> | null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fallbackReason: string | null,
loading: boolean,
outcome: string | null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

|};

export type RecommendationsState = {|
byGuid: {
[guid: string]: Recommendations,
},
|};

export const initialState: RecommendationsState = {
byGuid: {},
};

export type AbortFetchRecommendationsParams = {|
guid: string,
|};

type AbortFetchRecommendationsAction = {|
type: typeof ABORT_FETCH_RECOMMENDATIONS,
payload: AbortFetchRecommendationsParams,
|};

export const abortFetchRecommendations = ({
guid,
}: AbortFetchRecommendationsParams = {}): AbortFetchRecommendationsAction => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

invariant(guid, 'guid is required');
return {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no invariant here?

type: ABORT_FETCH_RECOMMENDATIONS,
payload: { guid },
};
};

type FetchRecommendationsParams = {|
errorHandlerId: string,
guid: string,
recommended: boolean,
|};

export type FetchRecommendationsAction = {|
type: typeof FETCH_RECOMMENDATIONS,
payload: FetchRecommendationsParams,
|};

export const fetchRecommendations = ({
errorHandlerId,
guid,
recommended,
}: FetchRecommendationsParams = {}): FetchRecommendationsAction => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

invariant(errorHandlerId, 'errorHandlerId is required');
invariant(guid, 'guid is required');
invariant(typeof recommended === 'boolean', 'recommended is required');

return {
type: FETCH_RECOMMENDATIONS,
payload: { errorHandlerId, guid, recommended },
};
};

export type LoadRecommendationsParams = {|
addons: Array<ExternalAddonType>,
fallbackReason: string,
guid: string,
outcome: string,
|};

type LoadRecommendationsAction = {|
type: typeof LOAD_RECOMMENDATIONS,
payload: LoadRecommendationsParams,
|};

export const loadRecommendations = ({
addons,
fallbackReason,
guid,
outcome,
}: LoadRecommendationsParams = {}): LoadRecommendationsAction => {
invariant(addons, 'addons is required');
invariant(fallbackReason, 'fallbackReason is required');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, correct, good call!

invariant(guid, 'guid is required');
invariant(outcome, 'outcome is required');

return {
type: LOAD_RECOMMENDATIONS,
payload: { addons, guid, outcome, fallbackReason },
};
};

type GetRecommendationsByGuidParams = {|
guid: string,
state: RecommendationsState,
|};

export const getRecommendationsByGuid = (
{ guid, state }: GetRecommendationsByGuidParams
): Recommendations | null => {
invariant(guid, 'guid is required');

This comment was marked as resolved.

invariant(state, 'state is required');

return state.byGuid[guid] || null;
};

type Action =
| AbortFetchRecommendationsAction
| FetchRecommendationsAction
| LoadRecommendationsAction;

const reducer = (
state: RecommendationsState = initialState,
action: Action
): RecommendationsState => {
switch (action.type) {
case ABORT_FETCH_RECOMMENDATIONS:
return {
...state,
byGuid: {
...state.byGuid,
[action.payload.guid]: {
addons: null,
fallbackReason: null,
loading: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

outcome: null,
},
},
};

case FETCH_RECOMMENDATIONS:
return {
...state,
byGuid: {
...state.byGuid,
[action.payload.guid]: {
addons: null,
fallbackReason: null,
loading: true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

outcome: null,
},
},
};

case LOAD_RECOMMENDATIONS: {
const { fallbackReason, guid, outcome } = action.payload;

const addons = action.payload.addons
.map((addon) => createInternalAddon(addon));

return {
...state,
byGuid: {
...state.byGuid,
[guid]: {
addons,
fallbackReason,
loading: false,
outcome,
},
},
};
}

default:
return state;
}
};

export default reducer;
48 changes: 48 additions & 0 deletions src/amo/sagas/recommendations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/* @flow */
import { call, put, select, takeLatest } from 'redux-saga/effects';
import {
FETCH_RECOMMENDATIONS,
abortFetchRecommendations,
loadRecommendations,
} from 'amo/reducers/recommendations';
import * as api from 'amo/api/recommendations';
import log from 'core/logger';
import { createErrorHandler, getState } from 'core/sagas/utils';
import type { GetRecommendationsParams } from 'amo/api/recommendations';
import type {
FetchRecommendationsAction,
} from 'amo/reducers/recommendations';


export function* fetchRecommendations({
payload: { errorHandlerId, guid, recommended },
}: FetchRecommendationsAction): Generator<any, any, any> {
const errorHandler = createErrorHandler(errorHandlerId);
yield put(errorHandler.createClearingAction());

try {
const state = yield select(getState);

const params: GetRecommendationsParams = {
api: state.api, guid, recommended,
};
const recommendations = yield call(api.getRecommendations, params);
const { fallback_reason: fallbackReason, outcome, results: addons }
= recommendations;

yield put(loadRecommendations({
addons,
fallbackReason,
guid,
outcome,
}));
} catch (error) {
log.warn(`Failed to fetch user collections: ${error}`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/user collections/recommendations

yield put(errorHandler.createErrorAction(error));
yield put(abortFetchRecommendations({ guid }));
}
}

export default function* recommendationsSaga(): Generator<any, any, any> {
yield takeLatest(FETCH_RECOMMENDATIONS, fetchRecommendations);
}
2 changes: 2 additions & 0 deletions src/amo/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import addonsByAuthors from 'amo/reducers/addonsByAuthors';
import collections from 'amo/reducers/collections';
import home from 'amo/reducers/home';
import landing from 'amo/reducers/landing';
import recommendations from 'amo/reducers/recommendations';
import reviews from 'amo/reducers/reviews';
import userAbuseReports from 'amo/reducers/userAbuseReports';
import users from 'amo/reducers/users';
Expand Down Expand Up @@ -51,6 +52,7 @@ export default function createStore({
landing,
languageTools,
redirectTo,
recommendations,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be before redirectTo?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

reviews,
routing,
search,
Expand Down
35 changes: 35 additions & 0 deletions tests/unit/amo/api/test_recommendations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import * as api from 'core/api';
import { getRecommendations } from 'amo/api/recommendations';
import { createApiResponse } from 'tests/unit/helpers';
import { dispatchClientMetadata } from 'tests/unit/amo/helpers';


describe(__filename, () => {
it('calls the recommendations API', async () => {
const mockApi = sinon.mock(api);
const apiState = dispatchClientMetadata().store.getState().api;

const params = {
guid: 'addon-guid',
recommended: true,
};

mockApi
.expects('callApi')
.withArgs({
auth: true,
endpoint:
'addons/recommendations/',
params,
state: apiState,
})
.once()
.returns(createApiResponse());

await getRecommendations({
api: apiState,
...params,
});
mockApi.verify();
});
});
82 changes: 82 additions & 0 deletions tests/unit/amo/reducers/test_recommendations.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
import reducer, {
abortFetchRecommendations,
fetchRecommendations,
getRecommendationsByGuid,
initialState,
loadRecommendations,
} from 'amo/reducers/recommendations';
import { createInternalAddon } from 'core/reducers/addons';
import { createStubErrorHandler } from 'tests/unit/helpers';
import { fakeAddon } from 'tests/unit/amo/helpers';


describe(__filename, () => {
it('initializes properly', () => {
const state = reducer(undefined, {});
expect(state).toEqual(initialState);
});

it('ignores unrelated actions', () => {
const state = reducer(initialState, { type: 'UNRELATED_ACTION' });
expect(state).toEqual(initialState);
});

it('sets the loading flag when fetching recommendations', () => {
const guid = 'some-guid';
const state = reducer(undefined, fetchRecommendations({
errorHandlerId: createStubErrorHandler().id,
guid,
recommended: true,
}));

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

it('loads recommendations', () => {
const addons = [fakeAddon, fakeAddon];
const fallbackReason = 'timeout';
const guid = 'some-guid';
const outcome = 'recommended_fallback';
const state = reducer(undefined, loadRecommendations({
addons,
fallbackReason,
guid,
outcome,
}));

const expectedAddons = addons.map((addon) => createInternalAddon(addon));

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

expect(loadedRecommendations).toEqual({
addons: expectedAddons,
fallbackReason,
loading: false,
outcome,
});
});

it('resets the loading flag when fetching is aborted', () => {
const guid = 'some-guid';
const state = reducer(undefined, fetchRecommendations({
errorHandlerId: createStubErrorHandler().id,
guid,
recommended: true,
}));

expect(state.byGuid[guid].loading).toEqual(true);

const newState = reducer(state, abortFetchRecommendations({ guid }));
expect(newState.byGuid[guid].loading).toEqual(false);
});

describe('getRecommendationsByGuid', () => {
it('returns null if no recommendations exist for the guid', () => {
const state = reducer(undefined, {});
const guid = 'a-non-existent-guid';

expect(getRecommendationsByGuid({ guid, state })).toEqual(null);
});
});
});
Loading