From 1a8a068552c0e9723e3bc60237fe8d5683a0474e Mon Sep 17 00:00:00 2001 From: Chan Jun Da <65345505+chan-j-d@users.noreply.github.com> Date: Sat, 18 Feb 2023 22:18:52 +0800 Subject: [PATCH] Fix #493: Fix erroneous no location reminder (#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 --- .../nrw/commons/upload/UploadActivity.java | 23 +++++---- .../nrw/commons/upload/UploadContract.java | 6 +++ .../nrw/commons/upload/UploadPresenter.java | 32 +++++++++++-- .../nrw/commons/upload/UploadPresenterTest.kt | 47 +++++++++++++++++++ 4 files changed, 93 insertions(+), 15 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java index 12890f133..f09724547 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/UploadActivity.java @@ -470,6 +470,16 @@ public class UploadActivity extends BaseActivity implements UploadContract.View, .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 class UploadActivity extends BaseActivity implements UploadContract.View, ((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(); } } diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadContract.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadContract.java index e8f365314..2ce133a7c 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/UploadContract.java +++ b/app/src/main/java/fr/free/nrw/commons/upload/UploadContract.java @@ -24,6 +24,12 @@ public interface UploadContract { 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 getUploadableFiles(); void showHideTopCard(boolean shouldShow); diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadPresenter.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadPresenter.java index e4a9d78a8..1f8deaa7c 100644 --- a/app/src/main/java/fr/free/nrw/commons/upload/UploadPresenter.java +++ b/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 class UploadPresenter implements UploadContract.UserActionListener { @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); diff --git a/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadPresenterTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadPresenterTest.kt index e3dff1f68..6de055a4b 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadPresenterTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadPresenterTest.kt @@ -6,6 +6,7 @@ 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 @@ -13,6 +14,7 @@ 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 = ArrayList() + private var uploadableItems: ArrayList = 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()) + + // 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()) + } + + @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()) + } + @Test fun handleSubmitTestUserLoggedInAndLimitedConnectionOn() { `when`(