Allow kiwix-serve to get suggestions of custom range #591

maneeshpm · 2021-07-25T07:06:53Z

This PR will allow kiwix serve to retrieve suggestions of a given range that can be specified in the front end call. These changes will open up the backend to implement continuous scroll feature as specified in the issue. But that is rather working on the front end side and this PR is only "allowing" to enable the feature.

Changes included in this PR:

check for two new arguments start, suggestionLength in the handle_suggest API that will retrieve suggestions in the range given by (start, start+suggestionLength).

codecov · 2021-07-25T07:07:00Z

Codecov Report

Merging #591 (7d67569) into fix_get_results (20cdefc) will decrease coverage by 0.00%.
The diff coverage is 50.00%.

❗ Current head 7d67569 differs from pull request most recent head af825d4. Consider uploading reports for the commit af825d4 to get more accurate results

@@                 Coverage Diff                 @@
##           fix_get_results     openzim/libzim#591      +/-   ##
===================================================
- Coverage            64.92%   64.91%   -0.01%     
===================================================
  Files                   53       53              
  Lines                 3738     3737       -1     
  Branches              1881     1874       -7     
===================================================
- Hits                  2427     2426       -1     
  Misses                1309     1309              
  Partials                 2        2

Impacted Files	Coverage Δ
include/searcher.h	`33.33% <ø> (ø)`
src/searcher.cpp	`47.10% <44.44%> (ø)`
src/server/internalServer.cpp	`83.58% <100.00%> (-0.04%)`	⬇️

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 20cdefc...af825d4. Read the comment docs.

maneeshpm · 2021-07-25T07:29:25Z

src/server/internalServer.cpp

@@ -353,6 +353,7 @@ SuggestionsList_t getSuggestions(const zim::Archive* const archive,
    // TODO: This case should be handled by libzim
    std::vector<std::string> variants = getTitleVariants(queryString);
    int currCount = 0;
+    int suggestionCount = end - start;


Worth mentioning that getting results of a certain range in a non-Xapian based search is not supported as of now, so we will simply consider the count of suggestions here.
This will anyway have to be implemented on libzim side after openzim/libzim#574

mgautierfr

Code is good but I'm a bit disturbed by the idea of adding a somehow hidden feature, not tested and not used.

It would be nice to have this change associated with a change in the front end to use it (and some tests of the feature)

src/server/internalServer.cpp

kelson42 · 2021-07-28T09:40:34Z

Code is good but I'm a bit disturbed by the idea of adding a somehow hidden feature, not tested and not used.

It would be nice to have this change associated with a change in the front end to use it (and some tests of the feature)

I'm supportive of this comment. IMO, we should have a dedicated automated test able that way for example to retrieve by range of 10 suggestions, around 100 suggestions. The case of stopping before the end, and reaching the end of available suggestions should be tested. Cases of giving out-of-bands values to the ABI should be properly tested as well.

maneeshpm · 2021-07-29T17:57:25Z

Thanks for the suggestion @kelson42 @mgautierfr. It revealed a nasty error openzim/libzim#595. Working on a fix, shouldn't take much time.

maneeshpm · 2021-07-30T17:56:05Z

@kelson42 Added some unit tests for checking the added changes.

Rebasing the PR on openzim/libzim#597 just to show the solution employed to our earlier tackled issue is employed here as well.

kelson42 · 2021-07-31T16:48:55Z

@MananJethwani Would you please verify that this feature would effectively allow us for example to provide an "infinite" scroll possibility in the suggestion selecbox?

kelson42 · 2021-08-11T05:47:03Z

@MananJethwani Coukd you please validate this PR? See my latest comment.
@maneeshpm What is the overall status? Are we expecting new review by @mgautierfr?

maneeshpm · 2021-08-11T06:56:43Z

@kelson42 The last review pointed out a lack of unit testing in this pr. I have added some new tests to demonstrate the features added. And hence, a new review is necessary.

kelson42 · 2021-08-18T09:14:27Z

@maneeshpm Can you please rebase this branch?

mgautierfr

It seems the two first commits are already merged in master.
They are keep in the PR by the rebase because they are using tab instead of space. We should simply remove them.

The update fix only one of my remarks. It add unit-test.
But we are adding a feature that no one is using. It would be nice to
make the kiwix-serve "frontend" uses the feature provided by the backend.
But this is js development and it should probably be made by a different developer. @kelson42 I let you decide on this. Having unit-test is enough for me.

maneeshpm · 2021-08-19T14:13:19Z

Rebased to fix the commit history.
I agree with @mgautierfr, will let anyone with experience in js build the front end. Till now, we have "enabled" the option from the backend.

mgautierfr

Sorry, I'm adding a late remark.

src/server/internalServer.cpp

This will allow handle_suggest API to accept two arguments `start` and `suggestionLength` that will allow handle_suggest to retrieve suggestions in the given range rather than the default 0-10 range.

maneeshpm force-pushed the suggestion_range branch from f03a11f to cce75f6 Compare July 25, 2021 07:07

maneeshpm requested a review from mgautierfr July 25, 2021 07:21

maneeshpm commented Jul 25, 2021

View reviewed changes

kelson42 force-pushed the suggestion_range branch from cce75f6 to 1bd389c Compare July 25, 2021 08:11

mgautierfr requested changes Jul 27, 2021

View reviewed changes

src/server/internalServer.cpp Outdated Show resolved Hide resolved

maneeshpm force-pushed the suggestion_range branch from 1bd389c to 78d1b2b Compare July 30, 2021 17:33

maneeshpm changed the base branch from master to fix_get_results July 30, 2021 17:34

maneeshpm force-pushed the suggestion_range branch from 78d1b2b to af825d4 Compare July 30, 2021 17:47

maneeshpm requested a review from mgautierfr July 30, 2021 18:22

maneeshpm force-pushed the fix_get_results branch 2 times, most recently from 796d3d8 to c88dfa2 Compare August 4, 2021 13:09

maneeshpm force-pushed the suggestion_range branch from af825d4 to 72d4c19 Compare August 4, 2021 13:17

maneeshpm force-pushed the fix_get_results branch 3 times, most recently from 9cff130 to 9addd82 Compare August 4, 2021 13:51

Base automatically changed from fix_get_results to master August 4, 2021 13:58

maneeshpm force-pushed the suggestion_range branch 2 times, most recently from 2896b35 to fad6919 Compare August 11, 2021 06:49

mgautierfr requested changes Aug 19, 2021

View reviewed changes

maneeshpm force-pushed the suggestion_range branch 2 times, most recently from 199bf96 to 8066232 Compare August 19, 2021 14:06

mgautierfr requested changes Aug 19, 2021

View reviewed changes

src/server/internalServer.cpp Outdated Show resolved Hide resolved

Allow kiwix-serve to get suggestions of custom range

61209ea

This will allow handle_suggest API to accept two arguments `start` and `suggestionLength` that will allow handle_suggest to retrieve suggestions in the given range rather than the default 0-10 range.

maneeshpm force-pushed the suggestion_range branch from 8066232 to 61209ea Compare August 19, 2021 15:35

maneeshpm requested a review from mgautierfr August 19, 2021 15:54

mgautierfr approved these changes Aug 19, 2021

View reviewed changes

kelson42 merged commit ea6413f into master Aug 20, 2021

kelson42 deleted the suggestion_range branch August 20, 2021 06:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow kiwix-serve to get suggestions of custom range #591

Allow kiwix-serve to get suggestions of custom range #591

maneeshpm commented Jul 25, 2021

codecov bot commented Jul 25, 2021 •

edited

Loading

maneeshpm Jul 25, 2021 •

edited

Loading

mgautierfr left a comment

kelson42 commented Jul 28, 2021

maneeshpm commented Jul 29, 2021

maneeshpm commented Jul 30, 2021 •

edited

Loading

kelson42 commented Jul 31, 2021

kelson42 commented Aug 11, 2021

maneeshpm commented Aug 11, 2021

kelson42 commented Aug 18, 2021

mgautierfr left a comment

maneeshpm commented Aug 19, 2021

mgautierfr left a comment

Allow kiwix-serve to get suggestions of custom range #591

Allow kiwix-serve to get suggestions of custom range #591

Conversation

maneeshpm commented Jul 25, 2021

codecov bot commented Jul 25, 2021 • edited Loading

Codecov Report

maneeshpm Jul 25, 2021 • edited Loading

Choose a reason for hiding this comment

mgautierfr left a comment

Choose a reason for hiding this comment

kelson42 commented Jul 28, 2021

maneeshpm commented Jul 29, 2021

maneeshpm commented Jul 30, 2021 • edited Loading

kelson42 commented Jul 31, 2021

kelson42 commented Aug 11, 2021

maneeshpm commented Aug 11, 2021

kelson42 commented Aug 18, 2021

mgautierfr left a comment

Choose a reason for hiding this comment

maneeshpm commented Aug 19, 2021

mgautierfr left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 25, 2021 •

edited

Loading

maneeshpm Jul 25, 2021 •

edited

Loading

maneeshpm commented Jul 30, 2021 •

edited

Loading