4837 single user email import #5582

jomapp · 2023-06-30T08:22:55Z

Draft pull request for single user email import.

Notes:

Email import documentation (in tutadb) will be updated in the following days.
There are several open TODOs to be found in the documentation.
Main entry points are: ImapAdSync.ts as well as ImapImporter.ts.
See mail from 06/30/23.

charlag

Great job! It's a complicated machine with a lot of parts and I think we need to really make it as understandable as possible.

I've left some comments in places but it would be too much to repeat it every time.

Here are some general code style comments:

use for-of loop instead of foreach whenever possible
use const instead of let whenever possible
collapse fields and constructors whenever possible
prefer plain objects/interfaces to classes, especially in remote invocation context
do not use streams unless you really need them, prefer plain fields
do not prepare view attributes before rendering, just use them whenever you render a component

Besides that we need a lot of JSDOC comments for different methods/classes. I know there's another doc being prepared and it's good to have high-level overview there but it should still be possible to grasp what the class does without referring to it.

charlag · 2023-07-03T07:03:52Z

buildSrc/DevBuild.js

@@ -23,7 +21,7 @@ export async function runDevBuild({ stage, host, desktop, clean, ignoreMigration
 		if (ignoreMigrations) {
 			console.warn("CAUTION: Offline migrations are not being validated.")
 		} else {
-			checkOfflineDatabaseMigrations()
+			//checkOfflineDatabaseMigrations()


would it make sense to just add a migration that drops the db if that's what you need?

charlag · 2023-07-03T07:41:34Z

src/api/worker/facades/lazy/ImportImapFacade.ts

+	) {}
+
+	async initializeImapImport(initializeParams: InitializeImapImportParams): Promise<ImportImapAccountSyncState> {
+		const mailGroupId = this.userFacade.getGroupId(GroupType.Mail)


I would prefer if we passed the mail group in, shared mailboxes are a thing after all

charlag · 2023-07-03T07:48:33Z

src/api/worker/facades/lazy/ImportImapFacade.ts

+		importImapAccountSyncStateId: Id,
+		parentFolderId: IdTuple | null,
+	): Promise<ImportImapFolderSyncState | undefined> {
+		if (imapMailbox.name) {


what happens when there's no name?

charlag · 2023-07-03T07:53:12Z

src/api/worker/facades/lazy/ImportMailFacade.ts

+		const mailGroupId = this.userFacade.getGroupId(GroupType.Mail)
+		const mailGroupKey = this.userFacade.getGroupKey(mailGroupId)
+		const sk = aes128RandomKey()
+		const service = createImportMailPostIn()


please pass things directly into create call rather than assigning them (also generally)

charlag · 2023-07-03T07:53:57Z

src/api/worker/facades/lazy/ImportMailFacade.ts

+	/**
+	 * Uploads the given data files or sets the file if it is already existing and returns all ImportAttachments
+	 */
+	async _createAddedImportAttachments(


why is it underscored and not private?

charlag · 2023-07-03T14:17:23Z

src/settings/imapimport/EnterImapCredentialsPage.ts

+			value: vnode.attrs.data.model.imapAccountHost(),
+			oninput: vnode.attrs.data.model.imapAccountHost,
+			helpLabel: () => {
+				const host = vnode.attrs.data.model.imapAccountHost()


what is host for here?

charlag · 2023-07-03T14:18:13Z

src/settings/imapimport/EnterImapCredentialsPage.ts

+				}
+			},
+		}
+		const imapAccountPortAttrs: TextFieldAttrs = {


should this be a number field?

charlag · 2023-07-03T14:23:31Z

src/settings/imapimport/ImapImportSettingsViewer.ts

+
+	private async requestImapImportAccountSyncState() {
+		const importImapAccountSyncState = await locator.imapImporterFacade.loadImportImapAccountSyncState()
+		const rootImportMailFolder = await locator.imapImporterFacade.loadRootImportFolder()


can it fail? what happens?

charlag · 2023-07-03T14:24:21Z

src/settings/imapimport/ImapImportSettingsViewer.ts

+	private imapImportState: stream<ImapImportState>
+
+	constructor() {
+		this.imapAccountHost = stream<string>("")


steams are also unnecessary here

charlag · 2023-07-03T14:25:49Z

tsconfig.json

@@ -7,7 +7,7 @@
 		"outDir": "build/dist",
 		"incremental": true
 	},
-	"include": ["src/", "libs/*.ts", "types/*.d.ts"],
+	"include": ["src/", "libs/*.ts", "types/*.d.ts", "node_modules/imapflow/lib/types.d.ts"],


other types are picked up automatically, why is this necessary?;

charlag

Great job! It's a complicated machine with a lot of parts and I think we need to really make it as understandable as possible.

I've left some comments in places but it would be too much to repeat it every time.

Here are some general code style comments:

use for-of loop instead of foreach whenever possible
use const instead of let whenever possible
collapse fields and constructors whenever possible
prefer plain objects/interfaces to classes, especially in remote invocation context
do not use streams unless you really need them, prefer plain fields
do not prepare view attributes before rendering, just use them whenever you render a component

Besides that we need a lot of JSDOC comments for different methods/classes. I know there's another doc being prepared and it's good to have high-level overview there but it should still be possible to grasp what the class does without referring to it.

ganthern · 2023-07-05T12:18:12Z

src/desktop/imapimport/DesktopImapImportSystemFacade.ts

+import { ApplicationWindow } from "../ApplicationWindow.js"
+
+export class DesktopImapImportSystemFacade implements ImapImportSystemFacade, AdSyncEventListener {
+	constructor(private readonly win: ApplicationWindow) {}


don't take ApplicationWindow, rather keep a list/record of active ImapImportFacades (maybe per userId or mailbox id)

remove imapImportFacade from ApplicationWindow

ganthern · 2023-07-05T12:20:50Z

src/api/worker/imapimport/ImapImporter.ts

+	host: string
+	port: string
+	username: string
+	password: string | null


can we type this in a way that requires at least one of password/accessToken

ganthern · 2023-07-05T12:46:22Z

src/settings/imapimport/AddImapImportWizard.ts

+	}
+
+	get rootImportMailFolderName(): Stream<string> {
+		if (this._rootImportMailFolderName().length == 0) {


this is probably not correct

ganthern · 2023-07-05T12:47:49Z

src/settings/imapimport/AddImapImportWizard.ts

@@ -0,0 +1,163 @@
+import stream from "mithril/stream"


there are some missing validations for this model as well as missing error handling for broken imap server etc

ganthern · 2023-07-05T12:51:56Z

src/settings/imapimport/AddImapImportWizard.ts

+assertMainOrNode()
+
+export type AddImapImportData = {
+	model: ImapImportModel


this type indirection is unnecessary

ganthern · 2023-07-05T14:09:20Z

src/desktop/imapimport/adsync/ImapSyncSession.ts

+
+	onStartSyncSessionProcess(processId: number, nextMailboxToDownload: ImapSyncSessionMailbox): void {
+		if (this.state == SyncSessionState.RUNNING) {
+			console.log("onStartSyncSessionProcess : processId: " + processId + " -> " + nextMailboxToDownload.mailboxState.path)


concat in console.log

ganthern · 2023-07-05T14:11:04Z

src/desktop/imapimport/adsync/ImapSyncSessionMailbox.ts

+				break
+			case ImapMailboxSpecialUse.TRASH:
+			case ImapMailboxSpecialUse.ARCHIVE:
+			case ImapMailboxSpecialUse.ALL:


can we skip this, since it would give us all mails again?

ganthern · 2023-07-05T14:15:10Z

src/desktop/imapimport/adsync/ImapSyncSessionProcess.ts

+					this.adSyncOptimizer.optimizedSyncSessionMailbox.mailCount,
+				)
+				.then((deletedUids) => {
+					this.handleDeletedUids(deletedUids, openedImapMailbox, adSyncEventListener)


apparently, it's intentional to prevent the imap server from limiting us because of repetitive commands

ganthern · 2023-07-05T14:16:47Z

src/desktop/imapimport/adsync/ImapSyncSessionProcess.ts

+					nextUidFetchRequest.uidFetchSequenceString,
+					{
+						uid: true,
+						// @ts-ignore


ganthern · 2023-07-05T14:18:37Z

src/desktop/imapimport/adsync/ImapSyncSessionProcess.ts

+		})
+	}
+
+	private setupImapFlowErrorHandler(imapClient: ImapFlow, adSyncEventListener: AdSyncEventListener) {


should this be made use-once

jomapp added 7 commits June 28, 2023 17:13

Implement IMAP import

f495760

Add full support for IMAP QRESYNC feature

7f7bb3b

Add defaultDownloadBatchSize to AdSyncConfig

32a14b3

Do not calculate mail-diff if IMAP mailbox is empty

91cf1cb

Copy imapflow and mailparser node modules to libs

d22161a

Clean up code and extend AdSyncConfig

a746670

Final cleanup

a9e8aea

jomapp requested review from charlag, armhub, bedhub and ganthern June 30, 2023 08:22

jomapp linked an issue Jun 30, 2023 that may be closed by this pull request

Adaptive email import load control #4841

Open

2 tasks

charlag reviewed Jul 3, 2023

View reviewed changes

Minor fixes

e724beb

ganthern reviewed Jul 5, 2023

View reviewed changes

small improvements

5ccef48

charlag mentioned this pull request Nov 16, 2023

Single user email import #4837

Open

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

4837 single user email import #5582

4837 single user email import #5582

jomapp commented Jun 30, 2023

charlag left a comment

charlag Jul 3, 2023

charlag Jul 3, 2023

charlag Jul 3, 2023

charlag Jul 3, 2023

charlag Jul 3, 2023

charlag Jul 3, 2023

charlag Jul 3, 2023

charlag Jul 3, 2023

charlag Jul 3, 2023

charlag Jul 3, 2023

charlag left a comment

ganthern Jul 5, 2023

ganthern Jul 5, 2023

ganthern Jul 5, 2023

ganthern Jul 5, 2023

ganthern Jul 5, 2023

ganthern Jul 5, 2023

ganthern Jul 5, 2023

ganthern Jul 5, 2023

ganthern Jul 5, 2023

ganthern Jul 5, 2023

4837 single user email import #5582

Are you sure you want to change the base?

4837 single user email import #5582

Conversation

jomapp commented Jun 30, 2023

charlag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charlag left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment