Fix bugs in peer review flow (#3039)

* Fix bugs in peer review flow

* Fix tests

* Bug fixes

* Fix remaining issues with peer review
This commit is contained in:
Vivek Maskara 2019-06-26 20:10:49 +05:30 committed by neslihanturan
parent 667372512b
commit 5dc45a53d3
8 changed files with 140 additions and 125 deletions

View file

@ -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<Boolean> 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();
}

View file

@ -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<String> categories) {
reviewController.onCategoriesRefreshed(categories);
reviewPagerAdapter.updateCategories();
}
public void swipeToNext() {

View file

@ -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<String> 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<String> 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();
}
}

View file

@ -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;
}

View file

@ -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<String> 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 = "<b>" + catString + "</b>";
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();
}

View file

@ -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();

View file

@ -558,4 +558,5 @@ Upload your first media by tapping on the add button.</string>
<string name="upload_cancelled">Cancelled Upload</string>
<string name="previous_image_title_description_not_found">There is no data for previous image\'s title or description</string>
<string name="dialog_box_text_nomination">Why should %1$s be deleted?</string>
<string name="review_is_uploaded_by">%1$s is uploaded by: %2$s</string>
</resources>

View file

@ -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)