Without specifying the "Path" attribute of the cookie in the "Set-Cookie" header
we end up with multiple instances of the cookie for different URLs. We
want a single "global" cookie for kiwix-serve. Besides we want it to be
"permanent" rather than a session cookie, hence the large (1-year-long)
TTL value for the "Max-Age" attribute.
- Description of a test point was not updated in an earlier commit
that added proper handling of the Accept-Language header. Also
after enhancing the limited implementation it made sense to
add another test point demonstrating that the most suitable language
(rather than just the first one in the list) is selected.
- Now failures of the test case because of a missing Set-Cookie header
are more informative.
The question mark (?) is not a valid filename character on Windows.
Changing to a the pound sign (#) so that this repository can still be
cloned on Windows.
Specifying the = symbol with single-character options makes that
character included in the option value (e.g. -l=en results in the
language of the ZIM file being set to =en).
Directly pointing the suggestion link to a /content/... URL avoids
an unnecessary redirection by the server (and an associated bug
related to redirection of URLs with URI-encoded special symbols in
them that - in the current implementation - go into the target URL
in decoded form).
This change fixes two issues:
1. Presence of URL-specific special symbols (such as ? or #) in the book
and/or article name resulted in a wrong suggestion link. This is
fixed by URI-encoding the book name and the path, too.
2. Presence of a single quote symbol in the book and/or article name
resulted in invalid javascript code in the href attribute of the
suggestion link.
The single quote (') symbol is not URL-encoded (unlike its double quote
counterpart). As a result, enclosing a URL-encoded string in single
quotes may result in invalid javascript. Using double quotes instead is
safe, since both double quote (") and backslash (\) symbols (which are
the only special symbols for such quoting) undergo URL-encoding.
We need a fake language for tests that won't be affected by
modifications made by 3rd party translators (see kiwix/libkiwix#749).
- static/i18n/hy.json was cloned as static/i18n/test.json
- usage of "hy" in unit-tests was replaced with "test"
Before this change RequestContext::get_query() returned a reordered
query string (alphabetically sorted by the parameter names).
This fix facilitiates testing of responses where the request URL appears
in the response.
Before this change cacheids were computed only for those static
resources that were referenced from other resources via KIWIXCACHEID.
A few static resources without such references existed.
Now all resources under skin/ have their cacheids computed.
During static resource preprocessing and compilation their cacheid
values are embedded into libkiwix and can be accessed at runtime.
If a static resource is requsted without specifying any cacheid
it is served as dynamic content (with short TTL and the library id
used for the ETag, though using the cacheid for the ETag would
be better).
If a cacheid is supplied in the request it must match the cacheid of the
resource (otherwise a 404 Not Found error is returned) whereupon the
resource is served as immutable content.
Known issues:
- One issue is caused by the fact that some static resources don't get a
cacheid; this is resolved in the next commit.
- Interaction of this change with the support for dynamically customizing
static resources (via KIWIX_SERVE_CUSTOMIZED_RESOURCES env var) was
not addressed.
One (hopefully, last) remaining relative URL to a static resource
is the reference to ./search-icon.svg found in skin/index.css to which
KIWIXCACHEID could not be applied because of the limitations of the
resource preprocessing script `kiwix-resources`.
A rebase invalidated the cacheids in the previous commits of the
iframe_based_content_viewer branch. This commit fixes only the current
state leaving the history with wrong cacheids - this can be an issue
for `git bisect` being executed on a commit range overlapping with
the iframe_based_content_viewer branch.
`--nosearchbar` option of `kiwix-serve` (despite its misleading name)
was used to disable the entire taskbar. This commit accounts for the
existence of that option only partially:
1. Links to books on the welcome/library page are affected - by default
books are displayed in the viewer, but in a kiwix-serve instance run
with --nosearchbar books are loaded in the top window.
2. The `/viewer` endpoint is enabled unconditionally, so if anyone
enters the viewer URL in the address bar they will see books in the
viewer.
Made the viewer respect the `--blockexternal` and `--nolibrarybutton`
options of `kiwix-serve`. Those options are passed to the viewer
via the dynamically generated resource `/viewer_settings.js`.
The only place that the root link is now used is in /skin/index.js,
so added it in static/templates/index.html. But it seems that nothing
prevents us from from switching from aboslute paths to relative paths
in /skin/index.js, which will eliminate the need for the root link
altogether.
As a result of this change content is never decorated by kiwix serve.
Before this fix there were two issues with the taskbar search box:
1. The book used for the suggestions API was resolved only once during
the page load and didn't change during navigation.
2. The current book could not be resolved from a search URL.
Now both issues are fixed.
The greenish taskbar placeholder is gone. The appearance of the old taskbar
is restored. However the taskbar currently contains only the library
button (but the latter leads to the currently blank welcome page).
The next goal is to redirect old-style /book/path/to/entry URLs to
/content/book/path/to/entry, which seemed pretty trivial.
However, given the current handling of some endpoint URLs, more work was
required to ensure that invalid endpoint URLs (e.g. "/random/number" or
"/suggest/fr") are not interpreted as content URLs. Previously, that was
not a user-observable issue, since the result would be an immediate 404
error (except in certain edge cases, like handling the request for
"/random/number" when there is a book with name "random" containing an
article at path "/number"). With redirection of URLs that were assumed
to refer to content a 404 error would be issued for the
transformed URL ("/content/random/number") which may be confusing.
Therefore this change is to ensure the correct routing of endpoint URL
handling.
Book content is now served under /content/book/...
The old access to book content via a top-level URL /book/... is so far
preserved for backward compatibility.
Redirects were changed to use the new URL scheme. Links in the search results
still use the old scheme.
If the server is initialized with a library.xml file, then the id
specified in the XML file is used (rather than the UUID recorded in the
ZIM file).
Note that in test/data/library.xml the book ids are fake and
different from the real ZIM IDs; that file was created for testing
of the /catalog endpoint which doesn't access ZIM content, so the
the same ZIM file zimfile.zim was added to library.xml three times as
three different books (with unique human-friendly ids). This explains
the diff in test/library_server.cpp.
Now after porting index.js and taskbar.js to vanilla JS, it is time to remove files.
Deleted static/skin/jquery-ui
Updated customIndexPage template in README.md.
Thank you for your service, jQuery :)
This change centers tiles on welcome page to give a more consistent whitespace look on both sides.
For this, the layout in Isotope JS is changed to masonry.
This change introduces filtering by tags.
To filter, the user can click on the tag name and it will filter it.
A label is added (clickable) to show the tag filter, it can be clicked to remove the filter
This removes the onclick handler around the reset-filter link which redirected to '/?lang='
Everything under the handler was already done on window.onload
Previously, if the following steps were executed:
1. Click a book tile/visit an unrelated link from the address bar
2. Press back button
Then forward history was discarded (forward button gets disabled).
This happened because of the window.history.pushState on every window.onload event. This led to the same link being added in history and thus discarding the previous "forward-history"
This change adds a condition to only push the current state if the queries are not same.
One important missing test is that the content of the customized
resource is read from storage every time rather than once. Testing
that requirement would involve creating temporary files which is a
little more work.
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.
Xapian version 1.4.18 contains a bug in snippet generation caused by
incorrect handling of stemming.
The test-point with a search pattern "beatles" produced snippets with no
highlights of the search term. Debugging showed that the search pattern
"beatles" was transformed to a search term "beatl" which then didn't
match the word "beatles" in the text from which a snippet had to be
extracted.
The test case passed on my development machine as well as for most CI
configurations. However the "Packages / build-deb (ubuntu-bionic)"
variant failed because of a slightly different handling of punctuation
at the snippet boundaries:
Test context:
url: /ROOT/search?pattern=beatles&content=zimfile
actual snippet: ...side "Yellow Submarine" ...........
expected snippet: ...-side "Yellow Submarine" ...........
Above mismatch resulted in a looser comparison of the snippet contents
and failed the requirement that the snippet MUST contain highlights
(this is how the said bug in Xapian was discovered).
An attempt to change the search pattern to "field" didn't eliminate the
problem. Despite the search pattern itself being in singular form (i.e.
identical to its stemmed version) the plural form "fields" in the
snippet was still not highlighted.
Using for a search pattern an adjective instead of a noun achieved the
desired outcome.
The "expected" snippets in the test data must be a union of all possible
snippets produced at runtime for a given (document, search terms) pair
on all platforms of interest:
- Overlapping snippets must be properly merged
- Non-overlapping snippets can be joined with a " ... " in between.
This is a preliminary implementation checking only the following
cases:
- no search results
- all search results fitting on a single page
The second test-case fails because of a bug in search renderer (leading
to the pagination footer being pointlessly enabled). Will fix it in the
next commit.
Taskbar injected by a server adds distraction to unit-tests focusing
on the HTML contents of the returned pages. The new test-suite
TaskbarlessServerTest will have taskbar disabled.
Previously, on clicking Magnet, we were redirecting to a different site:
https://download.kiwix.org/zim/other/xyzBookWithDate.zim.magnet
This had the real magnet link as page content
Now we use the real magnet link in the href, thus not redirecting and starting the download right away.
Fix#767
The cache-id of resources now includes dependency information. This commit
illustrates that property with the changed cache-id of skin/index.js which
depends on skin/{download,hash,magnet,bittorent}.png.
The implementation is not fool-proof - cyclic dependency between
resources is not detected and will lead to infinite recursion.
The story of search_results.css
static/skin/search_results.css was extracted from
static/templates/no_search_result.html before the latter was dropped.
static/templates/no_search_result.html in turn seems to be a copied and
edited version of static/templates/search_result.html.
In the context of exploratory work on the internationalization of
kiwix-serve (PR #679) I noticed duplication of inline CSS across those
two templates and intended to eliminated it. That goal was not fully
accomplished (static/templates/search_result.html remained untouched)
because by that time PR #679 grew too big and the efforts were diverted
into splitting it into smaller ones. Thus search_results.css slipped
into one of those small PRs, without making much sense because nothing
really justifies preserving custom CSS in the "Fulltext search unavailable"
error page.
At the same time, it served as the only case where a link to a cacheable
resource is generated in C++ code (rather than found in a template).
This poses certain problems to the handling of cache-ids. A workaround
is to expel the URL into a template so that it is processed by
`kiwix-resources`. This commit merely demonstrates that solution. But
whether it should be preserved (or rather the "Fulltext search
unavailable" page should be deprived of CSS) is questionable.
In template resources (found under static/templates), strings of the
form "PATH/TO/STATIC/RESOURCE?KIWIXCACHEID" are expanded into
"PATH/TO/STATIC/RESOURCE?cacheid=CACHEIDVAL" where CACHEIDVAL is a
8-digit hexadecimal hash digest of the file at
static/PATH/TO/STATIC/RESOURCE.
Introduced a new unit-test which will ensure that static resources of
kiwix-serve have the cache ids applied to them in the links embedded into
the HTML code.
At this point there are no cache ids. The new unit-test will help to
visualize how they come into existence.
In the absence of the "userlang" query parameter in the URL, the value
of the "Accept-Language" header is used. However, it is assumed that
"Accept-Language" specifies a single language (rather than a comma
separated list of languages possibly weighted with quality values).
Example:
Accept-Language: fr
// should work
Accept-Language: fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5
// The requested language will be considered to be
// "fr-CH, fr;q=0.9, en;q=0.8, de;q=0.7, *;q=0.5".
// The i18n code will fail to find resources for such a language
// and will use the default "en" instead.
The "Fulltext search unavailable" error page is now generated using the
static/templates/error.html template. Also added two test cases checking
that error page.
Currently an internal server error can be triggered by accessing an
entry belonging to a redirect loop. The ZIM file (test/data/poor.zim)
containing such a loop was copied from openzim/zim-tools repository
(test/data/zimfiles/poor.zim).
Instead of passing the `bookName` and `bookTitle` parameters to
`Response::build_404()`, `withTaskbarInfo()` is applied to its result
when needed. Note, that in `InternalServer::handle_raw()`
`withTaskbarInfo()` was not utilized since the results of the `/raw`
endpoint are not supposed to be decorated with a taskbar.
Before this change the meaning of test data in the ServerTest.404WithBodyTesting
unit test entirely depended on the number of entries:
- 2 entries: url, expected body
- 4 entries: url, book name, book title and expected body
This was fragile and non scalable (if other combinations of expected
response data are needed).
This commit defines a mini-DSL taking advantage of operator overloading
that allows to define test data in a robust way with the help of the compiler.
Some code in `TestContentIn404HtmlResponse` is obsoleted by this change
however it is not removed in this commit so that the change is easier to
understand. That will be done next.
The `ServerTest.RandomOnNonExistentBook` unit test was replaced with a
more general one testing multiple 404 scenarios where the content of the
body is checked too.
This test was introduced with the purpose of testing the error message
in the 404 page returned by /random for a non-existent book. The actual
expected output currently present in this new unit-test is too much for
that purpose and may become a maintenance burden if more tests of that
kind are added.
Packages/build-deb CI flows failed on ubuntu-bionic and ubuntu-focal
with the following mismatch in the ServerTest.suggestions unit-test:
```
[ RUN ] ServerTest.suggestions
../test/server.cpp:715: Failure
Expected equality of these values:
r->body
Which is: ...
removeEOLWhitespaceMarkers(expectedResponse)
Which is: ...
With diff:
@@ -2,5 +2,5 @@
{
\"value\" : \"Ray (movie)\",
- \"label\" : \"Ray (<b>movie</b>...\",
+ \"label\" : \"Ray (<b>movie</b>)\",
\"kind\" : \"path\"
, \"path\" : \"A/Ray_(movie)\"
Test context:
url: /ROOT/suggest?content=zimfile&term=movie
```
For some reason (probably, a bug), the implementation of
`Xapian::MSet::snippet()` on those platforms decided that a single closing
parenthesis is more than is appropriate for inclusion in the snippet and
replaced it with a (longer) ellipsis.
Taking advantage of the necessity to work around that bug, the
ServerTest.suggestions's functional coverage was enhanced - the
problematic test point was replaced with a new one using a phrase
instead of a single term.
The HumanReadableId can contains special char (`&`/`=`/...)
As it is used as to create a url in the opds template,
we must url encode it.
- We don't need to encode the book id as it is a uuid, it never contains
special char.
- We don't need to encode the book url as it is read from the library and
the url must already be correctly encoded in the library.xml.
(tests modified accordingly)
Veloman Yunkan
Adam Lamar
Emmanuel Engelhart
Matthieu Gautier
Nikhil Tanwar
