Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(backend): add scheduled send functionality #1091

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

feat(backend): add scheduled send functionality #1091

wants to merge 4 commits into from
+1,122 −615

Conversation

amaninyumu1
Copy link
Member

@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch from 7aaffb2 to 0c58a6a Compare June 16, 2024 14:58
@marclaporte
Copy link
Member

Will this work with JMAP as well?

@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch from 0c58a6a to 0aef136 Compare June 19, 2024 08:09
@josaphatim josaphatim requested a review from kroky June 19, 2024 08:12
@amaninyumu1 amaninyumu1 marked this pull request as ready for review June 19, 2024 08:12
@kroky
Copy link
Member

kroky commented Jun 20, 2024

Yes, anything we add for IMAP, we should check JMAP support as well. Overall, it is built on top of IMAP, so it should work but please check if the new Scheduled folder is OK for JMAP.

Overall, the code is good but I have one concern - it only sends out while you are actively using Cypht and it seems to warn of unsent/scheduled messages each time you leave a page (onbeforeunload). That would be annoying for the end user. I believe this comment #576 (comment) proposes using sendAt support from jmap servers or scheduled send - if it is supported, we use it, if not, we fallback to a more annoying option. At any rate, it should be configurable.

This could be fully-supported in the Cypht-Tiki integration where there could be a Tiki command run periodically in the scheduler that checks and sends scheduled messages.

@josaphatim
Copy link
Member

@kroky I added a commit to fix issue concerning onbeforeunload event and also add the option to change schedule time or send the message immediately. Can you check please.

@josaphatim josaphatim force-pushed the Cypht-delay-send-later-scheduled-sending branch 2 times, most recently from 36599a8 to 2da4ff4 Compare June 27, 2024 13:45
@marclaporte
Copy link
Member

@amaninyumu1 If you need help with JMAP, please reach out to @Shadow243 as he set up a JMAP server for testing.

@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch 2 times, most recently from 45374ae to bdf9d5d Compare July 29, 2024 20:29
@Baraka24 Baraka24 force-pushed the Cypht-delay-send-later-scheduled-sending branch 5 times, most recently from cf0574d to af1c185 Compare August 2, 2024 10:21
@marclaporte
Copy link
Member

@amaninyumu1 "This branch has conflicts that must be resolved"

@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch 2 times, most recently from 141f8d9 to 16293f5 Compare September 14, 2024 06:13
@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch from 16293f5 to 41c9b95 Compare September 26, 2024 06:16
@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch from 41c9b95 to 3fb7802 Compare October 8, 2024 02:47
@marclaporte
Copy link
Member

@amaninyumu1 This branch has conflicts that must be resolved

@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch from 3fb7802 to e725bcc Compare October 22, 2024 19:54
@amaninyumu1
Copy link
Member Author

Hello @marclaporte , @kroky I resolved the conflicts. please review

kroky
kroky reviewed Oct 23, 2024
View reviewed changes
modules/smtp/modules.php Outdated Show resolved Hide resolved
kroky
kroky reviewed Oct 23, 2024
View reviewed changes
modules/smtp/modules.php Outdated Show resolved Hide resolved
kroky
kroky reviewed Oct 23, 2024
View reviewed changes
modules/smtp/modules.php Outdated Show resolved Hide resolved
kroky
kroky reviewed Oct 23, 2024
View reviewed changes
modules/smtp/modules.php Outdated Show resolved Hide resolved
@kroky
Copy link
Member

kroky commented Oct 23, 2024

There are still conflicts and I believe big part of the reason is #1266 - @jacob-js , can you check if some handlers in this PR need to be updated according to the router changes you did in 1266?

@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch 2 times, most recently from 439801a to 5dc5670 Compare December 17, 2024 10:37
@mercihabam
Copy link
Member

@amaninyumu1 you need to correct the commit history before I give you any other round of review.

@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch 2 times, most recently from 23e01d4 to 5fef6a2 Compare December 17, 2024 14:44
@amaninyumu1
Copy link
Member Author

Hello @mercihabam I merged the two commits that were before to make one. Maybe it's me who doesn't understand the maneuver to follow. please guide me

@mercihabam
Copy link
Member

@amaninyumu1 I did not ask you to combine the commits into one. I'm telling you to do the opposite. Please re-read carefully the related comments!

@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch from 5fef6a2 to ea33598 Compare December 17, 2024 16:54
@amaninyumu1
Copy link
Member Author

I understand I misunderstood the comment. To be honest about the commit history, I don't know how to go about it. please guide me. THANKS

@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch from ea33598 to 6e7357e Compare December 21, 2024 06:20
@amaninyumu1
Copy link
Member Author

Hello @mercihabam , @kroky . please merge this PR if there is anything to fix we can do it later, we risk returning to square one with this presence repeating conflicts

@mercihabam
Copy link
Member

@amaninyumu1, I'm sorry, but we cannot merge this as long as it still contains poor code that risks introducing regressions.
Also, the commit history! I still need to emphasize that aspect. You recently pushed a commit, but what changes did it bring to your existing code?

@amaninyumu1
Copy link
Member Author

if you can't merge this while it still contains poor code that risks introducing regressions. I'm ready to fix that for the PR to merge. But when you say that it's the commit history that blocks evolution... I don't understand that

@marclaporte
Copy link
Member

But when you say that it's the commit history that blocks evolution... I don't understand that

Please see: #1091 (comment)

The current situation is causing more work for the reviewers because they need to re-review everything instesd of just your latest modifications. Once everything is done, there will be the option to squash commits.

@amaninyumu1
Copy link
Member Author

Okay I understand @marclaporte , soon I will make a commit for each change. because going back to the old history risks losing the current changes.

@amaninyumu1 amaninyumu1 closed this Jan 2, 2025
@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch from 6e7357e to 32d2d9c Compare January 2, 2025 19:26
@amaninyumu1 amaninyumu1 reopened this Jan 2, 2025
@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch from 756605f to d7964a4 Compare January 3, 2025 15:15
@mercihabam
Copy link
Member

Kudos, @amaninyumu1, for resolving all the threads! Now, let's review the end-user functionality of what the code produces. Please include a video demonstration in the chat.

@amaninyumu1 amaninyumu1 force-pushed the Cypht-delay-send-later-scheduled-sending branch from 6409b31 to d36293a Compare January 7, 2025 19:41
@amaninyumu1
feat(backend): add scheduled send functionality resolve conflicts in …
ce29754 
…modules/imap/functions.php
@amaninyumu1
feat(backend): add scheduled send functionality
d36293a 
Removed redundant showRoutingToast() and hideRoutingToast() calls in reload_and_redirect function as navigate() handles them. Streamlined the process for scheduling and snoozing actions.
@amaninyumu1
feat(backend): clarify reload_and_redirect logic by removing unnecess…
382ca1c 
…ary isReloading
@amaninyumu1
Copy link
Member Author

Hello @mercihabam I just added a new commit to fix server error Uncaught Error: Call to undefined method Hm_Mailbox::get_mailbox_status() which occurred after rebasing with the master branch. Please review

@amaninyumu1
Copy link
Member Author

amaninyumu1 commented Jan 7, 2025 •
edited
Loading

For the video demonstration I am blocked by this error server Undefined array key "list_page" inmodules\imap\handler_modules.php` which blocks the display of messages in all folders

@amaninyumu1
feat(backend): refactor the code to fix the server error Uncaught Err…
127713a 
…or: Call to undefined method Hm_Mailbox::get_mailbox_status()
kroky
kroky reviewed Jan 9, 2025
View reviewed changes
modules/smtp/modules.php
foreach ($ids as $msg_part) {
list($imap_server_id, $msg_id, $folder) = explode('_', $msg_part);
$cache = Hm_IMAP_List::get_cache($this->cache, $imap_server_id);
$imap = Hm_IMAP_List::connect($imap_server_id, $cache);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that EWS has been merged to master, we bypass direct imap object usage from Hm_IMAP_List in all handler modules and instead use Hm_Mailbox which is a bridge between handlers and imap/ews implementation classes. Please update this code according to what is currently present in imap handler_modules after merging (or rebasing) latest mater.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that in order to merge upstream changes successfully, it is not only a matter of resolving the conflicts but understanding the upstream changes and doing local changes to your code accordingly.

@mercihabam
Copy link
Member

For the video demonstration I am blocked by this error server Undefined array key "list_page" inmodules\imap\handler_modules.php` which blocks the display of messages in all folders

@amaninyumu1 can you fix?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@josaphatim josaphatim josaphatim left review comments

@mercihabam mercihabam mercihabam left review comments

@kroky kroky kroky left review comments

@marclaporte marclaporte marclaporte left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@amaninyumu1 @marclaporte @kroky @josaphatim @mercihabam