diff --git a/features/rageshake/api/src/main/kotlin/io/element/android/features/rageshake/api/reporter/BugReporter.kt b/features/rageshake/api/src/main/kotlin/io/element/android/features/rageshake/api/reporter/BugReporter.kt index 4f48871666..ec739f423b 100644 --- a/features/rageshake/api/src/main/kotlin/io/element/android/features/rageshake/api/reporter/BugReporter.kt +++ b/features/rageshake/api/src/main/kotlin/io/element/android/features/rageshake/api/reporter/BugReporter.kt @@ -25,7 +25,7 @@ interface BugReporter { * @param withDevicesLogs true to include the device log * @param withCrashLogs true to include the crash logs * @param withScreenshot true to include the screenshot - * @param theBugDescription the bug description + * @param problemDescription the bug description * @param canContact true if the user opt in to be contacted directly * @param listener the listener */ @@ -33,9 +33,9 @@ interface BugReporter { withDevicesLogs: Boolean, withCrashLogs: Boolean, withScreenshot: Boolean, - theBugDescription: String, + problemDescription: String, canContact: Boolean = false, - listener: BugReporterListener? + listener: BugReporterListener ) /** diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt index 5eeb078275..1d90a60cff 100644 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/bugreport/BugReportPresenter.kt @@ -143,7 +143,7 @@ class BugReportPresenter @Inject constructor( withDevicesLogs = formState.sendLogs, withCrashLogs = hasCrashLogs && formState.sendLogs, withScreenshot = formState.sendScreenshot, - theBugDescription = formState.description, + problemDescription = formState.description, canContact = formState.canContact, listener = listener ) diff --git a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt index ddde3bd420..b4e8a2effb 100755 --- a/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt +++ b/features/rageshake/impl/src/main/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporter.kt @@ -44,7 +44,6 @@ import io.element.android.libraries.sessionstorage.api.SessionStore import kotlinx.coroutines.CancellationException import kotlinx.coroutines.flow.first import kotlinx.coroutines.withContext -import okhttp3.Call import okhttp3.MediaType.Companion.toMediaTypeOrNull import okhttp3.OkHttpClient import okhttp3.Request @@ -89,11 +88,6 @@ class DefaultBugReporter @Inject constructor( private const val LOG_DIRECTORY_NAME = "logs" } - // the pending bug report call - private var bugReportCall: Call? = null - - // boolean to cancel the bug report - private val isCancelled = false private val logcatCommandDebug = arrayOf("logcat", "-d", "-v", "threadtime", "*:*") private var currentTracingFilter: String? = null @@ -103,252 +97,197 @@ class DefaultBugReporter @Inject constructor( withDevicesLogs: Boolean, withCrashLogs: Boolean, withScreenshot: Boolean, - theBugDescription: String, + problemDescription: String, canContact: Boolean, - listener: BugReporterListener? + listener: BugReporterListener, ) { // enumerate files to delete val bugReportFiles: MutableList = ArrayList() + var response: Response? = null try { var serverError: String? = null withContext(coroutineDispatchers.io) { - var bugDescription = theBugDescription val crashCallStack = crashDataStore.crashInfo().first() - - if (crashCallStack.isNotEmpty() && withCrashLogs) { - bugDescription += "\n\n\n\n--------------------------------- crash call stack ---------------------------------\n" - bugDescription += crashCallStack + val bugDescription = buildString { + append(problemDescription) + if (crashCallStack.isNotEmpty() && withCrashLogs) { + append("\n\n\n\n--------------------------------- crash call stack ---------------------------------\n") + append(crashCallStack) + } } - - val gzippedFiles = ArrayList() - + val gzippedFiles = mutableListOf() if (withDevicesLogs) { val files = getLogFiles().sortedByDescending { it.lastModified() } - files.mapNotNullTo(gzippedFiles) { f -> + files.mapNotNullTo(gzippedFiles) { file -> when { - isCancelled -> null - f.extension == "gz" -> f - else -> compressFile(f) + file.extension == "gz" -> file + else -> compressFile(file) } } files.deleteAllExceptMostRecent() } - - if (!isCancelled && (withCrashLogs || withDevicesLogs)) { + if (withCrashLogs || withDevicesLogs) { saveLogCat() val gzippedLogcat = compressFile(logCatErrFile) - if (null != gzippedLogcat) { - if (gzippedFiles.isEmpty()) { - gzippedFiles.add(gzippedLogcat) - } else { - gzippedFiles.add(0, gzippedLogcat) - } + if (gzippedLogcat != null) { + gzippedFiles.add(0, gzippedLogcat) } } - val sessionData = sessionStore.getLatestSession() val deviceId = sessionData?.deviceId ?: "undefined" val userId = sessionData?.userId?.let { UserId(it) } - - if (!isCancelled) { - // build the multi part request - val builder = BugReporterMultipartBody.Builder() - .addFormDataPart("text", bugDescription) - .addFormDataPart("app", context.getString(R.string.bug_report_app_name)) - .addFormDataPart("user_agent", userAgentProvider.provide()) - .addFormDataPart("user_id", userId?.toString() ?: "undefined") - .addFormDataPart("can_contact", canContact.toString()) - .addFormDataPart("device_id", deviceId) - .apply { - userId?.let { - matrixClientProvider.getOrNull(it)?.let { client -> - val curveKey = client.encryptionService().deviceCurve25519() - val edKey = client.encryptionService().deviceEd25519() - if (curveKey != null && edKey != null) { - addFormDataPart("device_keys", "curve25519:$curveKey, ed25519:$edKey") - } - } - } - } - .addFormDataPart("device", Build.MODEL.trim()) - .addFormDataPart("locale", Locale.getDefault().toString()) - .addFormDataPart("sdk_sha", sdkMetadata.sdkGitSha) - .addFormDataPart("local_time", LocalDateTime.now().format(DateTimeFormatter.ISO_DATE_TIME)) - .addFormDataPart("utc_time", LocalDateTime.ofInstant(Instant.now(), ZoneOffset.UTC).format(DateTimeFormatter.ISO_DATE_TIME)) - .addFormDataPart("app_id", buildMeta.applicationId) - // Nightly versions have a custom version name suffix that we should remove for the bug report - .addFormDataPart("Version", buildMeta.versionName.replace("-nightly", "")) - currentTracingFilter?.let { - builder.addFormDataPart("tracing_filter", it) - } - - // add the gzipped files, don't cancel the whole upload if only some file failed to upload - var totalUploadedSize = 0L - var uploadedSomeLogs = false - for (file in gzippedFiles) { - try { - val requestBody = file.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull()) - totalUploadedSize += requestBody.contentLength() - // If we are about to upload more than the max request size, stop here - if (totalUploadedSize > ApplicationConfig.MAX_LOG_UPLOAD_SIZE) { - Timber.e("Could not upload file ${file.name} because it would exceed the max request size") - break - } - builder.addFormDataPart("compressed-log", file.name, requestBody) - uploadedSomeLogs = true - } catch (e: CancellationException) { - throw e - } catch (e: Exception) { - Timber.e(e, "## sendBugReport() : fail to attach file ${file.name}") + // build the multi part request + val builder = BugReporterMultipartBody.Builder() + .addFormDataPart("text", bugDescription) + .addFormDataPart("app", context.getString(R.string.bug_report_app_name)) + .addFormDataPart("user_agent", userAgentProvider.provide()) + .addFormDataPart("user_id", userId?.toString() ?: "undefined") + .addFormDataPart("can_contact", canContact.toString()) + .addFormDataPart("device_id", deviceId) + .addFormDataPart("device", Build.MODEL.trim()) + .addFormDataPart("locale", Locale.getDefault().toString()) + .addFormDataPart("sdk_sha", sdkMetadata.sdkGitSha) + .addFormDataPart("local_time", LocalDateTime.now().format(DateTimeFormatter.ISO_DATE_TIME)) + .addFormDataPart("utc_time", LocalDateTime.ofInstant(Instant.now(), ZoneOffset.UTC).format(DateTimeFormatter.ISO_DATE_TIME)) + .addFormDataPart("app_id", buildMeta.applicationId) + // Nightly versions have a custom version name suffix that we should remove for the bug report + .addFormDataPart("Version", buildMeta.versionName.replace("-nightly", "")) + .addFormDataPart("label", buildMeta.versionName) + .addFormDataPart("label", buildMeta.flavorDescription) + .addFormDataPart("branch_name", buildMeta.gitBranchName) + userId?.let { + matrixClientProvider.getOrNull(it)?.let { client -> + val curveKey = client.encryptionService().deviceCurve25519() + val edKey = client.encryptionService().deviceEd25519() + if (curveKey != null && edKey != null) { + builder.addFormDataPart("device_keys", "curve25519:$curveKey, ed25519:$edKey") } } - - bugReportFiles.addAll(gzippedFiles) - - if (gzippedFiles.isNotEmpty() && !uploadedSomeLogs) { - serverError = "Couldn't upload any logs, please retry." - return@withContext - } - - if (withScreenshot) { - screenshotHolder.getFileUri() - ?.toUri() - ?.toFile() - ?.let { screenshotFile -> - try { - builder.addFormDataPart( - "file", - screenshotFile.name, - screenshotFile.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull()) - ) - } catch (e: Exception) { - Timber.e(e, "## sendBugReport() : fail to write screenshot") - } - } - } - - // add some github labels - builder.addFormDataPart("label", buildMeta.versionName) - builder.addFormDataPart("label", buildMeta.flavorDescription) - builder.addFormDataPart("branch_name", buildMeta.gitBranchName) - - if (crashCallStack.isNotEmpty() && withCrashLogs) { - builder.addFormDataPart("label", "crash") - } - - val requestBody = builder.build() - - // add a progress listener - requestBody.setWriteListener { totalWritten, contentLength -> - val percentage = if (-1L != contentLength) { - if (totalWritten > contentLength) { - 100 - } else { - (totalWritten * 100 / contentLength).toInt() - } - } else { - 0 - } - - if (isCancelled && null != bugReportCall) { - bugReportCall!!.cancel() - } - - Timber.v("## onWrite() : $percentage%") - try { - listener?.onProgress(percentage) - } catch (e: Exception) { - Timber.e(e, "## onProgress() : failed") - } - } - - // build the request - val request = Request.Builder() - .url(bugReporterUrlProvider.provide()) - .post(requestBody) - .build() - - var responseCode = HttpURLConnection.HTTP_INTERNAL_ERROR - var response: Response? = null - var errorMessage: String? = null - - // trigger the request + } + if (crashCallStack.isNotEmpty() && withCrashLogs) { + builder.addFormDataPart("label", "crash") + } + currentTracingFilter?.let { + builder.addFormDataPart("tracing_filter", it) + } + // add the gzipped files, don't cancel the whole upload if only some file failed to upload + var totalUploadedSize = 0L + var uploadedSomeLogs = false + for (file in gzippedFiles) { try { - bugReportCall = okHttpClient.get().newCall(request) - response = bugReportCall!!.execute() - responseCode = response.code + val requestBody = file.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull()) + totalUploadedSize += requestBody.contentLength() + // If we are about to upload more than the max request size, stop here + if (totalUploadedSize > ApplicationConfig.MAX_LOG_UPLOAD_SIZE) { + Timber.e("Could not upload file ${file.name} because it would exceed the max request size") + break + } + builder.addFormDataPart("compressed-log", file.name, requestBody) + uploadedSomeLogs = true } catch (e: CancellationException) { throw e } catch (e: Exception) { - Timber.e(e, "response") - errorMessage = e.localizedMessage + Timber.e(e, "## sendBugReport() : fail to attach file ${file.name}") } - - // if the upload failed, try to retrieve the reason - if (responseCode != HttpURLConnection.HTTP_OK) { - if (null != errorMessage) { - serverError = "Failed with error $errorMessage" - } else if (response?.body == null) { - serverError = "Failed with error $responseCode" + } + bugReportFiles.addAll(gzippedFiles) + if (gzippedFiles.isNotEmpty() && !uploadedSomeLogs) { + serverError = "Couldn't upload any logs, please retry." + return@withContext + } + if (withScreenshot) { + screenshotHolder.getFileUri() + ?.toUri() + ?.toFile() + ?.let { screenshotFile -> + try { + builder.addFormDataPart( + "file", + screenshotFile.name, + screenshotFile.asRequestBody(MimeTypes.OctetStream.toMediaTypeOrNull()) + ) + } catch (e: Exception) { + Timber.e(e, "## sendBugReport() : fail to write screenshot") + } + } + } + val requestBody = builder.build() + // add a progress listener + requestBody.setWriteListener { totalWritten, contentLength -> + val percentage = if (-1L != contentLength) { + if (totalWritten > contentLength) { + 100 + } else { + (totalWritten * 100 / contentLength).toInt() + } + } else { + 0 + } + Timber.v("## onWrite() : $percentage%") + listener.onProgress(percentage) + } + // build the request + val request = Request.Builder() + .url(bugReporterUrlProvider.provide()) + .post(requestBody) + .build() + var errorMessage: String? = null + // trigger the request + try { + response = okHttpClient.get() + .newCall(request) + .execute() + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + Timber.e(e, "Error executing the request") + errorMessage = e.localizedMessage + } + val responseCode = response?.code + // if the upload failed, try to retrieve the reason + if (responseCode != HttpURLConnection.HTTP_OK) { + serverError = if (errorMessage != null) { + "Failed with error $errorMessage" + } else { + val responseBody = response?.body + if (responseBody == null) { + "Failed with error $responseCode" } else { try { - val inputStream = response.body!!.byteStream() - serverError = inputStream.use { - buildString { - var ch = it.read() - while (ch != -1) { - append(ch.toChar()) - ch = it.read() - } - } + val inputStream = responseBody.byteStream() + val serverErrorJson = inputStream.use { + it.readBytes().toString(Charsets.UTF_8) } - // check if the error message - serverError?.let { - try { - val responseJSON = JSONObject(it) - serverError = responseJSON.getString("error") - } catch (e: CancellationException) { - throw e - } catch (e: JSONException) { - Timber.e(e, "doInBackground ; Json conversion failed") - } - } - // should never happen - if (null == serverError) { - serverError = "Failed with error $responseCode" + try { + val responseJSON = JSONObject(serverErrorJson) + responseJSON.getString("error") + } catch (e: CancellationException) { + throw e + } catch (e: JSONException) { + Timber.e(e, "Json conversion failed") + "Failed with error $responseCode" } } catch (e: CancellationException) { throw e } catch (e: Exception) { Timber.e(e, "## sendBugReport() : failed to parse error") + "Failed with error $responseCode" } } } } } - withContext(coroutineDispatchers.main) { - bugReportCall = null - if (null != listener) { - try { - if (isCancelled) { - listener.onUploadCancelled() - } else if (null == serverError) { - listener.onUploadSucceed() - } else { - listener.onUploadFailed(serverError) - } - } catch (e: CancellationException) { - throw e - } catch (e: Exception) { - Timber.e(e, "## onPostExecute() : failed") - } - } + if (serverError == null) { + listener.onUploadSucceed() + } else { + listener.onUploadFailed(serverError) } } finally { // delete the generated files when the bug report process has finished for (file in bugReportFiles) { file.safeDelete() } + response?.close() } } @@ -424,17 +363,17 @@ class DefaultBugReporter @Inject constructor( * @param streamWriter the stream writer */ private fun getLogCatError(streamWriter: OutputStreamWriter) { - val logcatProc: Process + val logcatProcess: Process try { - logcatProc = Runtime.getRuntime().exec(logcatCommandDebug) + logcatProcess = Runtime.getRuntime().exec(logcatCommandDebug) } catch (e1: IOException) { return } try { - val separator = System.getProperty("line.separator") - logcatProc.inputStream + val separator = System.lineSeparator() + logcatProcess.inputStream .reader() .buffered(ApplicationConfig.MAX_LOG_UPLOAD_SIZE.toInt()) .forEachLine { line -> diff --git a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/FakeBugReporter.kt b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/FakeBugReporter.kt index a4d4f83da9..922481cedf 100644 --- a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/FakeBugReporter.kt +++ b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/bugreport/FakeBugReporter.kt @@ -33,29 +33,29 @@ class FakeBugReporter(val mode: Mode = Mode.Success) : BugReporter { withDevicesLogs: Boolean, withCrashLogs: Boolean, withScreenshot: Boolean, - theBugDescription: String, + problemDescription: String, canContact: Boolean, - listener: BugReporterListener?, + listener: BugReporterListener, ) { delay(100) - listener?.onProgress(0) + listener.onProgress(0) delay(100) - listener?.onProgress(50) + listener.onProgress(50) delay(100) when (mode) { Mode.Success -> Unit Mode.Failure -> { - listener?.onUploadFailed(A_FAILURE_REASON) + listener.onUploadFailed(A_FAILURE_REASON) return } Mode.Cancel -> { - listener?.onUploadCancelled() + listener.onUploadCancelled() return } } - listener?.onProgress(100) + listener.onProgress(100) delay(100) - listener?.onUploadSucceed() + listener.onUploadSucceed() } override fun logDirectory(): File { diff --git a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporterTest.kt b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporterTest.kt index cb28c973dc..ad7f6b76e7 100755 --- a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporterTest.kt +++ b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/reporter/DefaultBugReporterTest.kt @@ -63,7 +63,7 @@ class DefaultBugReporterTest { withDevicesLogs = true, withCrashLogs = true, withScreenshot = true, - theBugDescription = "a bug occurred", + problemDescription = "a bug occurred", canContact = true, listener = object : BugReporterListener { override fun onUploadCancelled() { @@ -130,7 +130,7 @@ class DefaultBugReporterTest { withDevicesLogs = true, withCrashLogs = true, withScreenshot = true, - theBugDescription = "a bug occurred", + problemDescription = "a bug occurred", canContact = true, listener = object : BugReporterListener { override fun onUploadCancelled() {} @@ -198,9 +198,9 @@ class DefaultBugReporterTest { withDevicesLogs = true, withCrashLogs = true, withScreenshot = true, - theBugDescription = "a bug occurred", + problemDescription = "a bug occurred", canContact = true, - listener = null + listener = NoopBugReporterListener(), ) val request = server.takeRequest() @@ -240,9 +240,9 @@ class DefaultBugReporterTest { withDevicesLogs = true, withCrashLogs = true, withScreenshot = true, - theBugDescription = "a bug occurred", + problemDescription = "a bug occurred", canContact = true, - listener = null + listener = NoopBugReporterListener(), ) val request = server.takeRequest() @@ -308,7 +308,7 @@ class DefaultBugReporterTest { withDevicesLogs = true, withCrashLogs = true, withScreenshot = true, - theBugDescription = "a bug occurred", + problemDescription = "a bug occurred", canContact = true, listener = object : BugReporterListener { override fun onUploadCancelled() { diff --git a/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/reporter/NoopBugReporterListener.kt b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/reporter/NoopBugReporterListener.kt new file mode 100644 index 0000000000..7a0ec207dd --- /dev/null +++ b/features/rageshake/impl/src/test/kotlin/io/element/android/features/rageshake/impl/reporter/NoopBugReporterListener.kt @@ -0,0 +1,26 @@ +/* + * Copyright (c) 2024 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.features.rageshake.impl.reporter + +import io.element.android.features.rageshake.api.reporter.BugReporterListener + +class NoopBugReporterListener : BugReporterListener { + override fun onUploadCancelled() = Unit + override fun onUploadFailed(reason: String?) = Unit + override fun onProgress(progress: Int) = Unit + override fun onUploadSucceed() = Unit +}