search

joplin / plugin-email

This plugin adds the ability to fetch email messages and converts them to Joplin notes in various formats, either by monitoring any new or unread messages from a specific email address or a specific mailbox, or by uploading downloaded email messages to the plugin without having to be logged in.
29 stars 4 forks source link

week9,10,11 #6

Closed bishoy-magdy closed 2 years ago

bishoy-magdy commented 2 years ago

What has been done

When I installed html-to-text, I found an error when compiling, and I modified the webpack file in rules to add read mjs for the grammar file link.

roman-r-m commented 2 years ago

I left a few comments but tbh I found it very hard to review - it's huge PR and some parts of the code aren't clear to me. Next time please try to commit in smaller self-contained pieces, this way I could have reviewed commit by commit.

bishoy-magdy commented 2 years ago

I left a few comments but tbh I found it very hard to review - it's huge PR and some parts of the code aren't clear to me. Next time please try to commit in smaller self-contained pieces, this way I could have reviewed commit by commit.

okay, sorry about that.

roman-r-m commented 2 years ago

A few more general comments not tied to any particular place in the code. Not sure if there's time to implement but I'll leave them here anyway.

  1. Would be good to have a way of refreshing the mailboxes on the main screen. For instance, I logged on to set up the plugin and realised I forgot to create a dedicated joplin folder in my email account. I then created it but the plugin could not see it because it fetched the list only once.

  2. Some basic user guide could be useful. For instance, once I logged on into my email I am presented with the Main Screen which asks me to enter some email in the "From" field. Not clear what should go there, is it the same email account that I've just logged into? If so, there should probably be a dropdown as the list of the configured accounts should be known at this point. And if there's only one account (as I suspect will be in 99% of the cases), then it makes sense to pre-populate the input.

  3. Same as the 1st item but for Joplin notebooks. In fact I found that even closing and re-opening the plugin panel does not seem to refresh the list of IMAP or Joplin folders. Is restarting the app the only option?

  4. With "Fetching & Monitoring MailBox" it seems to be scanning the mailbox every 5 seconds or so? I'd say it's a bit excessive, probably once a minute is enough.

  5. When importing attachments the name of the file is stored in the note with unneeded slashes, e.g. "file_name" becomes "file_name"

bishoy-magdy commented 2 years ago
  1. Would be good to have a way of refreshing the mailboxes on the main screen. For instance, I logged on to set up the plugin and realised I forgot to create a dedicated joplin folder in my email account. I then created it but the plugin could not see it because it fetched the list only once.
  2. Same as the 1st item but for Joplin notebooks. In fact I found that even closing and re-opening the plugin panel does not seem to refresh the list of IMAP or Joplin folders. Is restarting the app the only option?

I solved the first and third problems by adding the refresh button. Once you click the refresh button, the mailbox and folders will be updated, and the imap will be connected again if it breaks.

f1

Demo

https://user-images.githubusercontent.com/58605547/188420245-3b643f0b-5658-4cd2-8cd5-73ba54276aba.mp4

bishoy-magdy commented 2 years ago

For instance, once I logged on into my email I am presented with the Main Screen which asks me to enter some email in the "From" field. Not clear what should go there, is it the same email account that I've just logged into? If so, there should probably be a dropdown as the list of the configured accounts should be known at this point. And if there's only one account (as I suspect will be in 99% of the cases), then it makes sense to pre-populate the input.

@, # works when forwarding messages to yourself after adding @ or # in the subject or in the first line of email content(In this case, you will enter your account in the "From" field );

To be more generalized @, # works for any subject that has mentions or hashtags.

Otherwise, the converted messages from the account entered by the user ("From" field ) will be stored in the Joplin email messages folder automatically.

This is only for "From" field

  1. Some basic user guide could be useful.

Honestly, I was confused about how to add a guide, but I tried to add tooltips beside the ''From'' field. If you think it's not suitable, please tell me and I will remove it. Plus, I will write how to use it with examples and a demo video in the Readme I'm almost done with it, and I will create a new PR for it.

https://user-images.githubusercontent.com/58605547/188420756-1c5f592b-e61e-4876-95ec-10c862ff699d.mp4

bishoy-magdy commented 2 years ago
  1. With "Fetching & Monitoring MailBox" it seems to be scanning the mailbox every 5 seconds or so? I'd say it's a bit excessive, probably once a minute is enough.

I changed it to every minute, I used 5s just for testing.

bishoy-magdy commented 2 years ago
  1. When importing attachments the name of the file is stored in the note with unneeded slashes, e.g. "file_name" becomes "file_name"

I think I don't understand this point. Could you please be more specific because the attachment names are the same as the names in the Joplin resource? I didn't change anything in the file name.

f4

bishoy-magdy commented 2 years ago

If you have anything else to edit or see something inconsistent, please let me know. thanks

roman-r-m commented 2 years ago

The refresh button is great. One minor issue is that it may not be clear to everyone what exactly it's supposed to refresh. Maybe moving it closer to the respective inputs can help, not sure.

Btw, I have just realised that you haven't published the plugin yet. I think we're now in a position where you can do it and make an announcement on the forum. Maybe we will get useful feedback from people, especially around UI.

Also, just noticed that package name in package.json is a bit weird:

"name": "joplin-plugin-email-plugin",

you may want to correct that before publishing.

roman-r-m commented 2 years ago

Honestly, I was confused about how to add a guide, but I tried to add tooltips beside the ''From'' field. If you think it's not suitable, please tell me and I will remove it.

I thought maybe put some instructions into README.md

roman-r-m commented 2 years ago
  1. When importing attachments the name of the file is stored in the note with unneeded slashes, e.g. "file_name" becomes "file_name"

I think I don't understand this point. Could you please be more specific because the attachment names are the same as the names in the Joplin resource? I didn't change anything in the file name.

I sent an email with a PDF attached, the file name was List_of_supporting_documents_UK_en.pdf. When the plugin pulled the email and created a note, the text was:

[List\_of\_supporting\_documents\_UK_en.pdf](:/5947bcc6916043f7a677962b47387a40)

The name of the resource in the DB looks to be correct.

PackElend commented 2 years ago

what a review session thx both @bishoy-magdy make sure you have it well covered in your weekly and publish function changes/update in the forum

bishoy-magdy commented 2 years ago

The refresh button is great. One minor issue is that it may not be clear to everyone what exactly it's supposed to refresh. Maybe moving it closer to the respective inputs can help, not sure.

hmmmm, I had put two refresh buttons next to the mailbox and notebook selectors, and I think it's clear now what these buttons do now!

11

But anyway, I will mention this point in the UI feedback post.

Btw, I have just realised that you haven't published the plugin yet. I think we're now in a position where you can do it and make an announcement on the forum. Maybe we will get useful feedback from people, especially around UI.

After merging this PR, I'll publish the project in npm (to be on the most recent github update) and now I'm preparing a post about UI feedback with polls.

Also, just noticed that package name in package.json is a bit weird:

"name": "joplin-plugin-email-plugin",

you may want to correct that before publishing.

I fixed it (joplin-plugin-email)

bishoy-magdy commented 2 years ago
  1. When importing attachments the name of the file is stored in the note with unneeded slashes, e.g. "file_name" becomes "file_name"

I think I don't understand this point. Could you please be more specific because the attachment names are the same as the names in the Joplin resource? I didn't change anything in the file name.

I sent an email with a PDF attached, the file name was List_of_supporting_documents_UK_en.pdf. When the plugin pulled the email and created a note, the text was:

[List\_of\_supporting\_documents\_UK_en.pdf](:/5947bcc6916043f7a677962b47387a40)

The name of the resource in the DB looks to be correct.

This change does not happen from the plugin but from the markdown Joplin render. Check it out by using the Joplin clipper and trying to clip any page that has the underscore "firstsecond" to makedown.

I think it is used slash to save the style in the case of tables or anything else. It looks like glue.

https://user-images.githubusercontent.com/58605547/188588721-1007d9ab-3ee8-4678-a4f0-ac5351d9d5a6.mp4

roman-r-m commented 2 years ago

Thanks, let's merge then. Hopefully we can get some feedback from other users

bishoy-magdy commented 2 years ago

Thanks, let's merge then.

Big thanks for this code review ❤️❤️❤️ and sorry for this huge PR. It will never happen again.

Hopefully we can get some feedback from other users

We will do our best.