Skip to content
This repository has been archived by the owner on Nov 7, 2020. It is now read-only.

Searching Algolia #65

Merged
merged 10 commits into from
Apr 4, 2020
Merged

Searching Algolia #65

merged 10 commits into from
Apr 4, 2020

Conversation

nathanbillis
Copy link
Contributor

Search is fully functional with Algolia

@nathanbillis nathanbillis added enhancement New feature or request Minor Minor release eg. user story being complete Patch bug fixes or improvements labels Apr 3, 2020
@sleepy-evelyn
Copy link
Contributor

sleepy-evelyn commented Apr 4, 2020

Works really well.
Ingredients and name filters are seperately clearly. Tried searching 'cooker' and it turned up only within the name for the one of the recipes which is brilliant.
Also works with filters incredibly well and the queries are done very fast considering the amount of operations they need to perform, especially for ingredients matching.

I can talk about adding Author searching and the different approaches we could use. The best approach will be the one that limits queries.

If possible, I know it's extra time but i'm sure Espresso tests can be written in about 30 mins for this and would definately be worth it. IE search, does the proper recipes show up. etc... Manual testing works as well tho.

Major Bugs:

  • Can search a random string of characters that aren't letters/numbers and it will return the entire database.
    image
    Returned basically everything...

image

  • Crashes completely randomly i'd say one out of 50 queries. Have absolutely no idea why it happens but it seems like a null object reference so we could handle it and make sure the error is handled as a soft error and dosen't crash the app.
    Process: com.group4sweng.scranplan, PID: 19041
    java.lang.NullPointerException: Attempt to invoke virtual method 'int java.lang.String.length()' on a null object reference
        at java.net.URLEncoder.encode(URLEncoder.java:204)
        at com.algolia.search.saas.Index.<init>(Index.java:79)
        at com.algolia.search.saas.Client.getIndex(Client.java:153)
        at com.group4sweng.scranplan.SearchFunctions.SearchListFragment.onCreateView(SearchListFragment.java:161)
        at androidx.fragment.app.Fragment.performCreateView(Fragment.java:2698)
        at androidx.fragment.app.FragmentStateManager.createView(FragmentStateManager.java:320)
        at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1187)
        at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1356)
        at androidx.fragment.app.FragmentManager.moveFragmentToExpectedState(FragmentManager.java:1434)
        at androidx.fragment.app.FragmentManager.moveToState(FragmentManager.java:1497)
        at androidx.fragment.app.BackStackRecord.executeOps(BackStackRecord.java:447)
        at androidx.fragment.app.FragmentManager.executeOps(FragmentManager.java:2169)
        at androidx.fragment.app.FragmentManager.executeOpsTogether(FragmentManager.java:1992)
        at androidx.fragment.app.FragmentManager.removeRedundantOperationsAndExecute(FragmentManager.java:1947)
        at androidx.fragment.app.FragmentManager.execPendingActions(FragmentManager.java:1849)
        at androidx.fragment.app.FragmentManager$4.run(FragmentManager.java:413)
        at android.os.Handler.handleCallback(Handler.java:883)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

Point it crashed at.
image
(p.s. the red text is a bug with android studio that requires a restart to update the layout. I'm pretty sure I synced the gradle files before launching).

Minor Bugs
Most of these other ones already existed before adding improved querying support I think

  • Have to press back twice after querying. This I think was originally in there with the Firebase query approach but needs to be fixed.
  • No more results dialog appears at the bottom regardless.
  • When changing filter options before searching the main home screen dosen't update with the new recipes available.
  • Unique search filter options stay selected even after switching between the mealplanner and timeline but reset when visiting other activities to the profiles defaults. IE go to public profile and they reset but switch tabs to mealplanner and they remain the same.

@sleepy-evelyn
Copy link
Contributor

sleepy-evelyn commented Apr 4, 2020

Possible improvements (optional):

  • Failsafe for strings of length < 3. Prevents incredibly long list of single character matches.
  • Failsafe of say 500 matches. Hard limit so that when we add more recipes we can't query what is basically the entire database.
  • Make the user unable to search without inputting at least 1 number and(or) letter to prevent revealing the entire database.

p.s. I know this is quite a lot of stuff to add/change + the branch already looks fairly complex and most of it's existing stuff that needed fixing.
Main issues i'd say atm are the major bugs, and the double backspace thing which the user will immediately notice.

@nathanbillis
Copy link
Contributor Author

To briefly explain how this searching works. When a new recipe is made a cloud function is run which updates the Algolia database with the new recipe details. Then when searching for a query the string (together with any filters) is passed directly to Algolia which returns a JSON file with the results. This file is then traversed to find the UUID of the recipes which is then passed into a method that gets the recipe document which intern is past to the original display mechanism which Louis wrote when first writing the search.

Using Algolia instead of queries with firebase allows us so much more customisation with our results and ranking methods, instead of 30 lines of code its 1. We can also limit on Algolia the number of results it returns without even going into the code for the app. Algolia has 3 (or so) indexes for recipes each for the custom rankings together with a google style ranking system that we don’t use.

Stuff like author searching is out of the remit of this original user story which was to fix the two issues for searching. If other stuff thats not related to those stories needs doing then they should end up on a new branch so this doesn’t become stale. This branch is only 24 hours old and was one commit off the master making it easy to merge. As soon as we add extra stuff we should be branching because its a different feature.

Bugs 🐛

  • Can search a random string of characters that aren't letters/numbers and it will return the entire database. - This feature appears due to the way that Algolia normalises both searching and query’s, to fix it would involve blocking a lot of characters app side, I think it can be fixed in algolia but needs more research.
  • Have to press back twice after querying. This I think was originally in there with the Firebase query approach but needs to be fixed. - Need to look into this because I can’t get it to replicate on my physical device but can only in the simulator.
  • No more results dialog appears at the bottom regardless. - This is a feature so people don’t just think the search doesn’t work. It can however be changed to say no results found
  • When changing filter options before searching the main home screen dosen't update with the new recipes available. - This is outside of the original remit for this user story, if thats a feature that needs adding, we should open an issue and discuss it at the next meeting.
  • Unique search filter options stay selected even after switching between the mealplanner and timeline but reset when visiting other activities to the profiles defaults. IE go to public profile and they reset but switch tabs to mealplanner and they remain the same. - Same as above
  • Failsafe for strings of length < 3. Prevents incredibly long list of single character matches. - This is something that can be done directly in Algolia
  • Failsafe of say 500 matches. Hard limit so that when we add more recipes we can't query what is basically the entire database. - Algolia already won’t return the whole database it has a limit, I have not changed it currently because we only have 15 recipes but its as simple as changing a number on their interface.
  • Make the user unable to search without inputting at least 1 number and(or) letter to prevent revealing the entire database. - Hopefully this can be limited in Algolia directly too but at what point do we cut off searching for characters

@nathanbillis
Copy link
Contributor Author

@nathanbillis
Copy link
Contributor Author

HomeTest.java is the espresso test originally written for this search

@sleepy-evelyn
Copy link
Contributor

sleepy-evelyn commented Apr 4, 2020

Thanks for the clarity about how the whole system works. That’s really handy to help understand what we can and can’t do with Algolia.
If u want I can look into the stuff with Algolia next week, including the character limits and restrictions alongside the profile tests but I’m holding off a bit this week to get other uni work done. Hope that’s ok.

If we can’t implement input checks in Algolia (we probably can) the kitchen sink way would be to check if any character of number exists in the input and the input is greater than a certain string length. (2 in this case) Literally just compare to a list of all available characters and numbers in English. That would fix 3 issues immediately. This method would be useful to add as a test as I think a lot of what we get marked on is good tests for this module.

So the no more results found removal is so that if recipes are retrieved successfully we don’t need to display it. Only when no recipe is found at all.
EDIT: Realised this was intentional but as you stated it’s probably better to put ‘no results found’

Not sure if you skipped the main bug but you need to add a check at what I think was the ‘Index scoreIndex’ for a null object reference that returns from the search safely or fix the error altogether otherwise occasionally the search query will crash the app. Assuming this is already being worked on and tested.

@nathanbillis
Copy link
Contributor Author

Not sure if you skipped the main bug but you need to add a check at the ‘Index scoreIndex’ for a null object reference that returns from the search safely otherwise occasionally the search query will crash the app. Assuming this is being worked on and tested.

Done

@nathanbillis
Copy link
Contributor Author

This is the type of analytics we get from the search
image

@nathanbillis nathanbillis merged commit cf6337b into master Apr 4, 2020
@nathanbillis nathanbillis deleted the searchBugFix branch April 4, 2020 13:21
@nathanbillis
Copy link
Contributor Author

@nathanbillis
Copy link
Contributor Author

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request Minor Minor release eg. user story being complete Patch bug fixes or improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Searching for recipe and clicking on the no more results Queries failing
2 participants