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

Add exception handling #10

Closed
diogosantana opened this issue Feb 24, 2016 · 9 comments
Closed

Add exception handling #10

diogosantana opened this issue Feb 24, 2016 · 9 comments
Assignees
Labels

Comments

@diogosantana
Copy link

There is no specific exception handling for sending e-mail. This cause a lot of undesirable behavior:

  1. No log record are created
  2. No e-mail is sended, even if only one rcpt is invalid
  3. Server returns HTTP STATUS 500 with the stackstrace string as body, making the browser to show the body as text

You could use setSendPartial to prevent behavior number 2, but it still will needed to handle exception.

It also will be nice to log which e-mails were invalid with SendFailedException.getInvalidAddresses.

Sorry not to provide a PR, but I don't work with Scala.

@McFoggy McFoggy added the bug label Feb 24, 2016
McFoggy added a commit to McFoggy/gitbucket-announce-plugin that referenced this issue Feb 25, 2016
@McFoggy
Copy link
Member

McFoggy commented Feb 25, 2016

@diogosantana I have introduced checks but unfortunately my local SMTP does not fail when using invalid user/emails adress ; so I am not sure it will solve the issue as expected.
Could you please confirm that the modifications fix your problems in your case.
I have prepared gitbucket-announce-plugin-1.3-alpha1 alpha release for tests : stop gitbucket, download the new jar, replace gitbucket-announce-plugin.jar in your GITBUCKET\plugins directory with this new one, and relaunch.
Thanks for your feedback.

@diogosantana
Copy link
Author

Sure. I'll test it and feed back the result here.
Em 25/02/2016 05:31, "Matthieu Brouillard" notifications@github.com
escreveu:

@diogosantana https://github.com/diogosantana I have introduced checks
but unfortunately my local SMTP does not fail when using invalid
user/emails adress ; so I am not sure it will solve the issue as expected.
Could you please confirm that the modifications fix your problems in your
case.
I have prepared gitbucket-announce-plugin-1.3-alpha1
https://github.com/McFoggy/gitbucket-announce-plugin/releases/tag/1.3-alpha1
alpha release for tests : stop gitbucket, download the new jar, replace
gitbucket-announce-plugin.jar in your GITBUCKET\plugins directory with
this new one, and relaunch.
Thanks for your feedback.


Reply to this email directly or view it on GitHub
#10 (comment)
.

@McFoggy McFoggy self-assigned this Feb 25, 2016
@diogosantana
Copy link
Author

@McFoggy I do realize that my installation were using Java 7, so I got class version error. I changed to Java 8, but I was still using GitBucket 3.10 (Java 7 compiled) and got the same problem.

Now I will migrate the environment to Java 8 and GitBucket 3.11, but it will take a while because of work requirements. I'll let you know.

@McFoggy
Copy link
Member

McFoggy commented Feb 28, 2016

@diogosantana any news? could you try with your SMTP server?

@diogosantana
Copy link
Author

Not yet. My ops team will upgrade the server environment to match GitBucket requeriments, but it will take some days. My expectation is later this week.

@diogosantana
Copy link
Author

@McFoggy It worked, the e-mail message were sent to all valid address and a error message was registered to the log. But item 3 is stil occuring.

gitbucket-announce-plugin-issue10

@McFoggy
Copy link
Member

McFoggy commented Mar 6, 2016

@diogosantana I have adapted the code and published a temporary version on my fork

Using gmail smtp I do not receive any failure when using non existing emails, so again I need you to test. Thanks for your support and feedback.

@diogosantana
Copy link
Author

It worked. But when I simulate another kind of Exception (authetication failed) the page show "Announce has been sent.".

I do contribute a PR to your fork, please have a look. With those changes all went well.

@diogosantana
Copy link
Author

The PR: McFoggy#1

@McFoggy McFoggy closed this as completed in 08139db Mar 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants