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.

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

Retrying to send fail messages (enh) #7180

Closed

Conversation

sazanof
Copy link
Collaborator

@sazanof sazanof commented Sep 6, 2022

Hello everyone!
I will be glad if you consider this improvement and give your comments. =)
Maybe it refers to #6401

Basically there are 2.5 improvements:

  • Batch sending of emails with the failed status
    • The interface for sending a single message has been slightly redesigned
  • Background sending of emails with the failed status when the page loads

And here are the screenshots:

Peek 2022-09-06 16-03

Peek 2022-09-06 16-04

Perhaps another processing logic is needed here, so I would like to discuss this.
Thanks!

@sazanof sazanof changed the title Retry send fail messages (enh) Retrying to send fail messages (enh) Sep 6, 2022
@sazanof sazanof self-assigned this Sep 6, 2022
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

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

Very nice work! :) Design feedback:

  • The "Retry" icon button on the right might be better as just a text-only tertiary button with text "Retry".
  • The loading spinner doesn’t seem to be the standard one from the Vue components?
  • For long subject or error subline, the text needs to be ellipsized a bit earlier to not overlap with the icon

@sazanof sazanof force-pushed the enh/retry-message-send-on-fail branch from f463551 to b2b90c9 Compare October 15, 2022 10:58
sazanof pushed a commit to sazanof/nextcloud_mail that referenced this pull request Oct 15, 2022
Signed-off-by: Mikhail Sazanov <m@sazanof.ru>
@sazanof
Copy link
Collaborator Author

sazanof commented Oct 15, 2022

Very nice work! :) Design feedback:

  • The "Retry" icon button on the right might be better as just a text-only tertiary button with text "Retry".
  • The loading spinner doesn’t seem to be the standard one from the Vue components?
  • For long subject or error subline, the text needs to be ellipsized a bit earlier to not overlap with the icon

@jancborchardt done. Please take a look.

@sazanof sazanof requested a review from jancborchardt October 15, 2022 11:34
Mikhail Sazanov added 7 commits November 3, 2022 22:08
Signed-off-by: Mikhail Sazanov <m@sazanof.ru>
Signed-off-by: Mikhail Sazanov <m@sazanof.ru>
Signed-off-by: Mikhail Sazanov <m@sazanof.ru>
Signed-off-by: Mikhail Sazanov <m@sazanof.ru>
Signed-off-by: Mikhail Sazanov <m@sazanof.ru>
Signed-off-by: Mikhail Sazanov <m@sazanof.ru>
Signed-off-by: Mikhail Sazanov <m@sazanof.ru>
@sazanof sazanof force-pushed the enh/retry-message-send-on-fail branch from c6e4520 to 2e97866 Compare November 3, 2022 20:24
}
},
},
created() {
Copy link
Member

Choose a reason for hiding this comment

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

this means with every page load we retry sending those messages. I'm not sure if this is how we want it.

shouldn't each message have some kind of retry counter so it's only retried a few times? with the first iteration of the outbox feature we continuously retried sending failed messages. but that caused more trouble than expected. so this was deliberately disabled via #6602.

also see #6540. there are some side effects with blindly resending ever pending message in the outbox.

Copy link
Collaborator Author

@sazanof sazanof Nov 9, 2022

Choose a reason for hiding this comment

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

@ChristophWurst thank you! As you said, we can retry X-times to send messages only if message failed when it sent first time. In other cases, do not retry sending. What di you think?

In addition, if automatic sending is still not needed, can I leave the updated functionality for resending messages manually =)

Copy link
Member

Choose a reason for hiding this comment

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

Let's try with a counter 👍

Copy link
Collaborator Author

@sazanof sazanof Oct 12, 2023

Choose a reason for hiding this comment

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

Hello everyone!
Is it better to turn on the counter when the page loads, or to attract something like a permanent storage (localstorage, database)? Although probably the latter has more chances for bugs?

Copy link
Member

Choose a reason for hiding this comment

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

I'd say the counter should be in the backend

@ChristophWurst
Copy link
Member

also friendly ping here

@ChristophWurst ChristophWurst mentioned this pull request Nov 29, 2023
24 tasks
@ChristophWurst
Copy link
Member

#9364 and related handle this on the backend. The frontend trigger is obsolete

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants