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

Fix blocked message in Suggested Edits, and use coroutines. #4882

Merged
merged 5 commits into from
Aug 20, 2024
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
9 changes: 3 additions & 6 deletions app/src/main/java/org/wikipedia/dataclient/Service.kt
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,10 @@ interface Service {
suspend fun getSiteInfoWithMagicWords(): MwQueryResponse

@GET(MW_API_PREFIX + "action=parse&prop=text&mobileformat=1")
fun parsePage(@Query("page") pageTitle: String): Observable<MwParseResponse>
suspend fun parsePage(@Query("page") pageTitle: String): MwParseResponse

@GET(MW_API_PREFIX + "action=parse&prop=text&mobileformat=1")
fun parseText(@Query("text") text: String): Observable<MwParseResponse>
suspend fun parseText(@Query("text") text: String): MwParseResponse

@GET(MW_API_PREFIX + "action=parse&prop=text&mobileformat=1&mainpage=1")
suspend fun parseTextForMainPage(@Query("page") mainPageTitle: String): MwParseResponse
Expand Down Expand Up @@ -317,10 +317,7 @@ interface Service {
@get:GET(MW_API_PREFIX + "action=query&meta=authmanagerinfo|tokens&amirequestsfor=create&type=createaccount")
val authManagerInfo: Observable<MwQueryResponse>

@get:GET(MW_API_PREFIX + "action=query&meta=userinfo&uiprop=groups|blockinfo|editcount|latestcontrib|hasmsg")
val userInfo: Observable<MwQueryResponse>

@GET(MW_API_PREFIX + "action=query&meta=userinfo&uiprop=groups|blockinfo|editcount|latestcontrib|hasmsg")
@GET(MW_API_PREFIX + "action=query&meta=userinfo&uiprop=groups|blockinfo|editcount|latestcontrib|hasmsg|options")
suspend fun getUserInfo(): MwQueryResponse

@GET(MW_API_PREFIX + "action=query&list=users&usprop=editcount|groups|registration|rights")
Expand Down
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package org.wikipedia.dataclient.mwapi

import kotlinx.coroutines.CoroutineExceptionHandler
import kotlinx.coroutines.runBlocking
import kotlinx.serialization.SerialName
import kotlinx.serialization.Serializable
import org.wikipedia.dataclient.ServiceError
import org.wikipedia.util.DateUtil
import org.wikipedia.util.StringUtil
import org.wikipedia.util.ThrowableUtil
import org.wikipedia.util.log.L
import java.util.*

@Serializable
Expand Down Expand Up @@ -36,7 +39,11 @@ class MwServiceError(val code: String? = null,
init {
// Special case: if it's a Blocked error, parse the blockinfo structure ourselves.
if (("blocked" == code || "autoblocked" == code) && data?.blockinfo != null) {
html = ThrowableUtil.getBlockMessageHtml(data.blockinfo)
runBlocking(CoroutineExceptionHandler { _, t ->
L.e(t)
}) {
html = ThrowableUtil.getBlockMessageHtml(data.blockinfo)
}
}
}

Expand Down
31 changes: 17 additions & 14 deletions app/src/main/java/org/wikipedia/edit/EditSectionActivity.kt
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@ import androidx.core.os.postDelayed
import androidx.core.view.isGone
import androidx.core.view.isVisible
import androidx.core.widget.doAfterTextChanged
import androidx.lifecycle.lifecycleScope
import com.google.android.material.dialog.MaterialAlertDialogBuilder
import io.reactivex.rxjava3.android.schedulers.AndroidSchedulers
import io.reactivex.rxjava3.disposables.CompositeDisposable
import io.reactivex.rxjava3.schedulers.Schedulers
import kotlinx.coroutines.CoroutineExceptionHandler
import kotlinx.coroutines.launch
import org.wikipedia.Constants
import org.wikipedia.R
import org.wikipedia.WikipediaApp
Expand All @@ -41,7 +44,6 @@ import org.wikipedia.databinding.DialogWithCheckboxBinding
import org.wikipedia.databinding.ItemEditActionbarButtonBinding
import org.wikipedia.dataclient.ServiceFactory
import org.wikipedia.dataclient.mwapi.MwException
import org.wikipedia.dataclient.mwapi.MwParseResponse
import org.wikipedia.dataclient.mwapi.MwServiceError
import org.wikipedia.dataclient.okhttp.OkHttpConnectionFactory
import org.wikipedia.edit.insertmedia.InsertMediaActivity
Expand Down Expand Up @@ -418,23 +420,24 @@ class EditSectionActivity : BaseActivity(), ThemeChooserDialog.Callback, EditPre
*/
private fun handleEditingException(caught: MwException) {
val code = caught.title

// In the case of certain AbuseFilter responses, they are sent as a code, instead of a
// fully parsed response. We need to make one more API call to get the parsed message:
if (code.startsWith("abusefilter-") && caught.message.contains("abusefilter-") && caught.message.length < 100) {
disposables.add(ServiceFactory.get(pageTitle.wikiSite).parsePage("MediaWiki:" + StringUtil.sanitizeAbuseFilterCode(caught.message))
.subscribeOn(Schedulers.io())
.observeOn(AndroidSchedulers.mainThread())
.subscribe({ response: MwParseResponse -> showError(MwException(MwServiceError(code, response.text))) }) { showError(it) })
} else if ("editconflict" == code) {
MaterialAlertDialogBuilder(this@EditSectionActivity)
lifecycleScope.launch(CoroutineExceptionHandler { _, t ->
showError(t)
}) {
// In the case of certain AbuseFilter responses, they are sent as a code, instead of a
// fully parsed response. We need to make one more API call to get the parsed message:
if (code.startsWith("abusefilter-") && caught.message.contains("abusefilter-") && caught.message.length < 100) {
val response = ServiceFactory.get(pageTitle.wikiSite).parsePage("MediaWiki:" + StringUtil.sanitizeAbuseFilterCode(caught.message))
showError(MwException(MwServiceError(code, response.text)))
} else if ("editconflict" == code) {
MaterialAlertDialogBuilder(this@EditSectionActivity)
.setTitle(R.string.edit_conflict_title)
.setMessage(R.string.edit_conflict_message)
.setPositiveButton(R.string.edit_conflict_dialog_ok_button_text, null)
.show()
resetToStart()
} else {
showError(caught)
resetToStart()
} else {
showError(caught)
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,12 @@ class SuggestedEditsTasksFragment : Fragment() {

setUpTasks()

if (displayedTasks.isEmpty() && !viewModel.blockMessageWikipedia.isNullOrEmpty()) {
clearContents()
setIPBlockedStatus()
return
}

binding.tasksRecyclerView.adapter!!.notifyDataSetChanged()
setUserStatsViewsAndTooltips()

Expand Down Expand Up @@ -376,10 +382,6 @@ class SuggestedEditsTasksFragment : Fragment() {
displayedTasks.add(addImageCaptionsTask)
displayedTasks.add(addImageTagsTask)
}

if (displayedTasks.isEmpty() && !viewModel.blockMessageWikipedia.isNullOrEmpty()) {
setIPBlockedStatus()
}
}

private inner class TaskViewCallback : SuggestedEditsTaskView.Callback {
Expand Down
25 changes: 12 additions & 13 deletions app/src/main/java/org/wikipedia/util/ThrowableUtil.kt
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
package org.wikipedia.util

import android.content.Context
import androidx.annotation.WorkerThread
import io.reactivex.rxjava3.core.Observable
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.async
import kotlinx.coroutines.withContext
import org.json.JSONException
import org.wikipedia.R
import org.wikipedia.WikipediaApp
Expand All @@ -13,7 +14,6 @@ import org.wikipedia.dataclient.mwapi.MwException
import org.wikipedia.dataclient.mwapi.MwServiceError
import org.wikipedia.dataclient.okhttp.HttpStatusException
import org.wikipedia.login.LoginFailedException
import org.wikipedia.util.log.L
import java.net.SocketException
import java.net.SocketTimeoutException
import java.net.UnknownHostException
Expand Down Expand Up @@ -91,17 +91,16 @@ object ThrowableUtil {
return t is MwException && t.error.code?.contains("notloggedin") == true
}

@WorkerThread
fun getBlockMessageHtml(blockInfo: MwServiceError.BlockInfo, wikiSite: WikiSite = WikipediaApp.instance.wikiSite): String {
suspend fun getBlockMessageHtml(blockInfo: MwServiceError.BlockInfo, wikiSite: WikiSite = WikipediaApp.instance.wikiSite): String {
var html = ""
Observable.zip(ServiceFactory.get(wikiSite).userInfo,
ServiceFactory.get(wikiSite).parsePage("MediaWiki:Blockedtext"),
ServiceFactory.get(wikiSite).parseText(blockInfo.blockReason)) { userInfoResponse, blockedParseResponse, reasonParseResponse ->
parseBlockedError(
blockedParseResponse.text, blockInfo,
reasonParseResponse.text, userInfoResponse.query?.userInfo!!.name
)
}.blockingSubscribe({ html = it }) { L.e(it) }
withContext(Dispatchers.IO) {
val userInfoCall = async { ServiceFactory.get(wikiSite).getUserInfo() }
val blockedPageCall = async { ServiceFactory.get(wikiSite).parsePage("MediaWiki:Blockedtext") }
val reasonTextCall = async { ServiceFactory.get(wikiSite).parseText(blockInfo.blockReason) }

html = parseBlockedError(blockedPageCall.await().text, blockInfo,
reasonTextCall.await().text, userInfoCall.await().query?.userInfo!!.name)
}
return html
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.wikipedia.login

import kotlinx.coroutines.runBlocking
import org.junit.Test
import org.wikipedia.test.MockRetrofitTest

Expand All @@ -9,27 +10,10 @@ class UserExtendedInfoClientTest : MockRetrofitTest() {
fun testRequestSuccess() {
enqueueFromFile("user_extended_info.json")
val id = 24531888
apiService.userInfo.test().await()
.assertComplete().assertNoErrors()
.assertValue {
it.query?.userInfo?.id == id &&
it.query?.getUserResponse("USER")?.name == "USER"
}
}

@Test
@Throws(Throwable::class)
fun testRequestResponse404() {
enqueue404()
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the rest of these tests because I no longer understand what they're testing. Are they testing if Retrofit works correctly? That's already done in Retrofit's own test suite. Are they testing if the Java language works correctly? I'm not sure that's within the scope of what we should worry about.

apiService.userInfo.test().await()
.assertError(Exception::class.java)
}

@Test
@Throws(Throwable::class)
fun testRequestResponseMalformed() {
enqueueMalformed()
apiService.userInfo.test().await()
.assertError(Exception::class.java)
runBlocking {
val userInfo = apiService.getUserInfo()
assert(userInfo.query?.userInfo?.id == id)
assert(userInfo.query?.getUserResponse("USER")?.name == "USER")
}
}
}
Loading