Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfix/p18 uploads #3869

Merged
merged 6 commits into from
Jul 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -269,6 +282,7 @@ public void setContentProviderUri(Uri contentProviderUri) {
this.contentProviderUri = contentProviderUri;
}


@Override
public int describeContents() {
return 0;
Expand All @@ -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) {
Expand All @@ -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>() {
Expand Down
2 changes: 1 addition & 1 deletion app/src/main/java/fr/free/nrw/commons/db/AppDatabase.java
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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;
}

Expand Down
6 changes: 6 additions & 0 deletions app/src/main/java/fr/free/nrw/commons/upload/UploadModel.java
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -326,6 +328,10 @@ public boolean equals(@Nullable Object obj) {
public int hashCode() {
return mediaUri.hashCode();
}

public void setHasInvalidLocation(boolean hasInvalidLocation) {
this.hasInvalidLocation=hasInvalidLocation;
}
}

}
10 changes: 9 additions & 1 deletion app/src/main/java/fr/free/nrw/commons/upload/UploadService.java
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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())

}
Expand Down Expand Up @@ -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)
}

Expand All @@ -168,7 +168,7 @@ class UploadMediaPresenterTest {
*/
@Test
fun handleBadImageBaseTestEmptyTitle() {
uploadMediaPresenter.handleBadImage(-3)
uploadMediaPresenter.handleBadImage(-3, uploadItem)
verify(view).showMessage(ArgumentMatchers.anyInt(), ArgumentMatchers.anyInt())
}

Expand All @@ -177,7 +177,7 @@ class UploadMediaPresenterTest {
*/
@Test
fun handleBadImageBaseTestFileNameExists() {
uploadMediaPresenter.handleBadImage(-4)
uploadMediaPresenter.handleBadImage(-4, uploadItem)
verify(view).showDuplicatePicturePopup()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
2 changes: 1 addition & 1 deletion build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down