Added subscription to folders

[josaphatim]

Pullrequest

Add support for IMAP folder subscriptions

Issues

• fixes Add support for IMAP folder subscriptions #582
• relates https://avan.tech/item81126

[jasonmunro]

I appreciate your work on this, but I think we should have discussed this a bit more :)

1. This changes the default behavior to subscribed folders only being displayed. This should be a user level setting and disabled by default. I would prefer to have to opt-in to showing subscribed folders only. Subscription options should be hidden unless enabled.

2. The subscribe/unsubscribe list on the folders page appears to do a recursive list of all folders, this is considered bad form for an IMAP client. It is possible with some server configurations that this will return a TON of entries (old IMAP servers used to use the users UNIX home directory as the mailbox, and a recursive list all could easily return 100K of mostly unusable filenames). With that said it may not be as much of an issue these days so we may be able to get away with it. Currently we only list folders at the top level of the mailbox, and if you want to drive down to subfolders clicking the "+" does a list of that parent folder only which we cache in the folder list until it is reset. You will note other components on folders page use the same folder tree by cloning it from the folder list and following the same pattern.

3. subscribing to a subfolder failed, top level folder subscribe/unsubscribe worked as expected.

4. The log shows that a handful of modules are being re-enabled on the same page id which is incorrect. Modules can only be enabled once for a particular page id.

[2] => Already registered module for ajax_imap_folders_create re-attempted: login
    [3] => Already registered module for ajax_imap_folders_create re-attempted: default_page_data
    [4] => Already registered module for ajax_imap_folders_create re-attempted: load_user_data
    [5] => Already registered module for ajax_imap_folders_create re-attempted: language
    [6] => Already registered module for ajax_imap_folders_create re-attempted: date
    [7] => Already registered module for ajax_imap_folders_create re-attempted: http_headers
    [8] => Already registered module for ajax_imap_folders_create re-attempted: load_imap_servers_from_config
    [9] => Already registered module for ajax_imap_folders_create re-attempted: process_folder_create
    [10] => Already registered module for ajax_imap_folders_create re-attempted: imap_bust_cache
    [11] => Already registered module for ajax_imap_folders_create re-attempted: close_session_early

5. maybe consider moving the subscription options to their own page. If we keep the recursive list all folders IMAP command then this would reduce the times we issue the command to the IMAP server to only we absolutely need to.

6. After subscribing/unsubscribing the folder list needs to be reset otherwise the changes are not visible.

[marclaporte]

"we should have discussed this a bit more"

Indeed, we could have a process to ask more questions beforehand. (Ex.: will a merge request on XYZ be accepted? How would you prefer we do Y?) But a lot of things we start don't necessarily get finished soon after. Priorities shift. And some of the developers are in training. So the risk is that we'd open up a lot of discussions, and that many of them lead nowhere. It's more important that we optimize your time. So I am quite OK that the discussion starts on a merge request. This means the junior developer has a good grasp of the challenge, and a fruitful discussion can ensue. And please do feel comfortable in asking for major changes to a MR. The devs are in training and this is a great way for them to learn.

Thanks!

[dumblob]

Indeed, we could have a process to ask more questions beforehand.

That is definitely an overkill IMHO.

I think Jason simply meant that the effort put into this PR could be better focused if a few things got first described in words instead of code as describing in words would probably be sufficient for understanding and would take less time and less effort for the author 😉.

I for myself find it actually mostly better to "show the code" as soon as possible as that completely eliminates misunderstandings which are often the major source of delays in implementation of features.

[josaphatim]

Hello @jasonmunro. I pushed a new commit. Please check.

[jasonmunro]

Looks like we have some conflicts here that need to be resolved before re-checking. Thanks!

[josaphatim]

Looks like we have some conflicts here that need to be resolved before re-checking. Thanks!

Already resolved. Thanks.

[kroky]

I think Jason's comments were addressed but there are a few functionality issues. Let's solve them and then we can merge.

[josaphatim]

if (subscription) {

Hello @kroky I made changes.

[josaphatim]

@kroky can you check please, I fixed some issues :

• When parent is unsubscribed but children subscribed and styling to show that parent folder is disabled
• Folder name issue when parent has children and some other flags

[kroky]

All looks good to me but it needs some extensive testing. I might be able to do it after the holidays.

[josaphatim]

Hello @kroky. Any update on this ?

[kroky]

It still seems a bit shaky. I see + LSUB in the folder list now which doesn't have any children.

I also go to manage folders, create a new test folder under INBOX, manage to move emails in and out. Then, turn on show only subscribed folders. I don't see the test one anymore. I go to manage folders, settings icon on the top right, clicked the checkbox of test folder. I still don't see it in the list. Reloaded the folder list but I don't see it. Now I can't go to subscribed folders anymore. I go to manage folders, click the top right hand side cog icon, it redirects to cypht home page. Something's wrong here.

[kroky]

Also, please make sure you merge latest master into this branch, so you can actually test against the .env and other enhancements there.

[josaphatim]

It still seems a bit shaky. I see + LSUB in the folder list now which doesn't have any children.

I also go to manage folders, create a new test folder under INBOX, manage to move emails in and out. Then, turn on show only subscribed folders. I don't see the test one anymore. I go to manage folders, settings icon on the top right, clicked the checkbox of test folder. I still don't see it in the list. Reloaded the folder list but I don't see it. Now I can't go to subscribed folders anymore. I go to manage folders, click the top right hand side cog icon, it redirects to cypht home page. Something's wrong here.

Thanks @kroky

Can you check again. I took in account everything you reported here. But one thing I found is that when create a folder by default it is unsubscribed.