From ad7dddaac439b3af4332e43552fc25994a74f219 Mon Sep 17 00:00:00 2001 From: Rohit Verma <101377978+rohit9625@users.noreply.github.com> Date: Wed, 25 Jun 2025 08:54:03 +0530 Subject: [PATCH] Fix infinite loading circular progress bar after nominating for deletion (#6324) * fix: infinite loading progress bar after nominating for deletion * add logs for testing * refactor: use globalFileUsage instead of achievement to append in reason Fetching achievements is a time consuming operation and globalFileUsage gives the similar result in optimal time * test(ReasonBuilder): fix tests according to new behavior * refactor: remove logs added for testing * test: await for async getReason method call --------- Co-authored-by: Neel Doshi Co-authored-by: Nicolas Raoul --- app/build.gradle.kts | 1 + .../free/nrw/commons/delete/DeleteHelper.kt | 2 - .../free/nrw/commons/delete/ReasonBuilder.kt | 51 +++++++----- .../nrw/commons/media/MediaDetailFragment.kt | 79 +++++++++++-------- .../nrw/commons/mwapi/OkHttpJsonApiClient.kt | 1 + app/src/main/res/values/strings.xml | 3 +- .../nrw/commons/delete/ReasonBuilderTest.kt | 15 ++-- gradle/libs.versions.toml | 2 + 8 files changed, 88 insertions(+), 66 deletions(-) diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 9c53155a7..2e391a24f 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -347,6 +347,7 @@ dependencies { // Kotlin + coroutines implementation(libs.androidx.work.runtime.ktx) implementation(libs.androidx.work.runtime) + implementation(libs.kotlinx.coroutines.rx2) testImplementation(libs.androidx.work.testing) //Glide diff --git a/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.kt b/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.kt index 09959d0ef..3f9ae47ac 100644 --- a/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.kt +++ b/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.kt @@ -53,7 +53,6 @@ class DeleteHelper @Inject constructor( media: Media?, reason: String? ): Single? { - if(context == null && media == null) { return null } @@ -86,7 +85,6 @@ class DeleteHelper @Inject constructor( * @return */ private fun delete(media: Media, reason: String): Observable { - Timber.d("thread is delete %s", Thread.currentThread().name) val summary = "Nominating ${media.filename} for deletion." val calendar = Calendar.getInstance() val fileDeleteString = """ diff --git a/app/src/main/java/fr/free/nrw/commons/delete/ReasonBuilder.kt b/app/src/main/java/fr/free/nrw/commons/delete/ReasonBuilder.kt index 09018c249..c4cd73b8a 100644 --- a/app/src/main/java/fr/free/nrw/commons/delete/ReasonBuilder.kt +++ b/app/src/main/java/fr/free/nrw/commons/delete/ReasonBuilder.kt @@ -2,21 +2,19 @@ package fr.free.nrw.commons.delete import android.annotation.SuppressLint import android.content.Context - -import fr.free.nrw.commons.utils.DateUtil -import java.util.Locale - -import javax.inject.Inject -import javax.inject.Singleton - import fr.free.nrw.commons.Media import fr.free.nrw.commons.R -import fr.free.nrw.commons.profile.achievements.FeedbackResponse import fr.free.nrw.commons.auth.SessionManager import fr.free.nrw.commons.mwapi.OkHttpJsonApiClient +import fr.free.nrw.commons.utils.DateUtil import fr.free.nrw.commons.utils.ViewUtilWrapper import io.reactivex.Single +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.rx2.rxSingle import timber.log.Timber +import java.util.Locale +import javax.inject.Inject +import javax.inject.Singleton /** * This class handles the reason for deleting a Media object @@ -29,6 +27,8 @@ class ReasonBuilder @Inject constructor( private val viewUtilWrapper: ViewUtilWrapper ) { + private val defaultFileUsagePageSize = 10 + /** * To process the reason and append the media's upload date and uploaded_by_me string * @param media @@ -39,7 +39,7 @@ class ReasonBuilder @Inject constructor( if (media == null || reason == null) { return Single.just("Not known") } - return fetchArticleNumber(media, reason) + return getAndAppendFileUsage(media, reason) } /** @@ -54,27 +54,36 @@ class ReasonBuilder @Inject constructor( } } - private fun fetchArticleNumber(media: Media, reason: String): Single { - return if (checkAccount()) { - okHttpJsonApiClient - .getAchievements(sessionManager.userName) - .map { feedbackResponse -> appendArticlesUsed(feedbackResponse, media, reason) } - } else { - Single.just("") + private fun getAndAppendFileUsage(media: Media, reason: String): Single { + return rxSingle(context = Dispatchers.IO) { + if (!checkAccount()) return@rxSingle "" + + try { + val globalFileUsage = okHttpJsonApiClient.getGlobalFileUsages( + fileName = media.filename, + pageSize = defaultFileUsagePageSize + ) + val globalUsages = globalFileUsage?.query?.pages?.sumOf { it.fileUsage.size } ?: 0 + + appendArticlesUsed(globalUsages, media, reason) + } catch (e: Exception) { + Timber.e(e, "Error fetching file usage") + throw e + } } } /** - * Takes the uploaded_by_me string, the upload date, name of articles using images + * Takes the uploaded_by_me string, the upload date, no. of articles using images * and appends it to the received reason - * @param feedBack object + * @param fileUsages No. of files/articles using this image * @param media whose upload data is to be fetched - * @param reason + * @param reason string to be appended */ @SuppressLint("StringFormatInvalid") - private fun appendArticlesUsed(feedBack: FeedbackResponse, media: Media, reason: String): String { + private fun appendArticlesUsed(fileUsages: Int, media: Media, reason: String): String { val reason1Template = context.getString(R.string.uploaded_by_myself) - return reason + String.format(Locale.getDefault(), reason1Template, prettyUploadedDate(media), feedBack.articlesUsingImages) + return reason + String.format(Locale.getDefault(), reason1Template, prettyUploadedDate(media), fileUsages) .also { Timber.i("New Reason %s", it) } } diff --git a/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.kt b/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.kt index 8a4d530c4..f371b733f 100644 --- a/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.kt +++ b/app/src/main/java/fr/free/nrw/commons/media/MediaDetailFragment.kt @@ -74,7 +74,6 @@ import fr.free.nrw.commons.BuildConfig import fr.free.nrw.commons.CameraPosition import fr.free.nrw.commons.CommonsApplication import fr.free.nrw.commons.CommonsApplication.Companion.instance -import fr.free.nrw.commons.locationpicker.LocationPicker import fr.free.nrw.commons.Media import fr.free.nrw.commons.MediaDataExtractor import fr.free.nrw.commons.R @@ -102,6 +101,7 @@ import fr.free.nrw.commons.explore.depictions.WikidataItemDetailsActivity import fr.free.nrw.commons.kvstore.JsonKvStore import fr.free.nrw.commons.language.AppLanguageLookUpTable import fr.free.nrw.commons.location.LocationServiceManager +import fr.free.nrw.commons.locationpicker.LocationPicker import fr.free.nrw.commons.media.MediaDetailPagerFragment.MediaDetailProvider import fr.free.nrw.commons.profile.ProfileActivity import fr.free.nrw.commons.review.ReviewHelper @@ -116,6 +116,7 @@ import fr.free.nrw.commons.utils.LangCodeUtils.getLocalizedResources import fr.free.nrw.commons.utils.PermissionUtils.PERMISSIONS_STORAGE import fr.free.nrw.commons.utils.PermissionUtils.checkPermissionsAndPerformAction import fr.free.nrw.commons.utils.PermissionUtils.hasPermission +import fr.free.nrw.commons.utils.ViewUtil import fr.free.nrw.commons.utils.ViewUtil.showShortToast import fr.free.nrw.commons.utils.ViewUtilWrapper import fr.free.nrw.commons.wikidata.mwapi.MwQueryPage.Revision @@ -125,6 +126,7 @@ import io.reactivex.android.schedulers.AndroidSchedulers import io.reactivex.schedulers.Schedulers import org.apache.commons.lang3.StringUtils import timber.log.Timber +import java.lang.String.format import java.util.Date import java.util.Locale import java.util.Objects @@ -1646,7 +1648,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C getString(R.string.cancel), { val reason: String = input.text.toString() - onDeleteClickeddialogtext(reason) + onDeleteClickedDialogText(reason) }, {}, input @@ -1700,26 +1702,48 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C resultSingle .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) - .subscribe { _ -> - if (applicationKvStore.getBoolean( - String.format( - NOMINATING_FOR_DELETION_MEDIA, media!!.imageUrl - ), false - ) - ) { - applicationKvStore.remove( - String.format( - NOMINATING_FOR_DELETION_MEDIA, - media!!.imageUrl - ) - ) - callback!!.nominatingForDeletion(index) - } - } + .subscribe(this::handleDeletionResult, this::handleDeletionError); + } + + /** + * Disables Progress Bar and Update delete button text. + */ + private fun disableProgressBar() { + activity?.run { + runOnUiThread(Runnable { + binding.progressBarDeletion.visibility = View.GONE + }) + } ?: return // Prevent NullPointerException when fragment is not attached to activity + } + + private fun handleDeletionResult(success: Boolean) { + if (success) { + binding.nominateDeletion.text = getString(R.string.nominated_for_deletion_btn) + ViewUtil.showLongSnackbar(requireView(), getString(R.string.nominated_for_deletion)) + disableProgressBar() + checkAndClearDeletionFlag() + } else { + disableProgressBar() + } + } + + private fun handleDeletionError(throwable: Throwable) { + throwable.printStackTrace() + disableProgressBar() + checkAndClearDeletionFlag() + } + + private fun checkAndClearDeletionFlag() { + if (applicationKvStore + .getBoolean(format(NOMINATING_FOR_DELETION_MEDIA, media!!.imageUrl), false) + ) { + applicationKvStore.remove(format(NOMINATING_FOR_DELETION_MEDIA, media!!.imageUrl)) + callback!!.nominatingForDeletion(index) + } } @SuppressLint("CheckResult") - private fun onDeleteClickeddialogtext(reason: String) { + private fun onDeleteClickedDialogText(reason: String) { applicationKvStore.putBoolean( String.format( NOMINATING_FOR_DELETION_MEDIA, @@ -1736,22 +1760,7 @@ class MediaDetailFragment : CommonsDaggerSupportFragment(), CategoryEditHelper.C resultSingletext .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) - .subscribe { _ -> - if (applicationKvStore.getBoolean( - String.format( - NOMINATING_FOR_DELETION_MEDIA, media!!.imageUrl - ), false - ) - ) { - applicationKvStore.remove( - String.format( - NOMINATING_FOR_DELETION_MEDIA, - media!!.imageUrl - ) - ) - callback!!.nominatingForDeletion(index) - } - } + .subscribe(this::handleDeletionResult, this::handleDeletionError); } private fun onSeeMoreClicked() { diff --git a/app/src/main/java/fr/free/nrw/commons/mwapi/OkHttpJsonApiClient.kt b/app/src/main/java/fr/free/nrw/commons/mwapi/OkHttpJsonApiClient.kt index 4fa7979d2..a2f92c2e6 100644 --- a/app/src/main/java/fr/free/nrw/commons/mwapi/OkHttpJsonApiClient.kt +++ b/app/src/main/java/fr/free/nrw/commons/mwapi/OkHttpJsonApiClient.kt @@ -281,6 +281,7 @@ class OkHttpJsonApiClient @Inject constructor( FeedbackResponse::class.java ) } catch (e: Exception) { + e.printStackTrace() return@fromCallable FeedbackResponse(0, 0, 0, FeaturedImages(0, 0), 0, "") } } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 68dba88be..c9a2d16c8 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -431,7 +431,7 @@ I changed my mind, I don\'t want it to be publicly visible anymore Sorry this picture is not interesting for an encyclopedia - Uploaded by myself on %1$s, used in %2$d article(s). + Uploaded by myself on %1$s, used in %2$d article(s) at least. Welcome to Commons!\n Upload your first media by tapping on the add button. @@ -876,4 +876,5 @@ Upload your first media by tapping on the add button. Show in Nearby Created and uploaded by: %1$s Created by %1$s and uploaded by %2$s + Nominated for Deletion diff --git a/app/src/test/kotlin/fr/free/nrw/commons/delete/ReasonBuilderTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/delete/ReasonBuilderTest.kt index e89a02dec..bd67475ee 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/delete/ReasonBuilderTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/delete/ReasonBuilderTest.kt @@ -5,13 +5,14 @@ import android.content.res.Resources import fr.free.nrw.commons.Media import fr.free.nrw.commons.R import fr.free.nrw.commons.auth.SessionManager +import fr.free.nrw.commons.fileusages.GlobalFileUsagesResponse import fr.free.nrw.commons.mwapi.OkHttpJsonApiClient -import fr.free.nrw.commons.profile.achievements.FeedbackResponse import fr.free.nrw.commons.profile.leaderboard.LeaderboardResponse import fr.free.nrw.commons.profile.leaderboard.UpdateAvatarResponse import fr.free.nrw.commons.utils.ViewUtilWrapper import io.reactivex.Observable import io.reactivex.Single +import kotlinx.coroutines.test.runTest import media import org.junit.Before import org.junit.Test @@ -58,16 +59,16 @@ class ReasonBuilderTest { PowerMockito.`when`(context?.getString(R.string.user_not_logged_in)) .thenReturn("Log-in expired. Please log in again.") - reasonBuilder!!.getReason(mock(Media::class.java), "test") + reasonBuilder!!.getReason(mock(Media::class.java), "test").test().await() verify(sessionManager, times(1))!!.forceLogin(any(Context::class.java)) } @Test - fun getReason() { + fun getReason() = runTest { `when`(sessionManager?.userName).thenReturn("Testuser") `when`(sessionManager?.doesAccountExist()).thenReturn(true) - `when`(okHttpJsonApiClient!!.getAchievements(anyString())) - .thenReturn(Single.just(mock(FeedbackResponse::class.java))) + `when`(okHttpJsonApiClient!!.getGlobalFileUsages(anyString(), anyInt())) + .thenReturn(mock(GlobalFileUsagesResponse::class.java)) `when`(okHttpJsonApiClient!!.getLeaderboard(anyString(), anyString(), anyString(), anyString(), anyString())) .thenReturn(Observable.just(mock(LeaderboardResponse::class.java))) `when`(okHttpJsonApiClient!!.setAvatar(anyString(), anyString())) @@ -75,8 +76,8 @@ class ReasonBuilderTest { val media = media(filename = "test_file", dateUploaded = Date()) - reasonBuilder!!.getReason(media, "test") + reasonBuilder!!.getReason(media, "test").test().await() verify(sessionManager, times(0))!!.forceLogin(any(Context::class.java)) - verify(okHttpJsonApiClient, times(1))!!.getAchievements(anyString()) + verify(okHttpJsonApiClient, times(1))!!.getGlobalFileUsages(anyString(), anyInt()) } } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index f38357b04..df4b7bb43 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -16,6 +16,7 @@ constraintlayout = "1.1.3" coordinates2country = "1.8" dexcount = "4.0.0" githubTripletPlay = "2.7.2" +kotlinxCoroutinesRx2 = "1.8.0" osmdroidAndroid = "6.1.17" testCore = "1.4.0" coreKtx = "1.9.0" @@ -127,6 +128,7 @@ dagger-compiler = { module = "com.google.dagger:dagger-compiler", version.ref = facebook-fresco = { module = "com.facebook.fresco:fresco", version.ref = "frescoVersion" } glide-compiler = { module = "com.github.bumptech.glide:compiler", version.ref = "glide" } glide = { module = "com.github.bumptech.glide:glide", version.ref = "glide" } +kotlinx-coroutines-rx2 = { module = "org.jetbrains.kotlinx:kotlinx-coroutines-rx2", version.ref = "kotlinxCoroutinesRx2" } photoview = { module = "com.github.chrisbanes:PhotoView", version.ref = "photoviewVersion" } # RxJava and Reactive Programming