Add category page (close #1378) #1398

tofumatt · 2016-11-21T12:22:43Z

Add the actual category URL (matches addons-server) including pagination.

Screenshot:

kumar303

Nice, I like how the category component extends the search component. I had some questions and requested a few changes.

kumar303 · 2016-11-21T16:40:04Z

src/amo/components/SearchPage.js

+      convertFiltersToQueryParams(filters);
+    const paginator = count && hasSearchParams > 0 ? (
+      <Paginate LinkComponent={LinkComponent} count={count} currentPage={page}
+        pathname={pathname} queryParams={queryParams} showPages={0} />


why is showPages always 0? Is that because the UI shouldn't ever show links to numbered pages?

Yeah; we use the numbered pages in the admin but not AMO.

It's a bit of a holdover, arguably showPages={0} should be the default. Filed: #1405

kumar303 · 2016-11-21T16:48:22Z

src/amo/containers/CategoryPage.js

+
+
+export function mapStateToProps(state, ownProps) {
+  const filters = {


I think it would make sense to move this definition and other relevant ones into the block if (filtersMatch(... since that's the only place they are needed. A function like mapStateToProps() will run on every application state change so it's a bit sensitive.

But these filters are declared here to compare them with state and make sure everything matches. They're passed to if (filtersMatch([...])) {, so moving them into there would mean duplication.

They're only passed to the state if they match.

Ah, ok, I see on a second look that all of these do need to be defined outside of the if block.

kumar303 · 2016-11-21T16:53:10Z

src/core/searchUtils.js

+  if (!isLoaded({ state: state.search, page, filters })) {
+    return performSearch({ dispatch, page, api: state.api, auth: state.auth, filters });
+  }
+  return true;


even though it's supported by asyncConnect, I think it's bad practice to mix promise and non-promise return values. I'd rather see: return Promise.resolve(true) or maybe simply Promise.resolve() if that's enough.

I agree actually, as it makes testing weird too. That said:

I've filed an issue for it (#1403), as changing this across the app would be out-of-scope for the PR.

kumar303 · 2016-11-21T16:59:37Z

tests/client/amo/containers/TestCategoryPage.js

+    });
+
+    assert.deepEqual(props, {
+      hasSearchParams: true,


this parameter will be overidden by ...state.search. Maybe this object should begin with ...state.search instead of end with it?

kumar303 · 2016-11-21T17:02:38Z

tests/client/amo/containers/TestCategoryPage.js

+
+
+describe('CategoryPage.mapStateToProps()', () => {
+  const state = {


I think it would be less fragile to create this state by dispatching real actions. It should be just as easy to do. Example: https://github.com/mozilla/addons-frontend/issues/1371

kumar303 · 2016-11-21T17:05:11Z

tests/client/amo/containers/TestCategoryPage.js

+      queryParams: { page: 1 },
+    });
+  });
+});


This appears to be missing a test for the case when state.search is empty (i.e. {}).

I don't think that should ever happen, as it's set by searchStart. Am I missing a path to an empty search state?

The component may render with state.search = {} for a split second if the API response is slow but, no worries, we could always add the test later if a bug appears with handling empty state.

kumar303 · 2016-11-21T17:06:20Z

tests/client/core/test_searchUtils.js

@@ -16,3 +21,77 @@ describe('searchUtils mapStateToProps()', () => {
    assert.deepEqual(props, { hasSearchParams: false });
  });
 });
+
+describe('searchUtils loadCategoryResultsIfNeeded()', () => {
+  const state = {


I'd also like to this state built by dispatching real actions, if possible

kumar303 · 2016-11-21T17:09:04Z

tests/client/core/test_searchUtils.js

+      slug: 'ad-block',
+    };
+    assert.strictEqual(
+      loadCategoryResultsIfNeeded({ store, location, params }), true);


I think this test would be better if it verified that mockApi is never called, which is a stronger indicator that data is not fetched from the API.

kumar303 · 2016-11-21T17:19:32Z

src/core/searchUtils.js

+  return true;
+}
+
+export function loadCategoryResultsIfNeeded({ store: { dispatch, getState }, location, params }) {


This could be shortened slightly to loadByCategoryIfNeeded

kumar303 · 2016-11-21T17:25:40Z

src/core/searchUtils.js

+    category: params.slug,
+    clientApp: params.application,
+  };
+
  if (!isLoaded({ state: state.search, page, filters })) {


Why do the search results need to be cached? I think it should make an API request every time. Otherwise, the single page app will go stale -- the only way a user can obtain newly added results is to refresh the page. The browser will already cache the API call as needed so I don't really see the value in adding a new caching layer on top of that.

Good point. Again, it was like that when I got here; the search page currently caches results. When flipping through paginated results it makes going back and forth between pages quick.

I've filed #1404, but I'll remove it from this bit of code because why not and I agree; we can always add it back.

tofumatt · 2016-11-22T00:36:06Z

Okay, I think I fixed all the issues (mostly regarding state testing) in the PR, and created issues for the things that were slightly out-of-scope. Ready for another r?

tofumatt · 2016-11-22T15:35:25Z

Odd: https://travis-ci.org/mozilla/addons-frontend/builds/177831776 is failing but passing locally, and the error is quite strange.

mstriemer · 2016-11-22T15:51:49Z

Try rebasing with master. That's what I was running into yesterday and switching back to npm seemed to fix it.

tofumatt · 2016-11-22T15:54:23Z

Ah right, cool. Figured this was related, cheers.

kumar303

r+wc

Thanks for filing all the fixup issues.

kumar303 · 2016-11-22T17:00:35Z

src/core/searchUtils.js

  if (!isLoaded({ state: state.search, page, filters })) {
    return performSearch({ dispatch, page, api: state.api, auth: state.auth, filters });
  }
  return true;
 }
+
+export function loadByCategoryIfNeeded(


This name no longer makes sense :) I think loadByCategory would be fine.

Is this making another API request on the client after JS loading on initial render? I think that's why we've normally included the caching.

Hmm, maybe it still needs caching. I was hoping to achieve the rules defined here: https://github.com/mozilla/addons-frontend/issues/1404#issuecomment-262306143

kumar303 · 2016-11-22T17:03:33Z

tests/client/amo/containers/TestCategoryPage.js

+    const store = createStore();
+    store.dispatch(searchStart({ filters }));
+    const mismatchedState = store.getState();
+    mismatchedState.search.filters.clientApp = 'nothing';


Using dispatch (like above) is the way to do it IMO. By using dispatch you are not coupling this code with the raw state representation. Here would be a way to rewrite it:

store.dispatch(searchStart({ filters: { ...filters, clientApp: 'nothing' } })); const mismatchedState = store.getState();

Oops, good point. Will do.

kumar303 · 2016-11-22T17:09:14Z

tests/client/amo/containers/TestCategoryPage.js

+      queryParams: { page: 1 },
+    });
+  });
+});


The component may render with state.search = {} for a split second if the API response is slow but, no worries, we could always add the test later if a bug appears with handling empty state.

kumar303 · 2016-11-22T17:09:47Z

tests/client/core/test_searchUtils.js

@@ -16,3 +20,38 @@ describe('searchUtils mapStateToProps()', () => {
    assert.deepEqual(props, { hasSearchParams: false });
  });
 });
+
+describe('searchUtils loadByCategoryIfNeeded()', () => {


loadByCategoryIfNeeded would need to be renamed too

tofumatt assigned kumar303 Nov 21, 2016

tofumatt added the pull request ready label Nov 21, 2016

kumar303 suggested changes Nov 21, 2016

View reviewed changes

This was referenced May 5, 2024

Always return a Promise for functions passed to asyncConnect mozilla/addons#9971

Closed

Discontinue search result caching mozilla/addons#9972

Closed

Paginate should default to showPages={0} mozilla/addons#9973

Closed

tofumatt force-pushed the categories-page-1378 branch 2 times, most recently from b535565 to c7f363e Compare November 22, 2016 00:35

tofumatt force-pushed the categories-page-1378 branch from c7f363e to 0d1d89c Compare November 22, 2016 15:55

kumar303 approved these changes Nov 22, 2016

View reviewed changes

Add category page (close #1378)

98c26b0

tofumatt force-pushed the categories-page-1378 branch from 0d1d89c to 98c26b0 Compare November 23, 2016 16:25

tofumatt merged commit 53e5f4c into master Nov 23, 2016

tofumatt deleted the categories-page-1378 branch November 23, 2016 16:43

tofumatt removed the pull request ready label Nov 23, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add category page (close #1378) #1398

Add category page (close #1378) #1398

tofumatt commented Nov 21, 2016

kumar303 left a comment

kumar303 Nov 21, 2016

tofumatt Nov 21, 2016

kumar303 Nov 21, 2016

tofumatt Nov 21, 2016

kumar303 Nov 22, 2016

kumar303 Nov 21, 2016

tofumatt Nov 21, 2016

kumar303 Nov 21, 2016

kumar303 Nov 21, 2016

kumar303 Nov 21, 2016

tofumatt Nov 21, 2016

kumar303 Nov 22, 2016

kumar303 Nov 21, 2016

kumar303 Nov 21, 2016

kumar303 Nov 21, 2016

kumar303 Nov 21, 2016

tofumatt Nov 21, 2016

tofumatt commented Nov 22, 2016

tofumatt commented Nov 22, 2016

mstriemer commented Nov 22, 2016

tofumatt commented Nov 22, 2016 via email

kumar303 left a comment

kumar303 Nov 22, 2016

mstriemer Nov 22, 2016

kumar303 Nov 22, 2016

kumar303 Nov 22, 2016

tofumatt Nov 22, 2016

kumar303 Nov 22, 2016

kumar303 Nov 22, 2016



		export function mapStateToProps(state, ownProps) {
		const filters = {



		describe('CategoryPage.mapStateToProps()', () => {
		const state = {

Add category page (close #1378) #1398

Add category page (close #1378) #1398

Conversation

tofumatt commented Nov 21, 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

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 22, 2016

tofumatt commented Nov 22, 2016

mstriemer commented Nov 22, 2016

tofumatt commented Nov 22, 2016 via email

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