week 6 & 7& 8 #5

bishoy-magdy · 2022-08-10T15:47:06Z

What has been done

Added the feature of fetching the message data from imap.
Installed the email parser.
Added the feature of parsing the HTML message and extracting the body using DOMParser.
Added the feature of marking the converted message in the plugin without editing the message or adding flags.
Added the feature to save the last change on the screen (main screen) when the plugin is closed.
Created email parser class and added the unit test for the email parser class.
Added the ability to convert email notes to text.
Created the class responsible for posting notes to Joplin.
Added inline attachments to note.
Added the feature of converting emails to notes without logging in.

Demo

A demo example of the latest feature that was added

offline2.mp4

src/core/emailParser.ts

src/core/imap.ts

roman-r-m · 2022-08-16T20:04:37Z

src/core/imap.ts

+            }
+
+            try {
+                const value = await joplin.settings.value(CONVERTED_MESSAGES);


This setting will grow over time, could become a performance bottleneck at some point.

I came up with a better solution: get the converted message and store it in a variable, then query that variable to see if there is a new message, and when there is, the values are updated. This means that the converted message in Joplin is not only updated until there is a new message(If you have another solution please tell me).

Not sure what you mean by variable here? You need to persists this information between restarts somehow.

It seems to me that the easiest way to store this information is to use the email account itself - you either delete messages you've converted, mark them read, or move to another folder.
Or you could only store the date of the last seen email and only process newer. This way you store it on the client but at least it's just one number per mailbox.

indeed, the thought came to my mind to store this information by using the email account itself, but there is no standard way to mark the message. To be more clear, some email providers allow you to mark messages while others don't.

But I found a solution One of the flags that are standard for all email providers is the 'SEEN' flag. Every message is flagged as 'UNSEEN' if the message is new or the user marks it as 'UNSEEN'.

So I will add an additional condition in the search, which is to search for the messages that are "from" a specific email and marked as "UNSEEN" and then mark the messages as "SEEN" after fetching them.

I think this solution is better than deleting. and the user can mark as an "unread" message any number of messages in their email provider.

Sorry for the delay in suggesting this solution. It took me a lot of time to test this solution across different email providers.

example:

git2.mp4

src/core/postAttachments.ts

roman-r-m · 2022-08-16T20:10:03Z

src/core/postAttachments.ts

+                    ],
+                );
+
+                attachmentsProp.push({contentId: contentId, id: resource.id});


might be worth cleaning up the temp file afterwards

I tried deleting folders in this place, but it will slow down when there are a lot of messages that you want to convert because it will create and delete and so on. What I did is every time the plugin starts, it deletes the old attachments and starts creating a new temporary folder.

Could potentially be a security risk, say, if you run the plugin in Joplin portable from a USB drive, your emails will be left on the computer.

can't the delete job be a self-running/isolated background job?
The "old" temp folder could be renamed, a new one created, old, now renamed, deleted in the background

Could potentially be a security risk, say, if you run the plugin in Joplin portable from a USB drive, your emails will be left on the computer.

Okay, I refacted the function to this mechanism.

Create a temp folder

Post all emails including attachments to Joplin.

Remove a temp folder

can't the delete job be a self-running/isolated background job?
The "old" temp folder could be renamed, a new one created, old, now renamed, deleted in the background

It's a sync function, which means I can't go to the next line after finishing this function(delete a temp folder).

You could in theory use a web worker (or whatever the equivalent of threads in the JS world is called) but this looks like an overkill.

You could in theory use a web worker (or whatever the equivalent of threads in the JS world is called) but this looks like an overkill.

This looks very good since we can add all the notes and attachments posting functions in the other JS thread.
I will read more about web worker. and I will definitely add it if I notice its presence will be important.

src/core/postNote.ts

roman-r-m · 2022-08-19T19:50:13Z

src/core/postNote.ts

-        const emailFolders = joplinFolders.items.filter((e)=>{
-            return this.folders.includes(e.title);
-        });
-
        const emailTags = joplinTags.items.filter((e)=>{
            return this.tags.includes(e.title);


I can't find how these tags are set right now, but I think this will stop working if a tag is renamed. Best to use IDs as you did with folder.

I guess it's not a concern for manual one-time import but for monitoring mailbox it is.

I moved this code to the 'addTags' function and fixed it so that if 'has_more' in Joplin tags is true, this function now takes a set of tags and creates all tags that are not inside Joplin tags, and it returns all mentioned tags with all properties.

If something is not clear, please tell me.

removed the feature of local marking messages, added the feature of marking messages using the email providers, fixed If 'has_more' is true in Joplin tags or folders, refacted TempFolder mechanism

roman-r-m · 2022-08-23T21:26:37Z

src/core/imap.ts


+                        setupTempFolder = new SetupTempFolder();


A minor thing but there is no reason to create a new instance on every iteration.
In fact, you don't need to create any instances of this class at all - all method can be made static

OK, it will be changed in the next PR.

roman-r-m · 2022-08-23T21:27:40Z

src/core/imap.ts

-                        await this.updateConvertedMessages(newMessages);
-                        this.uids.push(...newMessages);
-
+                        setupTempFolder.removeTempFolder();


Not sure if it will be any faster but you don't need to delete the whole folder, only specific files corresponding to already processed emails.

Deleting an entire file is a little faster than looping through each file and deleting it.

only specific files corresponding to already processed emails.

This is actually only what will be in the file, which means that the whole file will be deleted, and I also want to leave no trace after doing what is required.

I tested it by converting more than 100 eml files. Some of them contain large files containing videos and images, and the conversion process works fine.

roman-r-m · 2022-08-23T21:33:42Z

Overall looks good, I'm going to merge despite a couple of small comments. Feel free to ignore those or address in subsequent PRs.

bishoy-magdy · 2022-08-27T01:47:04Z

Overall looks good, I'm going to merge despite a couple of small comments. Feel free to ignore those or address in subsequent PRs.

Big thanks for this code review. In parallel, I made some changes for the purpose of improvement for some code that will be in the next PR.

week6,7,8

156b2bf

bishoy-magdy requested a review from roman-r-m August 16, 2022 09:24

roman-r-m reviewed Aug 16, 2022

View reviewed changes

src/core/emailParser.ts Outdated Show resolved Hide resolved

roman-r-m reviewed Aug 16, 2022

View reviewed changes

src/core/imap.ts Outdated Show resolved Hide resolved

roman-r-m reviewed Aug 16, 2022

View reviewed changes

src/core/postAttachments.ts Outdated Show resolved Hide resolved

roman-r-m reviewed Aug 16, 2022

View reviewed changes

update -- week6,7,8

5f96f62

bishoy-magdy requested a review from roman-r-m August 17, 2022 22:11

roman-r-m reviewed Aug 19, 2022

View reviewed changes

update -- week6,7,8

250d95e

removed the feature of local marking messages, added the feature of marking messages using the email providers, fixed If 'has_more' is true in Joplin tags or folders, refacted TempFolder mechanism

roman-r-m reviewed Aug 23, 2022

View reviewed changes

roman-r-m merged commit 53b1faa into main Aug 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

week 6 & 7& 8 #5

week 6 & 7& 8 #5

bishoy-magdy commented Aug 10, 2022

roman-r-m Aug 16, 2022

bishoy-magdy Aug 17, 2022

roman-r-m Aug 19, 2022

bishoy-magdy Aug 21, 2022

bishoy-magdy Aug 21, 2022

roman-r-m Aug 16, 2022

bishoy-magdy Aug 17, 2022

roman-r-m Aug 19, 2022

PackElend Aug 20, 2022

bishoy-magdy Aug 21, 2022

bishoy-magdy Aug 21, 2022

roman-r-m Aug 23, 2022

bishoy-magdy Aug 27, 2022

roman-r-m Aug 19, 2022

bishoy-magdy Aug 21, 2022

roman-r-m Aug 23, 2022

bishoy-magdy Aug 27, 2022

roman-r-m Aug 23, 2022

bishoy-magdy Aug 27, 2022

roman-r-m commented Aug 23, 2022

bishoy-magdy commented Aug 27, 2022

week 6 & 7& 8 #5

week 6 & 7& 8 #5

Conversation

bishoy-magdy commented Aug 10, 2022

What has been done

Demo

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

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

roman-r-m commented Aug 23, 2022

bishoy-magdy commented Aug 27, 2022