Fix definition of `UPGRADE_ONLY` and `ALLOW_DOWNGRADE`.

`MigrationMode` was kind of defined in the context of an internal mode
used by `migrateBookmark(...)`.
But now, with `getBestTargetBookId`, it is broken.

This commit fix that and the associated implementation.
Now `UPGRADE_ONLY` will make `getBestTargetBookId` return only newer books.
and `ALLOW_DOWNGRADE` will return older books only if current book is
invalid.

include/library.h

@@ -56,14 +56,18 @@ enum supportedListMode {
 };
 
 enum MigrationMode {
-  /** When migrating bookmarks, do not allow to migrate to an older book than the currently pointed one.
+  /** When migrating bookmarks, do not allow to migrate to an older book than the currently pointed one
+   * (or date stored in the bookmark if book is invalid)
    *
-   * This is enforce only if the bookmark point to a valid books. If bookmark point to a invalid book,
+   * If no newer books are found, no upgrade is made.
-   * it will be migrated to an existing one, even if older.
    */
   UPGRADE_ONLY = 0,
 
-   /** Allow to migrate the bookmark to an older book. */
+  /** Try hard to do a migration. This mostly does:
+   * - Try to find a newer book.
+   * - If book is invalid: find a best book, potentially older.
+   * Older book will never be returned if current book is a valid one.
+   */
   ALLOW_DOWNGRADE = 1,
 };
 
@@ -466,7 +470,7 @@ private: // functions
   AttributeCounts getBookAttributeCounts(BookStrPropMemFn p) const;
   std::vector<std::string> getBookPropValueSet(BookStrPropMemFn p) const;
   BookIdCollection filterViaBookDB(const Filter& filter) const;
-  void cleanupBookCollection(BookIdCollection& books, const std::string& sourceBookId, MigrationMode migrationMode) const;
+  std::string getBestFromBookCollection(BookIdCollection books, const Bookmark& bookmark, MigrationMode migrationMode) const;
   unsigned int getBookCount_not_protected(const bool localBooks, const bool remoteBooks) const;
   void updateBookDB(const Book& book);
   void dropCache(const std::string& bookId);

src/library.cpp

@@ -176,19 +176,53 @@ std::tuple<int, int> Library::migrateBookmarks(MigrationMode migrationMode) {
   return std::make_tuple(changed, invalidBookmarks);
 }
 
-void Library::cleanupBookCollection(BookIdCollection& books, const std::string& sourceBookId, MigrationMode migrationMode) const {
+std::string Library::getBestFromBookCollection(BookIdCollection books, const Bookmark& bookmark, MigrationMode migrationMode) const {
-  sort(books, DATE, false);
+  // This function try to get the best book for a bookmark from a book collection.
+  // It assumes that all books in the collection are "acceptable".
+  // (this definiton is not clear but for now it is book's name is equal to bookmark's bookName)
+  //
+  // The algorithm first sort the colletion by "flavour equality" and date.
+  // "flavour equality" is if book's flavour is same that bookmark's flavour (let's say "flavourA" here)
+  // So we have the sorted collection:
+  // - flavourA, date 5
+  // - flavourA, date 4
+  // - flavourB, date 6
+  // - flavourC, date 5
+  // - flavourB, date 3
+  //
+  // Then, depending of migrationMode:
+  // - If ALLOW_DOWNGRADE => take the first one
+  // - If UPGRADE_ONLY => loop on books until we find a book newer than bookmark.
+  //   So if bookmark date is 5 => flavourB, date 6
+  //      if bookmark date is 4 => flavourA, date 5
+  //      if bookmark date is 7 => No book
+
+  if (books.empty()) {
+    return "";
+  }
+
+  sort(books, DATE, false);
+  stable_sort(books.begin(), books.end(), [&](const std::string& bookId1, const std::string& bookId2) {
+    const auto& book1 = getBookById(bookId1);
+    const auto& book2 = getBookById(bookId2);
+    bool same_flavour1 = book1.getFlavour() == bookmark.getBookFlavour();
+    bool same_flavour2 = book2.getFlavour() == bookmark.getBookFlavour();
+    // return True if bookId1 is before bookId2, ie if same_flavour1 and not same_flavour2
+    return same_flavour1 > same_flavour2;
+  });
 
-  // Remove source book
-  auto foundIT = std::find(books.begin(), books.end(), sourceBookId);
-  if (foundIT != books.end()) {
   if (migrationMode == ALLOW_DOWNGRADE) {
-      books.erase(foundIT);
+    return books[0];
   } else {
-      // books is sorted by date, remove the source and all older books
+    for (const auto& bookId: books) {
-      books.erase(foundIT, books.end());
+      const auto& book = getBookById(bookId);
+      if (book.getDate() >= bookmark.getDate()) {
+        return bookId;
       }
     }
+  }
+
+  return "";
 }
 
 std::string remove_quote(std::string input) {
@@ -212,21 +246,14 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark, MigrationMode
     book_filter.query("title:\"" + remove_quote(bookmark.getBookTitle()) + "\"");
   }
   auto targetBooks = filter(book_filter);
-  cleanupBookCollection(targetBooks, bookmark.getBookId(), migrationMode);
+  auto bestBook = getBestFromBookCollection(targetBooks, bookmark, migrationMode);
-    
+  if (bestBook.empty()) {
-  if (targetBooks.empty()) {
+    try {
-    // No existing book found for the target, or the same than source.
+      getBookById(bookmark.getBookId());
-    return "";
+      return bookmark.getBookId();
+    } catch (std::out_of_range&) {}
   }
-  if (targetBooks.size() != 1) {
+  return bestBook;
-    // We have several, reduce to same flavour
-    auto flavouredTargetBooks = filter(book_filter.flavour(bookmark.getBookFlavour()));
-    cleanupBookCollection(flavouredTargetBooks, bookmark.getBookId(), migrationMode);
-    if (!flavouredTargetBooks.empty()) {
-      targetBooks = flavouredTargetBooks;
-    }
-  }
-  return targetBooks[0];
 }
 
 int Library::migrateBookmarks(const std::string& sourceBookId, MigrationMode migrationMode) {
@@ -620,7 +647,6 @@ Xapian::Query flavourQuery(const std::string& name)
   return Xapian::Query("XF" + normalizeText(name));
 }
 
-
 Xapian::Query multipleParamQuery(const std::string& commaSeparatedList, const std::string& prefix)
 {
   Xapian::Query q;

test/library.cpp

@@ -32,7 +32,7 @@ const char * sampleOpdsStream = R"(
     <id>urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd</id>
     <icon>/meta?name=favicon&amp;content=wikipedia_fr_tunisie_novid_2018-10</icon>
     <updated>2018-10-08T00:00::00:Z</updated>
-    <dc:issued>8 Oct 2018</dc:issued>
+    <dc:issued>2018-10-08T00:00::00:Z</dc:issued>
     <language>fra</language>
     <summary>Le meilleur de Wikipédia sur la Tunisie</summary>
     <tags>wikipedia;novid;_ftindex</tags>
@@ -54,7 +54,7 @@ const char * sampleOpdsStream = R"(
     <flavour>novid</flavour>
     <id>urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater</id>
     <updated>2019-10-08T00:00::00:Z</updated>
-    <dc:issued>8 Oct 2019</dc:issued>
+    <dc:issued>2019-10-08T00:00::00:Z</dc:issued>
     <language>fra</language>
     <summary>Le meilleur de Wikipédia sur la Tunisie. Updated in 2019</summary>
     <author>
@@ -68,7 +68,7 @@ const char * sampleOpdsStream = R"(
     <flavour>other_flavour</flavour>
     <id>urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_flavour</id>
     <updated>2018-10-08T00:00::00:Z</updated>
-    <dc:issued>8 Oct 2018</dc:issued>
+    <dc:issued>2018-10-08T00:00::00:Z</dc:issued>
     <language>fra</language>
     <summary>Le meilleur de Wikipédia sur la Tunisie. With another flavour</summary>
     <author>
@@ -82,7 +82,7 @@ const char * sampleOpdsStream = R"(
     <flavour>other_flavour</flavour>
     <id>urn:uuid:0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater_flavour</id>
     <updated>2019-10-08T00:00::00:Z</updated>
-    <dc:issued>8 Oct 2019</dc:issued>
+    <dc:issued>2019-10-08T00:00::00:Z</dc:issued>
     <language>fra</language>
     <summary>Le meilleur de Wikipédia sur la Tunisie. Updated in 2019, and other flavour</summary>
     <author>
@@ -329,7 +329,7 @@ TEST(LibraryOpdsImportTest, allInOne)
   EXPECT_EQ(book1.getFlavour(), "novid");
   EXPECT_EQ(book1.getLanguages(), Langs{ "fra" });
   EXPECT_EQ(book1.getCommaSeparatedLanguages(), "fra");
-  EXPECT_EQ(book1.getDate(), "8 Oct 2018");
+  EXPECT_EQ(book1.getDate(), "2018-10-08");
   EXPECT_EQ(book1.getDescription(), "Le meilleur de Wikipédia sur la Tunisie");
   EXPECT_EQ(book1.getCreator(), "Wikipedia");
   EXPECT_EQ(book1.getPublisher(), "Wikipedia Publishing House");
@@ -634,23 +634,69 @@ TEST_F(LibraryTest, MigrateBookmark)
     EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_updated1yearlater_flavour");
 }
 
-TEST_F(LibraryTest, MigrateBookmarkOlder)
+TEST_F(LibraryTest, GetBestTargetBookIdOlder)
 {
-    auto bookId1 = "0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater";
+    auto bookId = std::string("0c45160e-f917-760a-9159-dfe3c53cdcdd");
 
-    auto book1 = lib->getBookById(bookId1);
+    auto book = lib->getBookById(bookId);
 
-    lib->addBookmark(createBookmark(book1));
+    auto validBookmark = createBookmark(book);
+    lib->addBookmark(validBookmark);
 
-    auto onlyValidBookmarks = lib->getBookmarks();
+    ASSERT_EQ(lib->getBestTargetBookId(validBookmark, kiwix::UPGRADE_ONLY), bookId+"_updated1yearlater");
-
+    ASSERT_EQ(lib->getBestTargetBookId(validBookmark, kiwix::ALLOW_DOWNGRADE), bookId+"_updated1yearlater");
-    ASSERT_EQ(onlyValidBookmarks.size(), 1);
-    EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1);
-
-    ASSERT_EQ(lib->migrateBookmarks(bookId1), 0);
-    ASSERT_EQ(lib->migrateBookmarks(bookId1, kiwix::ALLOW_DOWNGRADE), 1);
 }
 
+TEST_F(LibraryTest, GetBestTargetBookIdNewer)
+{
+    auto bookId = std::string("0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater");
+
+    auto book = lib->getBookById(bookId);
+    EXPECT_EQ(book.getDate(), "2019-10-08");
+
+    auto validBookmark = createBookmark(book);
+    // Make the bookmark more recent than any books in the library.
+    // (But still pointing to existing book)
+    validBookmark.setDate("2020-10-08");
+    lib->addBookmark(validBookmark);
+
+    //  The best book for the bookmark is bookId...
+    ASSERT_EQ(lib->getBestTargetBookId(validBookmark, kiwix::UPGRADE_ONLY), bookId);
+    // but there is not migration to do as the bookmark already point to it.
+    ASSERT_EQ(lib->migrateBookmarks(bookId, kiwix::UPGRADE_ONLY), 0);
+
+    ASSERT_EQ(lib->getBestTargetBookId(validBookmark, kiwix::ALLOW_DOWNGRADE), bookId);
+}
+
+TEST_F(LibraryTest, GetBestTargetBookIdInvalidOlder)
+{
+    auto bookId = std::string("0c45160e-f917-760a-9159-dfe3c53cdcdd");
+
+    auto book = lib->getBookById(bookId);
+
+    auto invalidBookmark = createBookmark(book);
+    invalidBookmark.setBookId("invalid-book-id");
+    lib->addBookmark(invalidBookmark);
+
+    ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::UPGRADE_ONLY), bookId+"_updated1yearlater");
+    ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::ALLOW_DOWNGRADE), bookId+"_updated1yearlater");
+}
+
+TEST_F(LibraryTest, GetBestTargetBookIdInvalidNewer)
+{
+    auto bookId = std::string("0c45160e-f917-760a-9159-dfe3c53cdcdd");
+
+    auto book = lib->getBookById(bookId);
+    EXPECT_EQ(book.getDate(), "2018-10-08");
+
+    auto invalidBookmark = createBookmark(book);
+    invalidBookmark.setBookId("invalid-book-id");
+    invalidBookmark.setDate("2020-10-08");
+    lib->addBookmark(invalidBookmark);
+
+    ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::UPGRADE_ONLY), "");
+    ASSERT_EQ(lib->getBestTargetBookId(invalidBookmark, kiwix::ALLOW_DOWNGRADE), bookId+"_updated1yearlater");
+}
 
 TEST_F(LibraryTest, sanityCheck)
 {