Fix commons-app#493: Fix erroneous no location reminder (commons-app#5145)

* Add showAlertDialog option for UploadPresenter to use

* Move location reminder logic from UploadActivity to UploadPresenter

* Add test cases for dialog alert with handleSubmit

* Change threshold variable name to be more descriptive

* Fix broken reference to renamed constant in tests

Author: chan-j-d

app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java

@@ -470,6 +470,16 @@ private void showInfoAlert(int titleStringID, int messageStringId, Runnable posi
                 .show();
     }
 
+    @Override
+    public void showAlertDialog(int messageResourceId, Runnable onPositiveClick) {
+        DialogUtil.showAlertDialog(this,
+            "",
+            getString(messageResourceId),
+            getString(R.string.ok),
+            onPositiveClick,
+            false);
+    }
+
     @Override
     public void onNextButtonClicked(int index) {
         if (index < fragments.size() - 1) {
@@ -478,18 +488,7 @@ public void onNextButtonClicked(int index) {
             ((LinearLayoutManager) rvThumbnails.getLayoutManager())
                 .scrollToPositionWithOffset((index > 0) ? index-1 : 0, 0);
         } else {
-            if(defaultKvStore.getInt(COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES, 0) >= 10){
-                DialogUtil.showAlertDialog(this,
-                    "",
-                    getString(R.string.location_message),
-                    getString(R.string.ok),
-                    () -> {
-                        defaultKvStore.putInt(COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES, 0);
-                        presenter.handleSubmit();
-                    }, false);
-            } else {
-                presenter.handleSubmit();
-            }
+            presenter.handleSubmit();
         }
     }
 

app/src/main/java/fr/free/nrw/commons/upload/UploadContract.java

@@ -24,6 +24,12 @@ public interface View {
 
         void showMessage(int messageResourceId);
 
+        /**
+         * Displays an alert with message given by {@code messageResourceId}.
+         * {@code onPositiveClick} is run after acknowledgement.
+         */
+        void showAlertDialog(int messageResourceId, Runnable onPositiveClick);
+
         List<UploadableFile> getUploadableFiles();
 
         void showHideTopCard(boolean shouldShow);

app/src/main/java/fr/free/nrw/commons/upload/UploadPresenter.java

@@ -35,6 +35,8 @@ public class UploadPresenter implements UploadContract.UserActionListener {
     public static final String COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES
         = "number_of_consecutive_uploads_without_coordinates";
 
+    public static final int CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES_REMINDER_THRESHOLD = 10;
+
 
     @Inject
     UploadPresenter(UploadRepository uploadRepository,
@@ -51,6 +53,31 @@ public class UploadPresenter implements UploadContract.UserActionListener {
     @SuppressLint("CheckResult")
     @Override
     public void handleSubmit() {
+        boolean hasLocationProvidedForNewUploads = false;
+        for (UploadItem item : repository.getUploads()) {
+            if (item.getGpsCoords().getImageCoordsExists()) {
+                hasLocationProvidedForNewUploads = true;
+            }
+        }
+        boolean hasManyConsecutiveUploadsWithoutLocation = defaultKvStore.getInt(
+            COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES, 0) >=
+            CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES_REMINDER_THRESHOLD;
+
+        if (hasManyConsecutiveUploadsWithoutLocation && !hasLocationProvidedForNewUploads) {
+            defaultKvStore.putInt(COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES, 0);
+            view.showAlertDialog(
+                R.string.location_message,
+                () -> {defaultKvStore.putInt(
+                    COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES,
+                    0);
+                    processContributionsForSubmission();
+                });
+        } else {
+            processContributionsForSubmission();
+        }
+    }
+
+    private void processContributionsForSubmission() {
         if (view.isLoggedIn()) {
             view.showProgress(true);
             repository.buildContributions()
@@ -72,9 +99,8 @@ public void onSubscribe(Disposable d) {
 
                         @Override
                         public void onNext(Contribution contribution) {
-                            if(contribution.getDecimalCoords() == null){
-                                final int recentCount
-                                    = defaultKvStore.getInt(
+                            if (contribution.getDecimalCoords() == null) {
+                                final int recentCount = defaultKvStore.getInt(
                                     COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES, 0);
                                 defaultKvStore.putInt(
                                     COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES, recentCount + 1);

app/src/test/kotlin/fr/free/nrw/commons/upload/UploadPresenterTest.kt

@@ -6,13 +6,15 @@ import fr.free.nrw.commons.contributions.Contribution
 import fr.free.nrw.commons.filepicker.UploadableFile
 import fr.free.nrw.commons.kvstore.JsonKvStore
 import fr.free.nrw.commons.repository.UploadRepository
+import fr.free.nrw.commons.upload.ImageCoordinates
 import io.reactivex.Observable
 import org.junit.Before
 import org.junit.Test
 import org.mockito.ArgumentMatchers
 import org.mockito.InjectMocks
 import org.mockito.Mock
 import org.mockito.Mockito.`when`
+import org.mockito.Mockito.times
 import org.mockito.MockitoAnnotations
 import java.util.*
 
@@ -38,10 +40,16 @@ class UploadPresenterTest {
     @Mock
     private lateinit var anotherUploadableFile: UploadableFile
 
+    @Mock
+    private lateinit var imageCoords: ImageCoordinates
+    @Mock
+    private lateinit var uploadItem: UploadItem
+
     @InjectMocks
     lateinit var uploadPresenter: UploadPresenter
 
     private var uploadableFiles: ArrayList<UploadableFile> = ArrayList()
+    private var uploadableItems: ArrayList<UploadItem> = ArrayList()
 
     /**
      * initial setup, test environment
@@ -70,6 +78,45 @@ class UploadPresenterTest {
         verify(repository).buildContributions()
     }
 
+    @Test
+    fun handleSubmitImagesNoLocationWithConsecutiveNoLocationUploads() {
+        `when`(imageCoords.imageCoordsExists).thenReturn(false)
+        `when`(uploadItem.getGpsCoords()).thenReturn(imageCoords)
+        `when`(repository.uploads).thenReturn(uploadableItems)
+        uploadableItems.add(uploadItem)
+
+        // test 1 - insufficient count
+        `when`(
+            defaultKvStore.getInt(UploadPresenter.COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES, 0))
+                .thenReturn(UploadPresenter.CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES_REMINDER_THRESHOLD - 1)
+        uploadPresenter.handleSubmit()
+        // no alert dialog expected as insufficient consecutive count
+        verify(view, times(0)).showAlertDialog(ArgumentMatchers.anyInt(), ArgumentMatchers.any<Runnable>())
+
+        // test 2 - sufficient count
+        `when`(
+            defaultKvStore.getInt(UploadPresenter.COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES, 0))
+            .thenReturn(UploadPresenter.CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES_REMINDER_THRESHOLD)
+        uploadPresenter.handleSubmit()
+        // alert dialog expected as consecutive count is at threshold
+        verify(view).showAlertDialog(ArgumentMatchers.anyInt(), ArgumentMatchers.any<Runnable>())
+    }
+
+    @Test
+    fun handleSubmitImagesWithLocationWithConsecutiveNoLocationUploads() {
+        `when`(
+            defaultKvStore.getInt(UploadPresenter.COUNTER_OF_CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES, 0))
+            .thenReturn(UploadPresenter.CONSECUTIVE_UPLOADS_WITHOUT_COORDINATES_REMINDER_THRESHOLD)
+        `when`(imageCoords.imageCoordsExists).thenReturn(true)
+        `when`(uploadItem.getGpsCoords()).thenReturn(imageCoords)
+        `when`(repository.uploads).thenReturn(uploadableItems)
+        uploadableItems.add(uploadItem)
+        uploadPresenter.handleSubmit()
+        // no alert dialog expected
+        verify(view, times(0))
+            .showAlertDialog(ArgumentMatchers.anyInt(), ArgumentMatchers.any<Runnable>())
+    }
+
     @Test
     fun handleSubmitTestUserLoggedInAndLimitedConnectionOn() {
         `when`(