Do not use erroneous thumb url util function (#2901)

maskaravivek · ashishkumar468 · commit a003e9706fc8 · 2019-04-20T15:03:52.000+05:30
* Do not use erroneous thumb url util function

* Remove unused code

* Fix image loading for peer review

* Clear disposable
diff --git a/app/src/main/java/fr/free/nrw/commons/Media.java b/app/src/main/java/fr/free/nrw/commons/Media.java
@@ -173,9 +173,6 @@ public Uri getLocalUri() {
      */
     @Nullable
     public String getImageUrl() {
-        if (imageUrl == null && this.getFilename() != null) {
-            imageUrl = Utils.makeThumbBaseUrl(this.getFilename());
-        }
         return imageUrl;
     }
 
diff --git a/app/src/main/java/fr/free/nrw/commons/Utils.java b/app/src/main/java/fr/free/nrw/commons/Utils.java
@@ -9,11 +9,8 @@
 import android.view.View;
 import android.widget.Toast;
 
-import org.apache.commons.codec.binary.Hex;
-import org.apache.commons.codec.digest.DigestUtils;
 import org.wikipedia.dataclient.WikiSite;
 import org.wikipedia.page.PageTitle;
-import org.wikipedia.util.UriUtil;
 
 import java.util.Locale;
 import java.util.regex.Pattern;
@@ -34,18 +31,6 @@ public static PageTitle getPageTitle(@NonNull String title) {
         return new PageTitle(title, new WikiSite(BuildConfig.COMMONS_URL));
     }
 
-    /**
-     * Creates an URL for thumbnail
-     *
-     * @param filename Thumbnail file name
-     * @return URL of thumbnail
-     */
-    public static String makeThumbBaseUrl(@NonNull String filename) {
-        String name = getPageTitle(filename).getPrefixedText();
-        String sha = new String(Hex.encodeHex(DigestUtils.md5(name)));
-        return String.format("%s/%s/%s/%s", BuildConfig.IMAGE_URL_BASE, sha.substring(0, 1), sha.substring(0, 2), UriUtil.encodeURL(name));
-    }
-
     /**
      * Generates licence name with given ID
      * @param license License ID
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsSyncAdapter.java b/app/src/main/java/fr/free/nrw/commons/contributions/ContributionsSyncAdapter.java
@@ -118,9 +118,8 @@ public void onPerformSync(Account account, Bundle bundle, String authority,
                     Timber.d("Skipping %s", filename);
                     continue;
                 }
-                String thumbUrl = Utils.makeThumbBaseUrl(filename);
                 Date dateUpdated = image.getDateUpdated();
-                Contribution contrib = new Contribution(null, thumbUrl, filename,
+                Contribution contrib = new Contribution(null, null, filename,
                         "", -1, dateUpdated, dateUpdated, user,
                         "", "");
                 contrib.setState(STATE_COMPLETED);
diff --git a/app/src/main/java/fr/free/nrw/commons/review/ReviewActivity.java b/app/src/main/java/fr/free/nrw/commons/review/ReviewActivity.java
@@ -18,27 +18,20 @@
 
 import javax.inject.Inject;
 
-import androidx.appcompat.app.AlertDialog;
 import androidx.appcompat.widget.Toolbar;
 import androidx.drawerlayout.widget.DrawerLayout;
 import butterknife.BindView;
 import butterknife.ButterKnife;
-import butterknife.OnClick;
 import fr.free.nrw.commons.Media;
 import fr.free.nrw.commons.R;
-import fr.free.nrw.commons.Utils;
-import fr.free.nrw.commons.achievements.AchievementsActivity;
 import fr.free.nrw.commons.auth.AuthenticatedActivity;
 import fr.free.nrw.commons.delete.DeleteHelper;
 import fr.free.nrw.commons.mwapi.MediaWikiApi;
 import fr.free.nrw.commons.utils.DialogUtil;
-import fr.free.nrw.commons.utils.MediaDataExtractorUtil;
 import fr.free.nrw.commons.utils.ViewUtil;
 import io.reactivex.android.schedulers.AndroidSchedulers;
 import io.reactivex.disposables.CompositeDisposable;
-import io.reactivex.disposables.Disposable;
 import io.reactivex.schedulers.Schedulers;
-import timber.log.Timber;
 
 public class ReviewActivity extends AuthenticatedActivity {
 
@@ -125,45 +118,34 @@ public boolean runRandomizer() {
         progressBar.setVisibility(View.VISIBLE);
         reviewPager.setCurrentItem(0);
         compositeDisposable.add(reviewHelper.getRandomMedia()
-                .map(Media::getFilename)
                 .subscribeOn(Schedulers.io())
                 .observeOn(AndroidSchedulers.mainThread())
                 .subscribe(this::updateImage));
         return true;
     }
 
     @SuppressLint("CheckResult")
-    private void updateImage(String fileName) {
+    private void updateImage(Media media) {
+        String fileName = media.getFilename();
         if (fileName.length() == 0) {
             ViewUtil.showShortSnackbar(drawerLayout, R.string.error_review);
             return;
         }
-        simpleDraweeView.setImageURI(Utils.makeThumbBaseUrl(fileName));
+
+        simpleDraweeView.setImageURI(media.getImageUrl());
+
         reviewController.onImageRefreshed(fileName); //file name is updated
-        compositeDisposable.add(reviewHelper.getFirstRevisionOfFile("File:" + fileName)
+        compositeDisposable.add(reviewHelper.getFirstRevisionOfFile(fileName)
                 .subscribeOn(Schedulers.io())
                 .observeOn(AndroidSchedulers.mainThread())
                 .subscribe(revision -> {
                     reviewController.firstRevision = revision;
                     reviewPagerAdapter.updateFileInformation(fileName);
-                    ((TextView) imageCaption).setText(fileName + " is uploaded by: " + revision.getUser());
+                    imageCaption.setText(fileName + " is uploaded by: " + revision.getUser());
                     progressBar.setVisibility(View.GONE);
                 }));
         reviewPager.setCurrentItem(0);
-
-        Disposable disposable = mwApi.fetchMediaByFilename("File:" + fileName)
-                .subscribeOn(Schedulers.io())
-                .observeOn(AndroidSchedulers.mainThread())
-                .subscribe(mediaResult -> {
-                    ArrayList<String> categories = MediaDataExtractorUtil.extractCategories(mediaResult.getWikiSource());
-                    updateCategories(categories);
-                }, this::categoryFetchError);
-        compositeDisposable.add(disposable);
-    }
-
-    private void categoryFetchError(Throwable throwable) {
-        Timber.e(throwable, "Error fetching categories");
-        ViewUtil.showShortSnackbar(drawerLayout, R.string.error_review_categories);
+        updateCategories(media.getCategories());
     }
 
     private void updateCategories(ArrayList<String> categories) {
@@ -180,6 +162,12 @@ public void swipeToNext() {
         }
     }
 
+    @Override
+    public void onDestroy() {
+        super.onDestroy();
+        compositeDisposable.clear();
+    }
+
     public void showSkipImageInfo(){
         DialogUtil.showAlertDialog(ReviewActivity.this,
                 getString(R.string.skip_image),
diff --git a/app/src/main/java/fr/free/nrw/commons/review/ReviewHelper.java b/app/src/main/java/fr/free/nrw/commons/review/ReviewHelper.java
@@ -31,23 +31,28 @@ public ReviewHelper(OkHttpJsonApiClient okHttpJsonApiClient, MediaWikiApi mediaW
         this.mediaWikiApi = mediaWikiApi;
     }
 
+    Single<Media> getRandomMedia() {
+        return getRandomFileChange()
+                .flatMap(fileName -> okHttpJsonApiClient.getMedia(fileName, false));
+    }
+
     /**
-     * Gets a random media file for review.
+     * 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
      *
      * @return
      */
-    Single<Media> getRandomMedia() {
+    private Single<String> getRandomFileChange() {
         return okHttpJsonApiClient.getRecentFileChanges()
                 .map(this::findImageInRecentChanges)
                 .flatMap(title -> mediaWikiApi.pageExists("Commons:Deletion_requests/" + title)
                         .map(pageExists -> new Pair<>(title, pageExists)))
                 .map((Pair<String, Boolean> pair) -> {
                     if (!pair.second) {
-                        return new Media(pair.first.replace("File:", ""));
+                        return pair.first;
                     }
                     throw new Exception("Already nominated for deletion");
                 }).retry(MAX_RANDOM_TRIES);
diff --git a/app/src/main/java/fr/free/nrw/commons/utils/MediaDataExtractorUtil.java b/app/src/main/java/fr/free/nrw/commons/utils/MediaDataExtractorUtil.java
@@ -8,26 +8,6 @@
 import java.util.regex.Pattern;
 
 public class MediaDataExtractorUtil {
-
-    /**
-     * We could fetch all category links from API, but we actually only want the ones
-     * directly in the page source so they're editable. In the future this may change.
-     *
-     * @param source wikitext source code
-     */
-    public static ArrayList<String> extractCategories(String source) {
-        ArrayList<String> categories = new ArrayList<>();
-        Pattern regex = Pattern.compile("\\[\\[\\s*Category\\s*:([^]]*)\\s*\\]\\]", Pattern.CASE_INSENSITIVE);
-        Matcher matcher = regex.matcher(source);
-        while (matcher.find()) {
-            String cat = matcher.group(1).trim();
-            categories.add(cat);
-        }
-
-        return categories;
-    }
-
-
     /**
      * Extracts a list of categories from | separated category string
      *
diff --git a/app/src/test/kotlin/fr/free/nrw/commons/contributions/ContributionDaoTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/contributions/ContributionDaoTest.kt
@@ -180,23 +180,6 @@ class ContributionDaoTest {
         }
     }
 
-    @Test
-    fun saveNewContribution_nullableImageUrlUsesFileAsBackup() {
-        whenever(client.insert(isA(), isA())).thenReturn(contentUri)
-        val contribution = createContribution(true, null, null, null, "filePath")
-
-        testObject.save(contribution)
-
-        assertEquals(contentUri, contribution.contentUri)
-        verify(client).insert(eq(BASE_URI), captor.capture())
-        captor.firstValue.let {
-            // Nullable fields are absent if null
-            assertFalse(it.containsKey(Table.COLUMN_LOCAL_URI))
-            assertFalse(it.containsKey(Table.COLUMN_UPLOADED))
-            assertEquals(Utils.makeThumbBaseUrl("filePath"), it.getAsString(Table.COLUMN_IMAGE_URL))
-        }
-    }
-
     @Test
     fun saveNewContribution_nullableFieldsAreNonNull() {
         whenever(client.insert(isA(), isA())).thenReturn(contentUri)
diff --git a/app/src/test/kotlin/fr/free/nrw/commons/review/ReviewHelperTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/review/ReviewHelperTest.kt
@@ -10,8 +10,7 @@ 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.mock
+import org.mockito.Mockito.*
 import org.mockito.MockitoAnnotations
 import org.wikipedia.dataclient.mwapi.MwQueryPage
 import org.wikipedia.dataclient.mwapi.RecentChange
@@ -51,6 +50,10 @@ class ReviewHelperTest {
 
         `when`(mediaWikiApi?.pageExists(ArgumentMatchers.anyString()))
                 .thenReturn(Single.just(false))
+        `when`(okHttpJsonApiClient?.getMedia(ArgumentMatchers.anyString(), ArgumentMatchers.anyBoolean()))
+                .thenReturn(Single.just(mock(Media::class.java)))
+
+
         val randomMedia = reviewHelper?.randomMedia?.blockingGet()
 
         assertTrue(randomMedia is Media)

Original file line number	Diff line number	Diff line change
`@@ -173,9 +173,6 @@ public Uri getLocalUri() {`
`173`	`173`	`*/`
`174`	`174`	`@Nullable`
`175`	`175`	`public String getImageUrl() {`
`176`		`- if (imageUrl == null && this.getFilename() != null) {`
`177`		`- imageUrl = Utils.makeThumbBaseUrl(this.getFilename());`
`178`		`- }`
`179`	`176`	`return imageUrl;`
`180`	`177`	`}`
`181`	`178`