From 5dc45a53d35baeb93495c3190886ea969fe31351 Mon Sep 17 00:00:00 2001 From: Vivek Maskara Date: Wed, 26 Jun 2019 20:10:49 +0530 Subject: [PATCH] Fix bugs in peer review flow (#3039) * Fix bugs in peer review flow * Fix tests * Bug fixes * Fix remaining issues with peer review --- .../free/nrw/commons/delete/DeleteHelper.java | 34 +++++-- .../nrw/commons/review/ReviewActivity.java | 18 ++-- .../nrw/commons/review/ReviewController.java | 89 ++++++++--------- .../free/nrw/commons/review/ReviewHelper.java | 6 +- .../commons/review/ReviewImageFragment.java | 96 +++++++++++-------- .../commons/review/ReviewPagerAdapter.java | 15 ++- app/src/main/res/values/strings.xml | 1 + .../nrw/commons/review/ReviewHelperTest.kt | 6 +- 8 files changed, 140 insertions(+), 125 deletions(-) diff --git a/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java b/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java index 1b72833dd..08b13acfa 100644 --- a/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java +++ b/app/src/main/java/fr/free/nrw/commons/delete/DeleteHelper.java @@ -1,9 +1,12 @@ package fr.free.nrw.commons.delete; +import android.annotation.SuppressLint; import android.content.Context; import android.content.Intent; import android.net.Uri; +import androidx.appcompat.app.AlertDialog; + import java.text.SimpleDateFormat; import java.util.ArrayList; import java.util.Calendar; @@ -12,17 +15,16 @@ import java.util.Locale; import javax.inject.Inject; import javax.inject.Singleton; -import androidx.appcompat.app.AlertDialog; import fr.free.nrw.commons.BuildConfig; import fr.free.nrw.commons.Media; import fr.free.nrw.commons.auth.SessionManager; import fr.free.nrw.commons.mwapi.MediaWikiApi; import fr.free.nrw.commons.notification.NotificationHelper; -import fr.free.nrw.commons.review.ReviewActivity; -import fr.free.nrw.commons.utils.ViewUtil; import fr.free.nrw.commons.review.ReviewController; import fr.free.nrw.commons.utils.ViewUtilWrapper; import io.reactivex.Single; +import io.reactivex.android.schedulers.AndroidSchedulers; +import io.reactivex.schedulers.Schedulers; import timber.log.Timber; import static fr.free.nrw.commons.notification.NotificationHelper.NOTIFICATION_DELETE; @@ -57,7 +59,6 @@ public class DeleteHelper { */ public Single makeDeletion(Context context, Media media, String reason) { viewUtil.showShortToast(context, "Trying to nominate " + media.getDisplayTitle() + " for deletion"); - return Single.fromCallable(() -> delete(media, reason)) .flatMap(result -> Single.fromCallable(() -> showDeletionNotification(context, media, result))); @@ -99,7 +100,8 @@ public class DeleteHelper { try { editToken = mwApi.getEditToken(); - if (editToken.equals("+\\")) { + + if(editToken == null) { return false; } @@ -143,7 +145,12 @@ public class DeleteHelper { * @param question * @param problem */ - public void askReasonAndExecute(Media media, Context context, String question, ReviewController.DeleteReason problem) { + @SuppressLint("CheckResult") + public void askReasonAndExecute(Media media, + Context context, + String question, + ReviewController.DeleteReason problem, + ReviewController.ReviewCallback reviewCallback) { AlertDialog.Builder alert = new AlertDialog.Builder(context); alert.setTitle(question); @@ -183,12 +190,19 @@ public class DeleteHelper { } } - ((ReviewActivity) context).reviewController.swipeToNext(); - ((ReviewActivity) context).runRandomizer(); + makeDeletion(context, media, reason) + .subscribeOn(Schedulers.io()) + .observeOn(AndroidSchedulers.mainThread()) + .subscribe(aBoolean -> { + if (aBoolean) { + reviewCallback.onSuccess(); + } else { + reviewCallback.onFailure(); + } + }); - makeDeletion(context, media, reason); }); - alert.setNegativeButton("Cancel", null); + alert.setNegativeButton("Cancel", (dialog, which) -> reviewCallback.onFailure()); AlertDialog d = alert.create(); d.show(); } 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 index cbb9848ca..48cde85db 100644 --- a/app/src/main/java/fr/free/nrw/commons/review/ReviewActivity.java +++ b/app/src/main/java/fr/free/nrw/commons/review/ReviewActivity.java @@ -20,8 +20,6 @@ import com.facebook.drawee.view.SimpleDraweeView; import com.google.android.material.navigation.NavigationView; import com.viewpagerindicator.CirclePageIndicator; -import java.util.ArrayList; - import javax.inject.Inject; import butterknife.BindView; @@ -92,6 +90,9 @@ public class ReviewActivity extends AuthenticatedActivity { private CompositeDisposable compositeDisposable = new CompositeDisposable(); + public Media getMedia() { + return media; + } @Override protected void onAuthCookieAcquired(String authCookie) { @@ -163,23 +164,18 @@ public class ReviewActivity extends AuthenticatedActivity { simpleDraweeView.setImageURI(media.getImageUrl()); - reviewController.onImageRefreshed(fileName); //file name is updated + reviewController.onImageRefreshed(media); //file name is updated compositeDisposable.add(reviewHelper.getFirstRevisionOfFile(fileName) .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) .subscribe(revision -> { reviewController.firstRevision = revision; - reviewPagerAdapter.updateFileInformation(fileName); - imageCaption.setText(fileName + " is uploaded by: " + revision.getUser()); + reviewPagerAdapter.updateFileInformation(); + String caption = String.format(getString(R.string.review_is_uploaded_by), fileName, revision.getUser()); + imageCaption.setText(caption); progressBar.setVisibility(View.GONE); })); reviewPager.setCurrentItem(0); - updateCategories(media.getCategories()); - } - - private void updateCategories(ArrayList categories) { - reviewController.onCategoriesRefreshed(categories); - reviewPagerAdapter.updateCategories(); } public void swipeToNext() { diff --git a/app/src/main/java/fr/free/nrw/commons/review/ReviewController.java b/app/src/main/java/fr/free/nrw/commons/review/ReviewController.java index b4bf05e2c..c98c7ed3e 100644 --- a/app/src/main/java/fr/free/nrw/commons/review/ReviewController.java +++ b/app/src/main/java/fr/free/nrw/commons/review/ReviewController.java @@ -7,17 +7,16 @@ import android.content.Context; import android.view.Gravity; import android.widget.Toast; -import org.wikipedia.dataclient.mwapi.MwQueryPage; +import androidx.annotation.NonNull; +import androidx.annotation.Nullable; +import androidx.core.app.NotificationCompat; -import java.util.ArrayList; +import org.wikipedia.dataclient.mwapi.MwQueryPage; import javax.inject.Inject; import javax.inject.Singleton; -import androidx.annotation.NonNull; -import androidx.annotation.Nullable; -import androidx.core.app.NotificationCompat; -import androidx.viewpager.widget.ViewPager; +import fr.free.nrw.commons.CommonsApplication; import fr.free.nrw.commons.Media; import fr.free.nrw.commons.R; import fr.free.nrw.commons.auth.SessionManager; @@ -31,13 +30,11 @@ import timber.log.Timber; @Singleton public class ReviewController { - private String fileName; - public static final int NOTIFICATION_SEND_THANK = 0x102; - protected static ArrayList categories; - public static final int NOTIFICATION_CHECK_CATEGORY = 0x101; + private static final int NOTIFICATION_SEND_THANK = 0x102; + private static final int NOTIFICATION_CHECK_CATEGORY = 0x101; private final DeleteHelper deleteHelper; @Nullable - public MwQueryPage.Revision firstRevision; // TODO: maybe we can expand this class to include fileName + MwQueryPage.Revision firstRevision; // TODO: maybe we can expand this class to include fileName @Inject MediaWikiApi mwApi; @Inject @@ -46,32 +43,19 @@ public class ReviewController { private NotificationCompat.Builder notificationBuilder; private Media media; - private ViewPager viewPager; - private ReviewActivity reviewActivity; - ReviewController(DeleteHelper deleteHelper, Context context) { this.deleteHelper = deleteHelper; - reviewActivity = (ReviewActivity) context; - viewPager = ((ReviewActivity) context).reviewPager; + CommonsApplication.createNotificationChannel(context.getApplicationContext()); + notificationManager = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE); + notificationBuilder = new NotificationCompat.Builder(context, CommonsApplication.NOTIFICATION_CHANNEL_ID_ALL); } - public void onImageRefreshed(String fileName) { - this.fileName = fileName; - media = new Media("File:" + fileName); - ReviewController.categories = new ArrayList<>(); + void onImageRefreshed(Media media) { + this.media = media; } - public void onCategoriesRefreshed(ArrayList categories) { - ReviewController.categories = categories; - } - - public void swipeToNext() { - int nextPos = viewPager.getCurrentItem() + 1; - if (nextPos <= 3) { - viewPager.setCurrentItem(nextPos); - } else { - reviewActivity.runRandomizer(); - } + public Media getMedia() { + return media; } public enum DeleteReason { @@ -79,30 +63,32 @@ public class ReviewController { COPYRIGHT_VIOLATION } - public void reportSpam(@NonNull Activity activity) { - deleteHelper.askReasonAndExecute(new Media("File:" + fileName), + void reportSpam(@NonNull Activity activity, ReviewCallback reviewCallback) { + Timber.d("Report spam for %s", media.getFilename()); + deleteHelper.askReasonAndExecute(media, activity, activity.getResources().getString(R.string.review_spam_report_question), - DeleteReason.SPAM); + DeleteReason.SPAM, + reviewCallback); } - public void reportPossibleCopyRightViolation(@NonNull Activity activity) { - deleteHelper.askReasonAndExecute(new Media("File:" + fileName), + void reportPossibleCopyRightViolation(@NonNull Activity activity, ReviewCallback reviewCallback) { + Timber.d("Report spam for %s", media.getFilename()); + deleteHelper.askReasonAndExecute(media, activity, activity.getResources().getString(R.string.review_c_violation_report_question), - DeleteReason.COPYRIGHT_VIOLATION); + DeleteReason.COPYRIGHT_VIOLATION, + reviewCallback); } @SuppressLint("CheckResult") - public void reportWrongCategory(@NonNull Activity activity) { + void reportWrongCategory(@NonNull Activity activity, ReviewCallback reviewCallback) { Context context = activity.getApplicationContext(); ApplicationlessInjection .getInstance(context) .getCommonsApplicationComponent() .inject(this); - notificationManager = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE); - notificationBuilder = new NotificationCompat.Builder(context); Toast toast = new Toast(context); toast.setGravity(Gravity.CENTER, 0, 0); toast = Toast.makeText(context, context.getString(R.string.check_category_toast, media.getDisplayTitle()), Toast.LENGTH_SHORT); @@ -120,7 +106,8 @@ public class ReviewController { try { editToken = mwApi.getEditToken(); - if (editToken.equals("+\\")) { + + if (editToken == null) { return false; } publishProgress(context, 1); @@ -136,15 +123,17 @@ public class ReviewController { .subscribeOn(Schedulers.io()) .observeOn(AndroidSchedulers.mainThread()) .subscribe((result) -> { - String message = ""; - String title = ""; + String message; + String title; if (result) { title = context.getString(R.string.check_category_success_title); message = context.getString(R.string.check_category_success_message, media.getDisplayTitle()); + reviewCallback.onSuccess(); } else { title = context.getString(R.string.check_category_failure_title); message = context.getString(R.string.check_category_failure_message, media.getDisplayTitle()); + reviewCallback.onFailure(); } notificationBuilder.setDefaults(NotificationCompat.DEFAULT_ALL) @@ -176,15 +165,13 @@ public class ReviewController { notificationManager.notify(NOTIFICATION_CHECK_CATEGORY, notificationBuilder.build()); } - @SuppressLint("CheckResult") - public void sendThanks(@NonNull Activity activity) { + @SuppressLint({"CheckResult", "StringFormatInvalid"}) + void sendThanks(@NonNull Activity activity) { Context context = activity.getApplicationContext(); ApplicationlessInjection .getInstance(context) .getCommonsApplicationComponent() .inject(this); - notificationManager = (NotificationManager) context.getSystemService(Context.NOTIFICATION_SERVICE); - notificationBuilder = new NotificationCompat.Builder(context); Toast toast = new Toast(context); toast.setGravity(Gravity.CENTER, 0, 0); toast = Toast.makeText(context, context.getString(R.string.send_thank_toast, media.getDisplayTitle()), Toast.LENGTH_SHORT); @@ -200,7 +187,7 @@ public class ReviewController { try { editToken = mwApi.getEditToken(); - if (editToken.equals("+\\")) { + if (editToken == null) { return false; } publishProgress(context, 1); @@ -238,4 +225,10 @@ public class ReviewController { }, Timber::e); } + + public interface ReviewCallback { + void onSuccess(); + + void onFailure(); + } } 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 index 30903a89a..03f67bfcf 100644 --- a/app/src/main/java/fr/free/nrw/commons/review/ReviewHelper.java +++ b/app/src/main/java/fr/free/nrw/commons/review/ReviewHelper.java @@ -6,6 +6,7 @@ import org.wikipedia.dataclient.mwapi.MwQueryPage; import org.wikipedia.dataclient.mwapi.RecentChange; import org.wikipedia.util.DateUtil; +import java.util.Collections; import java.util.Date; import java.util.Random; @@ -53,7 +54,7 @@ public class ReviewHelper { return reviewInterface.getRecentChanges(rcStart) .map(mwQueryResponse -> mwQueryResponse.query().getRecentChanges()) .map(recentChanges -> { - //Collections.shuffle(recentChanges); + Collections.shuffle(recentChanges); return recentChanges; }) .flatMapIterable(changes -> changes) @@ -113,7 +114,8 @@ public class ReviewHelper { * @return */ private boolean isChangeReviewable(RecentChange recentChange) { - if (recentChange.getType().equals("log") && !(recentChange.getOldRevisionId() == 0)) { + if ((recentChange.getType().equals("log") && !(recentChange.getOldRevisionId() == 0)) + || !recentChange.getType().equals("log")) { return false; } diff --git a/app/src/main/java/fr/free/nrw/commons/review/ReviewImageFragment.java b/app/src/main/java/fr/free/nrw/commons/review/ReviewImageFragment.java index 5f4e16d29..5952956ed 100644 --- a/app/src/main/java/fr/free/nrw/commons/review/ReviewImageFragment.java +++ b/app/src/main/java/fr/free/nrw/commons/review/ReviewImageFragment.java @@ -10,26 +10,26 @@ import android.view.ViewGroup; import android.widget.Button; import android.widget.ProgressBar; import android.widget.TextView; + +import java.util.List; + 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.di.CommonsDaggerSupportFragment; -import org.wikipedia.dataclient.mwapi.MwQueryPage; public class ReviewImageFragment extends CommonsDaggerSupportFragment { - public static final int SPAM = 0; - public static final int COPYRIGHT = 1; - public static final int CATEGORY = 2; - public static final int THANKS = 3; + static final int CATEGORY = 2; + private static final int SPAM = 0; + private static final int COPYRIGHT = 1; + private static final int THANKS = 3; private int position; - private String fileName; - private String catString; public ProgressBar progressBar; - private MwQueryPage.Revision revision; @BindView(R.id.tv_review_question) TextView textViewQuestion; @@ -40,24 +40,21 @@ public class ReviewImageFragment extends CommonsDaggerSupportFragment { @BindView(R.id.button_no) Button noButton; - - public void update(int position, String fileName) { + public void update(int position) { this.position = position; - this.fileName = fileName; - } - public void updateCategories(Iterable categories) { - if (categories != null && isAdded()) { - catString = TextUtils.join(", ", categories); + private String updateCategoriesQuestion() { + Media media = getReviewActivity().getMedia(); + if (media != null && media.getCategories() != null && isAdded()) { + String catString = TextUtils.join(", ", media.getCategories()); if (catString != null && !catString.equals("") && textViewQuestionContext != null) { catString = "" + catString + ""; String stringToConvertHtml = String.format(getResources().getString(R.string.review_category_explanation), catString); - textViewQuestionContext.setText(Html.fromHtml(stringToConvertHtml)); - } else if (textViewQuestionContext != null) { - textViewQuestionContext.setText(getResources().getString(R.string.review_no_category)); + return Html.fromHtml(stringToConvertHtml).toString(); } } + return getResources().getString(R.string.review_no_category); } @Override @@ -75,29 +72,34 @@ public class ReviewImageFragment extends CommonsDaggerSupportFragment { String question, explanation, yesButtonText, noButtonText; switch (position) { - case COPYRIGHT: - question = getString(R.string.review_copyright); - explanation = getString(R.string.review_copyright_explanation); - yesButtonText = getString(R.string.review_copyright_yes_button_text); - noButtonText = getString(R.string.review_copyright_no_button_text); - yesButton.setOnClickListener(view -> getReviewActivity().reviewController.reportPossibleCopyRightViolation(requireActivity())); - break; - case CATEGORY: - question = getString(R.string.review_category); - explanation = getString(R.string.review_no_category); - yesButtonText = getString(R.string.review_category_yes_button_text); - noButtonText = getString(R.string.review_category_no_button_text); - yesButton.setOnClickListener(view -> { - getReviewActivity().reviewController.reportWrongCategory(requireActivity()); - getReviewActivity().swipeToNext(); - }); - break; case SPAM: question = getString(R.string.review_spam); explanation = getString(R.string.review_spam_explanation); yesButtonText = getString(R.string.review_spam_yes_button_text); noButtonText = getString(R.string.review_spam_no_button_text); - yesButton.setOnClickListener(view -> getReviewActivity().reviewController.reportSpam(requireActivity())); + yesButton.setOnClickListener(view -> getReviewActivity() + .reviewController.reportSpam(requireActivity(), getReviewCallback())); + break; + case COPYRIGHT: + question = getString(R.string.review_copyright); + explanation = getString(R.string.review_copyright_explanation); + yesButtonText = getString(R.string.review_copyright_yes_button_text); + noButtonText = getString(R.string.review_copyright_no_button_text); + yesButton.setOnClickListener(view -> getReviewActivity() + .reviewController + .reportPossibleCopyRightViolation(requireActivity(), getReviewCallback())); + break; + case CATEGORY: + question = getString(R.string.review_category); + explanation = updateCategoriesQuestion(); + yesButtonText = getString(R.string.review_category_yes_button_text); + noButtonText = getString(R.string.review_category_no_button_text); + yesButton.setOnClickListener(view -> { + getReviewActivity() + .reviewController + .reportWrongCategory(requireActivity(), getReviewCallback()); + getReviewActivity().swipeToNext(); + }); break; case THANKS: question = getString(R.string.review_thanks); @@ -122,16 +124,26 @@ public class ReviewImageFragment extends CommonsDaggerSupportFragment { textViewQuestionContext.setText(explanation); yesButton.setText(yesButtonText); noButton.setText(noButtonText); - - if (position == CATEGORY) { - updateCategories(ReviewController.categories); - } - return layoutView; } + private ReviewController.ReviewCallback getReviewCallback() { + return new ReviewController + .ReviewCallback() { + @Override + public void onSuccess() { + getReviewActivity().runRandomizer(); + } + + @Override + public void onFailure() { + //do nothing + } + }; + } + @OnClick(R.id.button_no) - public void onNoButtonClicked() { + void onNoButtonClicked() { getReviewActivity().swipeToNext(); } diff --git a/app/src/main/java/fr/free/nrw/commons/review/ReviewPagerAdapter.java b/app/src/main/java/fr/free/nrw/commons/review/ReviewPagerAdapter.java index 98879a332..000fb8374 100644 --- a/app/src/main/java/fr/free/nrw/commons/review/ReviewPagerAdapter.java +++ b/app/src/main/java/fr/free/nrw/commons/review/ReviewPagerAdapter.java @@ -6,11 +6,13 @@ import androidx.fragment.app.Fragment; import androidx.fragment.app.FragmentManager; import androidx.fragment.app.FragmentStatePagerAdapter; +import java.util.List; + public class ReviewPagerAdapter extends FragmentStatePagerAdapter { - ReviewImageFragment[] reviewImageFragments; + private ReviewImageFragment[] reviewImageFragments; - public ReviewPagerAdapter(FragmentManager fm) { + ReviewPagerAdapter(FragmentManager fm) { super(fm); reviewImageFragments = new ReviewImageFragment[]{ new ReviewImageFragment(), @@ -25,18 +27,13 @@ public class ReviewPagerAdapter extends FragmentStatePagerAdapter { return reviewImageFragments.length; } - public void updateFileInformation(String fileName) { + void updateFileInformation() { for (int i = 0; i < getCount(); i++) { ReviewImageFragment fragment = reviewImageFragments[i]; - fragment.update(i, fileName); + fragment.update(i); } } - public void updateCategories() { - ReviewImageFragment categoryFragment = reviewImageFragments[ReviewImageFragment.CATEGORY]; - categoryFragment.updateCategories(ReviewController.categories); - } - @Override public Fragment getItem(int position) { Bundle bundle = new Bundle(); diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 0c6ba28f0..b160e1e8b 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -558,4 +558,5 @@ Upload your first media by tapping on the add button. Cancelled Upload There is no data for previous image\'s title or description Why should %1$s be deleted? + %1$s is uploaded by: %2$s 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 index d9c808f77..ebd5d4baf 100644 --- a/app/src/test/kotlin/fr/free/nrw/commons/review/ReviewHelperTest.kt +++ b/app/src/test/kotlin/fr/free/nrw/commons/review/ReviewHelperTest.kt @@ -48,9 +48,9 @@ class ReviewHelperTest { `when`(mockRevision.user).thenReturn("TestUser") `when`(mwQueryPage.revisions()).thenReturn(listOf(mockRevision)) - val recentChange = getMockRecentChange("test", "File:Test1.jpeg", 0) - val recentChange1 = getMockRecentChange("test", "File:Test2.png", 0) - val recentChange2 = getMockRecentChange("test", "File:Test3.jpg", 0) + val recentChange = getMockRecentChange("log", "File:Test1.jpeg", 0) + val recentChange1 = getMockRecentChange("log", "File:Test2.png", 0) + val recentChange2 = getMockRecentChange("log", "File:Test3.jpg", 0) val mwQueryResult = mock(MwQueryResult::class.java) `when`(mwQueryResult.recentChanges).thenReturn(listOf(recentChange, recentChange1, recentChange2)) `when`(mwQueryResult.firstPage()).thenReturn(mwQueryPage)