Fix fetching of bookmarks (#2905)

* Fix fetching of bookmarks

* With more robust tests

app/src/main/java/fr/free/nrw/commons/bookmarks/Bookmark.java

@@ -9,8 +9,8 @@ public class Bookmark {
     private String mediaName;
     private String mediaCreator;
 
-    public Bookmark(String mediaName, String mediaCreator) {
+    public Bookmark(String mediaName, String mediaCreator, Uri contentUri) {
-        this.contentUri = BookmarkPicturesContentProvider.uriForName(mediaName);
+        this.contentUri = contentUri;
         this.mediaName = mediaName == null ? "" : mediaName;
         this.mediaCreator = mediaCreator == null ? "" : mediaCreator;
     }

app/src/main/java/fr/free/nrw/commons/bookmarks/pictures/BookmarkPicturesController.java

@@ -1,5 +1,7 @@
 package fr.free.nrw.commons.bookmarks.pictures;
 
+import org.apache.commons.lang3.StringUtils;
+
 import java.util.ArrayList;
 import java.util.List;
 
@@ -9,6 +11,10 @@ import javax.inject.Singleton;
 import fr.free.nrw.commons.Media;
 import fr.free.nrw.commons.bookmarks.Bookmark;
 import fr.free.nrw.commons.mwapi.OkHttpJsonApiClient;
+import io.reactivex.Observable;
+import io.reactivex.ObservableSource;
+import io.reactivex.Single;
+import io.reactivex.functions.Function;
 
 @Singleton
 public class BookmarkPicturesController {
@@ -30,22 +36,21 @@ public class BookmarkPicturesController {
      * Loads the Media objects from the raw data stored in DB and the API.
      * @return a list of bookmarked Media object
      */
-    List<Media> loadBookmarkedPictures() {
+    Single<List<Media>> loadBookmarkedPictures() {
         List<Bookmark> bookmarks = bookmarkDao.getAllBookmarks();
         currentBookmarks = bookmarks;
-        ArrayList<Media> medias = new ArrayList<>();
+        return Observable.fromIterable(bookmarks)
-        for (Bookmark bookmark : bookmarks) {
+                .flatMap((Function<Bookmark, ObservableSource<Media>>) this::getMediaFromBookmark)
-            List<Media> tmpMedias = okHttpJsonApiClient
+                .filter(media -> media != null && !StringUtils.isBlank(media.getFilename()))
-                    .getMediaList("search", bookmark.getMediaName())
+                .toList();
-                    .blockingGet();
-            for (Media m : tmpMedias) {
-                if (m.getCreator().trim().equals(bookmark.getMediaCreator().trim())) {
-                    medias.add(m);
-                    break;
     }
-            }
+
-        }
+    private Observable<Media> getMediaFromBookmark(Bookmark bookmark) {
-        return medias;
+        Media dummyMedia = new Media("");
+        return okHttpJsonApiClient.getMedia(bookmark.getMediaName(), false)
+                .map(media -> media == null ? dummyMedia : media)
+                .onErrorReturn(throwable -> dummyMedia)
+                .toObservable();
     }
 
     /**
@@ -54,10 +59,7 @@ public class BookmarkPicturesController {
      */
     boolean needRefreshBookmarkedPictures() {
         List<Bookmark> bookmarks = bookmarkDao.getAllBookmarks();
-        if (bookmarks.size() == currentBookmarks.size()) {
+        return bookmarks.size() != currentBookmarks.size();
-            return false;
-        }
-        return true;
     }
 
     /**

app/src/main/java/fr/free/nrw/commons/bookmarks/pictures/BookmarkPicturesDao.java

@@ -149,9 +149,11 @@ public class BookmarkPicturesDao {
 
     @NonNull
     Bookmark fromCursor(Cursor cursor) {
+        String fileName = cursor.getString(cursor.getColumnIndex(Table.COLUMN_MEDIA_NAME));
         return new Bookmark(
-                cursor.getString(cursor.getColumnIndex(Table.COLUMN_MEDIA_NAME)),
+                fileName,
-                cursor.getString(cursor.getColumnIndex(Table.COLUMN_CREATOR))
+                cursor.getString(cursor.getColumnIndex(Table.COLUMN_CREATOR)),
+                BookmarkPicturesContentProvider.uriForName(fileName)
         );
     }
 

app/src/main/java/fr/free/nrw/commons/bookmarks/pictures/BookmarkPicturesFragment.java

@@ -2,8 +2,6 @@ package fr.free.nrw.commons.bookmarks.pictures;
 
 import android.annotation.SuppressLint;
 import android.os.Bundle;
-import androidx.annotation.NonNull;
-import androidx.annotation.Nullable;
 import android.view.LayoutInflater;
 import android.view.View;
 import android.view.ViewGroup;
@@ -19,6 +17,8 @@ import java.util.concurrent.TimeUnit;
 
 import javax.inject.Inject;
 
+import androidx.annotation.NonNull;
+import androidx.annotation.Nullable;
 import butterknife.BindView;
 import butterknife.ButterKnife;
 import dagger.android.support.DaggerFragment;
@@ -28,7 +28,6 @@ import fr.free.nrw.commons.bookmarks.BookmarksActivity;
 import fr.free.nrw.commons.category.GridViewAdapter;
 import fr.free.nrw.commons.utils.NetworkUtils;
 import fr.free.nrw.commons.utils.ViewUtil;
-import io.reactivex.Observable;
 import io.reactivex.android.schedulers.AndroidSchedulers;
 import io.reactivex.disposables.CompositeDisposable;
 import io.reactivex.schedulers.Schedulers;
@@ -121,7 +120,7 @@ public class BookmarkPicturesFragment extends DaggerFragment {
         progressBar.setVisibility(VISIBLE);
         statusTextView.setVisibility(GONE);
 
-        compositeDisposable.add(Observable.fromCallable(() -> controller.loadBookmarkedPictures())
+        compositeDisposable.add(controller.loadBookmarkedPictures()
                 .subscribeOn(Schedulers.io())
                 .observeOn(AndroidSchedulers.mainThread())
                 .timeout(TIMEOUT_SECONDS, TimeUnit.SECONDS)

app/src/main/java/fr/free/nrw/commons/media/MediaDetailPagerFragment.java

@@ -30,6 +30,7 @@ import fr.free.nrw.commons.Media;
 import fr.free.nrw.commons.R;
 import fr.free.nrw.commons.auth.SessionManager;
 import fr.free.nrw.commons.bookmarks.Bookmark;
+import fr.free.nrw.commons.bookmarks.pictures.BookmarkPicturesContentProvider;
 import fr.free.nrw.commons.bookmarks.pictures.BookmarkPicturesDao;
 import fr.free.nrw.commons.category.CategoryDetailsActivity;
 import fr.free.nrw.commons.category.CategoryImagesActivity;
@@ -262,7 +263,8 @@ public class MediaDetailPagerFragment extends CommonsDaggerSupportFragment imple
                     // Initialize bookmark object
                     bookmark = new Bookmark(
                             m.getFilename(),
-                            m.getCreator()
+                            m.getCreator(),
+                            BookmarkPicturesContentProvider.uriForName(m.getFilename())
                     );
                     updateBookmarkState(menu.findItem(R.id.menu_bookmark_current_image));
 

app/src/test/kotlin/fr/free/nrw/commons/bookmarks/pictures/BookmarkPictureDaoTest.kt

@@ -5,6 +5,7 @@ import android.content.ContentValues
 import android.database.Cursor
 import android.database.MatrixCursor
 import android.database.sqlite.SQLiteDatabase
+import android.net.Uri
 import android.os.RemoteException
 import com.nhaarman.mockito_kotlin.*
 import fr.free.nrw.commons.BuildConfig
@@ -33,7 +34,7 @@ class BookmarkPictureDaoTest {
 
     @Before
     fun setUp() {
-        exampleBookmark = Bookmark("mediaName", "creatorName")
+        exampleBookmark = Bookmark("mediaName", "creatorName", Uri.EMPTY)
         testObject = BookmarkPicturesDao { client }
     }
 

app/src/test/kotlin/fr/free/nrw/commons/bookmarks/pictures/BookmarkPicturesControllerTest.java

@@ -0,0 +1,109 @@
+package fr.free.nrw.commons.bookmarks.pictures;
+
+import android.net.Uri;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+
+import java.util.ArrayList;
+import java.util.List;
+
+import fr.free.nrw.commons.Media;
+import fr.free.nrw.commons.bookmarks.Bookmark;
+import fr.free.nrw.commons.mwapi.OkHttpJsonApiClient;
+import io.reactivex.Single;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyString;
+import static org.mockito.Mockito.when;
+
+/**
+ * Tests for bookmark pictures controller
+ */
+public class BookmarkPicturesControllerTest {
+
+    @Mock
+    OkHttpJsonApiClient okHttpJsonApiClient;
+    @Mock
+    BookmarkPicturesDao bookmarkDao;
+
+    @InjectMocks
+    BookmarkPicturesController bookmarkPicturesController;
+
+
+    /**
+     * Init mocks
+     */
+    @Before
+    public void setup() {
+        MockitoAnnotations.initMocks(this);
+        Media mockMedia = getMockMedia();
+        when(bookmarkDao.getAllBookmarks())
+                .thenReturn(getMockBookmarkList());
+        when(okHttpJsonApiClient.getMedia(anyString(), anyBoolean()))
+                .thenReturn(Single.just(mockMedia));
+    }
+
+    /**
+     * Get mock bookmark list
+     * @return
+     */
+    private List<Bookmark> getMockBookmarkList() {
+        ArrayList<Bookmark> list = new ArrayList<>();
+        list.add(new Bookmark("File:Test1.jpg", "Maskaravivek", Uri.EMPTY));
+        list.add(new Bookmark("File:Test2.jpg", "Maskaravivek", Uri.EMPTY));
+        return list;
+    }
+
+    /**
+     * Test case where all bookmark pictures are fetched and media is found against it
+     */
+    @Test
+    public void loadBookmarkedPictures() {
+        List<Media> bookmarkedPictures = bookmarkPicturesController.loadBookmarkedPictures().blockingGet();
+        assertEquals(2, bookmarkedPictures.size());
+    }
+
+    /**
+     * Test case where all bookmark pictures are fetched and only one media is found
+     */
+    @Test
+    public void loadBookmarkedPicturesForNullMedia() {
+        when(okHttpJsonApiClient.getMedia("File:Test1.jpg", false))
+                .thenReturn(Single.error(new NullPointerException("Error occurred")));
+        when(okHttpJsonApiClient.getMedia("File:Test2.jpg", false))
+                .thenReturn(Single.just(getMockMedia()));
+        List<Media> bookmarkedPictures = bookmarkPicturesController.loadBookmarkedPictures().blockingGet();
+        assertEquals(1, bookmarkedPictures.size());
+    }
+
+    private Media getMockMedia() {
+        return new Media("File:Test.jpg");
+    }
+
+    /**
+     * Test case where current bookmarks don't match the bookmarks in DB
+     */
+    @Test
+    public void needRefreshBookmarkedPictures() {
+        boolean needRefreshBookmarkedPictures = bookmarkPicturesController.needRefreshBookmarkedPictures();
+        assertTrue(needRefreshBookmarkedPictures);
+    }
+
+    /**
+     * Test case where the DB is up to date with the bookmarks loaded in the list
+     */
+    @Test
+    public void doNotNeedRefreshBookmarkedPictures() {
+        List<Media> bookmarkedPictures = bookmarkPicturesController.loadBookmarkedPictures().blockingGet();
+        assertEquals(2, bookmarkedPictures.size());
+        boolean needRefreshBookmarkedPictures = bookmarkPicturesController.needRefreshBookmarkedPictures();
+        assertFalse(needRefreshBookmarkedPictures);
+    }
+}