Do not migrate bookmarks to an older book.

At least, it must be explicitly asked by the user.

include/library.h

@@ -55,6 +55,18 @@ enum supportedListMode {
   NOVALID = 1 << 5
 };
 
+enum MigrationMode {
+  /** When migrating bookmarks, do not allow to migrate to an older book than the currently pointed one.
+   *
+   * This is enforce only if the bookmark point to a valid books. If bookmark point to a invalid book,
+   * it will be migrated to an existing one, even if older.
+   */
+  UPGRADE_ONLY = 0,
+
+   /** Allow to migrate the bookmark to an older book. */
+  ALLOW_DOWNGRADE = 1,
+};
+
 class Filter {
   public: // types
     using Tags = std::vector<std::string>;
@@ -272,18 +284,21 @@ class Library: public std::enable_shared_from_this<Library>
    *
    * @return A tuple<int, int>: <The number of bookmarks updated>, <Number of invalid bookmarks before migration was performed>.
    */
-  std::tuple<int, int> migrateBookmarks();
+  std::tuple<int, int> migrateBookmarks(MigrationMode migrationMode = ALLOW_DOWNGRADE);
 
   /**
    * Migrate all bookmarks associated to a specific book.
    *
    * All bookmarks associated to `sourceBookId` book will be migrated to a better book.
    * "Better book", will be determined using heuristics based on book name, flavour and date.
-
+   * Be carrefull, when using with `migrationModde == ALLOW_DOWGRADE`, the bookmark will be migrated to a different book
-   * @param source the source bookId of the bookmarks to migrate.
+   * even if the bookmark points to an existing book and the other book is older than the current one.
+   *
+   * @param sourceBookId the source bookId of the bookmarks to migrate.
+   * @param migrationMode how we will find the best book.
    * @return The number of bookmarks updated.
    */
-  int migrateBookmarks(const std::string& sourceBookId);
+  int migrateBookmarks(const std::string& sourceBookId, MigrationMode migrationMode = UPGRADE_ONLY);
 
   /**
    * Migrate bookmarks
@@ -441,7 +456,8 @@ private: // functions
   AttributeCounts getBookAttributeCounts(BookStrPropMemFn p) const;
   std::vector<std::string> getBookPropValueSet(BookStrPropMemFn p) const;
   BookIdCollection filterViaBookDB(const Filter& filter) const;
-  std::string getBestTargetBookId(const Bookmark& bookmark) const;
+  void cleanupBookCollection(BookIdCollection& books, const std::string& sourceBookId, MigrationMode migrationMode) const;
+  std::string getBestTargetBookId(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

@@ -157,7 +157,7 @@ bool Library::removeBookmark(const std::string& zimId, const std::string& url)
   return false;
 }
 
-std::tuple<int, int> Library::migrateBookmarks() {
+std::tuple<int, int> Library::migrateBookmarks(MigrationMode migrationMode) {
   std::set<std::string> sourceBooks;
   int invalidBookmarks = 0;
   {
@@ -171,12 +171,27 @@ std::tuple<int, int> Library::migrateBookmarks() {
   }
   int changed = 0;
   for(auto& sourceBook:sourceBooks) {
-    changed += migrateBookmarks(sourceBook);
+    changed += migrateBookmarks(sourceBook, migrationMode);
   }
   return std::make_tuple(changed, invalidBookmarks);
 }
 
-std::string Library::getBestTargetBookId(const Bookmark& bookmark) const {
+void Library::cleanupBookCollection(BookIdCollection& books, const std::string& sourceBookId, MigrationMode migrationMode) const {
+  sort(books, DATE, false);
+
+  // Remove source book
+  auto foundIT = std::find(books.begin(), books.end(), sourceBookId);
+  if (foundIT != books.end()) {
+    if (migrationMode == ALLOW_DOWNGRADE) {
+      books.erase(foundIT);
+    } else {
+      // books is sorted by date, remove the source and all older books
+      books.erase(foundIT, books.end());
+    }
+  }
+}
+
+std::string Library::getBestTargetBookId(const Bookmark& bookmark, MigrationMode migrationMode) const {
   // Search for a existing book with the same name
   auto book_filter = Filter();
   if (!bookmark.getBookName().empty()) {
@@ -191,11 +206,7 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark) const {
     book_filter.query("title:\""+bookmark.getBookTitle() + "\"");
   }
   auto targetBooks = filter(book_filter);
-  // Remove source book
+  cleanupBookCollection(targetBooks, bookmark.getBookId(), migrationMode);
-  auto foundIT = std::find(targetBooks.begin(), targetBooks.end(), bookmark.getBookId());
-  if (foundIT != targetBooks.end()) {
-    targetBooks.erase(foundIT);
-  }
     
   if (targetBooks.empty()) {
     // No existing book found for the target, or the same than source.
@@ -204,15 +215,15 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark) const {
   if (targetBooks.size() != 1) {
     // 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;
     }
-    sort(targetBooks, DATE, false);
   }
   return targetBooks[0];
 }
 
-int Library::migrateBookmarks(const std::string& sourceBookId) {
+int Library::migrateBookmarks(const std::string& sourceBookId, MigrationMode migrationMode) {
   std::lock_guard<std::recursive_mutex> lock(m_mutex);
 
   Bookmark firstBookmarkToChange;
@@ -227,7 +238,7 @@ int Library::migrateBookmarks(const std::string& sourceBookId) {
     return 0;
   }
 
-  std::string betterBook = getBestTargetBookId(firstBookmarkToChange);
+  std::string betterBook = getBestTargetBookId(firstBookmarkToChange, migrationMode);
 
   if (betterBook.empty()) {
     return 0;
@@ -244,7 +255,7 @@ int Library::migrateBookmarks(const std::string& sourceBookId, const std::string
   for (auto& bookmark:m_bookmarks) {
     if (bookmark.getBookId() == sourceBookId) {
       bookmark.setBookId(targetBookId);
-      changed += 1;
+      changed +=1;
     }
   }
   return changed;

test/library.cpp

@@ -617,6 +617,23 @@ TEST_F(LibraryTest, MigrateBookmark)
     EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_updated1yearlater_flavour");
 }
 
+TEST_F(LibraryTest, MigrateBookmarkOlder)
+{
+    auto bookId1 = "0c45160e-f917-760a-9159-dfe3c53cdcdd_updated1yearlater";
+
+    auto book1 = lib->getBookById(bookId1);
+
+    lib->addBookmark(createBookmark(book1));
+
+    auto onlyValidBookmarks = lib->getBookmarks();
+
+    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, sanityCheck)
 {