fixup! Introduce migrateBookmarks to move (invalid) bookmarks to ne…

…w books.
kiwix · Jan 29, 2024 · 434becd · 434becd
1 parent 0e96b3c
commit 434becd
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 15 deletions.
diff --git a/include/library.h b/include/library.h
@@ -55,6 +55,11 @@ enum supportedListMode {
   NOVALID = 1 << 5
 };
 
+enum MigrationMode {
+  UPGRADE_ONLY = 0,
+  ALLOW_DOWNGRADE = 1,
+ };
+
 class Filter {
   public: // types
     using Tags = std::vector<std::string>;
@@ -283,7 +288,7 @@ class Library: public std::enable_shared_from_this<Library>
    * @param allowDowngrade `true` if "heuristics" can found a book older than the currently set.
    * @return The number of bookmarks updated.
    */
-  int migrateBookmarks(const std::string& source, bool allowDowngrade = false);
+  int migrateBookmarks(const std::string& source, MigrationMode allowDowngrade = UPGRADE_ONLY);
 
   /**
    * Migrate bookmarks
@@ -440,7 +445,7 @@ class Library: public std::enable_shared_from_this<Library>
   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, bool allowDowngrade) 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);

diff --git a/src/library.cpp b/src/library.cpp
@@ -169,12 +169,12 @@ int Library::migrateBookmarks() {
   }
   int changed = 0;
   for(auto& sourceBook:sourceBooks) {
-    changed += migrateBookmarks(sourceBook, true);
+    changed += migrateBookmarks(sourceBook, ALLOW_DOWNGRADE);
   }
   return changed;
 }
 
-std::string Library::getBestTargetBookId(const Bookmark& bookmark, bool allowDowngrade) const {
+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()) {
@@ -194,7 +194,7 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark, bool allowDow
   // Remove source book
   auto foundIT = std::find(targetBooks.begin(), targetBooks.end(), bookmark.getBookId());
   if (foundIT != targetBooks.end()) {
-    if (allowDowngrade) {
+    if (migrationMode == ALLOW_DOWNGRADE) {
       targetBooks.erase(foundIT);
     } else {
       // targetBooks is sorted by date, remove the source and all oldest books
@@ -213,7 +213,7 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark, bool allowDow
     // Remove source book
     auto foundIT = std::find(flavouredTargetBooks.begin(), flavouredTargetBooks.end(), bookmark.getBookId());
     if (foundIT != flavouredTargetBooks.end()) {
-      if (allowDowngrade) {
+      if (migrationMode == ALLOW_DOWNGRADE) {
         flavouredTargetBooks.erase(foundIT);
       } else {
         // targetBooks is sorted by date, remove the source and all oldest books
@@ -227,7 +227,7 @@ std::string Library::getBestTargetBookId(const Bookmark& bookmark, bool allowDow
   return targetBooks[0];
 }
 
-int Library::migrateBookmarks(const std::string& source, bool allowDowngrade) {
+int Library::migrateBookmarks(const std::string& source, MigrationMode migrationMode) {
   std::lock_guard<std::recursive_mutex> lock(m_mutex);
 
   Bookmark firstBookmarkToChange;
@@ -242,7 +242,7 @@ int Library::migrateBookmarks(const std::string& source, bool allowDowngrade) {
     return 0;
   }
 
-  std::string betterBook = getBestTargetBookId(firstBookmarkToChange, allowDowngrade);
+  std::string betterBook = getBestTargetBookId(firstBookmarkToChange, migrationMode);
 
   if (betterBook.empty()) {
     return 0;

diff --git a/test/library.cpp b/test/library.cpp
@@ -569,12 +569,9 @@ TEST_F(LibraryTest, MigrateBookmark)
     EXPECT_EQ(allBookmarks[4].getBookId(), bookId2);
     EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_date_flavour");
 
-    // Suprisingly, `char *` make c++ prefers `migrateBookmarks(..., bool)` instead of `std::string`.
-    // [??] Is is a problem ? It should appear only if user use `char *` (so use C or literal string).
-    // A solution would be to rename one method, but which one ?
-    ASSERT_EQ(lib->migrateBookmarks(bookId1, std::string(bookId2)), 0); // No more bookId1 bookmark
+    ASSERT_EQ(lib->migrateBookmarks(bookId1, bookId2), 0); // No more bookId1 bookmark
 
-    ASSERT_EQ(lib->migrateBookmarks(bookId1+"_date", std::string(bookId2)), 2);
+    ASSERT_EQ(lib->migrateBookmarks(bookId1+"_date", bookId2), 2);
     onlyValidBookmarks = lib->getBookmarks();
     allBookmarks = lib->getBookmarks(false);
 
@@ -593,7 +590,7 @@ TEST_F(LibraryTest, MigrateBookmark)
     EXPECT_EQ(allBookmarks[4].getBookId(), bookId2);
     EXPECT_EQ(allBookmarks[5].getBookId(), bookId1+"_date_flavour");
 
-    ASSERT_EQ(lib->migrateBookmarks("invalid-book-id", std::string(bookId1)), 1);
+    ASSERT_EQ(lib->migrateBookmarks("invalid-book-id", bookId1), 1);
 
     onlyValidBookmarks = lib->getBookmarks();
     allBookmarks = lib->getBookmarks(false);
@@ -630,7 +627,7 @@ TEST_F(LibraryTest, MigrateBookmarkOlder)
     EXPECT_EQ(onlyValidBookmarks[0].getBookId(), bookId1);
 
     ASSERT_EQ(lib->migrateBookmarks(bookId1), 0);
-    ASSERT_EQ(lib->migrateBookmarks(bookId1, true), 1);
+    ASSERT_EQ(lib->migrateBookmarks(bookId1, kiwix::ALLOW_DOWNGRADE), 1);
 }