diff --git a/src/mail-app/mail/model/MailModel.ts b/src/mail-app/mail/model/MailModel.ts index b018cf92d6..8534add993 100644 --- a/src/mail-app/mail/model/MailModel.ts +++ b/src/mail-app/mail/model/MailModel.ts @@ -5,6 +5,7 @@ import { FolderSystem } from "../../../common/api/common/mail/FolderSystem.js" import { assertNotNull, collectToMap, + downcast, getFirstOrThrow, groupBy, groupByAndMap, @@ -233,24 +234,24 @@ export class MailModel { return mailboxDetail && this.inboxRuleHandler()?.findAndApplyMatchingRule(mailboxDetail, mail, isLeaderClient) }) - if (!isWebClient()) { - const mailDetails = await this.mailFacade.loadMailDetailsBlob(mail) - this.spamHandler().storeTrainingDatum(mail, mailDetails) - } else { + if (isWebClient()) { // we only need to show notifications explicitly on the webapp this._showNotification(isRuleTargetFolder ?? sourceMailFolder, mail) - } + } else { + const mailDetails = await this.mailFacade.loadMailDetailsBlob(mail) + this.spamHandler().storeTrainingDatum(mail, mailDetails) - if (isRuleTargetFolder) { - return { processingDone: Promise.resolve() } - } else if ( - mail.processingState === ProcessingState.INBOX_RULE_NOT_PROCESSED || - mail.processingState === ProcessingState.INBOX_RULE_PROCESSED_AND_SPAM_PREDICTION_PENDING - ) { - const folderSystem = this.getFolderSystemByGroupId(assertNotNull(mail._ownerGroup)) - if (sourceMailFolder && folderSystem) { - const predictPromise = this.spamHandler().predictSpamForNewMail(mail, sourceMailFolder, folderSystem) - return { processingDone: predictPromise } + if (isRuleTargetFolder) { + return { processingDone: Promise.resolve() } + } else if ( + mail.processingState === ProcessingState.INBOX_RULE_NOT_PROCESSED || + mail.processingState === ProcessingState.INBOX_RULE_PROCESSED_AND_SPAM_PREDICTION_PENDING + ) { + const folderSystem = this.getFolderSystemByGroupId(assertNotNull(mail._ownerGroup)) + if (sourceMailFolder && folderSystem) { + const predictPromise = this.spamHandler().predictSpamForNewMail(mail, mailDetails, sourceMailFolder, folderSystem) + return { processingDone: downcast(predictPromise) } + } } } } diff --git a/src/mail-app/mail/model/SpamClassificationHandler.ts b/src/mail-app/mail/model/SpamClassificationHandler.ts index e04d54c8f1..ff53a0b3a8 100644 --- a/src/mail-app/mail/model/SpamClassificationHandler.ts +++ b/src/mail-app/mail/model/SpamClassificationHandler.ts @@ -60,9 +60,7 @@ export class SpamClassificationHandler { } } - public async predictSpamForNewMail(mail: Mail, folder: MailFolder, folderSystem: FolderSystem): Promise { - const mailDetails = await this.mailFacade.loadMailDetailsBlob(mail) - + public async predictSpamForNewMail(mail: Mail, mailDetails: MailDetails, sourceFolder: MailFolder, folderSystem: FolderSystem): Promise { const spamPredMailDatum: SpamPredMailDatum = { subject: mail.subject, body: getMailBodyText(mailDetails.body), @@ -70,7 +68,7 @@ export class SpamClassificationHandler { } const isSpam = (await this.spamClassifier?.predict(spamPredMailDatum)) ?? null - if (isSpam && folder.folderType === MailSetKind.INBOX) { + if (isSpam && sourceFolder.folderType === MailSetKind.INBOX) { const spamFolder = assertNotNull(folderSystem.getSystemFolderByType(MailSetKind.SPAM)) if (this.spamMoveMailData) { this.spamMoveMailData.mails.push(mail._id) @@ -83,7 +81,8 @@ export class SpamClassificationHandler { }) } this.sendMoveMailServiceRequest(this.mailFacade) - } else if (!isSpam && folder.folderType === MailSetKind.SPAM) { + return spamFolder + } else if (!isSpam && sourceFolder.folderType === MailSetKind.SPAM) { const hamFolder = assertNotNull(folderSystem.getSystemFolderByType(MailSetKind.INBOX)) if (this.hamMoveMailData) { this.hamMoveMailData.mails.push(mail._id) @@ -96,9 +95,11 @@ export class SpamClassificationHandler { }) } this.sendMoveMailServiceRequest(this.mailFacade) + return hamFolder } else { this.classifierResultServiceMailIds.push(mail._id) this.sendClassifierResultServiceRequest(this.mailFacade) + return sourceFolder } } diff --git a/test/tests/api/worker/crypto/CryptoFacadeTest.ts b/test/tests/api/worker/crypto/CryptoFacadeTest.ts index cf2274056c..2b2c657796 100644 --- a/test/tests/api/worker/crypto/CryptoFacadeTest.ts +++ b/test/tests/api/worker/crypto/CryptoFacadeTest.ts @@ -10,6 +10,7 @@ import { GroupType, PermissionType, PresentableKeyVerificationState, + ProcessingState, PublicKeyIdentifierType, } from "../../../../../src/common/api/common/TutanotaConstants.js" import { @@ -1854,7 +1855,7 @@ o.spec("CryptoFacadeTest", function () { mailDetailsDraft: null, sets: [], keyVerificationState: null, - isInboxRuleApplied: true, + processingState: ProcessingState.INBOX_RULE_APPLIED, clientSpamClassifierResult: null, }) diff --git a/test/tests/mail/MailModelTest.ts b/test/tests/mail/MailModelTest.ts index a0d0fcd01e..82be6c4c83 100644 --- a/test/tests/mail/MailModelTest.ts +++ b/test/tests/mail/MailModelTest.ts @@ -1,15 +1,15 @@ import o from "@tutao/otest" import { Notifications } from "../../../src/common/gui/Notifications.js" import { mock, Spy, spy, verify } from "@tutao/tutanota-test-utils" -import { MailSetKind, OperationType } from "../../../src/common/api/common/TutanotaConstants.js" +import { MailSetKind, OperationType, ProcessingState } from "../../../src/common/api/common/TutanotaConstants.js" import { BodyTypeRef, Mail, MailDetails, + MailDetailsBlob, MailDetailsBlobTypeRef, MailDetailsTypeRef, MailFolderTypeRef, - MailSetEntryTypeRef, MailTypeRef, } from "../../../src/common/api/entities/tutanota/TypeRefs.js" import { EntityClient } from "../../../src/common/api/common/EntityClient.js" @@ -84,29 +84,29 @@ o.spec("MailModelTest", function () { }) o("doesn't send notification for another folder", async function () { - const mailSetEntry = createTestEntity(MailSetEntryTypeRef, { _id: [anotherFolder.entries, "mailSetEntryId"] }) - restClient.addListInstances(mailSetEntry) + const mail = createTestEntity(MailTypeRef, { _id: ["mailBagListId", "mailId"], sets: [] }) + restClient.addListInstances(mail) await model.entityEventsReceived([ makeUpdate({ - instanceListId: getListId(mailSetEntry) as NonEmptyString, - instanceId: getElementId(mailSetEntry), + instanceListId: getListId(mail) as NonEmptyString, + instanceId: getElementId(mail), operation: OperationType.CREATE, }), ]) o(showSpy.invocations.length).equals(0) }) o("doesn't send notification for move operation", async function () { - const mailSetEntry = createTestEntity(MailSetEntryTypeRef, { _id: [inboxFolder.entries, "mailSetEntryId"] }) - restClient.addListInstances(mailSetEntry) + const mail = createTestEntity(MailTypeRef, { _id: ["mailBagListId", "mailId"], sets: [] }) + restClient.addListInstances(mail) await model.entityEventsReceived([ makeUpdate({ - instanceListId: getListId(mailSetEntry) as NonEmptyString, - instanceId: getElementId(mailSetEntry), + instanceListId: getListId(mail) as NonEmptyString, + instanceId: getElementId(mail), operation: OperationType.DELETE, }), makeUpdate({ - instanceListId: getListId(mailSetEntry) as NonEmptyString, - instanceId: getElementId(mailSetEntry), + instanceListId: getListId(mail) as NonEmptyString, + instanceId: getElementId(mail), operation: OperationType.CREATE, }), ]) @@ -145,14 +145,18 @@ o.spec("MailModelTest", function () { _ownerGroup: "mailGroup", mailDetails: ["detailsList", mailDetails._id], sets: [inboxFolder._id], + processingState: ProcessingState.INBOX_RULE_NOT_PROCESSED, }) - const mailDetailsBlob = createTestEntity(MailDetailsBlobTypeRef, { + const mailDetailsBlob: MailDetailsBlob = createTestEntity(MailDetailsBlobTypeRef, { _id: mail.mailDetails!, details: mailDetails, }) + restClient.addListInstances(mail) restClient.addBlobInstances(mailDetailsBlob) + when(mailFacade.loadMailDetailsBlob(mail)).thenResolve(mailDetails) + modelWithSpamAndInboxRule = mock( new MailModel( downcast({}), @@ -204,6 +208,7 @@ o.spec("MailModelTest", function () { await processingDone verify(spamClassifier.predict(anything()), { times: 0 }) }) + o("spam prediction happens when inbox rule is not applied", async () => { when(spamClassifier.predict(anything())).thenResolve(false) @@ -217,19 +222,6 @@ o.spec("MailModelTest", function () { await processingDone verify(spamClassifier.predict(anything()), { times: 1 }) }) - o("spam prediction happens when inbox rule throws", async () => { - when(spamClassifier.predict(anything())).thenResolve(false) - - const mailCreateEvent = makeUpdate({ - instanceListId: "mailListId", - instanceId: "mailId", - operation: OperationType.CREATE, - }) - when(inboxRuleHandler.findAndApplyMatchingRule(anything(), anything(), anything())).thenReject(new Error("Some error for inbox rule")) - const { processingDone } = await modelWithSpamAndInboxRule.entityEventsReceived([mailCreateEvent]) - await processingDone - verify(spamClassifier.predict(anything()), { times: 1 }) - }) o("does not try to do spam classification when downloading of mail fails on create mail event", async function () { when(inboxRuleHandler.findAndApplyMatchingRule(anything(), anything(), anything())).thenResolve(null) @@ -252,25 +244,6 @@ o.spec("MailModelTest", function () { verify(spamClassifier.predict(anything()), { times: 1 }) }) - o("no spam prediction for draft mails", async () => { - mail.mailDetails = null - mail.mailDetailsDraft = ["draftListId", "draftId"] - restClient.addListInstances(mail) - - when(inboxRuleHandler.findAndApplyMatchingRule(anything(), mail, anything())).thenResolve(inboxFolder) - - const mailCreateEvent = makeUpdate({ - instanceListId: "mailListId", - instanceId: "mailId", - operation: OperationType.CREATE, - }) - const { processingDone } = await modelWithSpamAndInboxRule.entityEventsReceived([mailCreateEvent]) - await processingDone - - verify(spamClassificationHandler.predictSpamForNewMail(mail, anything(), anything()), { times: 1 }) - verify(spamClassifier.predict(anything()), { times: 0 }) - }) - o("deletes a training datum for deleted mail event", async () => { const mailDeleteEvent = makeUpdate({ instanceListId: "mailListId", diff --git a/test/tests/mail/SpamClassificationHandlerTest.ts b/test/tests/mail/SpamClassificationHandlerTest.ts index 30e1b22958..452f28e348 100644 --- a/test/tests/mail/SpamClassificationHandlerTest.ts +++ b/test/tests/mail/SpamClassificationHandlerTest.ts @@ -7,25 +7,19 @@ import { Mail, MailDetails, MailDetailsTypeRef, - MailFolder, MailFolderTypeRef, MailTypeRef, } from "../../../src/common/api/entities/tutanota/TypeRefs" import { SpamClassifier, SpamTrainMailDatum } from "../../../src/mail-app/workerUtils/spamClassification/SpamClassifier" import { getMailBodyText } from "../../../src/common/api/common/CommonMailUtils" -import { MailSetKind } from "../../../src/common/api/common/TutanotaConstants" +import { MailSetKind, ProcessingState } from "../../../src/common/api/common/TutanotaConstants" import { ClientClassifierType } from "../../../src/common/api/common/ClientClassifierType" -import { assert, assertNotNull, defer, Nullable } from "@tutao/tutanota-utils" +import { assert, assertNotNull } from "@tutao/tutanota-utils" import { MailFacade } from "../../../src/common/api/worker/facades/lazy/MailFacade" import { createTestEntity } from "../TestUtils" import { SpamClassificationHandler } from "../../../src/mail-app/mail/model/SpamClassificationHandler" -import { EntityClient } from "../../../src/common/api/common/EntityClient" -import { ClientModelInfo } from "../../../src/common/api/common/EntityFunctions" -import { BulkMailLoader } from "../../../src/mail-app/workerUtils/index/BulkMailLoader" -import { EntityRestInterface } from "../../../src/common/api/worker/rest/EntityRestClient" import { FolderSystem } from "../../../src/common/api/common/mail/FolderSystem" import { isSameId } from "../../../src/common/api/common/utils/EntityUtils" -import { WebsocketConnectivityModel } from "../../../src/common/misc/WebsocketConnectivityModel" const { anything } = matchers @@ -35,12 +29,8 @@ o.spec("SpamClassificationHandlerTest", function () { let mail: Mail let spamClassifier: SpamClassifier let spamHandler: SpamClassificationHandler - let restClient: EntityRestInterface - let bulkMailLoader: BulkMailLoader - const inboxRuleOutcome = defer>() let folderSystem: FolderSystem let mailDetails: MailDetails - let connectivityModel: WebsocketConnectivityModel const inboxFolder = createTestEntity(MailFolderTypeRef, { _id: ["listId", "inbox"], folderType: MailSetKind.INBOX }) const trashFolder = createTestEntity(MailFolderTypeRef, { _id: ["listId", "trash"], folderType: MailSetKind.TRASH }) @@ -48,7 +38,6 @@ o.spec("SpamClassificationHandlerTest", function () { o.beforeEach(function () { spamClassifier = object() - restClient = object() body = createTestEntity(BodyTypeRef, { text: "Body Text" }) mailDetails = createTestEntity(MailDetailsTypeRef, { _id: "mailDetail", body }) @@ -59,15 +48,11 @@ o.spec("SpamClassificationHandlerTest", function () { _ownerGroup: "owner", mailDetails: ["detailsList", mailDetails._id], unread: true, - isInboxRuleApplied: false, + processingState: ProcessingState.INBOX_RULE_NOT_PROCESSED, clientSpamClassifierResult: null, }) - bulkMailLoader = object() folderSystem = object() - connectivityModel = object() - when(connectivityModel.isLeader()).thenReturn(true) - when(mailFacade.simpleMoveMails(anything(), anything(), ClientClassifierType.CLIENT_CLASSIFICATION)).thenResolve([]) when(folderSystem.getSystemFolderByType(MailSetKind.SPAM)).thenReturn(spamFolder) when(folderSystem.getSystemFolderByType(MailSetKind.INBOX)).thenReturn(inboxFolder) @@ -81,7 +66,7 @@ o.spec("SpamClassificationHandlerTest", function () { else throw new Error("Unknown mail Folder") }) when( - bulkMailLoader.loadMailDetails( + mailFacade.loadMailDetailsBlob( matchers.argThat((requestedMails: Array) => { assert(requestedMails.length === 1, "exactly one mail is requested at a time") return isSameId(requestedMails[0]._id, mail._id) @@ -89,14 +74,10 @@ o.spec("SpamClassificationHandlerTest", function () { ), anything(), ).thenDo(async () => [{ mail, mailDetails }]) - - const entityClient = new EntityClient(restClient, ClientModelInfo.getNewInstanceForTestsOnly()) spamHandler = new SpamClassificationHandler(mailFacade, spamClassifier) }) o("processSpam correctly verifies if email is stored in spam folder", async function () { - inboxRuleOutcome.resolve(null) - mail.sets = [spamFolder._id] when(spamClassifier.predict(anything())).thenResolve(false) const expectedTrainingData: SpamTrainMailDatum = { @@ -108,7 +89,7 @@ o.spec("SpamClassificationHandlerTest", function () { isSpamConfidence: 1, } - const finalResult = await spamHandler.predictSpamForNewMail(mail, folderSystem) + const finalResult = await spamHandler.predictSpamForNewMail(mail, mailDetails, spamFolder, folderSystem) verify(spamClassifier.storeSpamClassification(expectedTrainingData), { times: 1 }) verify(mailFacade.simpleMoveMails([mail._id], MailSetKind.INBOX, ClientClassifierType.CLIENT_CLASSIFICATION)) o(finalResult).deepEquals(inboxFolder) @@ -116,9 +97,8 @@ o.spec("SpamClassificationHandlerTest", function () { o("does not classify mail if the mail has non null client classification result", async function () { mail.sets = [inboxFolder._id] - mail.isInboxRuleApplied = false + mail.processingState = ProcessingState.INBOX_RULE_NOT_PROCESSED mail.clientSpamClassifierResult = createTestEntity(ClientSpamClassifierResultTypeRef) - inboxRuleOutcome.resolve(null) const expectedTrainingData: SpamTrainMailDatum = { mailId: mail._id, @@ -129,34 +109,14 @@ o.spec("SpamClassificationHandlerTest", function () { isSpamConfidence: 0, } - const finalResult = await spamHandler.predictSpamForNewMail(mail, folderSystem) + const finalResult = await spamHandler.predictSpamForNewMail(mail, mailDetails, inboxFolder, folderSystem) o(finalResult).equals(null) verify(mailFacade.simpleMoveMails(anything(), anything(), anything()), { times: 0 }) verify(spamClassifier.predict(anything()), { times: 0 }) verify(spamClassifier.storeSpamClassification(expectedTrainingData), { times: 1 }) }) - o("mail in spam folder is not classified but stored with confidence 0", async function () { - inboxRuleOutcome.resolve(null) - mail.sets = [trashFolder._id] - - const expectedTrainingData: SpamTrainMailDatum = { - mailId: mail._id, - subject: mail.subject, - body: getMailBodyText(body), - isSpam: false, - ownerGroup: "owner", - isSpamConfidence: 1, - } - - const finalResult = await spamHandler.predictSpamForNewMail(inboxRuleOutcome.promise, mail, folderSystem) - o(finalResult).deepEquals(null) - verify(mailFacade.simpleMoveMails(anything(), anything(), anything()), { times: 0 }) - verify(spamClassifier.storeSpamClassification(expectedTrainingData), { times: 1 }) - }) - o("processSpam moves mail to inbox when detected as such and its not already in inbox", async function () { - inboxRuleOutcome.resolve(null) when(spamClassifier.predict(anything())).thenResolve(false) mail.sets = [spamFolder._id] @@ -170,14 +130,13 @@ o.spec("SpamClassificationHandlerTest", function () { ownerGroup: "owner", } - const finalResult = await spamHandler.predictSpamForNewMail(inboxRuleOutcome.promise, mail, folderSystem) + const finalResult = await spamHandler.predictSpamForNewMail(mail, mailDetails, spamFolder, folderSystem) o(finalResult).deepEquals(inboxFolder) verify(spamClassifier.storeSpamClassification(expectedDatum), { times: 1 }) verify(mailFacade.simpleMoveMails([["listId", "elementId"]], MailSetKind.INBOX, ClientClassifierType.CLIENT_CLASSIFICATION), { times: 1 }) }) o("processSpam moves mail to spam when detected as such and its not already in spam", async function () { - inboxRuleOutcome.resolve(null) when(spamClassifier.predict(anything())).thenResolve(true) mail.sets = [inboxFolder._id] @@ -190,7 +149,7 @@ o.spec("SpamClassificationHandlerTest", function () { ownerGroup: "owner", } - const finalResult = await spamHandler.predictSpamForNewMail(inboxRuleOutcome.promise, mail, folderSystem) + const finalResult = await spamHandler.predictSpamForNewMail(mail, mailDetails, inboxFolder, folderSystem) o(finalResult).deepEquals(spamFolder) verify(spamClassifier.storeSpamClassification(expectedDatum), { times: 1 }) verify(mailFacade.simpleMoveMails([["listId", "elementId"]], MailSetKind.SPAM, ClientClassifierType.CLIENT_CLASSIFICATION), { times: 1 })