Fix #493: Fix erroneous no location reminder

[chan-j-d]

Description (required)

Fixes #4936

What changes did you make and why?
Moved the logic of handling the decision to show the reminder from UploadActivity to UploadPresenter. If I understood the MVP pattern correctly, I believe that it would be more appropriate for UploadPresenter to make the decision to show the reminder given it has direct access to the items being uploaded to check for location.

The above requires that the UploadContract.View also include a method to show the dialog. Two test cases were also added, one for when an image is uploaded without location and one when an image is uploaded with location.

Tests performed (required)

Tested Build Variant: Tested BetaDebug on Pixel XL with API level 30.
Additionally, did the following test on debug build:

1. Upload 10 images without location
2. Upload image without location. Correctly shows no location reminder
3. Upload another 10 images without location
4. Upload image with location. Correctly proceeds without location reminder.

Screenshots (for UI changes only)

Need help? See https://support.google.com/android/answer/9075928

---

Note: Please ensure that you have read CONTRIBUTING.md if this is your first pull request.

[chan-j-d]

A tangential question but I noticed that

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

Lines 460 to 471 in c88ce14

	private void showInfoAlert(int titleStringID, int messageStringId, Runnable positive, String... formatArgs) {
	new AlertDialog.Builder(this)
	.setTitle(titleStringID)
	.setMessage(getString(messageStringId, (Object[]) formatArgs))
	.setCancelable(true)
	.setPositiveButton(android.R.string.ok, (dialog, id) -> {
	positive.run();
	dialog.cancel();
	})
	.create()
	.show();
	}

this method is implemented in UploadActivity and is used only once but I think it might be more appropriate to use
DialogUtil::showAlertDialog which is already used elsewhere in the class. How should I go about proposing such a change?

[nicolas-raoul]

About showAlertDialog: Feel free to create a new issue about it, and then feel free to send a pull request for it if you want. Thanks for noticing! 🙂

[nicolas-raoul]

This is indeed much better to not do this in the Activity, thanks!

[nicolas-raoul]

Could you explain what probled or mechanism you think was causing the bug? Thanks! 🙂

[chan-j-d]

Yea sure! I believe the app records some metadata under defaultKvStore which shows the number of consecutive uploads without location. The tracking and editing of this number is done in UploadPresenter::handleSubmit which seems to do it correctly by individually reviewing each item uploaded individually.

The problem occurs as the actual reminder mechanism is done one step before in UploadActivity which checks to see if a reminder should be shown and then calls handleSubmit. However, I think UploadActivity has no access to the items being uploaded and essentially just looks at the value in defaultKvStore to decide if a reminder should be shown regardless of what the current items being uploaded are.

So this would explain how after you've uploaded many items without location, there is no reminder. Then the next image uploaded with location still has a reminder because the app looks at the count, sees that it is >= threshold and sends the reminder without reviewing the items that are about to be uploaded. This change now includes both the consecutive count and the current items to decide if it should show a reminder.

[nicolas-raoul]

Understood, thanks a lot!
The code looks great (just rename the variable if you have no objection), I begin testing.

[nicolas-raoul]

Tested:

1 upload with location
9 uploads without location
1 upload with location. no message, which is correct.
4 uploads without location
4 uploads without location
1 upload without location
1 upload without location. no message.
9 uploads without location. message appears.

In other words, the bug is fixed. Thanks a lot! 🙂