From 2b7ff8d283850f37e9963c114b8590ce5b17d784 Mon Sep 17 00:00:00 2001 From: Zorica Stojchevska Date: Thu, 18 Nov 2021 23:42:42 +0100 Subject: [PATCH 1/5] Renamed UploadAttachments.kt --- ...achments.kt => UploadAttachmentsWorker.kt} | 6 +-- .../android/usecase/compose/SaveDraft.kt | 6 +-- ...Test.kt => UploadAttachmentsWorkerTest.kt} | 44 +++++++++---------- .../android/usecase/compose/SaveDraftTest.kt | 24 +++++----- 4 files changed, 40 insertions(+), 40 deletions(-) rename app/src/main/java/ch/protonmail/android/attachments/{UploadAttachments.kt => UploadAttachmentsWorker.kt} (98%) rename app/src/test/java/ch/protonmail/android/attachments/{UploadAttachmentsTest.kt => UploadAttachmentsWorkerTest.kt} (96%) diff --git a/app/src/main/java/ch/protonmail/android/attachments/UploadAttachments.kt b/app/src/main/java/ch/protonmail/android/attachments/UploadAttachmentsWorker.kt similarity index 98% rename from app/src/main/java/ch/protonmail/android/attachments/UploadAttachments.kt rename to app/src/main/java/ch/protonmail/android/attachments/UploadAttachmentsWorker.kt index d5d37e775..7457874a5 100644 --- a/app/src/main/java/ch/protonmail/android/attachments/UploadAttachments.kt +++ b/app/src/main/java/ch/protonmail/android/attachments/UploadAttachmentsWorker.kt @@ -57,8 +57,8 @@ internal const val KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR = "keyUploadAttach private const val UPLOAD_ATTACHMENTS_WORK_NAME_PREFIX = "uploadAttachmentUniqueWorkName" private const val UPLOAD_ATTACHMENTS_MAX_RETRIES = 1 -class UploadAttachments @WorkerInject constructor( - @Assisted context: Context, +class UploadAttachmentsWorker @WorkerInject constructor( + @Assisted val context: Context, @Assisted params: WorkerParameters, private val dispatchers: DispatcherProvider, private val attachmentsRepository: AttachmentsRepository, @@ -209,7 +209,7 @@ class UploadAttachments @WorkerInject constructor( val constraints = Constraints.Builder() .setRequiredNetworkType(NetworkType.CONNECTED) .build() - val uploadAttachmentsRequest = OneTimeWorkRequestBuilder() + val uploadAttachmentsRequest = OneTimeWorkRequestBuilder() .setConstraints(constraints) .setInputData( workDataOf( diff --git a/app/src/main/java/ch/protonmail/android/usecase/compose/SaveDraft.kt b/app/src/main/java/ch/protonmail/android/usecase/compose/SaveDraft.kt index 770a83d78..ccb407f7d 100644 --- a/app/src/main/java/ch/protonmail/android/usecase/compose/SaveDraft.kt +++ b/app/src/main/java/ch/protonmail/android/usecase/compose/SaveDraft.kt @@ -24,7 +24,7 @@ import ch.protonmail.android.activities.messageDetails.repository.MessageDetails import ch.protonmail.android.api.models.room.messages.Message import ch.protonmail.android.api.models.room.pendingActions.PendingActionsDao import ch.protonmail.android.attachments.KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR -import ch.protonmail.android.attachments.UploadAttachments +import ch.protonmail.android.attachments.UploadAttachmentsWorker import ch.protonmail.android.core.Constants import ch.protonmail.android.core.Constants.MessageLocationType.ALL_DRAFT import ch.protonmail.android.core.Constants.MessageLocationType.ALL_MAIL @@ -55,7 +55,7 @@ class SaveDraft @Inject constructor( private val pendingActionsDao: PendingActionsDao, private val createDraftWorker: CreateDraftWorker.Enqueuer, @CurrentUsername private val username: String, - private val uploadAttachments: UploadAttachments.Enqueuer, + private val uploadAttachmentsWorker: UploadAttachmentsWorker.Enqueuer, private val userNotifier: UserNotifier ) { @@ -135,7 +135,7 @@ class SaveDraft @Inject constructor( localDraft: Message ): SaveDraftResult { val isMessageSending = params.trigger == SaveDraftTrigger.SendingMessage - return uploadAttachments.enqueue(params.newAttachmentIds, createdDraftId, isMessageSending) + return uploadAttachmentsWorker.enqueue(params.newAttachmentIds, createdDraftId, isMessageSending) .filter { it?.state?.isFinished == true } .map { if (it?.state == WorkInfo.State.FAILED) { diff --git a/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsTest.kt b/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsWorkerTest.kt similarity index 96% rename from app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsTest.kt rename to app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsWorkerTest.kt index 862e0d1f4..686224941 100644 --- a/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsTest.kt +++ b/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsWorkerTest.kt @@ -55,7 +55,7 @@ import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals -class UploadAttachmentsTest : CoroutinesTest { +class UploadAttachmentsWorkerTest : CoroutinesTest { @RelaxedMockK private lateinit var context: Context @@ -85,7 +85,7 @@ class UploadAttachmentsTest : CoroutinesTest { private lateinit var crypto: AddressCrypto @InjectMockKs - private lateinit var uploadAttachments: UploadAttachments + private lateinit var uploadAttachmentsWorker: UploadAttachmentsWorker private val currentUsername = "current username" @@ -114,7 +114,7 @@ class UploadAttachmentsTest : CoroutinesTest { every { cryptoFactory.create(Id(addressId), Name(currentUsername)) } returns crypto // When - UploadAttachments.Enqueuer(workManager).enqueue( + UploadAttachmentsWorker.Enqueuer(workManager).enqueue( attachmentIds, messageId, true @@ -154,7 +154,7 @@ class UploadAttachmentsTest : CoroutinesTest { givenFullValidInput(messageId, attachmentIds.toTypedArray()) coEvery { messageDetailsRepository.findMessageById(messageId) } returns message - uploadAttachments.doWork() + uploadAttachmentsWorker.doWork() val pendingUpload = PendingUpload(messageId) verify { pendingActionsDao.insertPendingForUpload(pendingUpload) } @@ -169,7 +169,7 @@ class UploadAttachmentsTest : CoroutinesTest { givenFullValidInput(messageId, attachmentIds.toTypedArray()) coEvery { messageDetailsRepository.findMessageById(messageId) } returns null - val result = uploadAttachments.doWork() + val result = uploadAttachmentsWorker.doWork() assertEquals( ListenableWorker.Result.failure( @@ -191,7 +191,7 @@ class UploadAttachmentsTest : CoroutinesTest { coEvery { messageDetailsRepository.findMessageById(messageId) } returns message every { pendingActionsDao.findPendingUploadByMessageId(messageId) } returns PendingUpload(messageId) - val result = uploadAttachments.doWork() + val result = uploadAttachmentsWorker.doWork() verify { pendingActionsDao.insertPendingForUpload(any()) } assertEquals(ListenableWorker.Result.success(), result) @@ -222,7 +222,7 @@ class UploadAttachmentsTest : CoroutinesTest { every { messageDetailsRepository.findAttachmentById("1") } returns attachment1 every { messageDetailsRepository.findAttachmentById("2") } returns attachment2 - val result = uploadAttachments.doWork() + val result = uploadAttachmentsWorker.doWork() coVerifyOrder { attachment1.setMessage(message) @@ -263,7 +263,7 @@ class UploadAttachmentsTest : CoroutinesTest { } every { parameters.runAttemptCount } returns 0 - val result = uploadAttachments.doWork() + val result = uploadAttachmentsWorker.doWork() assertEquals(ListenableWorker.Result.retry(), result) verify { pendingActionsDao.deletePendingUploadByMessageId("messageId8238") } @@ -298,7 +298,7 @@ class UploadAttachmentsTest : CoroutinesTest { } every { parameters.runAttemptCount } returns 4 - val result = uploadAttachments.doWork() + val result = uploadAttachmentsWorker.doWork() assertEquals( ListenableWorker.Result.failure( @@ -327,7 +327,7 @@ class UploadAttachmentsTest : CoroutinesTest { } every { parameters.runAttemptCount } returns 0 - val result = uploadAttachments.doWork() + val result = uploadAttachmentsWorker.doWork() assertEquals(ListenableWorker.Result.retry(), result) verify { pendingActionsDao.deletePendingUploadByMessageId("messageId9273584") } @@ -351,7 +351,7 @@ class UploadAttachmentsTest : CoroutinesTest { } every { parameters.runAttemptCount } returns 4 - val result = uploadAttachments.doWork() + val result = uploadAttachmentsWorker.doWork() assertEquals( ListenableWorker.Result.failure( @@ -381,7 +381,7 @@ class UploadAttachmentsTest : CoroutinesTest { every { messageDetailsRepository.findAttachmentById("1") } returns null every { messageDetailsRepository.findAttachmentById("2") } returns attachment2 - uploadAttachments.doWork() + uploadAttachmentsWorker.doWork() coVerifySequence { attachmentsRepository.upload(attachment2, crypto) } } @@ -411,7 +411,7 @@ class UploadAttachmentsTest : CoroutinesTest { every { messageDetailsRepository.findAttachmentById("1") } returns attachment1 every { messageDetailsRepository.findAttachmentById("2") } returns attachment2 - uploadAttachments.doWork() + uploadAttachmentsWorker.doWork() coVerify(exactly = 0) { attachmentsRepository.upload(attachment1, crypto) } coVerify { attachmentsRepository.upload(attachment2, crypto) } @@ -442,7 +442,7 @@ class UploadAttachmentsTest : CoroutinesTest { every { messageDetailsRepository.findAttachmentById("1") } returns attachment1 every { messageDetailsRepository.findAttachmentById("2") } returns attachment2 - uploadAttachments.doWork() + uploadAttachmentsWorker.doWork() coVerify { attachmentsRepository.upload(attachment1, crypto) } coVerify(exactly = 0) { attachmentsRepository.upload(attachment2, crypto) } @@ -473,7 +473,7 @@ class UploadAttachmentsTest : CoroutinesTest { every { messageDetailsRepository.findAttachmentById("1") } returns attachmentMock1 every { messageDetailsRepository.findAttachmentById("2") } returns attachmentMock2 - uploadAttachments.doWork() + uploadAttachmentsWorker.doWork() coVerify { attachmentsRepository.upload(attachmentMock1, crypto) } coVerify(exactly = 0) { attachmentsRepository.upload(attachmentMock2, crypto) } @@ -495,7 +495,7 @@ class UploadAttachmentsTest : CoroutinesTest { AttachmentsRepository.Result.Success("23421") } - uploadAttachments.doWork() + uploadAttachmentsWorker.doWork() coVerify { attachmentsRepository.uploadPublicKey(message, crypto) } } @@ -531,7 +531,7 @@ class UploadAttachmentsTest : CoroutinesTest { AttachmentsRepository.Result.Failure("failed") } - uploadAttachments.doWork() + uploadAttachmentsWorker.doWork() verify { attachmentMock1.deleteLocalFile() } verify(exactly = 0) { attachmentMock2.deleteLocalFile() } @@ -590,7 +590,7 @@ class UploadAttachmentsTest : CoroutinesTest { AttachmentsRepository.Result.Success(uploadedAttachmentMock2Id) } - uploadAttachments.doWork() + uploadAttachmentsWorker.doWork() val actualMessage = slot() val expectedAttachments = listOf(uploadedAttachmentMock1, uploadedAttachmentMock2) @@ -622,7 +622,7 @@ class UploadAttachmentsTest : CoroutinesTest { AttachmentsRepository.Result.Failure("Failed uploading attachment!") } - uploadAttachments.doWork() + uploadAttachmentsWorker.doWork() verify(exactly = 1) { messageDetailsRepository.findAttachmentById("1") } coVerify(exactly = 0) { messageDetailsRepository.saveMessageLocally(any()) } @@ -668,7 +668,7 @@ class UploadAttachmentsTest : CoroutinesTest { AttachmentsRepository.Result.Success(uploadedAttachment2Id) } - uploadAttachments.doWork() + uploadAttachmentsWorker.doWork() val actualMessage = slot() val expectedAttachments = listOf(attachmentMock1) @@ -690,7 +690,7 @@ class UploadAttachmentsTest : CoroutinesTest { coEvery { messageDetailsRepository.findMessageById(messageId) } returns message coEvery { attachmentsRepository.uploadPublicKey(message, crypto) } returns mockk() - val result = uploadAttachments.doWork() + val result = uploadAttachmentsWorker.doWork() coVerify(exactly = 0) { attachmentsRepository.uploadPublicKey(any(), any()) } assertEquals(ListenableWorker.Result.success(), result) @@ -713,7 +713,7 @@ class UploadAttachmentsTest : CoroutinesTest { AttachmentsRepository.Result.Success("82384") } - val result = uploadAttachments.doWork() + val result = uploadAttachmentsWorker.doWork() coVerify { attachmentsRepository.uploadPublicKey(any(), any()) } assertEquals(ListenableWorker.Result.success(), result) diff --git a/app/src/test/java/ch/protonmail/android/usecase/compose/SaveDraftTest.kt b/app/src/test/java/ch/protonmail/android/usecase/compose/SaveDraftTest.kt index 9b7c5c5d1..fd93b7154 100644 --- a/app/src/test/java/ch/protonmail/android/usecase/compose/SaveDraftTest.kt +++ b/app/src/test/java/ch/protonmail/android/usecase/compose/SaveDraftTest.kt @@ -27,7 +27,7 @@ import ch.protonmail.android.api.models.room.messages.Message import ch.protonmail.android.api.models.room.pendingActions.PendingActionsDao import ch.protonmail.android.api.models.room.pendingActions.PendingSend import ch.protonmail.android.attachments.KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR -import ch.protonmail.android.attachments.UploadAttachments +import ch.protonmail.android.attachments.UploadAttachmentsWorker import ch.protonmail.android.core.Constants.MessageActionType.FORWARD import ch.protonmail.android.core.Constants.MessageActionType.NONE import ch.protonmail.android.core.Constants.MessageActionType.REPLY @@ -72,7 +72,7 @@ class SaveDraftTest : CoroutinesTest { private lateinit var userNotifier: UserNotifier @RelaxedMockK - private lateinit var uploadAttachments: UploadAttachments.Enqueuer + private lateinit var uploadAttachmentsWorker: UploadAttachmentsWorker.Enqueuer @RelaxedMockK private lateinit var createDraftScheduler: Enqueuer @@ -351,7 +351,7 @@ class SaveDraftTest : CoroutinesTest { "previousSenderId132423" ) } answers { workerStatusFlow } - coEvery { uploadAttachments.enqueue(any(), "createdDraftMessageId", false) } returns buildWorkerResponse( + coEvery { uploadAttachmentsWorker.enqueue(any(), "createdDraftMessageId", false) } returns buildWorkerResponse( WorkInfo.State.SUCCEEDED ) @@ -404,7 +404,7 @@ class SaveDraftTest : CoroutinesTest { } answers { workerStatusFlow } val addressCrypto = mockk(relaxed = true) every { addressCryptoFactory.create(Id("addressId"), Name(currentUsername)) } returns addressCrypto - coEvery { uploadAttachments.enqueue(any(), "createdDraftMessageId345", false) } returns buildWorkerResponse( + coEvery { uploadAttachmentsWorker.enqueue(any(), "createdDraftMessageId345", false) } returns buildWorkerResponse( WorkInfo.State.SUCCEEDED ) @@ -421,7 +421,7 @@ class SaveDraftTest : CoroutinesTest { ) // Then - coVerify { uploadAttachments.enqueue(newAttachmentIds, "createdDraftMessageId345", false) } + coVerify { uploadAttachmentsWorker.enqueue(newAttachmentIds, "createdDraftMessageId345", false) } } } @@ -456,7 +456,7 @@ class SaveDraftTest : CoroutinesTest { } answers { workerStatusFlow } val addressCrypto = mockk(relaxed = true) every { addressCryptoFactory.create(Id("addressId"), Name(currentUsername)) } returns addressCrypto - coEvery { uploadAttachments.enqueue(any(), "createdDraftMessageId346", true) } returns buildWorkerResponse( + coEvery { uploadAttachmentsWorker.enqueue(any(), "createdDraftMessageId346", true) } returns buildWorkerResponse( WorkInfo.State.SUCCEEDED ) @@ -473,7 +473,7 @@ class SaveDraftTest : CoroutinesTest { ) // Then - coVerify { uploadAttachments.enqueue(newAttachmentIds, "createdDraftMessageId346", true) } + coVerify { uploadAttachmentsWorker.enqueue(newAttachmentIds, "createdDraftMessageId346", true) } assertEquals(SaveDraftResult.Success(apiDraft.messageId!!), result) } } @@ -523,7 +523,7 @@ class SaveDraftTest : CoroutinesTest { ) // Then - coVerify(exactly = 0) { uploadAttachments.enqueue(any(), any(), any()) } + coVerify(exactly = 0) { uploadAttachmentsWorker.enqueue(any(), any(), any()) } assertEquals(SaveDraftResult.Success(apiDraft.messageId!!), result) } } @@ -643,7 +643,7 @@ class SaveDraftTest : CoroutinesTest { coEvery { messageDetailsRepository.saveMessageLocally(message) } returns 9833L coEvery { messageDetailsRepository.findMessageById("newDraftId") } returns message.copy(messageId = "newDraftId") coEvery { messageDetailsRepository.findMessageById("45623") } returns message - coEvery { uploadAttachments.enqueue(newAttachmentIds, "newDraftId", false) } returns buildWorkerResponse( + coEvery { uploadAttachmentsWorker.enqueue(newAttachmentIds, "newDraftId", false) } returns buildWorkerResponse( WorkInfo.State.FAILED, uploadWorkOutputData ) @@ -697,7 +697,7 @@ class SaveDraftTest : CoroutinesTest { messageDetailsRepository.findMessageById("UploadDraftAttachmentsFailed") } returns message.copy(messageId = "newDraftId2384") coEvery { messageDetailsRepository.findMessageById("45623") } returns message - coEvery { uploadAttachments.enqueue(newAttachmentIds, "newDraftId2384", false) } returns buildWorkerResponse( + coEvery { uploadAttachmentsWorker.enqueue(newAttachmentIds, "newDraftId2384", false) } returns buildWorkerResponse( WorkInfo.State.CANCELLED ) every { @@ -757,7 +757,7 @@ class SaveDraftTest : CoroutinesTest { } answers { workerStatusFlow } val addressCrypto = mockk(relaxed = true) every { addressCryptoFactory.create(Id("addressId"), Name(currentUsername)) } returns addressCrypto - coEvery { uploadAttachments.enqueue(any(), "createdDraftMessageId345", false) } returns buildWorkerResponse( + coEvery { uploadAttachmentsWorker.enqueue(any(), "createdDraftMessageId345", false) } returns buildWorkerResponse( WorkInfo.State.SUCCEEDED ) @@ -774,7 +774,7 @@ class SaveDraftTest : CoroutinesTest { ) // Then - verify { uploadAttachments.enqueue(newAttachmentIds, "createdDraftMessageId345", false) } + verify { uploadAttachmentsWorker.enqueue(newAttachmentIds, "createdDraftMessageId345", false) } assertEquals(SaveDraftResult.Success("createdDraftMessageId345"), result) } } From 0aa4e656221432bbb8bcdbdf4490f0c1428a4372 Mon Sep 17 00:00:00 2001 From: Zorica Stojchevska Date: Thu, 18 Nov 2021 23:45:04 +0100 Subject: [PATCH 2/5] Add new type of notification error for failures with uploading attachments --- .../compose/send/SendMessageWorkerError.kt | 3 ++- .../notification/INotificationServer.kt | 6 ++++++ .../servers/notification/NotificationServer.kt | 18 ++++++++++++++++++ .../utils/notifier/AndroidUserNotifier.kt | 6 +++++- .../android/utils/notifier/UserNotifier.kt | 1 + 5 files changed, 32 insertions(+), 2 deletions(-) diff --git a/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorkerError.kt b/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorkerError.kt index aff102cdf..8d3686736 100644 --- a/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorkerError.kt +++ b/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorkerError.kt @@ -28,5 +28,6 @@ enum class SendMessageWorkerError { ErrorPerformingApiRequest, UserVerificationNeeded, ApiRequestReturnedBadBodyCode, - MessageAlreadySent + MessageAlreadySent, + UploadAttachmentsFailed } diff --git a/app/src/main/java/ch/protonmail/android/servers/notification/INotificationServer.kt b/app/src/main/java/ch/protonmail/android/servers/notification/INotificationServer.kt index 4b2b9380d..258ae5c7e 100644 --- a/app/src/main/java/ch/protonmail/android/servers/notification/INotificationServer.kt +++ b/app/src/main/java/ch/protonmail/android/servers/notification/INotificationServer.kt @@ -93,4 +93,10 @@ interface INotificationServer { ) fun notifySaveDraftError(errorMessage: String, messageSubject: String?, username: String) + + fun notifyAttachmentUploadError( + errorMessage: String, + messageSubject: String?, + username: String + ) } diff --git a/app/src/main/java/ch/protonmail/android/servers/notification/NotificationServer.kt b/app/src/main/java/ch/protonmail/android/servers/notification/NotificationServer.kt index 428cabe8c..1e314bdbf 100644 --- a/app/src/main/java/ch/protonmail/android/servers/notification/NotificationServer.kt +++ b/app/src/main/java/ch/protonmail/android/servers/notification/NotificationServer.kt @@ -594,4 +594,22 @@ class NotificationServer @Inject constructor( notificationManager.notify(username.hashCode() + NOTIFICATION_ID_SAVE_DRAFT_ERROR, notification) } + override fun notifyAttachmentUploadError( + errorMessage: String, + messageSubject: String?, + username: String + ) { + val title = context.getString(R.string.failed_uploading_attachment_online, messageSubject) + + val bigTextStyle = NotificationCompat.BigTextStyle() + .setBigContentTitle(title) + .setSummaryText(username) + .bigText(errorMessage) + + val notificationBuilder = createGenericErrorSendingMessageNotification(username) + .setStyle(bigTextStyle) + + val notification = notificationBuilder.build() + notificationManager.notify(username.hashCode() + NOTIFICATION_ID_SAVE_DRAFT_ERROR, notification) + } } diff --git a/app/src/main/java/ch/protonmail/android/utils/notifier/AndroidUserNotifier.kt b/app/src/main/java/ch/protonmail/android/utils/notifier/AndroidUserNotifier.kt index 7a2d098b3..24052a720 100644 --- a/app/src/main/java/ch/protonmail/android/utils/notifier/AndroidUserNotifier.kt +++ b/app/src/main/java/ch/protonmail/android/utils/notifier/AndroidUserNotifier.kt @@ -53,6 +53,11 @@ class AndroidUserNotifier @Inject constructor( notificationServer.notifySingleErrorSendingMessage(error, userManager.username) } + override fun showAttachmentUploadError(errorMessage: String, attachmentName: String?, messageSubject: String?) { + val error = "\"$attachmentName\" - $errorMessage" + notificationServer.notifyAttachmentUploadError(error, messageSubject, userManager.username) + } + override suspend fun showMessageSent() { withContext(dispatchers.Main) { context.showToast(R.string.message_sent) @@ -68,5 +73,4 @@ class AndroidUserNotifier @Inject constructor( message.addressID ) } - } diff --git a/app/src/main/java/ch/protonmail/android/utils/notifier/UserNotifier.kt b/app/src/main/java/ch/protonmail/android/utils/notifier/UserNotifier.kt index a7fc09c34..1407f7510 100644 --- a/app/src/main/java/ch/protonmail/android/utils/notifier/UserNotifier.kt +++ b/app/src/main/java/ch/protonmail/android/utils/notifier/UserNotifier.kt @@ -23,6 +23,7 @@ interface UserNotifier { fun showPersistentError(errorMessage: String, messageSubject: String?) fun showError(errorMessage: String) fun showSendMessageError(errorMessage: String, messageSubject: String?) + fun showAttachmentUploadError(errorMessage: String, attachmentName: String?, messageSubject: String?) suspend fun showMessageSent() fun showHumanVerificationNeeded(message: ch.protonmail.android.api.models.room.messages.Message) } From be4d40463eebec3c23172f796be81e7c4aa47609 Mon Sep 17 00:00:00 2001 From: Zorica Stojchevska Date: Fri, 19 Nov 2021 00:35:31 +0100 Subject: [PATCH 3/5] Change how we resolve if attachment isUploaded or not Now in order to consider an attachment as uploaded we check the `attachmentId` if it's not an integer then that attachment has been uploaded. --- .../api/models/room/messages/Attachment.kt | 25 +++++++++++-------- .../protonmail/android/utils/MessageUtils.kt | 11 ++++++++ 2 files changed, 26 insertions(+), 10 deletions(-) diff --git a/app/src/main/java/ch/protonmail/android/api/models/room/messages/Attachment.kt b/app/src/main/java/ch/protonmail/android/api/models/room/messages/Attachment.kt index a8a06c03c..745bc3656 100644 --- a/app/src/main/java/ch/protonmail/android/api/models/room/messages/Attachment.kt +++ b/app/src/main/java/ch/protonmail/android/api/models/room/messages/Attachment.kt @@ -29,6 +29,7 @@ import androidx.room.Index import androidx.room.PrimaryKey import ch.protonmail.android.api.models.AttachmentHeaders import ch.protonmail.android.utils.AppUtil +import ch.protonmail.android.utils.MessageUtils.isLocalAttachmentId import com.google.gson.annotations.Expose import com.google.gson.annotations.SerializedName import java.io.ByteArrayOutputStream @@ -79,7 +80,7 @@ data class Attachment @JvmOverloads constructor( @ColumnInfo(name = COLUMN_ATTACHMENT_MESSAGE_ID) var messageId: String = "", @ColumnInfo(name = COLUMN_ATTACHMENT_UPLOADED) - var isUploaded: Boolean = false, + var isUploaded: Boolean = !isLocalAttachmentId(attachmentId), @ColumnInfo(name = COLUMN_ATTACHMENT_UPLOADING) var isUploading: Boolean = false, @ColumnInfo(name = COLUMN_ATTACHMENT_SIGNATURE) @@ -203,17 +204,16 @@ data class Attachment @JvmOverloads constructor( keyPackets = localAttachment.keyPackets, headers = localAttachment.headers ) - } @Throws(MessagingException::class, NoSuchAlgorithmException::class, IOException::class) - fun fromMimeAttachment(part: Part, message_id: String, count: Int): Attachment { + fun fromMimeAttachment(part: Part, messageId: String, count: Int): Attachment { val data = getBytesFromInputStream(part.inputStream) - return fromMimeAttachment(part, data, message_id, count) + return fromMimeAttachment(part, data, messageId, count) } @Throws(MessagingException::class, NoSuchAlgorithmException::class, IOException::class) - fun fromMimeAttachment(part: Part, data: ByteArray, message_id: String, count: Int): Attachment { + fun fromMimeAttachment(part: Part, data: ByteArray, messageId: String, count: Int): Attachment { val attachment = Attachment() attachment.fileName = part.fileName ?: "default" @@ -237,18 +237,18 @@ data class Attachment @JvmOverloads constructor( md5.digest().forEach { b -> format.format("%02x", b) } - attachment.attachmentId = "PGPAttachment/$message_id/$format/$count" + attachment.attachmentId = "PGPAttachment/$messageId/$format/$count" } attachment.fileSize = data.size.toLong() attachment.mimeType = part.contentType.replace("(\\s*)?[\\r\\n]+(\\s*)".toRegex(), "") attachment.mimeData = data - attachment.messageId = message_id + attachment.messageId = messageId return attachment } @Throws(MessagingException::class, NoSuchAlgorithmException::class, IOException::class) - fun fromMimeAttachment(data: ByteArray, headers: InternetHeaders, message_id: String, count: Int): Attachment { - return fromMimeAttachment(MimeBodyPart(headers, ByteArray(0)), data, message_id, count) + fun fromMimeAttachment(data: ByteArray, headers: InternetHeaders, messageId: String, count: Int): Attachment { + return fromMimeAttachment(MimeBodyPart(headers, ByteArray(0)), data, messageId, count) } @Throws(IOException::class) @@ -273,7 +273,12 @@ data class Attachment @JvmOverloads constructor( useRandomIds: Boolean = true ): List { return localAttachmentList.map { localAttachment -> - fromLocalAttachment(messagesDatabase, localAttachment, localAttachmentList.indexOf(localAttachment).toLong(), useRandomIds) + fromLocalAttachment( + messagesDatabase, + localAttachment, + localAttachmentList.indexOf(localAttachment).toLong(), + useRandomIds + ) } } diff --git a/app/src/main/java/ch/protonmail/android/utils/MessageUtils.kt b/app/src/main/java/ch/protonmail/android/utils/MessageUtils.kt index 8a40769be..f2393b48f 100644 --- a/app/src/main/java/ch/protonmail/android/utils/MessageUtils.kt +++ b/app/src/main/java/ch/protonmail/android/utils/MessageUtils.kt @@ -211,6 +211,17 @@ object MessageUtils { return valid } + fun isLocalAttachmentId(attachmentId: String?): Boolean { + var valid = false + try { + attachmentId?.toBigInteger() + valid = true + } catch (e: IllegalArgumentException) { + // noop + } + return valid + } + fun calculateType(flags: Long): Message.MessageType { val received = flags and MessageFlag.RECEIVED.value == MessageFlag.RECEIVED.value val sent = flags and MessageFlag.SENT.value == MessageFlag.SENT.value From ddf79f05625ac1bc28137356ce823cc197fe00d6 Mon Sep 17 00:00:00 2001 From: Zorica Stojchevska Date: Fri, 19 Nov 2021 01:18:22 +0100 Subject: [PATCH 4/5] Fix sending message with wrong attachmentIds We split the handling of attachment that are already uploaded, we skip the upload in the worker and we don't upload them again. And for the attachment that are not uploaded but have missing needed data like `filePath` in this case we now fail permanantly the worker, and the whole flow after it, chained to it. MAILAND-2599 --- .../attachments/UploadAttachmentsWorker.kt | 51 +++++++++--- .../android/compose/send/SendMessageWorker.kt | 6 +- .../notification/NotificationServer.kt | 83 ++++++++++--------- .../android/usecase/compose/SaveDraft.kt | 2 +- .../protonmail/android/utils/MessageUtils.kt | 10 +-- .../utils/notifier/AndroidUserNotifier.kt | 5 +- .../android/utils/notifier/UserNotifier.kt | 2 +- app/src/main/res/values/strings.xml | 2 + .../UploadAttachmentsWorkerTest.kt | 75 ++++++++++++++--- .../compose/send/SendMessageWorkerTest.kt | 30 +++++++ .../android/usecase/compose/SaveDraftTest.kt | 83 ++++++++++++++++++- .../android/utils/MessageUtilsTest.kt | 62 ++++++++++++++ 12 files changed, 336 insertions(+), 75 deletions(-) create mode 100644 app/src/test/java/ch/protonmail/android/utils/MessageUtilsTest.kt diff --git a/app/src/main/java/ch/protonmail/android/attachments/UploadAttachmentsWorker.kt b/app/src/main/java/ch/protonmail/android/attachments/UploadAttachmentsWorker.kt index 7457874a5..bf7d3853f 100644 --- a/app/src/main/java/ch/protonmail/android/attachments/UploadAttachmentsWorker.kt +++ b/app/src/main/java/ch/protonmail/android/attachments/UploadAttachmentsWorker.kt @@ -33,6 +33,7 @@ import androidx.work.WorkInfo import androidx.work.WorkManager import androidx.work.WorkerParameters import androidx.work.workDataOf +import ch.protonmail.android.R import ch.protonmail.android.activities.messageDetails.repository.MessageDetailsRepository import ch.protonmail.android.api.models.room.messages.Message import ch.protonmail.android.api.models.room.pendingActions.PendingActionsDao @@ -81,7 +82,14 @@ class UploadAttachmentsWorker @WorkerInject constructor( Timber.w("Calling upload attachments from inside worker for $messageId.") return@withContext when (val result = upload(newAttachments, message, addressCrypto, isMessageSending)) { is Result.Success -> ListenableWorker.Result.success() - is Result.Failure -> retryOrFail(result.error, messageId = message.messageId) + is Result.Failure.UploadAttachment -> retryOrFail( + result.error, + messageId = message.messageId + ) + is Result.Failure.InvalidAttachment -> failureWithError( + result.error, + messageId = message.messageId + ) } } @@ -111,7 +119,7 @@ class UploadAttachmentsWorker @WorkerInject constructor( if (result is AttachmentsRepository.Result.Failure) { pendingActionsDao.deletePendingUploadByMessageId(messageId) - return@withContext Result.Failure(result.error) + return@withContext Result.Failure.UploadAttachment(result.error) } } @@ -126,12 +134,20 @@ class UploadAttachmentsWorker @WorkerInject constructor( messageId: String ): Result.Failure? { attachmentIds.forEach { attachmentId -> - val attachment = messageDetailsRepository.findAttachmentById(attachmentId) + val attachment = messageDetailsRepository.findAttachmentById(attachmentId) ?: return@forEach + + if (!attachment.isUploaded && (attachment.filePath == null || attachment.doesFileExist.not())) { + return Result.Failure.InvalidAttachment( + String.format(context.getString(R.string.attachment_failed_message_drafted), + attachment.fileName + ) + ) + } - if (attachment?.filePath == null || attachment.isUploaded || attachment.doesFileExist.not()) { + if (attachment.isUploaded) { Timber.d( - "Skipping attachment ${attachment?.attachmentId}: " + - "not found, invalid or was already uploaded = ${attachment?.isUploaded}" + "Skipping attachment ${attachment.attachmentId}: " + + "was already uploaded = ${attachment.isUploaded}" ) return@forEach } @@ -149,7 +165,7 @@ class UploadAttachmentsWorker @WorkerInject constructor( is AttachmentsRepository.Result.Failure -> { Timber.e("UploadAttachment $attachmentId to API for messageId $messageId FAILED.") pendingActionsDao.deletePendingUploadByMessageId(messageId) - return Result.Failure(result.error) + return Result.Failure.UploadAttachment(result.error) } } @@ -188,15 +204,28 @@ class UploadAttachmentsWorker @WorkerInject constructor( return failureWithError(error, exception, messageId) } - private fun failureWithError(error: String, exception: Throwable? = null, messageId: String?): ListenableWorker.Result { - Timber.e("UploadAttachments Worker failed permanently for $messageId. error = $error, exception = $exception. FAILING") - val errorData = workDataOf(KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to error) + private fun failureWithError( + error: String, + exception: Throwable? = null, + messageId: String? + ): ListenableWorker.Result { + Timber.e("UploadAttachments Worker failed permanently for " + + "$messageId. error = $error, exception = $exception. FAILING") + val errorData = workDataOf( + KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to error, + ) + messageId?.let { pendingActionsDao.deletePendingUploadByMessageId(it) } return ListenableWorker.Result.failure(errorData) } sealed class Result { object Success : Result() - data class Failure(val error: String) : Result() + sealed class Failure : Result() { + abstract val error: String + + class UploadAttachment(override val error: String) : Failure() + class InvalidAttachment(override val error: String) : Failure() + } } class Enqueuer @Inject constructor(private val workManager: WorkManager) { diff --git a/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt b/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt index 2e2fe5e69..0455d1c6b 100644 --- a/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt +++ b/app/src/main/java/ch/protonmail/android/compose/send/SendMessageWorker.kt @@ -55,6 +55,7 @@ import ch.protonmail.android.compose.send.SendMessageWorkerError.FetchSendPrefer import ch.protonmail.android.compose.send.SendMessageWorkerError.MessageAlreadySent import ch.protonmail.android.compose.send.SendMessageWorkerError.MessageNotFound import ch.protonmail.android.compose.send.SendMessageWorkerError.SavedDraftMessageNotFound +import ch.protonmail.android.compose.send.SendMessageWorkerError.UploadAttachmentsFailed import ch.protonmail.android.compose.send.SendMessageWorkerError.UserVerificationNeeded import ch.protonmail.android.core.Constants import ch.protonmail.android.core.Constants.MessageLocationType.ALL_MAIL @@ -172,7 +173,10 @@ class SendMessageWorker @WorkerInject constructor( saveMessageAsSent(message) failureWithError(MessageAlreadySent) } - + is SaveDraftResult.UploadDraftAttachmentsFailed -> { + pendingActionsDao.deletePendingSendByMessageId(message.messageId ?: "") + failureWithError(UploadAttachmentsFailed) + } else -> { pendingActionsDao.deletePendingSendByMessageId(message.messageId ?: "") showSendMessageError(message.subject) diff --git a/app/src/main/java/ch/protonmail/android/servers/notification/NotificationServer.kt b/app/src/main/java/ch/protonmail/android/servers/notification/NotificationServer.kt index 1e314bdbf..e9b9ceb18 100644 --- a/app/src/main/java/ch/protonmail/android/servers/notification/NotificationServer.kt +++ b/app/src/main/java/ch/protonmail/android/servers/notification/NotificationServer.kt @@ -69,6 +69,7 @@ private const val CHANNEL_ID_ONGOING_OPS = "ongoingOperations" private const val CHANNEL_ID_ACCOUNT = "account" private const val CHANNEL_ID_ATTACHMENTS = "attachments" private const val NOTIFICATION_ID_SAVE_DRAFT_ERROR = 6812 +private const val NOTIFICATION_ID_ATTACHMENT_ERROR = 6813 private const val NOTIFICATION_GROUP_ID_EMAIL = 99 private const val NOTIFICATION_ID_VERIFICATION = 2 private const val NOTIFICATION_ID_LOGGED_OUT = 3 @@ -347,7 +348,7 @@ class NotificationServer @Inject constructor( "com.android.systemui", ringtoneUri, Intent.FLAG_GRANT_READ_URI_PERMISSION ) } - } ?: RingtoneManager.getDefaultUri(TYPE_NOTIFICATION) + } ?: RingtoneManager.getDefaultUri(TYPE_NOTIFICATION) } catch (e: Exception) { Timber.i(e, "Unable to set notification ringtone") RingtoneManager.getDefaultUri(TYPE_NOTIFICATION) @@ -557,41 +558,54 @@ class NotificationServer @Inject constructor( notificationManager.notify(username.hashCode() + NOTIFICATION_ID_SENDING_FAILED, notification) } - override fun notifyMultipleErrorSendingMessage( - unreadSendingFailedNotifications: List, - user: User + private fun notifyErrorWithTitle( + username: String, + title: String, + bigText: String, + summaryText: String = username, + notificationID: Int ) { - - val notificationTitle = context.getString(R.string.message_sending_failures, unreadSendingFailedNotifications.size) - - // Create Notification Style val bigTextStyle = NotificationCompat.BigTextStyle() - .setBigContentTitle(notificationTitle) - .setSummaryText(user.defaultEmail ?: user.username ?: context.getString(R.string.app_name)) - .bigText(createSpannableBigText(unreadSendingFailedNotifications)) + .setBigContentTitle(title) + .setSummaryText(summaryText) + .bigText(bigText) - // Create notification builder - val notificationBuilder = createGenericErrorSendingMessageNotification(user.username) + val notificationBuilder = createGenericErrorSendingMessageNotification(username) .setStyle(bigTextStyle) - // Build and show notification val notification = notificationBuilder.build() - notificationManager.notify(user.username.hashCode() + NOTIFICATION_ID_SENDING_FAILED, notification) + notificationManager.notify(username.hashCode() + notificationID, notification) } - override fun notifySaveDraftError(errorMessage: String, messageSubject: String?, username: String) { - val title = context.getString(R.string.failed_saving_draft_online, messageSubject) - - val bigTextStyle = NotificationCompat.BigTextStyle() - .setBigContentTitle(title) - .setSummaryText(username) - .bigText(errorMessage) + override fun notifyMultipleErrorSendingMessage( + unreadSendingFailedNotifications: List, + user: User + ) { + val notificationTitle = context.getString( + R.string.message_sending_failures, unreadSendingFailedNotifications.size + ) - val notificationBuilder = createGenericErrorSendingMessageNotification(username) - .setStyle(bigTextStyle) + notifyErrorWithTitle( + user.username, + notificationTitle, + createSpannableBigText(unreadSendingFailedNotifications).toString(), + user.defaultEmail ?: user.username ?: context.getString(R.string.app_name), + NOTIFICATION_ID_SENDING_FAILED + ) + } - val notification = notificationBuilder.build() - notificationManager.notify(username.hashCode() + NOTIFICATION_ID_SAVE_DRAFT_ERROR, notification) + override fun notifySaveDraftError( + errorMessage: String, + messageSubject: String?, + username: String + ) { + val title = context.getString(R.string.failed_saving_draft_online, messageSubject) + notifyErrorWithTitle( + username, + title, + errorMessage, + notificationID = NOTIFICATION_ID_SAVE_DRAFT_ERROR + ) } override fun notifyAttachmentUploadError( @@ -600,16 +614,11 @@ class NotificationServer @Inject constructor( username: String ) { val title = context.getString(R.string.failed_uploading_attachment_online, messageSubject) - - val bigTextStyle = NotificationCompat.BigTextStyle() - .setBigContentTitle(title) - .setSummaryText(username) - .bigText(errorMessage) - - val notificationBuilder = createGenericErrorSendingMessageNotification(username) - .setStyle(bigTextStyle) - - val notification = notificationBuilder.build() - notificationManager.notify(username.hashCode() + NOTIFICATION_ID_SAVE_DRAFT_ERROR, notification) + notifyErrorWithTitle( + username, + title, + errorMessage, + notificationID = NOTIFICATION_ID_ATTACHMENT_ERROR + ) } } diff --git a/app/src/main/java/ch/protonmail/android/usecase/compose/SaveDraft.kt b/app/src/main/java/ch/protonmail/android/usecase/compose/SaveDraft.kt index ccb407f7d..548863285 100644 --- a/app/src/main/java/ch/protonmail/android/usecase/compose/SaveDraft.kt +++ b/app/src/main/java/ch/protonmail/android/usecase/compose/SaveDraft.kt @@ -142,7 +142,7 @@ class SaveDraft @Inject constructor( val errorMessage = requireNotNull( it.outputData.getString(KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR) ) - userNotifier.showPersistentError(errorMessage, localDraft.subject) + userNotifier.showAttachmentUploadError(errorMessage, localDraft.subject) return@map SaveDraftResult.UploadDraftAttachmentsFailed } if (it?.state == WorkInfo.State.CANCELLED) { diff --git a/app/src/main/java/ch/protonmail/android/utils/MessageUtils.kt b/app/src/main/java/ch/protonmail/android/utils/MessageUtils.kt index f2393b48f..282659295 100644 --- a/app/src/main/java/ch/protonmail/android/utils/MessageUtils.kt +++ b/app/src/main/java/ch/protonmail/android/utils/MessageUtils.kt @@ -212,14 +212,12 @@ object MessageUtils { } fun isLocalAttachmentId(attachmentId: String?): Boolean { - var valid = false - try { + return try { attachmentId?.toBigInteger() - valid = true - } catch (e: IllegalArgumentException) { - // noop + true + } catch (e: NumberFormatException) { + false } - return valid } fun calculateType(flags: Long): Message.MessageType { diff --git a/app/src/main/java/ch/protonmail/android/utils/notifier/AndroidUserNotifier.kt b/app/src/main/java/ch/protonmail/android/utils/notifier/AndroidUserNotifier.kt index 24052a720..81660eb20 100644 --- a/app/src/main/java/ch/protonmail/android/utils/notifier/AndroidUserNotifier.kt +++ b/app/src/main/java/ch/protonmail/android/utils/notifier/AndroidUserNotifier.kt @@ -53,9 +53,8 @@ class AndroidUserNotifier @Inject constructor( notificationServer.notifySingleErrorSendingMessage(error, userManager.username) } - override fun showAttachmentUploadError(errorMessage: String, attachmentName: String?, messageSubject: String?) { - val error = "\"$attachmentName\" - $errorMessage" - notificationServer.notifyAttachmentUploadError(error, messageSubject, userManager.username) + override fun showAttachmentUploadError(errorMessage: String, messageSubject: String?) { + notificationServer.notifyAttachmentUploadError(errorMessage, messageSubject, userManager.username) } override suspend fun showMessageSent() { diff --git a/app/src/main/java/ch/protonmail/android/utils/notifier/UserNotifier.kt b/app/src/main/java/ch/protonmail/android/utils/notifier/UserNotifier.kt index 1407f7510..93213411a 100644 --- a/app/src/main/java/ch/protonmail/android/utils/notifier/UserNotifier.kt +++ b/app/src/main/java/ch/protonmail/android/utils/notifier/UserNotifier.kt @@ -23,7 +23,7 @@ interface UserNotifier { fun showPersistentError(errorMessage: String, messageSubject: String?) fun showError(errorMessage: String) fun showSendMessageError(errorMessage: String, messageSubject: String?) - fun showAttachmentUploadError(errorMessage: String, attachmentName: String?, messageSubject: String?) + fun showAttachmentUploadError(errorMessage: String, messageSubject: String?) suspend fun showMessageSent() fun showHumanVerificationNeeded(message: ch.protonmail.android.api.models.room.messages.Message) } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index f256e3eb9..60c91f9d8 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -63,6 +63,7 @@ along with ProtonMail. If not, see https://www.gnu.org/licenses/. Email address is a duplicate Code is not valid Message sending failed. Your messages are saved in drafts folder + Please remove and re-attach \"%s\" and send the message again. Your message can be found in the "Drafts" folder. Open Close INBOX @@ -1102,6 +1103,7 @@ along with ProtonMail. If not, see https://www.gnu.org/licenses/. Invalid Firebase API key. Push notifications will not work. Failed saving online draft for message: "%s" + Attachment error for message: \"%s\" Can\'t open this message while it\'s being sent diff --git a/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsWorkerTest.kt b/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsWorkerTest.kt index 686224941..9631250b6 100644 --- a/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsWorkerTest.kt +++ b/app/src/test/java/ch/protonmail/android/attachments/UploadAttachmentsWorkerTest.kt @@ -28,6 +28,7 @@ import androidx.work.OneTimeWorkRequest import androidx.work.WorkManager import androidx.work.WorkerParameters import androidx.work.workDataOf +import ch.protonmail.android.R import ch.protonmail.android.activities.messageDetails.repository.MessageDetailsRepository import ch.protonmail.android.api.models.room.messages.Attachment import ch.protonmail.android.api.models.room.messages.Message @@ -173,7 +174,9 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { assertEquals( ListenableWorker.Result.failure( - workDataOf(KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to "Message not found") + workDataOf( + KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to "Message not found", + ) ), result ) @@ -184,12 +187,23 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { @Test fun uploadAttachmentsExecutesNormallyWhenUploadWasLeftOngoingForTheGivenMessage() { runBlockingTest { + val attachment1 = mockk(relaxed = true) { + every { attachmentId } returns "1" + every { filePath } returns "filePath1" + every { isUploaded } returns false + every { doesFileExist } returns true + } val attachmentIds = listOf("1") val messageId = "message-id-123842" val message = Message(messageId = messageId, addressID = "senderAddress1") givenFullValidInput(messageId, attachmentIds.toTypedArray()) + every { cryptoFactory.create(Id("senderAddress1"), Name(currentUsername)) } returns crypto coEvery { messageDetailsRepository.findMessageById(messageId) } returns message every { pendingActionsDao.findPendingUploadByMessageId(messageId) } returns PendingUpload(messageId) + every { messageDetailsRepository.findAttachmentById("1") } returns attachment1 + coEvery { attachmentsRepository.upload(attachment1, crypto) } answers { + AttachmentsRepository.Result.Success("1") + } val result = uploadAttachmentsWorker.doWork() @@ -275,12 +289,14 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { runBlockingTest { val attachment1 = mockk(relaxed = true) { every { attachmentId } returns "1" + every { fileName } returns "Attachment_1.jpg" every { filePath } returns "filePath1" every { isUploaded } returns false every { doesFileExist } returns true } val attachment2 = mockk(relaxed = true) { every { attachmentId } returns "2" + every { fileName } returns "Attachment_2.jpg" every { filePath } returns "filePath2" every { isUploaded } returns false every { doesFileExist } returns true @@ -302,7 +318,9 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { assertEquals( ListenableWorker.Result.failure( - workDataOf(KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to "Failed to upload attachment2") + workDataOf( + KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to "Failed to upload attachment2", + ) ), result ) @@ -313,6 +331,12 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { @Test fun uploadAttachmentsRetriesIfPublicKeyFailsToBeUploadedAndMaxRetriesWereNotReached() { runBlockingTest { + val attachment1 = mockk(relaxed = true) { + every { attachmentId } returns "1" + every { filePath } returns "filePath1" + every { isUploaded } returns false + every { doesFileExist } returns true + } val attachmentIds = listOf("1") val messageId = "messageId9273584" val username = "username" @@ -321,6 +345,7 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { every { cryptoFactory.create(Id("senderAddress13"), Name(username)) } returns crypto coEvery { messageDetailsRepository.findMessageById(messageId) } returns message every { userManager.username } returns username + every { messageDetailsRepository.findAttachmentById("1") } returns attachment1 every { userManager.getMailSettings(username)?.getAttachPublicKey() } returns true coEvery { attachmentsRepository.uploadPublicKey(message, crypto) } answers { AttachmentsRepository.Result.Failure("Failed to upload public key") @@ -337,6 +362,12 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { @Test fun uploadAttachmentsReturnsFailureIfPublicKeyFailsToBeUploadedAndMaxRetriesWereReached() { runBlockingTest { + val attachment1 = mockk(relaxed = true) { + every { attachmentId } returns "1" + every { filePath } returns "filePath1" + every { isUploaded } returns false + every { doesFileExist } returns true + } val attachmentIds = listOf("1") val messageId = "messageId9273585" val username = "username" @@ -345,6 +376,7 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { every { cryptoFactory.create(Id("senderAddress12"), Name(username)) } returns crypto coEvery { messageDetailsRepository.findMessageById(messageId) } returns message every { userManager.username } returns username + every { messageDetailsRepository.findAttachmentById("1") } returns attachment1 every { userManager.getMailSettings(username)?.getAttachPublicKey() } returns true coEvery { attachmentsRepository.uploadPublicKey(message, crypto) } answers { AttachmentsRepository.Result.Failure("Failed to upload public key") @@ -355,7 +387,9 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { assertEquals( ListenableWorker.Result.failure( - workDataOf(KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to "Failed to upload public key") + workDataOf( + KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to "Failed to upload public key", + ) ), result ) @@ -388,16 +422,18 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { } @Test - fun uploadAttachmentsSkipsUploadingIfAttachmentFilePathIsNull() { + fun uploadAttachmentsFailsWorkerIfAttachmentFilePathIsNull() { runBlockingTest { val attachment1 = mockk(relaxed = true) { every { attachmentId } returns "1" + every { fileName } returns "Attachment_1.jpg" every { filePath } returns null every { isUploaded } returns false every { doesFileExist } returns true } val attachment2 = mockk(relaxed = true) { every { attachmentId } returns "2" + every { fileName } returns "Attachment_2.jpg" every { filePath } returns "filePath2" every { isUploaded } returns false every { doesFileExist } returns true @@ -408,13 +444,27 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { givenFullValidInput(messageId, attachmentIds.toTypedArray()) every { cryptoFactory.create(Id("senderAddress4"), Name(currentUsername)) } returns crypto coEvery { messageDetailsRepository.findMessageById(messageId) } returns message + every { context.getString(R.string.attachment_failed_message_drafted) } returns + "Please remove and re-attach \"%s\" and send the message again. " + + "Your message can be found in the \"Drafts\" folder." every { messageDetailsRepository.findAttachmentById("1") } returns attachment1 every { messageDetailsRepository.findAttachmentById("2") } returns attachment2 - uploadAttachmentsWorker.doWork() + val result = uploadAttachmentsWorker.doWork() coVerify(exactly = 0) { attachmentsRepository.upload(attachment1, crypto) } - coVerify { attachmentsRepository.upload(attachment2, crypto) } + coVerify(exactly = 0) { attachmentsRepository.upload(attachment2, crypto) } + assertEquals( + ListenableWorker.Result.failure( + workDataOf( + KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to + "Please remove and re-attach \"Attachment_1.jpg\" and send the message again. " + + "Your message can be found in the \"Drafts\" folder.", + ) + ), + result + ) + verify { pendingActionsDao.deletePendingUploadByMessageId("messageId36926543") } } } @@ -487,7 +537,7 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { val messageId = "messageId823762" val message = Message(messageId = messageId, addressID = "senderAddress6") every { userManager.username } returns username - givenFullValidInput(messageId, isMessageSending = true) + givenFullValidInput(messageId, attachments = emptyArray(), isMessageSending = true) every { cryptoFactory.create(Id("senderAddress6"), Name(username)) } returns crypto coEvery { messageDetailsRepository.findMessageById(messageId) } returns message every { userManager.getMailSettings(username)?.getAttachPublicKey() } returns true @@ -581,8 +631,10 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { message.setAttachmentList(listOf(attachmentMock1, attachmentMock2)) every { messageDetailsRepository.findAttachmentById("1") } returns attachmentMock1 every { messageDetailsRepository.findAttachmentById("2") } returns attachmentMock2 - every { messageDetailsRepository.findAttachmentById(uploadedAttachmentMock1Id) } returns uploadedAttachmentMock1 - every { messageDetailsRepository.findAttachmentById(uploadedAttachmentMock2Id) } returns uploadedAttachmentMock2 + every { messageDetailsRepository.findAttachmentById(uploadedAttachmentMock1Id) } returns + uploadedAttachmentMock1 + every { messageDetailsRepository.findAttachmentById(uploadedAttachmentMock2Id) } returns + uploadedAttachmentMock2 coEvery { attachmentsRepository.upload(attachmentMock1, crypto) } answers { AttachmentsRepository.Result.Success(uploadedAttachmentMock1Id) } @@ -725,12 +777,11 @@ class UploadAttachmentsWorkerTest : CoroutinesTest { attachments: Array = arrayOf("attId62364"), isMessageSending: Boolean = false ) { - every { parameters.inputData.getStringArray(KEY_INPUT_UPLOAD_ATTACHMENTS_ATTACHMENT_IDS) } answers { attachments } + every { parameters.inputData.getStringArray(KEY_INPUT_UPLOAD_ATTACHMENTS_ATTACHMENT_IDS) } answers + { attachments } every { parameters.inputData.getString(KEY_INPUT_UPLOAD_ATTACHMENTS_MESSAGE_ID) } answers { messageId } every { parameters.inputData.getBoolean(KEY_INPUT_UPLOAD_ATTACHMENTS_IS_MESSAGE_SENDING, false) } answers { isMessageSending } } } - - diff --git a/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt b/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt index a62b181ff..1c6ac6247 100644 --- a/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt +++ b/app/src/test/java/ch/protonmail/android/compose/send/SendMessageWorkerTest.kt @@ -42,6 +42,7 @@ import ch.protonmail.android.api.models.factories.SendPreferencesFactory import ch.protonmail.android.api.models.messages.send.MessageSendBody import ch.protonmail.android.api.models.messages.send.MessageSendKey import ch.protonmail.android.api.models.messages.send.MessageSendPackage +import ch.protonmail.android.api.models.room.messages.Attachment import ch.protonmail.android.api.models.room.messages.Message import ch.protonmail.android.api.models.room.pendingActions.PendingActionsDao import ch.protonmail.android.core.Constants @@ -289,6 +290,35 @@ class SendMessageWorkerTest : CoroutinesTest { ) } + @Test + fun workerFailsWithoutNotifyingUserWhenSaveDraftFailsWithUploadAttachmentError() = runBlockingTest { + // we notify the user from the Save Draft Worker, + // refer to test `SaveDraftTest.notifyUserWithUploadAttachmentErrorWhenAttachmentIsBroken` + val messageDbId = 2834L + val messageId = "823472" + val message = Message().apply { + dbId = messageDbId + this.messageId = messageId + this.subject = "Subject 003" + this.Attachments = listOf(Attachment(attachmentId = "attachmentId234", fileName = "Attachment_234.jpg")) + } + givenFullValidInput(messageDbId, messageId) + coEvery { messageDetailsRepository.findMessageByMessageDbId(messageDbId) } returns message + coEvery { saveDraft(any()) } returns SaveDraftResult.UploadDraftAttachmentsFailed + every { parameters.runAttemptCount } returns 0 + + val result = worker.doWork() + + verify { pendingActionsDao.deletePendingSendByMessageId("823472") } + verify { userNotifier wasNot Called } + assertEquals( + ListenableWorker.Result.failure( + workDataOf(KEY_OUTPUT_RESULT_SEND_MESSAGE_ERROR_ENUM to "UploadAttachmentsFailed") + ), + result + ) + } + @Test fun workerFailsWithoutNotifyingUserWhenSaveDraftFailsWithMessageAlreadySentError() = runBlockingTest { val messageDbId = 2835L diff --git a/app/src/test/java/ch/protonmail/android/usecase/compose/SaveDraftTest.kt b/app/src/test/java/ch/protonmail/android/usecase/compose/SaveDraftTest.kt index fd93b7154..de2c83410 100644 --- a/app/src/test/java/ch/protonmail/android/usecase/compose/SaveDraftTest.kt +++ b/app/src/test/java/ch/protonmail/android/usecase/compose/SaveDraftTest.kt @@ -23,6 +23,7 @@ import androidx.work.Data import androidx.work.WorkInfo import androidx.work.workDataOf import ch.protonmail.android.activities.messageDetails.repository.MessageDetailsRepository +import ch.protonmail.android.api.models.room.messages.Attachment import ch.protonmail.android.api.models.room.messages.Message import ch.protonmail.android.api.models.room.pendingActions.PendingActionsDao import ch.protonmail.android.api.models.room.pendingActions.PendingSend @@ -619,7 +620,7 @@ class SaveDraftTest : CoroutinesTest { } @Test - fun saveDraftsShowPersistentErrorAndReturnsErrorWhenUploadingNewAttachmentsFails() { + fun saveDraftsShowUploadAttachmentErrorAndReturnsErrorWhenUploadingNewAttachmentsFails() { runBlockingTest { // Given val localDraftId = "8345" @@ -630,6 +631,10 @@ class SaveDraftTest : CoroutinesTest { decryptedBody = "Message body in plain text" localId = localDraftId subject = "Message Subject" + this.Attachments = listOf( + Attachment(attachmentId = "2345", fileName = "Attachment_2345.jpg"), + Attachment(attachmentId = "453", fileName = "Attachment_453.jpg"), + ) } val newAttachmentIds = listOf("2345", "453") val createDraftOutputData = workDataOf( @@ -638,7 +643,7 @@ class SaveDraftTest : CoroutinesTest { val createDraftWorkerResult = buildWorkerResponse(WorkInfo.State.SUCCEEDED, createDraftOutputData) val errorMessage = "Can't upload attachments" val uploadWorkOutputData = workDataOf( - KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to errorMessage + KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to errorMessage, ) coEvery { messageDetailsRepository.saveMessageLocally(message) } returns 9833L coEvery { messageDetailsRepository.findMessageById("newDraftId") } returns message.copy(messageId = "newDraftId") @@ -669,7 +674,79 @@ class SaveDraftTest : CoroutinesTest { ) // Then - verify { userNotifier.showPersistentError(errorMessage, "Message Subject") } + verify { userNotifier.showAttachmentUploadError(errorMessage, "Message Subject") } + assertEquals(SaveDraftResult.UploadDraftAttachmentsFailed, result) + } + } + + @Test + fun notifyUserWithUploadAttachmentErrorWhenAttachmentIsBroken() { + runBlockingTest { + // Given + val localDraftId = "8345" + val message = Message().apply { + dbId = 123L + this.messageId = "45623" + addressID = "addressId" + decryptedBody = "Message body in plain text" + localId = localDraftId + subject = "Message Subject" + this.Attachments = listOf( + Attachment( + attachmentId = "2345", + fileName = "Attachment_2345.jpg", + isUploaded = false, + filePath = null + ), + Attachment(attachmentId = "453", fileName = "Attachment_453.jpg"), + ) + } + val newAttachmentIds = listOf("2345", "453") + val createDraftOutputData = workDataOf( + KEY_OUTPUT_RESULT_SAVE_DRAFT_MESSAGE_ID to "newDraftId" + ) + val createDraftWorkerResult = buildWorkerResponse(WorkInfo.State.SUCCEEDED, createDraftOutputData) + val errorMessage = "Invalid attachment." + val uploadWorkOutputData = workDataOf( + KEY_OUTPUT_RESULT_UPLOAD_ATTACHMENTS_ERROR to errorMessage, + ) + coEvery { messageDetailsRepository.saveMessageLocally(message) } returns 9833L + coEvery { messageDetailsRepository.findMessageById("newDraftId") } returns + message.copy(messageId = "newDraftId") + coEvery { messageDetailsRepository.findMessageById("45623") } returns message + coEvery { + uploadAttachmentsWorker.enqueue( + newAttachmentIds, + "newDraftId", + false + ) + } returns buildWorkerResponse( + WorkInfo.State.FAILED, + uploadWorkOutputData + ) + every { + createDraftScheduler.enqueue( + message, + null, + FORWARD, + "previousSenderId132423" + ) + } answers { createDraftWorkerResult } + + // When + val result = saveDraft.invoke( + SaveDraftParameters( + message, + newAttachmentIds, + null, + FORWARD, + "previousSenderId132423", + SaveDraft.SaveDraftTrigger.UserRequested + ) + ) + + // Then + verify { userNotifier.showAttachmentUploadError(errorMessage, "Message Subject") } assertEquals(SaveDraftResult.UploadDraftAttachmentsFailed, result) } } diff --git a/app/src/test/java/ch/protonmail/android/utils/MessageUtilsTest.kt b/app/src/test/java/ch/protonmail/android/utils/MessageUtilsTest.kt new file mode 100644 index 000000000..4e25fd5e4 --- /dev/null +++ b/app/src/test/java/ch/protonmail/android/utils/MessageUtilsTest.kt @@ -0,0 +1,62 @@ +/* + * Copyright (c) 2020 Proton Technologies AG + * + * This file is part of ProtonMail. + * + * ProtonMail is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * ProtonMail is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with ProtonMail. If not, see https://www.gnu.org/licenses/. + */ + +package ch.protonmail.android.utils + +import org.junit.Test +import kotlin.test.assertEquals + +class MessageUtilsTest { + + @Test + fun `isLocalAttachmentId returning true if attachmentId is missing`() { + // given + val attachmentId = null + + // when + val result = MessageUtils.isLocalAttachmentId(attachmentId) + + // then + assertEquals(true, result) + } + + @Test + fun `isLocalAttachmentId returning true if attachmentId is local attachmentId`() { + // given + val attachmentId = "-1234567890" + + // when + val result = MessageUtils.isLocalAttachmentId(attachmentId) + + // then + assertEquals(true, result) + } + + @Test + fun `isLocalAttachmentId returning true if attachmentId is remote attachmentId`() { + // given + val attachmentId = "e-aIFdwkb1WtpBk9rTJY63afcvA9yE7lkjApbGzbdvn-Gbugg56h3LwUrD9b-qXWoMcJuNKecwhiv_mKG1ZrzQ==" + + // when + val result = MessageUtils.isLocalAttachmentId(attachmentId) + + // then + assertEquals(false, result) + } +} From 38855a2b1e7a6dc064d08af5f6b8dd014f03b75f Mon Sep 17 00:00:00 2001 From: Zorica Stojchevska Date: Mon, 22 Nov 2021 10:47:38 +0100 Subject: [PATCH 5/5] Changed build version #comment Just increased the build version code and name for a new release Affected: nothing --- .idea/vcs.xml | 23 +++++------------------ buildSrc/src/main/kotlin/ProtonMail.kt | 4 ++-- 2 files changed, 7 insertions(+), 20 deletions(-) diff --git a/.idea/vcs.xml b/.idea/vcs.xml index 08fa18257..3e19a6f0f 100644 --- a/.idea/vcs.xml +++ b/.idea/vcs.xml @@ -1,23 +1,10 @@ - + + + + + diff --git a/buildSrc/src/main/kotlin/ProtonMail.kt b/buildSrc/src/main/kotlin/ProtonMail.kt index 389cf2949..8c7c7237a 100644 --- a/buildSrc/src/main/kotlin/ProtonMail.kt +++ b/buildSrc/src/main/kotlin/ProtonMail.kt @@ -23,8 +23,8 @@ */ object ProtonMail { - const val versionName = "1.13.38" - const val versionCode = 785 + const val versionName = "1.13.39" + const val versionCode = 786 const val targetSdk = 30 const val minSdk = 21