Apps-android-commons: Refactor and convert CategoryPresenter to kotlin

Created on 6 May 2020  路  7Comments  路  Source: commons-app/apps-android-commons

Summary:

Current usage is difficult to change and non idiomatic Rx.

Would you like to work on the issue?

Yes

Most helpful comment

Seems like a small fix, raising a PR soon after verifying locally

All 7 comments

I'm re-opening this as I believe the conversion introduced a bug. The converted version lower-cases the search word before sending it to the allcategories API. I'm not sure why that's done. Anyways, as detailed in my comment in #3179 that the API is case-sensitive so lower-casing effectively reduces the category search space to only those categories that have lower case characters in their title. I certainly think we don't want that. So, why are we lower-casing the search keyword? Was this done by accident?

Also, its surprising we don't have any tests to catch this earlier. I think it would be good idea to add one.

On a related note, I also think there's not much benefit in lower-casing the words sent to the search API either. The algorithm used by search API is more clever than the simple prefix only search done by the allcategories API. You could read more about it here: Help:CirrusSearch - MediaWiki.

Yes, it sounds like we should remove the toLowerCase() call.

Seems like a small fix, raising a PR soon after verifying locally

@misaochan Just a reminder that we might want to ensure that we fix this before the corresponding change goes into production.

Hi @ashishkumar468 , did your PR go through? I can only seem to find the commit on your fork.

@misaochan I have raised a PR for the same on master, 2.13 anyways did not have the original change

Was this page helpful?
0 / 5 - 0 ratings