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

API functions for adding an add-on to a collection #4119

Merged
merged 5 commits into from
Jan 11, 2018
Merged
Show file tree
Hide file tree
Changes from 4 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
102 changes: 91 additions & 11 deletions src/amo/api/collections.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
/* @flow */
import { callApi } from 'core/api';
import { callApi, allPages } from 'core/api';
import type {
ExternalCollectionAddon, ExternalCollectionDetail,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just an aside but I think once we can't fit the entire import statement on one line we might as well do each one on a line; I think it's easier to scan the names if they're on newlines and it makes it easier to add/remove imports. 🤷🏻‍♂️

} from 'amo/reducers/collections';
import type { ApiStateType } from 'core/reducers/api';
import type { PaginatedApiResponse } from 'core/types/api';


type GetCollectionParams = {|
Expand All @@ -11,7 +15,7 @@ type GetCollectionParams = {|

export const getCollectionDetail = (
{ api, slug, user }: GetCollectionParams
) => {
): Promise<ExternalCollectionDetail> => {
if (!slug) {
throw new Error('slug is required');
}
Expand All @@ -28,42 +32,118 @@ export const getCollectionDetail = (

type GetCollectionAddonsParams = {|
...GetCollectionParams,
nextURL?: string,
page?: number,
|};

export const getCollectionAddons = (
{ api, page, slug, user }: GetCollectionAddonsParams
) => {
{ api, nextURL, page, slug, user }: GetCollectionAddonsParams
): Promise<PaginatedApiResponse<ExternalCollectionAddon>> => {
if (!slug) {
throw new Error('slug is required');
}
if (!user) {
throw new Error('user is required');
}

return callApi({
const request = {
auth: true,
endpoint: `accounts/account/${user}/collections/${slug}/addons`,
params: { page },
endpoint:
nextURL || `accounts/account/${user}/collections/${slug}/addons`,
params: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, I guess this is fine but it seems weird to pass undefined. Could we just leave this blank or use null? Just seems odd, is all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's necessary so that Flow knows that params is a valid key. It actually makes a bit of sense too because adding request.param when it's uninitialized could be a mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be okay to use null instead? It just seems weird to state a key as undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It has to be undefined so that the default value for params is used when callApi is invoked.

In fact, Flow will help you not make this mistake:

Error: src/amo/api/collections.js:60
 60:   return callApi(request);
                      ^^^^^^^ object literal. This type is incompatible with the expected param type of
 94: }: CallApiParams): Promise<any> {
        ^^^^^^^^^^^^^ object type. See: src/core/api/index.js:94
  Property `params` is incompatible:
     53:     params: null,
                     ^^^^ null. This type is incompatible with
     77:   params?: Object,
                    ^^^^^^ object type. See: src/core/api/index.js:77

state: api,
});
};
if (page) {
request.params = { page };
Copy link
Contributor

Choose a reason for hiding this comment

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

Would there ever be a case where other params exist? Maybe we should be defining params as {} above and doing request.params = { ...request.params, page }; here? It might make the above params: undefined code a bit easier to logically follow as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would there ever be a case where other params exist?

No. Here's an explanation that will hopefully clarify things.

This function does not accept request.params as input. The line of code you're looking at is a hard coded execution of callApi() to handle only two cases:

  1. Getting collection add-ons for a given user/collection at page 1 (the default page)
  2. Getting collection add-ons at page N (via the next URL returned by the API)

Also, due to the way spreads work, request.params should only be defined if page is set (this is why it's wrapped in an if statement). Otherwise, the page parameter from the next URL will be reset. Imagine this:

const query = {
  // This is how callApi() will extract ?page= from the `next` URL
  ...{ page: 2 },
  // This would be the custom `request.params` value passed to `callApi()`
  ...{ page: undefined }
};

In that code the value of query.page would be undefined which is not what we want. We want it to be 2. The fix is to execute the function more like:

callApi({ endpoint: nextUrlWithPageParam, params: undefined, ... })

Let me know if you have more questions about it.

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'll add some comments to the code

}

return callApi(request);
};

type GetAllCollectionAddonsParams = {|
...GetCollectionParams,
_allPages?: typeof allPages,
_getCollectionAddons?: typeof getCollectionAddons,
|};

export const getAllCollectionAddons = async (
{
api,
slug,
user,
_allPages = allPages,
_getCollectionAddons = getCollectionAddons,
}: GetAllCollectionAddonsParams
): Promise<Array<ExternalCollectionAddon>> => {
const { results } = await _allPages(
(nextURL) => _getCollectionAddons({ api, nextURL, slug, user })
);
return results;
};

type ListCollectionsParams = {|
api: ApiStateType,
nextURL?: string,
user: string | number,
|};

export const listCollections = (
{ api, user }: ListCollectionsParams
) => {
{ api, nextURL, user }: ListCollectionsParams
): Promise<PaginatedApiResponse<ExternalCollectionDetail>> => {
if (!user) {
throw new Error('The user parameter is required');
}
const endpoint = nextURL || `accounts/account/${user}/collections`;

return callApi({ auth: true, endpoint, state: api });
};

type GetAllUserCollectionsParams = {|
...ListCollectionsParams,
_allPages?: typeof allPages,
_listCollections?: typeof listCollections,
|};

export const getAllUserCollections = async (
{
api,
user,
_allPages = allPages,
_listCollections = listCollections,
}: GetAllUserCollectionsParams
): Promise<Array<ExternalCollectionDetail>> => {
const { results } = await _allPages(
(nextURL) => _listCollections({ api, nextURL, user })
);
return results;
};

type AddAddonToCollectionParams = {|
addon: string | number,
api: ApiStateType,
collection: string | number,
notes?: string,
user: string | number,
|};

export const addAddonToCollection = (
{ addon, api, collection, notes, user }: AddAddonToCollectionParams
): Promise<void> => {
if (!addon) {
throw new Error('The addon parameter is required');
}
if (!collection) {
throw new Error('The collection parameter is required');
}
if (!user) {
throw new Error('The user parameter is required');
}

return callApi({
auth: true,
endpoint: `accounts/account/${user}/collections`,
body: { addon, notes },
endpoint: `accounts/account/${user}/collections/${collection}/addons`,
method: 'POST',
state: api,
});
};
8 changes: 5 additions & 3 deletions src/amo/reducers/collections.js
Original file line number Diff line number Diff line change
Expand Up @@ -179,13 +179,15 @@ export const fetchCurrentCollectionPage = ({
};
};

type ExternalCollectionAddons = Array<{|
export type ExternalCollectionAddon = {|
addon: ExternalAddonType,
downloads: number,
notes: string | null,
|}>;
|};

type ExternalCollectionAddons = Array<ExternalCollectionAddon>;

type ExternalCollectionDetail = {|
export type ExternalCollectionDetail = {|
addon_count: number,
author: {|
id: number,
Expand Down
40 changes: 40 additions & 0 deletions src/core/api/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
} from 'core/searchUtils';
import type { ErrorHandlerType } from 'core/errorHandler';
import type { ApiStateType } from 'core/reducers/api';
import type { PaginatedApiResponse } from 'core/types/api';
import type { ReactRouterLocation } from 'core/types/router';


Expand Down Expand Up @@ -265,3 +266,42 @@ export function autocomplete({ api, filters }: AutocompleteParams) {
state: api,
});
}

type GetNextResponseType =
(nextURL?: string) => Promise<PaginatedApiResponse<any>>;

type AllPagesOptions = {| pageLimit: number |};

export const allPages = async (
Copy link
Contributor

Choose a reason for hiding this comment

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

This is neat and probably fine for now but as mentioned it might be nice to request an API for this. I'd think a single request/endpoint would be better than what we're doing here 😄

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 removed the code that makes a lot of API requests but we still need to fetch all of the user's collections to fill up the select box.

getNextResponse: GetNextResponseType,
{ pageLimit = 100 }: AllPagesOptions = {},
): Promise<PaginatedApiResponse<any>> => {
let results = [];
let nextURL;
let count = 0;
let pageSize = 0;

for (let page = 1; page <= pageLimit; page++) {
// eslint-disable-next-line no-await-in-loop
const response = await getNextResponse(nextURL);
if (!count) {
// Every response page returns a count for all results.
count = response.count;
}
if (!pageSize) {
pageSize = response.page_size;
}
results = results.concat(response.results);

if (response.next) {
nextURL = response.next;
log.debug(oneLine`Fetching next page "${nextURL}" of
${getNextResponse}`);
} else {
return { count, page_size: pageSize, results };
}
}

// If we get this far the callback may not be advancing pages correctly.
throw new Error(`Fetched too many pages (the limit is ${pageLimit})`);
};
3 changes: 1 addition & 2 deletions src/core/client/base.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/* global document */

import 'babel-polyfill';
import 'raf/polyfill';
import 'core/polyfill';
import { oneLine } from 'common-tags';
import config from 'config';
import FastClick from 'fastclick';
Expand Down
2 changes: 2 additions & 0 deletions src/core/polyfill.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
import 'babel-polyfill';
import 'raf/polyfill';
3 changes: 1 addition & 2 deletions src/core/server/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@ import fs from 'fs';
import path from 'path';
import https from 'https';

import 'babel-polyfill';
import 'raf/polyfill';
import 'core/polyfill';
import { oneLine } from 'common-tags';
import defaultConfig from 'config';
import Express from 'express';
Expand Down
Loading