Bugfix/p18 uploads (#3869)

* updated gradle plugin version * BugFix #3856 * Do not use preference for deciding acceptable lat long for nearby uploads, instead save the corresponding location in the Contribution via UploadItem * Marshall contribution's hasInvalidLocation * reset un-related changes * Fixed test cases * Minor code formatting and docs
commons-app · Jul 14, 2020 · e7d9315 · e7d9315
1 parent e471226
commit e7d9315
Show file tree

Hide file tree

Showing 10 changed files with 67 additions and 33 deletions.
diff --git a/app/src/main/java/fr/free/nrw/commons/contributions/Contribution.java b/app/src/main/java/fr/free/nrw/commons/contributions/Contribution.java
@@ -60,6 +60,19 @@ public class  Contribution extends Media {
     private String p18Value;
     public Uri contentProviderUri;
     public String dateCreatedSource;
+    public int hasInvalidLocation;
+
+    /**
+     * Set this true when ImageProcessor has said that the location is invalid
+     * @param hasInvalidLocation
+     */
+    public void setHasInvalidLocation(boolean hasInvalidLocation) {
+        this.hasInvalidLocation = hasInvalidLocation ? 1 : 0;
+    }
+
+    public boolean isHasInvalidLocation() {
+        return hasInvalidLocation == 1;
+    }
 
     public Contribution(Uri contentUri, String filename, Uri localUri, String imageUrl, Date dateCreated,
                         int state, long dataLength, Date dateUploaded, long transferred,
@@ -269,6 +282,7 @@ public void setContentProviderUri(Uri contentProviderUri) {
         this.contentProviderUri = contentProviderUri;
     }
 
+
     @Override
     public int describeContents() {
         return 0;
@@ -290,6 +304,7 @@ public void writeToParcel(Parcel dest, int flags) {
         dest.writeString(this.p18Value);
         dest.writeParcelable(this.contentProviderUri, flags);
         dest.writeString(this.dateCreatedSource);
+        dest.writeInt(this.hasInvalidLocation);
     }
 
     protected Contribution(Parcel in) {
@@ -307,6 +322,7 @@ protected Contribution(Parcel in) {
         this.p18Value = in.readString();
         this.contentProviderUri = in.readParcelable(Uri.class.getClassLoader());
         this.dateCreatedSource = in.readString();
+        this.hasInvalidLocation = in.readInt();
     }
 
     public static final Creator<Contribution> CREATOR = new Creator<Contribution>() {

diff --git a/app/src/main/java/fr/free/nrw/commons/db/AppDatabase.java b/app/src/main/java/fr/free/nrw/commons/db/AppDatabase.java
@@ -7,7 +7,7 @@
 import fr.free.nrw.commons.contributions.Contribution;
 import fr.free.nrw.commons.contributions.ContributionDao;
 
-@Database(entities = {Contribution.class}, version = 1, exportSchema = false)
+@Database(entities = {Contribution.class}, version = 2, exportSchema = false)
 @TypeConverters({Converters.class})
 abstract public class AppDatabase extends RoomDatabase {
     public abstract ContributionDao getContributionDao();

diff --git a/app/src/main/java/fr/free/nrw/commons/di/CommonsApplicationModule.java b/app/src/main/java/fr/free/nrw/commons/di/CommonsApplicationModule.java
@@ -7,6 +7,8 @@
 import android.view.inputmethod.InputMethodManager;
 import androidx.collection.LruCache;
 import androidx.room.Room;
+import androidx.room.migration.Migration;
+import androidx.sqlite.db.SupportSQLiteDatabase;
 import com.github.varunpant.quadtree.QuadTree;
 import com.google.gson.Gson;
 import dagger.Module;
@@ -52,6 +54,14 @@ public class CommonsApplicationModule {
     public static final String MAIN_THREAD="main_thread";
     private AppDatabase appDatabase;
 
+    static final Migration MIGRATION_1_2 = new Migration(1, 2) {
+        @Override
+        public void migrate(SupportSQLiteDatabase database) {
+            database.execSQL("ALTER TABLE contribution "
+                + " ADD COLUMN hasInvalidLocation INTEGER");
+        }
+    };
+
     public CommonsApplicationModule(Context applicationContext) {
         this.applicationContext = applicationContext;
     }
@@ -231,7 +241,9 @@ public QuadTree providesQuadTres() {
     @Provides
     @Singleton
     public AppDatabase provideAppDataBase() {
-        appDatabase=Room.databaseBuilder(applicationContext, AppDatabase.class, "commons_room.db").build();
+        appDatabase=Room.databaseBuilder(applicationContext, AppDatabase.class, "commons_room.db")
+            .addMigrations(MIGRATION_1_2)
+            .build();
         return appDatabase;
     }
 

diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadModel.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadModel.java
@@ -172,6 +172,7 @@ public Observable<Contribution> buildContributions() {
             contribution.setSource(item.source);
             contribution.setContentProviderUri(item.mediaUri);
             contribution.setDateUploaded(new Date());
+            contribution.setHasInvalidLocation(item.hasInvalidLocation);
 
             Timber.d("Created timestamp while building contribution is %s, %s",
                     item.getCreatedTimestamp(),
@@ -221,6 +222,7 @@ public static class UploadItem {
         private final String mimeType;
         private final String source;
         private ImageCoordinates gpsCoords;
+        private boolean hasInvalidLocation;
 
         public void setGpsCoords(ImageCoordinates gpsCoords) {
             this.gpsCoords = gpsCoords;
@@ -326,6 +328,10 @@ public boolean equals(@Nullable Object obj) {
         public int hashCode() {
             return mediaUri.hashCode();
         }
+
+        public void setHasInvalidLocation(boolean hasInvalidLocation) {
+            this.hasInvalidLocation=hasInvalidLocation;
+        }
     }
 
 }
diff --git a/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java b/app/src/main/java/fr/free/nrw/commons/upload/UploadService.java
@@ -282,7 +282,15 @@ private void uploadContribution(Contribution contribution) {
                         Timber.d("Contribution upload success. Initiating Wikidata edit for"
                                 + " entity id %s if necessary (if P18 is null). P18 value is %s",
                                 contribution.getWikiDataEntityId(), contribution.getP18Value());
-                        wikidataEditService.createClaimWithLogging(contribution.getWikiDataEntityId(), contribution.getWikiItemName(), canonicalFilename, contribution.getP18Value());
+                        if (!contribution.isHasInvalidLocation()) {
+                            wikidataEditService
+                                .createClaimWithLogging(contribution.getWikiDataEntityId(),
+                                    contribution.getWikiItemName(), canonicalFilename,
+                                    contribution.getP18Value());
+                        } else {
+                            Timber
+                                .d("Image location and nearby place location mismatched, so Wikidata item won't be edited");
+                        }
                         contribution.setFilename(canonicalFilename);
                         contribution.setImageUrl(uploadResult.getImageinfo().getOriginalUrl());
                         contribution.setState(Contribution.STATE_COMPLETED);

diff --git a/app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaPresenter.java b/app/src/main/java/fr/free/nrw/commons/upload/mediaDetails/UploadMediaPresenter.java
@@ -120,7 +120,7 @@ public void verifyImageQuality(UploadItem uploadItem) {
                 .observeOn(mainThreadScheduler)
                 .subscribe(imageResult -> {
                             view.showProgress(false);
-                            handleImageResult(imageResult);
+                        handleImageResult(imageResult, uploadItem);
                         },
                         throwable -> {
                             view.showProgress(false);
@@ -165,26 +165,31 @@ public void useSimilarPictureCoordinates(ImageCoordinates imageCoordinates, int
   /**
      * handles image quality verifications
      *
-     * @param imageResult
-     */
-    public void handleImageResult(Integer imageResult) {
+   * @param imageResult
+   * @param uploadItem
+   */
+    public void handleImageResult(Integer imageResult,
+        UploadItem uploadItem) {
         if (imageResult == IMAGE_KEEP || imageResult == IMAGE_OK) {
             view.onImageValidationSuccess();
+            uploadItem.setHasInvalidLocation(false);
         } else {
-            handleBadImage(imageResult);
+            handleBadImage(imageResult, uploadItem);
         }
     }
 
     /**
      * Handle  images, say empty title, duplicate file name, bad picture(in all other cases)
      *
      * @param errorCode
+     * @param uploadItem
      */
-    public void handleBadImage(Integer errorCode) {
+    public void handleBadImage(Integer errorCode,
+        UploadItem uploadItem) {
         Timber.d("Handle bad picture with error code %d", errorCode);
         if (errorCode
-                >= 8) { // If location of image and nearby does not match, then set shared preferences to disable wikidata edits
-            repository.saveValue("Picture_Has_Correct_Location", false);
+            >= 8) { // If location of image and nearby does not match, then set shared preferences to disable wikidata edits
+            uploadItem.setHasInvalidLocation(true);
         }
 
         switch (errorCode) {

diff --git a/app/src/main/java/fr/free/nrw/commons/wikidata/WikidataEditService.java b/app/src/main/java/fr/free/nrw/commons/wikidata/WikidataEditService.java
@@ -61,11 +61,6 @@ public void createClaimWithLogging(String wikidataEntityId, String wikiItemName,
             return;
         }
 
-        if (!(directKvStore.getBoolean("Picture_Has_Correct_Location", true))) {
-            Timber.d("Image location and nearby place location mismatched, so Wikidata item won't be edited");
-            return;
-        }
-
         if (p18Value != null && !p18Value.trim().isEmpty()) {
             Timber.d("Skipping creation of claim as p18Value is not empty, we won't override existing image");
             return;

diff --git a/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaPresenterTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/upload/UploadMediaPresenterTest.kt
@@ -109,20 +109,20 @@ class UploadMediaPresenterTest {
     @Test
     fun handleImageResult() {
         //Positive case test
-        uploadMediaPresenter.handleImageResult(IMAGE_KEEP)
+        uploadMediaPresenter.handleImageResult(IMAGE_KEEP, uploadItem)
         verify(view).onImageValidationSuccess()
 
         //Duplicate file name
-        uploadMediaPresenter.handleImageResult(FILE_NAME_EXISTS)
+        uploadMediaPresenter.handleImageResult(FILE_NAME_EXISTS, uploadItem)
         verify(view).showDuplicatePicturePopup()
 
         //Empty Title test
-        uploadMediaPresenter.handleImageResult(EMPTY_TITLE)
+        uploadMediaPresenter.handleImageResult(EMPTY_TITLE, uploadItem)
         verify(view).showMessage(ArgumentMatchers.anyInt(), ArgumentMatchers.anyInt())
 
         //Bad Picture test
         //Empty Title test
-        uploadMediaPresenter.handleImageResult(-7)
+        uploadMediaPresenter.handleImageResult(-7, uploadItem)
         verify(view).showBadImagePopup(ArgumentMatchers.anyInt())
 
     }
@@ -158,8 +158,8 @@ class UploadMediaPresenterTest {
      */
     @Test
     fun handleBadImageBaseTestInvalidLocation() {
-        uploadMediaPresenter.handleBadImage(8)
-        verify(repository).saveValue(ArgumentMatchers.anyString(), eq(false))
+        uploadMediaPresenter.handleBadImage(8, uploadItem)
+        verify(uploadItem).setHasInvalidLocation(true)
         verify(view).showBadImagePopup(8)
     }
 
@@ -168,7 +168,7 @@ class UploadMediaPresenterTest {
      */
     @Test
     fun handleBadImageBaseTestEmptyTitle() {
-        uploadMediaPresenter.handleBadImage(-3)
+        uploadMediaPresenter.handleBadImage(-3, uploadItem)
         verify(view).showMessage(ArgumentMatchers.anyInt(), ArgumentMatchers.anyInt())
     }
 
@@ -177,7 +177,7 @@ class UploadMediaPresenterTest {
      */
     @Test
     fun handleBadImageBaseTestFileNameExists() {
-        uploadMediaPresenter.handleBadImage(-4)
+        uploadMediaPresenter.handleBadImage(-4, uploadItem)
         verify(view).showDuplicatePicturePopup()
     }
 

diff --git a/app/src/test/kotlin/fr/free/nrw/commons/wikidata/WikidataEditServiceTest.kt b/app/src/test/kotlin/fr/free/nrw/commons/wikidata/WikidataEditServiceTest.kt
@@ -50,14 +50,6 @@ class WikidataEditServiceTest {
         verifyZeroInteractions(wikidataClient!!)
     }
 
-    @Test
-    fun noClaimsWhenLocationIsNotCorrect() {
-        `when`(directKvStore!!.getBoolean("Picture_Has_Correct_Location", true))
-                .thenReturn(false)
-        wikidataEditService!!.createClaimWithLogging("Q1", "","Test.jpg","")
-        verifyZeroInteractions(wikidataClient!!)
-    }
-
     @Test
     fun createClaimWithLogging() {
         `when`(directKvStore!!.getBoolean("Picture_Has_Correct_Location", true))

diff --git a/build.gradle b/build.gradle
@@ -7,7 +7,7 @@ buildscript {
         maven { url "https://plugins.gradle.org/m2/" }
     }
     dependencies {
-        classpath 'com.android.tools.build:gradle:3.6.1'
+        classpath 'com.android.tools.build:gradle:3.6.3'
         classpath "com.hiya:jacoco-android:0.2"
         classpath 'com.getkeepsafe.dexcount:dexcount-gradle-plugin:0.8.2'
         classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$KOTLIN_VERSION"