5161: Fix repeating images in peer review (#5170)

* fix API call to fetch the latest changes

* add database table to keep a track of reviewed and skipped images

* fix repeating reviewed or skipped images

* add removed newline again

* add necessary comments

* change from timber.e to timber.i in case there is no exception

* reintroduce the parameter rctag in the API URL

* modify API URL to retrieve latest uploads

* remove unused imports and code

* modify ReviewHelperTest and add new unit tests

* modify tests in ReviewHelperTest.kt

* add comments about the value of gcmlimit

app/src/androidTest/java/fr/free/nrw/commons/ReviewDaoTest.java

@@ -0,0 +1,74 @@
+package fr.free.nrw.commons;
+
+import static org.hamcrest.CoreMatchers.equalTo;
+import static org.hamcrest.MatcherAssert.assertThat;
+
+import android.content.Context;
+import androidx.room.Room;
+import androidx.test.core.app.ApplicationProvider;
+import androidx.test.ext.junit.runners.AndroidJUnit4;
+import fr.free.nrw.commons.db.AppDatabase;
+import fr.free.nrw.commons.review.ReviewDao;
+import fr.free.nrw.commons.review.ReviewEntity;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+@RunWith(AndroidJUnit4.class)
+public class ReviewDaoTest {
+
+    private ReviewDao reviewDao;
+    private AppDatabase database;
+
+    /**
+     * Set up the application database
+     */
+    @Before
+    public void createDb() {
+        Context context = ApplicationProvider.getApplicationContext();
+        database = Room.inMemoryDatabaseBuilder(
+                context, AppDatabase.class)
+            .allowMainThreadQueries()
+            .build();
+        reviewDao = database.ReviewDao();
+    }
+
+    /**
+     * Close the database
+     */
+    @After
+    public void closeDb() {
+        database.close();
+    }
+
+    /**
+     * Test insertion
+     * Also checks isReviewedAlready():
+     * Case 1: When image has been reviewed/skipped by the user
+     */
+    @Test
+    public void insert() {
+        // Insert data
+        String imageId = "1234";
+        ReviewEntity reviewEntity = new ReviewEntity(imageId);
+        reviewDao.insert(reviewEntity);
+
+        // Check insertion
+        // Covers the case where the image exists in the database
+        // And isReviewedAlready() returns true
+        Boolean isInserted = reviewDao.isReviewedAlready(imageId);
+        assertThat(isInserted, equalTo(true));
+    }
+
+    /**
+     * Test review status of the image
+     * Case 2: When image has not been reviewed/skipped
+     */
+    @Test
+    public void isReviewedAlready(){
+        String imageId = "5856";
+        Boolean isInserted = reviewDao.isReviewedAlready(imageId);
+        assertThat(isInserted, equalTo(false));
+    }
+}

app/src/main/java/fr/free/nrw/commons/db/AppDatabase.kt

@@ -6,6 +6,8 @@ import androidx.room.TypeConverters
 import fr.free.nrw.commons.contributions.Contribution
 import fr.free.nrw.commons.contributions.ContributionDao
 import fr.free.nrw.commons.customselector.database.*
+import fr.free.nrw.commons.review.ReviewDao
+import fr.free.nrw.commons.review.ReviewEntity
 import fr.free.nrw.commons.upload.depicts.Depicts
 import fr.free.nrw.commons.upload.depicts.DepictsDao
 
@@ -13,11 +15,12 @@ import fr.free.nrw.commons.upload.depicts.DepictsDao
  * The database for accessing the respective DAOs
  *
  */
-@Database(entities = [Contribution::class, Depicts::class, UploadedStatus::class, NotForUploadStatus::class], version = 14, exportSchema = false)
+@Database(entities = [Contribution::class, Depicts::class, UploadedStatus::class, NotForUploadStatus::class, ReviewEntity::class], version = 15, exportSchema = false)
 @TypeConverters(Converters::class)
 abstract class AppDatabase : RoomDatabase() {
     abstract fun contributionDao(): ContributionDao
     abstract fun DepictsDao(): DepictsDao;
     abstract fun UploadedStatusDao(): UploadedStatusDao;
     abstract fun NotForUploadStatusDao(): NotForUploadStatusDao
+    abstract fun ReviewDao(): ReviewDao
 }

app/src/main/java/fr/free/nrw/commons/di/CommonsApplicationModule.java

@@ -24,6 +24,7 @@ import fr.free.nrw.commons.data.DBOpenHelper;
 import fr.free.nrw.commons.db.AppDatabase;
 import fr.free.nrw.commons.kvstore.JsonKvStore;
 import fr.free.nrw.commons.location.LocationServiceManager;
+import fr.free.nrw.commons.review.ReviewDao;
 import fr.free.nrw.commons.settings.Prefs;
 import fr.free.nrw.commons.upload.UploadController;
 import fr.free.nrw.commons.upload.depicts.DepictsDao;
@@ -299,6 +300,14 @@ public class CommonsApplicationModule {
         return appDatabase.NotForUploadStatusDao();
     }
 
+    /**
+     * Get the reference of ReviewDao class
+     */
+    @Provides
+    public ReviewDao providesReviewDao(AppDatabase appDatabase){
+        return appDatabase.ReviewDao();
+    }
+
     @Provides
     public ContentResolver providesContentResolver(Context context){
         return context.getContentResolver();

app/src/main/java/fr/free/nrw/commons/review/ReviewActivity.java

@@ -114,7 +114,7 @@ public class ReviewActivity extends BaseActivity {
         ButterKnife.bind(this);
         setSupportActionBar(toolbar);
         getSupportActionBar().setDisplayHomeAsUpEnabled(true);
-
+        
         reviewController = new ReviewController(deleteHelper, this);
 
         reviewPagerAdapter = new ReviewPagerAdapter(getSupportFragmentManager());
@@ -209,6 +209,7 @@ public class ReviewActivity extends BaseActivity {
 
     @SuppressLint("CheckResult")
     private void updateImage(Media media) {
+        reviewHelper.addViewedImagesToDB(media.getPageId());
         this.media = media;
         String fileName = media.getFilename();
         if (fileName.length() == 0) {
@@ -225,7 +226,7 @@ public class ReviewActivity extends BaseActivity {
                 .subscribe(revision -> {
                     reviewController.firstRevision = revision;
                     reviewPagerAdapter.updateFileInformation();
-                    String caption = String.format(getString(R.string.review_is_uploaded_by), fileName, revision.getUser());
+                    @SuppressLint({"StringFormatInvalid", "LocalSuppress"}) String caption = String.format(getString(R.string.review_is_uploaded_by), fileName, revision.getUser());
                     imageCaption.setText(caption);
                     progressBar.setVisibility(View.GONE);
                     reviewImageFragment = getInstanceOfReviewImageFragment();

app/src/main/java/fr/free/nrw/commons/review/ReviewDao.java

@@ -0,0 +1,32 @@
+package fr.free.nrw.commons.review;
+
+import androidx.room.Dao;
+import androidx.room.Insert;
+import androidx.room.OnConflictStrategy;
+import androidx.room.Query;
+
+/**
+ * Dao interface for reviewed images database
+ */
+@Dao
+public interface ReviewDao {
+
+    /**
+     * Inserts reviewed/skipped image identifier into the database
+     *
+     * @param reviewEntity
+     */
+    @Insert(onConflict = OnConflictStrategy.IGNORE)
+    void insert(ReviewEntity reviewEntity);
+
+    /**
+     * Checks if the image has already been reviewed/skipped by the user
+     * Returns true if the identifier exists in the reviewed images table
+     *
+     * @param imageId
+     * @return
+     */
+    @Query( "SELECT EXISTS (SELECT * from `reviewed-images` where imageId = (:imageId))")
+    Boolean isReviewedAlready(String imageId);
+
+}

app/src/main/java/fr/free/nrw/commons/review/ReviewEntity.java

@@ -0,0 +1,19 @@
+package fr.free.nrw.commons.review;
+
+import androidx.annotation.NonNull;
+import androidx.room.Entity;
+import androidx.room.PrimaryKey;
+
+/**
+ * Entity to store reviewed/skipped images identifier
+ */
+@Entity(tableName = "reviewed-images")
+public class ReviewEntity {
+    @PrimaryKey
+    @NonNull
+    String imageId;
+
+    public ReviewEntity(String imageId) {
+        this.imageId = imageId;
+    }
+}

app/src/main/java/fr/free/nrw/commons/review/ReviewHelper.java

@@ -1,19 +1,20 @@
 package fr.free.nrw.commons.review;
 
 
+import androidx.annotation.VisibleForTesting;
 import fr.free.nrw.commons.Media;
 import fr.free.nrw.commons.media.MediaClient;
+import io.reactivex.Completable;
 import io.reactivex.Observable;
 import io.reactivex.Single;
+import io.reactivex.android.schedulers.AndroidSchedulers;
+import io.reactivex.schedulers.Schedulers;
 import java.util.Collections;
-import java.util.Date;
-import java.util.Random;
 import javax.inject.Inject;
 import javax.inject.Singleton;
 import org.apache.commons.lang3.StringUtils;
 import org.wikipedia.dataclient.mwapi.MwQueryPage;
-import org.wikipedia.dataclient.mwapi.RecentChange;
-import org.wikipedia.util.DateUtil;
+import timber.log.Timber;
 
 @Singleton
 public class ReviewHelper {
@@ -23,6 +24,9 @@ public class ReviewHelper {
     private final MediaClient mediaClient;
     private final ReviewInterface reviewInterface;
 
+    @Inject
+    ReviewDao dao;
+
     @Inject
     public ReviewHelper(MediaClient mediaClient, ReviewInterface reviewInterface) {
         this.mediaClient = mediaClient;
@@ -31,21 +35,14 @@ public class ReviewHelper {
 
     /**
      * Fetches recent changes from MediaWiki API
-     * Calls the API to get 10 changes in the last 1 hour
-     * Earlier we were getting changes for the last 30 days but as the API returns just 10 results
-     * its best to fetch for just last 1 hour.
+     * Calls the API to get the latest 50 changes
+     * When more results are available, the query gets continued beyond this range
      *
      * @return
      */
-    private Observable<RecentChange> getRecentChanges() {
-        final int RANDOM_SECONDS = 60 * 60;
-        Random r = new Random();
-        Date now = new Date();
-        Date startDate = new Date(now.getTime() - r.nextInt(RANDOM_SECONDS) * 1000L);
-
-        String rcStart = DateUtil.iso8601DateFormat(startDate);
-        return reviewInterface.getRecentChanges(rcStart)
-                .map(mwQueryResponse -> mwQueryResponse.query().getRecentChanges())
+    private Observable<MwQueryPage> getRecentChanges() {
+        return reviewInterface.getRecentChanges()
+                .map(mwQueryResponse -> mwQueryResponse.query().pages())
                 .map(recentChanges -> {
                     Collections.shuffle(recentChanges);
                     return recentChanges;
@@ -56,7 +53,6 @@ public class ReviewHelper {
 
     /**
      * Gets a random file change for review.
-     * - Picks the most recent changes in the last 30 day window
      * - Picks a random file from those changes
      * - Checks if the file is nominated for deletion
      * - Retries upto 5 times for getting a file which is not nominated for deletion
@@ -66,7 +62,9 @@ public class ReviewHelper {
     public Single<Media> getRandomMedia() {
         return getRecentChanges()
                 .flatMapSingle(change -> getRandomMediaFromRecentChange(change))
-                .filter(media -> !StringUtils.isBlank(media.getFilename()))
+                .filter(media -> !StringUtils.isBlank(media.getFilename())
+                   && !getReviewStatus(media.getPageId())    // Check if the image has already been shown to the user
+                )
                 .firstOrError();
     }
 
@@ -77,18 +75,34 @@ public class ReviewHelper {
      * @param recentChange
      * @return
      */
-    private Single<Media> getRandomMediaFromRecentChange(RecentChange recentChange) {
+    private Single<Media> getRandomMediaFromRecentChange(MwQueryPage recentChange) {
         return Single.just(recentChange)
-                .flatMap(change -> mediaClient.checkPageExistsUsingTitle("Commons:Deletion_requests/" + change.getTitle()))
+                .flatMap(change -> mediaClient.checkPageExistsUsingTitle("Commons:Deletion_requests/" + change.title()))
                 .flatMap(isDeleted -> {
                     if (isDeleted) {
-                        return Single.error(new Exception(recentChange.getTitle() + " is deleted"));
+                        return Single.error(new Exception(recentChange.title() + " is deleted"));
                     }
-                    return mediaClient.getMedia(recentChange.getTitle());
+                    return mediaClient.getMedia(recentChange.title());
                 });
 
     }
 
+    /**
+     * Checks if the image exists in the reviewed images entity
+     *
+     * @param image
+     * @return
+     */
+    @VisibleForTesting
+    Boolean getReviewStatus(String image){
+        if(dao == null){
+            return false;
+        }
+        return Observable.fromCallable(()-> dao.isReviewedAlready(image))
+                .subscribeOn(Schedulers.io())
+                .observeOn(AndroidSchedulers.mainThread()).blockingSingle();
+    }
+
     /**
      * Gets the first revision of the file from filename
      *
@@ -121,18 +135,32 @@ public class ReviewHelper {
      * @param recentChange
      * @return
      */
-    private boolean isChangeReviewable(RecentChange recentChange) {
-        if ((recentChange.getType().equals("log") && !(recentChange.getOldRevisionId() == 0))
-                || !recentChange.getType().equals("log")) {
-            return false;
-        }
-
+    private boolean isChangeReviewable(MwQueryPage recentChange) {
         for (String extension : imageExtensions) {
-            if (recentChange.getTitle().endsWith(extension)) {
+            if (recentChange.title().endsWith(extension)) {
                 return true;
             }
         }
 
         return false;
     }
+
+    /**
+     * Adds reviewed/skipped images to the database
+     *
+     * @param imageId
+     */
+    public void addViewedImagesToDB(String imageId) {
+        Completable.fromAction(() -> dao.insert(new ReviewEntity(imageId)))
+            .subscribeOn(Schedulers.io())
+            .observeOn(AndroidSchedulers.mainThread())
+            .subscribe(() -> {
+                // Inserted successfully
+                Timber.i("Image inserted successfully.");
+                },
+                throwable -> {
+                    Timber.e("Image not inserted into the reviewed images database");
+                }
+            );
+    }
 }

app/src/main/java/fr/free/nrw/commons/review/ReviewInterface.java

@@ -10,8 +10,18 @@ import retrofit2.http.Query;
  * Interface class for peer review calls
  */
 public interface ReviewInterface {
-    @GET("w/api.php?action=query&format=json&formatversion=2&list=recentchanges&rcprop=title|ids&rctype=new|log&rctoponly=1&rcnamespace=6&rctag=android%20app%20edit")
-    Observable<MwQueryResponse> getRecentChanges(@Query("rcstart") String rcStart);
+
+    /**
+     * Fetch recent changes from MediaWiki API
+     * Calls the API for the latest 50 changes (the default limit is 10)
+     * More data can be fetched beyond this limit as the API call includes a continuation field
+     * However, since it takes longer to check the review status from the database and display the images
+     * as they get repeated before more data is fetched in the background
+     * the limit is increased from 10 to 50 using gcmlimit
+     *
+     */
+    @GET("w/api.php?action=query&format=json&formatversion=2&generator=categorymembers&gcmtype=file&gcmsort=timestamp&gcmdir=desc&gcmtitle=Category:Uploaded_with_Mobile/Android&gcmlimit=50")
+    Observable<MwQueryResponse> getRecentChanges();
 
     @GET("w/api.php?action=query&format=json&formatversion=2&prop=revisions&rvprop=timestamp|ids|user&rvdir=newer&rvlimit=1")
     Observable<MwQueryResponse> getFirstRevisionOfFile(@Query("titles") String titles);

app/src/test/kotlin/fr/free/nrw/commons/review/ReviewHelperTest.kt

@@ -1,8 +1,10 @@
 package fr.free.nrw.commons.review
 
+import com.nhaarman.mockitokotlin2.mock
 import com.nhaarman.mockitokotlin2.whenever
 import fr.free.nrw.commons.Media
 import fr.free.nrw.commons.media.MediaClient
+import io.reactivex.Completable
 import io.reactivex.Observable
 import io.reactivex.Single
 import org.junit.Assert.assertNull
@@ -17,7 +19,7 @@ import org.mockito.MockitoAnnotations
 import org.wikipedia.dataclient.mwapi.MwQueryPage
 import org.wikipedia.dataclient.mwapi.MwQueryResponse
 import org.wikipedia.dataclient.mwapi.MwQueryResult
-import org.wikipedia.dataclient.mwapi.RecentChange
+import java.util.concurrent.Callable
 
 /**
  * Test class for ReviewHelper
@@ -32,6 +34,8 @@ class ReviewHelperTest {
     @InjectMocks
     var reviewHelper: ReviewHelper? = null
 
+    val dao = mock(ReviewDao::class.java)
+
     /**
      * Init mocks
      */
@@ -45,16 +49,12 @@ class ReviewHelperTest {
         `when`(mockRevision.user).thenReturn("TestUser")
         `when`(mwQueryPage.revisions()).thenReturn(listOf(mockRevision))
 
-        val recentChange = getMockRecentChange("log", "File:Test1.jpeg", 0)
-        val recentChange1 = getMockRecentChange("log", "File:Test2.png", 0)
-        val recentChange2 = getMockRecentChange("log", "File:Test3.jpg", 0)
         val mwQueryResult = mock(MwQueryResult::class.java)
-        `when`(mwQueryResult.recentChanges).thenReturn(listOf(recentChange, recentChange1, recentChange2))
         `when`(mwQueryResult.firstPage()).thenReturn(mwQueryPage)
         `when`(mwQueryResult.pages()).thenReturn(listOf(mwQueryPage))
         val mockResponse = mock(MwQueryResponse::class.java)
         `when`(mockResponse.query()).thenReturn(mwQueryResult)
-        `when`(reviewInterface?.getRecentChanges(ArgumentMatchers.anyString()))
+        `when`(reviewInterface?.getRecentChanges())
                 .thenReturn(Observable.just(mockResponse))
 
         `when`(reviewInterface?.getFirstRevisionOfFile(ArgumentMatchers.anyString()))
@@ -78,7 +78,7 @@ class ReviewHelperTest {
                 .thenReturn(Single.just(false))
 
         reviewHelper?.randomMedia
-        verify(reviewInterface, times(1))!!.getRecentChanges(ArgumentMatchers.anyString())
+        verify(reviewInterface, times(1))!!.getRecentChanges()
     }
 
     /**
@@ -90,7 +90,7 @@ class ReviewHelperTest {
                 .thenReturn(Single.just(true))
         val media = reviewHelper?.randomMedia?.blockingGet()
         assertNull(media)
-        verify(reviewInterface, times(1))!!.getRecentChanges(ArgumentMatchers.anyString())
+        verify(reviewInterface, times(1))!!.getRecentChanges()
     }
 
     /**
@@ -106,15 +106,7 @@ class ReviewHelperTest {
                 .thenReturn(Single.just(true))
 
         reviewHelper?.randomMedia
-        verify(reviewInterface, times(1))!!.getRecentChanges(ArgumentMatchers.anyString())
-    }
-
-    private fun getMockRecentChange(type: String, title: String, oldRevisionId: Long): RecentChange {
-        val recentChange = mock(RecentChange::class.java)
-        `when`(recentChange!!.type).thenReturn(type)
-        `when`(recentChange.title).thenReturn(title)
-        `when`(recentChange.oldRevisionId).thenReturn(oldRevisionId)
-        return recentChange
+        verify(reviewInterface, times(1))!!.getRecentChanges()
     }
 
     /**
@@ -126,4 +118,53 @@ class ReviewHelperTest {
 
         assertTrue(firstRevisionOfFile is MwQueryPage.Revision)
     }
+
+    /**
+     * Test the review status of the image
+     *  Case 1: Image identifier exists in the database
+     */
+    @Test
+    fun getReviewStatusWhenImageHasBeenReviewedAlready() {
+        val testImageId1 = "123456"
+        `when`(dao.isReviewedAlready(testImageId1)).thenReturn(true)
+
+        val observer = io.reactivex.observers.TestObserver<Boolean>()
+        Observable.fromCallable(Callable<Boolean> {
+            dao.isReviewedAlready(testImageId1)
+        }).subscribeWith(observer)
+        observer.assertValue(true)
+        observer.dispose()
+    }
+
+    /**
+     * Test the review status of the image
+     *  Case 2: Image identifier does not exist in the database
+     */
+    @Test
+    fun getReviewStatusWhenImageHasBeenNotReviewedAlready() {
+        val testImageId2 = "789101"
+        `when`(dao.isReviewedAlready(testImageId2)).thenReturn(false)
+
+        val observer = io.reactivex.observers.TestObserver<Boolean>()
+        Observable.fromCallable(Callable<Boolean> {
+            dao.isReviewedAlready(testImageId2)
+        }).subscribeWith(observer)
+        observer.assertValue(false)
+        observer.dispose()
+    }
+
+    /**
+     * Test the successful insertion of the image identifier into the database
+     */
+    @Test
+    fun addViewedImagesToDB() {
+        val testImageId = "123456"
+
+        val observer = io.reactivex.observers.TestObserver<Boolean>()
+        Completable.fromAction {
+            dao.insert(ReviewEntity(testImageId))
+        }.subscribeWith(observer)
+        observer.assertComplete()
+        observer.dispose()
+    }
 }

data-client/src/main/java/org/wikipedia/dataclient/mwapi/MwQueryResult.java

@@ -35,7 +35,6 @@ public class MwQueryResult extends BaseModel implements PostProcessingTypeAdapte
     @Nullable private NotificationList notifications;
     @Nullable private Map<String, Notification.UnreadNotificationWikiItem> unreadnotificationpages;
     @SerializedName("general") @Nullable private SiteInfo generalSiteInfo;
-    @Nullable private List<RecentChange> recentchanges;
     @SerializedName("wikimediaeditortaskscounts") @Nullable private EditorTaskCounts editorTaskCounts;
     @SerializedName("allimages") @Nullable private List<ImageDetails> allImages;
     @SerializedName("geosearch") @Nullable private List<GeoSearchItem> geoSearch;
@@ -103,10 +102,6 @@ public class MwQueryResult extends BaseModel implements PostProcessingTypeAdapte
         return captchaId;
     }
 
-    @Nullable public List<RecentChange> getRecentChanges() {
-        return recentchanges;
-    }
-
     @Nullable public ListUserResponse getUserResponse(@NonNull String userName) {
         if (users != null) {
             for (ListUserResponse user : users) {

data-client/src/main/java/org/wikipedia/dataclient/mwapi/RecentChange.java

@@ -1,42 +0,0 @@
-package org.wikipedia.dataclient.mwapi;
-
-import androidx.annotation.NonNull;
-import androidx.annotation.Nullable;
-
-import com.google.gson.annotations.SerializedName;
-
-import org.apache.commons.lang3.StringUtils;
-
-@SuppressWarnings("unused")
-public class RecentChange {
-    @Nullable private String type;
-    @Nullable private String title;
-    private long pageid;
-    private long revid;
-    @SerializedName("old_revid") private long oldRevisionId;
-    @Nullable private String timestamp;
-
-    @NonNull public String getType() {
-        return StringUtils.defaultString(type);
-    }
-
-    @NonNull public String getTitle() {
-        return StringUtils.defaultString(title);
-    }
-
-    public long getPageId() {
-        return pageid;
-    }
-
-    public long getRevId() {
-        return revid;
-    }
-
-    public long getOldRevisionId() {
-        return oldRevisionId;
-    }
-
-    public String getTimestamp() {
-        return StringUtils.defaultString(timestamp);
-    }
-}