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

kumar303 · 2018-01-03T23:00:57Z

codecov-io · 2018-01-03T23:38:08Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    mozilla/addons-server#4119      +/-   ##
==========================================
+ Coverage   96.31%   96.34%   +0.03%     
==========================================
  Files         187      187              
  Lines        4017     4053      +36     
  Branches      823      831       +8     
==========================================
+ Hits         3869     3905      +36     
  Misses        126      126              
  Partials       22       22

Impacted Files	Coverage Δ
src/core/client/base.js	`0% <ø> (ø)`	⬆️
src/amo/reducers/collections.js	`100% <ø> (ø)`	⬆️
src/core/server/base.js	`58.1% <ø> (ø)`	⬆️
src/amo/api/collections.js	`100% <100%> (ø)`	⬆️
src/core/api/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 a0ac2cd...332e358. Read the comment docs.

kumar303 · 2018-01-05T16:00:14Z

Let's hold off on this patch since there is a PR open to solve mozilla/addons#5117 and that will make the API calls here more sane.

kumar303 · 2018-01-09T23:41:37Z

This is ready for review now. Since the UI isn't here to try out, let me know if you have any questions. The end goal for these helpers is to implement this UI (which is working in a separate patch): mozilla/addons#11232

The implementation of getAllUserAddonCollections() may not perform super well but I'm not sure. I think we should try it. I was hoping we wouldn't have to page through all the user's collections to find which ones match the add-on but we do (because of cache-machine). However, we have just lifted cache-machine on collection endpoints so perhaps soon we can make a better API endpoint for it. Again, it might not even be a problem.

tofumatt

I'd like to know how difficult it would be to get more tailored APIs for what we're doing here. This implementation definitely has the potential to perform slowly and cause a fair few requests and it's a fair bit of extra code to add/grok.

If this is the only way to deal with this at least in the short term then okay, but is there an add-ons server issue that could give us an API that wouldn't require this kind of pagination-hunting to get our data? Maybe I'm missing something.

tofumatt · 2018-01-10T17:02:39Z

src/amo/api/collections.js

-    params: { page },
+    endpoint:
+      nextURL || `accounts/account/${user}/collections/${slug}/addons`,
+    params: undefined,


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.

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.

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

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

tofumatt · 2018-01-10T17:05:44Z

src/amo/api/collections.js

    state: api,
-  });
+  };
+  if (!nextURL || page) {


This seems an odd conditional. I'd think you'd want to check for either !nextURL && page (to make sure the param we're setting has a value below) or just check for a false-y nextURL... I guess I just find it strange but maybe there's a reason I'm missing here. 😄

tofumatt · 2018-01-10T17:11:16Z

src/core/api/index.js

+
+type AllPagesOptions = {| pageLimit: number |};
+
+export const allPages = async (


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 😄

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.

tofumatt · 2018-01-10T17:23:30Z

tests/unit/amo/api/test_collections.js

+      mockApi.verify();
+    });
+
+    it('posts notes about an addon', async () => {


This spec confused me at first because I thought it was about posting notes but really it's about posting an add-on with accompanying notes, right? I think it would be clearer to label it as "posts an add-on and notes" or something like that.

kumar303 · 2018-01-10T19:54:25Z

Actually, we found a way to simplify the UX for this which means some of these API helpers aren't needed. I'll prepare a new patch.

kumar303 · 2018-01-10T19:57:03Z

Ah whoops, I didn't realize you reviewed it already. Yes! We can definitely fix the performance now because I can remove the API request for getting all collection add-ons entirely. Sorry for the noise, I thought you were on PTO so I was waiting to see if I could get it fixed in time before commenting 😄 I may still be able to get it fixed today.

kumar303 · 2018-01-10T23:22:02Z

I removed getAllUserAddonCollections() because we no longer need it for the UI. This should be ready for another look.

tofumatt

It would probably still be cool to have an API added that's tailored to the result set we want here that would save us from combing through paginated requests to get more info, but it seems like that might actually be a rare instance anyway so this is a good start 👍

tofumatt · 2018-01-11T11:07:49Z

src/amo/api/collections.js

-import { callApi } from 'core/api';
+import { callApi, allPages } from 'core/api';
+import type {
+  ExternalCollectionAddon, ExternalCollectionDetail,


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. 🤷🏻‍♂️

tofumatt · 2018-01-11T11:09:59Z

src/amo/api/collections.js

-    params: { page },
+    endpoint:
+      nextURL || `accounts/account/${user}/collections/${slug}/addons`,
+    params: undefined,


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

tofumatt · 2018-01-11T11:11:41Z

src/amo/api/collections.js

-  });
+  };
+  if (page) {
+    request.params = { page };


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.

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:

Getting collection add-ons for a given user/collection at page 1 (the default page)

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.

I'll add some comments to the code

kumar303 · 2018-01-11T16:26:27Z

It would probably still be cool to have an API added that's tailored to the result set we want here that would save us from combing through paginated requests to get more info

That's not even possible. To fill a select box with the user's collections we need to get all collections. The API will only give us the first page (25 collections).

I put a safety guard in there to prevent us from fetching an insane amount of pages but in reality I doubt many users would have more than 25 collections anyway.

I think this is a problem we are going to have to solve eventually for other scenarios. We've been able to avoid it thus far by providing a UI to navigate between pages. That didn't seem practical for a select box.

Thanks for the review!

kumar303 requested a review from tofumatt January 3, 2018 23:00

kumar303 removed the request for review from tofumatt January 5, 2018 15:59

kumar303 added 2 commits January 9, 2018 17:34

API functions for adding an add-on to a collection

b484ac3

Add helper for fetching all API pages

43daf30

kumar303 force-pushed the collection-apis-4118 branch from 8f67294 to 43daf30 Compare January 9, 2018 23:34

kumar303 requested a review from tofumatt January 9, 2018 23:41

kumar303 mentioned this pull request Jan 10, 2018

Polyfill request animation in the test suite mozilla/addons#11248

Closed

tofumatt suggested changes Jan 10, 2018

View reviewed changes

Remove getAllUserAddonCollections and clean up

78fd5b4

kumar303 requested a review from tofumatt January 10, 2018 23:22

Fix lint

de041d8

tofumatt approved these changes Jan 11, 2018

View reviewed changes

cleanup

332e358

kumar303 merged commit 172da90 into mozilla:master Jan 11, 2018

kumar303 deleted the collection-apis-4118 branch January 11, 2018 16:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

kumar303 commented Jan 3, 2018

codecov-io commented Jan 3, 2018 •

edited

Loading

kumar303 commented Jan 5, 2018

kumar303 commented Jan 9, 2018

tofumatt left a comment

tofumatt Jan 10, 2018

kumar303 Jan 10, 2018

tofumatt Jan 11, 2018

kumar303 Jan 11, 2018

tofumatt Jan 10, 2018

tofumatt Jan 10, 2018

kumar303 Jan 10, 2018

tofumatt Jan 10, 2018

kumar303 commented Jan 10, 2018

kumar303 commented Jan 10, 2018

kumar303 commented Jan 10, 2018

tofumatt left a comment

tofumatt Jan 11, 2018

tofumatt Jan 11, 2018

tofumatt Jan 11, 2018

kumar303 Jan 11, 2018

kumar303 Jan 11, 2018

kumar303 commented Jan 11, 2018 •

edited

Loading


		type AllPagesOptions = {\| pageLimit: number \|};

		export const allPages = async (

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

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

Conversation

kumar303 commented Jan 3, 2018

codecov-io commented Jan 3, 2018 • edited Loading

Codecov Report

kumar303 commented Jan 5, 2018

kumar303 commented Jan 9, 2018

tofumatt 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

kumar303 commented Jan 10, 2018

kumar303 commented Jan 10, 2018

kumar303 commented Jan 10, 2018

tofumatt 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

kumar303 commented Jan 11, 2018 • edited Loading

codecov-io commented Jan 3, 2018 •

edited

Loading

kumar303 commented Jan 11, 2018 •

edited

Loading