Fix Search header doesn't mention type #3154 #3161

PunitGr · 2017-09-15T17:12:25Z

Fixes mozilla/addons#10778

This PR fixes the Search Header mention type issue which appears while searching a query.

I've updated the SearchText string by using addonType from filters which fixes this bug. Also, changed the addonType text to plural if search results are more than 1.

Screenshots

Test Case: All Passed

Search Preview after the fix:

After updating test cases

Thanks!

tofumatt

This is the right idea but unfortunately you'll need to create a string for each add-on type. I think doing it just for ADDON_TYPE_EXTENSION, ADDON_TYPE_DICT, ADDON_TYPE_LANG, and ADDON_TYPE_THEME like in https://github.com/mozilla/addons-frontend/blob/master/src/amo/components/SearchForm/index.js#L207 should be fine.

You'll also need to add tests for these strings. All your tests are passing but you're missing test coverage for the code you added, so you'll want to look at the TestSearchContextCard and add test cases for each one of these states.

tofumatt · 2017-09-15T17:27:40Z

src/amo/components/SearchContextCard/index.js


    let searchText;

-    if (!loading && query) {
+    if (!loading && query && addonType) {
+      const addon = addonType === 'persona' ? 'theme' : addonType;


Unfortunately addonType is not localised, so this won't work in any language that isn't English (and even then, addonType isn't very readible if it's something like langpack).

So this will need to be more of an if/else case like we have here: https://github.com/mozilla/addons-frontend/blob/master/src/amo/components/SearchForm/index.js#L207

Sorry about that!

Alright! I don't know why I didn't think of it. Sorry, my bad!

No worries, we've made the same mistake too, that's the only reason why I knew to look out for it! 😉

codecov-io · 2017-09-15T17:43:53Z

Codecov Report

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

@@            Coverage Diff             @@
##           master    mozilla/addons-frontend#3161      +/-   ##
==========================================
+ Coverage    95.1%   95.13%   +0.03%     
==========================================
  Files         155      155              
  Lines        2837     2856      +19     
  Branches      555      564       +9     
==========================================
+ Hits         2698     2717      +19     
  Misses        120      120              
  Partials       19       19

Impacted Files	Coverage Δ
src/amo/components/SearchContextCard/index.js	`100% <100%> (ø)`	⬆️
src/amo/components/AddonReviewList.js	`100% <0%> (ø)`	⬆️
src/amo/components/Addon/index.js	`100% <0%> (ø)`	⬆️
src/ui/components/ShowMoreCard/index.js	`100% <0%> (ø)`	⬆️
src/core/api/index.js	`96.77% <0%> (+0.16%)`	⬆️

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 2c22414...07a3983. Read the comment docs.

tofumatt

Thanks for the really good test specs (and the tests!)

This looks great, aside from just a few tweaks in the test file it's good. The only major thing to fix is to put the entire string in the gettext call (eg ``'%(count)s dictionaries found for "%(query)s"'instead of substituing in theaddonType`.)

We tried to do that in the past but there are edge cases with localisation. Sorry about that, it's annoying but it's the best way to get the best localisations.

Thanks so much for the quick reply to my earlier review though. 😄

tofumatt · 2017-09-16T00:17:55Z

src/amo/components/SearchContextCard/index.js


-    if (!loading && query) {
+    if (addonType === ADDON_TYPE_EXTENSION) {
+      addOn = i18n.ngettext('extension', 'extensions', count);


Oh, this is clever but unfortunately it's best to put the entire string in here instead. Some locales have complex pluralisation rules and it's best to have the entire string in context. I know it's a bit less elegant that way, but if you look at the example I referenced earlier that's how we do it. Could you put the entire string (in both singular and plural forms) in one gettext call?

Yes sure, thanks for letting me know regarding this. Will keep this in mind in future.

tofumatt · 2017-09-16T00:18:38Z

src/amo/components/SearchContextCard/index.js

+        '%(count)s %(addOn)s found for "%(query)s"'
+      ), { count: i18n.formatNumber(count), addOn, query }
+      );
+    } else if (!loading && query && !addonType) {


I think the !addonType is redundant; because you test for addonType above and this is otherwise the same check you can just remove that check here 😄

tofumatt · 2017-09-16T00:20:02Z

tests/unit/amo/components/TestSearchContextCard.js

+      .toIncludeText('2 extensions found for "test"');
+  });
+
+  // addonType = ADDON_TYPE_EXTENSION


You don't need these comments here, you wrote really good test specs actually (👍🏻) and the dispatchSearchResults calls show which type you're using so you can remove these comments.

tofumatt · 2017-09-16T00:20:40Z

tests/unit/amo/components/TestSearchContextCard.js

+      .toIncludeText('1 dictionary found for "test"');
+  });
+
+  it('should render plural form when multipe results are found with addonType as ADDON_TYPE_DICT', () => {


Typo here: multipe should be multiple. 😄

tofumatt · 2017-09-16T00:21:28Z

tests/unit/amo/components/TestSearchContextCard.js

+  });
+
+  // addonType = ADDON_TYPE_DICT
+  it('should render singular form when only one result is found with addonType as ADDON_TYPE_DICT', () => {


I think you can just end with ADDON_TYPE_DICT instead of addonType as ADDON_TYPE_DICT. Similar for the rest of the test specs.

They're otherwise really good though, thanks for writing really clear test specs! 👍

PunitGr · 2017-09-19T15:22:45Z

@tofumatt , I have updated the code. Please let me know if you recommend any more changes. Thanks.

kumar303 · 2017-09-19T16:09:43Z

src/amo/components/SearchContextCard/index.js

+          '%(count)s themes found for "%(query)s"',
+          count), { count: i18n.formatNumber(count), query }
+        );
+      }


This could use an else block that shows some kind of generic message like '12 results found for...'

PunitGr · 2017-09-19T16:32:53Z

@kumar303, I was gonna do that before but couldn't find a test case regarding the same when the addonType is something else. I've updated it.
Thanks

kumar303 · 2017-09-19T16:56:24Z

Thanks. The else branch now needs test coverage (unless I'm missing it).

PunitGr · 2017-09-19T17:31:13Z

@kumar303 , Here is what I think, the else statement doesn't work because it doesn't get any response from the server API. And if we don't get any response no result is rendered on the frontend. Don't you think there should be a response if the addonType someone mentioned doesn't exist in the database?

kumar303 · 2017-09-19T17:52:18Z

It's an edge case but it's still possible. You can get trigger a "valid" search with this archaic type (if you edit the URL):

http://localhost:3000/en-US/firefox/search/?platform=mac&q=firefox&type=theme

This theme type has been replaced by persona so we shouldn't use it but it should still render a heading in this case. I'm more worried about when we add a new type and maybe forget to update the code. We could either throw an error or display a generic message -- I like displaying a generic message better.

PunitGr · 2017-09-19T18:00:18Z

@kumar303 , it works for the cases if addonType exists in the DB, even for themes. But the problem occurs when there is searched addonType doesn't have a record in the DB like for http://localhost:3000/en-US/firefox/search/?platform=mac&q=firefox&type=xyz.
Since there is no record for xyz in the DB. The search gets stuck and no response is rendered.

PunitGr · 2017-09-19T18:30:41Z

@kumar303, Take this for an example,

The screen is stuck there since loading = true even at count = 0.

kumar303 · 2017-09-19T19:19:44Z

http://localhost:3000/en-US/firefox/search/?platform=mac&q=firefox&type=xyz.
Since there is no record for xyz in the DB. The search gets stuck and no response is rendered.

That looks like a separate bug. It's not so important since no user would see it while using the site.

PunitGr · 2017-09-19T19:32:52Z

@kumar303 , Okay.
I have added the test cases for else block.
Thanks!

kumar303

Thanks! It looks like you also addressed @tofumatt's change requests so we can merge this.

caitmuenster · 2017-09-20T15:40:30Z

Thanks so much for the patch, @PunitGr! Your contribution has been added to our recognition wiki.

If you'd like to set up a profile on mozillians.org, I would be happy to vouch for you.

Welcome onboard! I look forward to seeing you around. :)

PunitGr · 2017-09-20T17:14:53Z

Hi @caitmuenster Thank you so much for the invitation. I've set up my profile. I would be grateful if you can vouch for my profile here https://mozillians.org/en-US/u/PunitGr/

caitmuenster · 2017-09-20T17:23:48Z

\o/ You're all vouched!

PunitGr added 2 commits September 15, 2017 22:20

Fix Search header doesn't mention type #3154

4edaf53

Update searchText

2624ea3

tofumatt suggested changes Sep 15, 2017

View reviewed changes

PunitGr added 2 commits September 16, 2017 01:24

Add localization to addonType

ad7380f

Update test cases for SearchContextCard

4303858

tofumatt suggested changes Sep 16, 2017

View reviewed changes

PunitGr added 2 commits September 16, 2017 08:02

Update localisation for addonType

74618cc

Fix typo

2fd8c90

kumar303 reviewed Sep 19, 2017

View reviewed changes

Add else block for searchText

469709d

Update search result statement and add test cases for SearchCard

07a3983

kumar303 approved these changes Sep 20, 2017

View reviewed changes

kumar303 merged commit 47c66bd into mozilla:master Sep 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Search header doesn't mention type #3154 #3161

Fix Search header doesn't mention type #3154 #3161

PunitGr commented Sep 15, 2017 •

edited

tofumatt left a comment

tofumatt Sep 15, 2017

PunitGr Sep 15, 2017

tofumatt Sep 15, 2017 •

edited

codecov-io commented Sep 15, 2017 •

edited

tofumatt left a comment

tofumatt Sep 16, 2017

PunitGr Sep 16, 2017

tofumatt Sep 16, 2017

tofumatt Sep 16, 2017

tofumatt Sep 16, 2017

tofumatt Sep 16, 2017

PunitGr commented Sep 19, 2017

kumar303 Sep 19, 2017

PunitGr commented Sep 19, 2017 •

edited

kumar303 commented Sep 19, 2017

PunitGr commented Sep 19, 2017 •

edited

kumar303 commented Sep 19, 2017

PunitGr commented Sep 19, 2017 •

edited

PunitGr commented Sep 19, 2017 •

edited

kumar303 commented Sep 19, 2017

PunitGr commented Sep 19, 2017

kumar303 left a comment

caitmuenster commented Sep 20, 2017

PunitGr commented Sep 20, 2017

caitmuenster commented Sep 20, 2017

Fix Search header doesn't mention type #3154 #3161

Fix Search header doesn't mention type #3154 #3161

Conversation

PunitGr commented Sep 15, 2017 • edited

Fixes mozilla/addons#10778

Screenshots

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

tofumatt Sep 15, 2017 • edited

Choose a reason for hiding this comment

codecov-io commented Sep 15, 2017 • edited

Codecov Report

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

PunitGr commented Sep 19, 2017

Choose a reason for hiding this comment

PunitGr commented Sep 19, 2017 • edited

kumar303 commented Sep 19, 2017

PunitGr commented Sep 19, 2017 • edited

kumar303 commented Sep 19, 2017

PunitGr commented Sep 19, 2017 • edited

PunitGr commented Sep 19, 2017 • edited

kumar303 commented Sep 19, 2017

PunitGr commented Sep 19, 2017

kumar303 left a comment

Choose a reason for hiding this comment

caitmuenster commented Sep 20, 2017

PunitGr commented Sep 20, 2017

caitmuenster commented Sep 20, 2017

PunitGr commented Sep 15, 2017 •

edited

tofumatt Sep 15, 2017 •

edited

codecov-io commented Sep 15, 2017 •

edited

PunitGr commented Sep 19, 2017 •

edited

PunitGr commented Sep 19, 2017 •

edited

PunitGr commented Sep 19, 2017 •

edited

PunitGr commented Sep 19, 2017 •

edited