Merge pull request #1224 from kiwix/previewable_books_for_empty_root

Empty urlRootLocation doesn't disable book preview links

src/html_dumper.cpp

@@ -130,7 +130,7 @@ std::string HTMLDumper::dumpPlainHTML(kiwix::Filter filter) const
              RESOURCE::templates::no_js_library_page_html,
              kainjow::mustache::object{
                {"root", rootLocation},
-               {"contentServerUrl", onlyAsNonEmptyMustacheValue(contentServerUrl)},
+               {"contentAccessUrl", onlyAsNonEmptyMustacheValue(contentAccessUrl)},
                {"books", booksData },
                {"searchQuery", searchQuery},
                {"languages", languages},

src/library_dumper.h

@@ -51,11 +51,11 @@ class LibraryDumper
   void setRootLocation(const std::string& rootLocation) { this->rootLocation = rootLocation; }
 
   /**
-   * Set the URL of the content server.
+   * Set the URL for accessing book content
    *
-   * @param url the URL of the content server to use.
+   * @param url the URL of the /content endpoint of the content server
    */
-  void setContentServerUrl(const std::string& url) { this->contentServerUrl = url; }
+  void setContentAccessUrl(const std::string& url) { this->contentAccessUrl = url; }
 
   /**
    * Set some informations about the search results.
@@ -88,7 +88,7 @@ class LibraryDumper
   const kiwix::NameMapper* const nameMapper;
   std::string libraryId;
   std::string rootLocation;
-  std::string contentServerUrl;
+  std::string contentAccessUrl;
   std::string m_userLang;
   int m_totalResults;
   int m_startIndex;

src/opds_dumper.cpp

@@ -64,13 +64,13 @@ IllustrationInfo getBookIllustrationInfo(const Book& book)
 
 std::string fullEntryXML(const Book& book,
                          const std::string& rootLocation,
-                         const std::string& contentServerUrl,
+                         const std::string& contentAccessUrl,
                          const std::string& contentId)
 {
     const auto bookDate = book.getDate() + "T00:00:00Z";
     const kainjow::mustache::object data{
       {"root",  rootLocation},
-      {"contentServerUrl",  onlyAsNonEmptyMustacheValue(contentServerUrl)},
+      {"contentAccessUrl",  onlyAsNonEmptyMustacheValue(contentAccessUrl)},
       {"id", book.getId()},
       {"name", book.getName()},
       {"title", book.getTitle()},
@@ -111,7 +111,7 @@ BooksData getBooksData(const Library* library,
                        const NameMapper* nameMapper,
                        const std::vector<std::string>& bookIds,
                        const std::string& rootLocation,
-                       const std::string& contentServerUrl,
+                       const std::string& contentAccessUrl,
                        bool partial)
 {
   BooksData booksData;
@@ -121,7 +121,7 @@ BooksData getBooksData(const Library* library,
       const std::string contentId = nameMapper->getNameForId(bookId);
       const auto entryXML = partial
                           ? partialEntryXML(book, rootLocation)
-                          : fullEntryXML(book, rootLocation, contentServerUrl, contentId);
+                          : fullEntryXML(book, rootLocation, contentAccessUrl, contentId);
       booksData.push_back(kainjow::mustache::object{ {"entry", entryXML} });
     } catch ( const std::out_of_range& ) {
       // the book was removed from the library since its id was obtained
@@ -136,7 +136,7 @@ BooksData getBooksData(const Library* library,
 
 string OPDSDumper::dumpOPDSFeed(const std::vector<std::string>& bookIds, const std::string& query) const
 {
-  const auto booksData = getBooksData(library, nameMapper, bookIds, rootLocation, contentServerUrl, false);
+  const auto booksData = getBooksData(library, nameMapper, bookIds, rootLocation, contentAccessUrl, false);
   const kainjow::mustache::object template_data{
      {"date", gen_date_str()},
      {"root", rootLocation},
@@ -154,7 +154,7 @@ string OPDSDumper::dumpOPDSFeed(const std::vector<std::string>& bookIds, const s
 string OPDSDumper::dumpOPDSFeedV2(const std::vector<std::string>& bookIds, const std::string& query, bool partial) const
 {
   const auto endpointRoot = rootLocation + "/catalog/v2";
-  const auto booksData = getBooksData(library, nameMapper, bookIds, rootLocation, contentServerUrl, partial);
+  const auto booksData = getBooksData(library, nameMapper, bookIds, rootLocation, contentAccessUrl, partial);
 
   const char* const endpoint = partial ? "/partial_entries" : "/entries";
   const std::string url = endpoint + (query.empty() ? "" : "?" + query);
@@ -179,7 +179,7 @@ std::string OPDSDumper::dumpOPDSCompleteEntry(const std::string& bookId) const
   const std::string contentId = nameMapper->getNameForId(bookId);
   return XML_HEADER
          + "\n"
-         + fullEntryXML(book, rootLocation, contentServerUrl, contentId);
+         + fullEntryXML(book, rootLocation, contentAccessUrl, contentId);
 }
 
 std::string OPDSDumper::categoriesOPDSFeed() const

src/server/internalServer.cpp

@@ -850,6 +850,15 @@ std::string InternalServer::getNoJSDownloadPageHTML(const std::string& bookId, c
   );
 }
 
+void InternalServer::setContentAccessUrl(LibraryDumper& libDumper) const
+{
+  if ( !m_contentServerUrl.empty() ) {
+    libDumper.setContentAccessUrl(m_contentServerUrl + "/content");
+  } else if ( !m_catalogOnlyMode ) {
+    libDumper.setContentAccessUrl(m_root + "/content");
+  }
+}
+
 std::unique_ptr<Response> InternalServer::handle_no_js(const RequestContext& request)
 {
   const auto url = request.get_url();
@@ -857,11 +866,7 @@ std::unique_ptr<Response> InternalServer::handle_no_js(const RequestContext& req
   HTMLDumper htmlDumper(mp_library.get(), mp_nameMapper.get());
   htmlDumper.setRootLocation(m_root);
   htmlDumper.setLibraryId(getLibraryId());
-  if ( !m_contentServerUrl.empty() ) {
+  setContentAccessUrl(htmlDumper);
-    htmlDumper.setContentServerUrl(m_contentServerUrl);
-  } else if ( !m_catalogOnlyMode ) {
-    htmlDumper.setContentServerUrl(m_root);
-  }
   auto userLang = request.get_user_language();
   htmlDumper.setUserLanguage(userLang);
   std::string content;

src/server/internalServer.h

@@ -90,6 +90,7 @@ class SearchInfo {
 
 typedef kainjow::mustache::data MustacheData;
 class OPDSDumper;
+class LibraryDumper;
 
 class InternalServer {
   public:
@@ -163,6 +164,7 @@ class InternalServer {
 
     std::string getNoJSDownloadPageHTML(const std::string& bookId, const std::string& userLang) const;
     OPDSDumper getOPDSDumper() const;
+    void setContentAccessUrl(LibraryDumper& libDumper) const;
 
   private: // types
     class LockableSuggestionSearcher;

src/server/internalServer_catalog.cpp

@@ -56,11 +56,7 @@ OPDSDumper InternalServer::getOPDSDumper() const
   kiwix::OPDSDumper opdsDumper(mp_library.get(), mp_nameMapper.get());
   opdsDumper.setRootLocation(m_root);
   opdsDumper.setLibraryId(getLibraryId());
-  if ( !m_contentServerUrl.empty() ) {
+  setContentAccessUrl(opdsDumper);
-    opdsDumper.setContentServerUrl(m_contentServerUrl);
-  } else if ( !m_catalogOnlyMode ) {
-    opdsDumper.setContentServerUrl(m_root);
-  }
   return opdsDumper;
 }
 

static/templates/catalog_v2_entry.xml

@@ -13,8 +13,8 @@
     {{#icons}}<link rel="http://opds-spec.org/image/thumbnail"
           href="{{root}}/catalog/v2/illustration/{{{id}}}/?size={{icon_size}}"
           type="{{icon_mimetype}};width={{icon_size}};height={{icon_size}};scale=1"/>
-    {{/icons}}{{#contentServerUrl}}<link type="text/html" href="{{contentServerUrl}}/content/{{{content_id}}}" />
+    {{/icons}}{{#contentAccessUrl}}<link type="text/html" href="{{contentAccessUrl}}/{{{content_id}}}" />
-    {{/contentServerUrl}}
+    {{/contentAccessUrl}}
     <author>
       <name>{{author_name}}</name>
     </author>

static/templates/no_js_library_page.html

@@ -3,7 +3,6 @@
   <head>
     <meta charset="UTF-8" />
     <meta name="viewport" content="width=device-width,initial-scale=1" />
-    <link type="root" href="{{root}}">
     <title>{{translations.welcome-to-kiwix-server}}</title>
     <link
       type="text/css"
@@ -108,7 +107,7 @@
         <h3 class="kiwixHomeBody__results">{{translations.count-of-matching-books}}</h3>
         {{#books}}
         <div class="book__wrapper">
-            {{#contentServerUrl}}<a class="book__link" href="{{contentServerUrl}}/content/{{id}}" title="{{translations.preview-book}}" aria-label="{{translations.preview-book}}">{{/contentServerUrl}}
+            {{#contentAccessUrl}}<a class="book__link" href="{{contentAccessUrl}}/{{id}}" title="{{translations.preview-book}}" aria-label="{{translations.preview-book}}">{{/contentAccessUrl}}
             <div class="book__link__wrapper">
             <div class="book__icon" {{faviconAttr}}></div>
             <div class="book__header">
@@ -116,7 +115,7 @@
             </div>
             <div class="book__description" title="{{description}}">{{description}}</div>
             </div>
-            {{#contentServerUrl}}</a>{{/contentServerUrl}}
+            {{#contentAccessUrl}}</a>{{/contentAccessUrl}}
             <div class="book__meta">
               <div class="book__languageTag" title="{{langTag.langFullString}}" aria-label="{{langTag.langFullString}}">{{langTag.langShortString}}</div>
               <div class="book__tags"><div class="book__tags--wrapper">

test/library_server.cpp

@@ -18,11 +18,15 @@ protected:
   const int PORT = 8002;
 
 protected:
+  void resetServer(ZimFileServer::Cfg cfg) {
+    zfs1_.reset();
+    zfs1_.reset(new ZimFileServer(PORT, cfg, "./test/library.xml"));
+  }
+
   void resetServer(ZimFileServer::Options options, std::string contentServerUrl="") {
     ZimFileServer::Cfg cfg(options);
     cfg.contentServerUrl = contentServerUrl;
-    zfs1_.reset();
+    resetServer(cfg);
-    zfs1_.reset(new ZimFileServer(PORT, cfg, "./test/library.xml"));
   }
 
   void SetUp() override {
@@ -733,7 +737,8 @@ TEST_F(LibraryServerTest, catalog_v2_entries_catalog_only_mode)
   resetServer(ZimFileServer::CATALOG_ONLY_MODE, contentServerUrl);
   const auto r = zfs1_->GET("/ROOT%23%3F/catalog/v2/entries");
   EXPECT_EQ(r->status, 200);
-  EXPECT_EQ(maskVariableOPDSFeedData(r->body),
+
+  const std::string expectedOPDS =
     CATALOG_V2_ENTRIES_PREAMBLE("")
     "  <title>All Entries</title>\n"
     "  <updated>YYYY-MM-DDThh:mm:ssZ</updated>\n"
@@ -742,8 +747,27 @@ TEST_F(LibraryServerTest, catalog_v2_entries_catalog_only_mode)
     + fixContentLinks(INACCESSIBLEZIMFILE_CATALOG_ENTRY)
     + fixContentLinks(RAY_CHARLES_CATALOG_ENTRY)
     + fixContentLinks(UNCATEGORIZED_RAY_CHARLES_CATALOG_ENTRY) +
-    "</feed>\n"
+    "</feed>\n";
-  );
+
+  EXPECT_EQ(maskVariableOPDSFeedData(r->body), expectedOPDS);
+
+  { // test with empty rootLocation
+    const auto fixRoot = [=](std::string s) -> std::string {
+      s = replace(s, "/ROOT%23%3F/", "/");
+      s = replace(s, "/ROOT%23%3F/", "/");
+      return s;
+    };
+
+    ZimFileServer::Cfg serverCfg;
+    serverCfg.root = "";
+    serverCfg.options = ZimFileServer::CATALOG_ONLY_MODE;
+    serverCfg.contentServerUrl = contentServerUrl;
+    resetServer(serverCfg);
+
+    const auto r = zfs1_->GET("/catalog/v2/entries");
+    EXPECT_EQ(r->status, 200);
+    EXPECT_EQ(maskVariableOPDSFeedData(r->body), fixRoot(expectedOPDS));
+  }
 }
 
 TEST_F(LibraryServerTest, catalog_v2_entries_filtered_by_range)
@@ -1253,7 +1277,6 @@ TEST_F(LibraryServerTest, no_name_mapper_catalog_v2_individual_entry_access)
   "  <head>\n" \
   "    <meta charset=\"UTF-8\" />\n" \
   "    <meta name=\"viewport\" content=\"width=device-width,initial-scale=1\" />\n" \
-  "    <link type=\"root\" href=\"/ROOT%23%3F\">\n" \
   "    <title>Welcome to Kiwix Server</title>\n" \
   "    <link\n" \
   "      type=\"text/css\"\n" \
@@ -1555,11 +1578,13 @@ TEST_F(LibraryServerTest, noJS_catalogOnlyMode) {
     s = replace(s, "/ROOT%23%3F/content", contentServerUrl + "/content");
     return s;
   };
+
   resetServer(ZimFileServer::CATALOG_ONLY_MODE, contentServerUrl);
 
   auto r = zfs1_->GET("/ROOT%23%3F/nojs");
   EXPECT_EQ(r->status, 200);
-  EXPECT_EQ(r->body,
+
+  const std::string expectedHTML =
             HTML_PREAMBLE
             FILTERS_HTML("")
             HOME_BODY_TEXT("4")
@@ -1567,7 +1592,27 @@ TEST_F(LibraryServerTest, noJS_catalogOnlyMode) {
             + fixContentLinks(INACCESSIBLEZIMFILE_BOOK_HTML)
             + fixContentLinks(RAY_CHARLES_BOOK_HTML)
             + fixContentLinks(RAY_CHARLES_UNCTZ_BOOK_HTML)
-            + FINAL_HTML_TEXT);
+            + FINAL_HTML_TEXT;
+
+  EXPECT_EQ(r->body, expectedHTML);
+
+  { // test with empty rootLocation
+    const auto fixRoot = [=](std::string s) -> std::string {
+      s = replace(s, "/ROOT%23%3F/", "/");
+      return s;
+    };
+
+    ZimFileServer::Cfg serverCfg;
+    serverCfg.root = "";
+    serverCfg.options = ZimFileServer::CATALOG_ONLY_MODE;
+    serverCfg.contentServerUrl = contentServerUrl;
+
+    resetServer(serverCfg);
+
+    auto r = zfs1_->GET("/nojs");
+    EXPECT_EQ(r->status, 200);
+    EXPECT_EQ(r->body, fixRoot(expectedHTML));
+  }
 }
 
 #undef EXPECT_SEARCH_RESULTS