From f3a604380c8047655d6256e8ef34a7651206a728 Mon Sep 17 00:00:00 2001 From: Matthieu Gautier Date: Mon, 22 Jan 2024 15:51:42 +0100 Subject: [PATCH] Do not migrate bookmarks to an older book. At least, it must be explicitly asked by the user. --- include/library.h | 26 +++++++++++++++++++++----- src/library.cpp | 37 ++++++++++++++++++++++++------------- test/library.cpp | 17 +++++++++++++++++ 3 files changed, 62 insertions(+), 18 deletions(-) diff --git a/include/library.h b/include/library.h index 52f4185f3..895c20166 100644 --- a/include/library.h +++ b/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; @@ -272,18 +284,21 @@ class Library: public std::enable_shared_from_this * * @return A tuple: , . */ - std::tuple migrateBookmarks(); + std::tuple 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. - - * @param source the source bookId of the bookmarks to migrate. + * Be carrefull, when using with `migrationModde == ALLOW_DOWGRADE`, the bookmark will be migrated to a different book + * 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 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); diff --git a/src/library.cpp b/src/library.cpp index 3aa6157a1..e5cc989cc 100644 --- a/src/library.cpp +++ b/src/library.cpp @@ -157,7 +157,7 @@ bool Library::removeBookmark(const std::string& zimId, const std::string& url) return false; } -std::tuple Library::migrateBookmarks() { +std::tuple Library::migrateBookmarks(MigrationMode migrationMode) { std::set sourceBooks; int invalidBookmarks = 0; { @@ -171,12 +171,27 @@ std::tuple 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 - auto foundIT = std::find(targetBooks.begin(), targetBooks.end(), bookmark.getBookId()); - if (foundIT != targetBooks.end()) { - targetBooks.erase(foundIT); - } + cleanupBookCollection(targetBooks, bookmark.getBookId(), migrationMode); 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 lock(m_mutex); Bookmark firstBookmarkToChange; @@ -227,8 +238,8 @@ 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; diff --git a/test/library.cpp b/test/library.cpp index 5ed3ff498..d63a44865 100644 --- a/test/library.cpp +++ b/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) {