Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Render xml result - opensearch #731

Merged
merged 11 commits into from
Jun 3, 2022
Merged

Render xml result - opensearch #731

merged 11 commits into from
Jun 3, 2022

Conversation

mgautierfr
Copy link
Member

@mgautierfr mgautierfr commented Mar 23, 2022

Fixes #586

@stale
Copy link

stale bot commented Apr 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be now be reviewed manually. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@kelson42 kelson42 changed the title WIP Render xml result. WIP Render xml result - opensearch Apr 27, 2022
@stale stale bot removed the stale label Apr 27, 2022
@kelson42
Copy link
Collaborator

@mgautierfr This PR has not been updated since a month and is a blocker to the - pretty urgent to be released - milestone 10.2.0. As far as I understand it depends on #729 to fully work. But it would be great to finish/polish it ASAP (whatever if #729 is merged or not) so @veloman-yunkan can start to review it (and that way we can parallellize the work and speed-up).

@mgautierfr mgautierfr changed the base branch from search_rendering to multizimsearch May 4, 2022 17:02
@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #731 (3d911ce) into master (c4f7068) will increase coverage by 0.11%.
The diff coverage is 84.21%.

❗ Current head 3d911ce differs from pull request most recent head cc8ad9e. Consider uploading reports for the commit cc8ad9e to get more accurate results

@@            Coverage Diff             @@
##           master     #731      +/-   ##
==========================================
+ Coverage   63.48%   63.60%   +0.11%     
==========================================
  Files          59       59              
  Lines        4050     4077      +27     
  Branches     2204     2220      +16     
==========================================
+ Hits         2571     2593      +22     
- Misses       1477     1481       +4     
- Partials        2        3       +1     
Impacted Files Coverage Δ
include/search_renderer.h 100.00% <ø> (ø)
src/server/request_context.h 100.00% <ø> (ø)
src/server/internalServer_catalog_v2.cpp 92.94% <50.00%> (ø)
src/server/internalServer.cpp 84.75% <66.66%> (-0.83%) ⬇️
src/search_renderer.cpp 91.89% <100.00%> (+1.07%) ⬆️
src/server/request_context.cpp 82.82% <100.00%> (+1.38%) ⬆️
src/server/response.cpp 92.46% <100.00%> (+0.03%) ⬆️
src/server/response.h 100.00% <100.00%> (ø)

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 c4f7068...cc8ad9e. Read the comment docs.

@mgautierfr
Copy link
Member Author

@veloman-yunkan While this PR is based on #729 it would like a small review :)

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My expectation was that InternalServer::selectBooks() should have been updated by this PR.

src/server/internalServer.cpp Outdated Show resolved Hide resolved
src/server/internalServer.cpp Outdated Show resolved Hide resolved
src/search_renderer.cpp Outdated Show resolved Hide resolved
static/templates/search_result.xml Outdated Show resolved Hide resolved
static/ft_opensearchdescription.xml Outdated Show resolved Hide resolved
@mgautierfr mgautierfr force-pushed the multizimsearch branch 2 times, most recently from 96ea2c8 to 8893f84 Compare May 11, 2022 09:43
@kelson42 kelson42 changed the title WIP Render xml result - opensearch Render xml result - opensearch May 12, 2022
@kelson42 kelson42 marked this pull request as ready for review May 12, 2022 04:10
@kelson42
Copy link
Collaborator

@veloman-yunkan Any new feedback on this PR?

@veloman-yunkan
Copy link
Collaborator

@kelson42 I will review it by Monday morning.

static/templates/error.xml Outdated Show resolved Hide resolved
src/server/response.cpp Show resolved Hide resolved
results.xml Outdated Show resolved Hide resolved
@mgautierfr mgautierfr force-pushed the multizimsearch branch 4 times, most recently from 813e6fb to 1b3da9f Compare May 24, 2022 15:07
@mgautierfr mgautierfr force-pushed the multizimsearch branch 2 times, most recently from 13c8318 to 995d15f Compare May 31, 2022 12:47
@mgautierfr mgautierfr force-pushed the multizimsearch branch 2 times, most recently from 684e5dc to a7651d0 Compare June 2, 2022 10:39
Base automatically changed from multizimsearch to master June 2, 2022 10:49
static/ft_opensearchdescription.xml Outdated Show resolved Hide resolved
static/templates/search_result.xml Outdated Show resolved Hide resolved
static/templates/search_result.xml Outdated Show resolved Hide resolved
src/server/internalServer.cpp Outdated Show resolved Hide resolved
src/server/response.cpp Show resolved Hide resolved
@mgautierfr mgautierfr force-pushed the opensearch branch 2 times, most recently from 624a74d to 3d911ce Compare June 3, 2022 10:01
static/templates/search_result.xml Outdated Show resolved Hide resolved
src/server/internalServer.cpp Outdated Show resolved Hide resolved
src/server/response.cpp Outdated Show resolved Hide resolved
src/server/response.cpp Outdated Show resolved Hide resolved
Comment on lines +38 to +46
const std::vector<std::string> LARGE_SEARCH_RESULTS = {
SEARCH_RESULT(
/*link*/ "/ROOT/zimfile/A/Genius_+_Soul_=_Jazz",
/*title*/ "Genius + Soul = Jazz",
/*snippet*/ R"SNIPPET(...Grammy Hall of Fame in 2011. It was re-issued in the UK, first in 1989 on the Castle Communications "Essential Records" label, and by Rhino Records in 1997 on a single CD together with Charles' 1970 My Kind of <b>Jazz</b>. In 2010, Concord Records released a deluxe edition comprising digitally remastered versions of Genius + Soul = <b>Jazz</b>, My Kind of <b>Jazz</b>, <b>Jazz</b> Number II, and My Kind of <b>Jazz</b> Part 3. Professional ratings Review scores Source Rating Allmusic link Warr.org link Encyclopedia of Popular Music...)SNIPPET",
/*bookTitle*/ "Ray Charles",
/*wordCount*/ "242"
),

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We better avoid this huge duplication between server_xml_search.cpp and server_html_search.cpp test suites

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree with you. But I think it is better for now to merge the PR as it is now to avoid too long review process (as long as the code is working).
But we need to do that in next future.

@mgautierfr mgautierfr force-pushed the opensearch branch 2 times, most recently from c64cce7 to 1708074 Compare June 3, 2022 13:28
mgautierfr and others added 11 commits June 3, 2022 15:46
The file starts now to be too long.

- Move testing of the search html result in `test/server_html_search.cpp`
- Move common code used to launch server and so
  in `test/server_testing_tools.h'

This is mainly code move with a small change:
Instead of setting the default PORT (8001) as a const int in the
`ServerTest` class, we now use SERVER_PORT.
SERVER_PORT must be defined before include `server_testing_tools.h`.
This allow several test to be run in parallele without trying to open
the same port.
It avoid some duplication around the actual data to test.
`test/server_xml_search.cpp` is a plain copy of
`test/server_html_search.cpp`
@veloman-yunkan
Copy link
Collaborator

CodeFactor's failure is due to a bug in CodeFactor - nothing of the kind reported by CodeFactor is in the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement opensearch backend to kiwix-serve
3 participants