phpmyadmin / error-reporting-server

phpMyAdmin server side component for the error reporting system
MIT License
19 stars 28 forks source link

Send Email Notifications on newly added reports #153

Closed devenbansod closed 7 years ago

devenbansod commented 7 years ago

Send these notifications to developers@phpmyadmin.net

Fix #31

Signed-off-by: Deven Bansod devenbansod.bits@gmail.com

codecov[bot] commented 7 years ago

Codecov Report

Merging #153 into master will increase coverage by 2.2%. The diff coverage is 100%.

@@            Coverage Diff             @@
##             master    #153     +/-   ##
==========================================
+ Coverage     43.99%   46.2%   +2.2%     
- Complexity      286     296     +10     
==========================================
  Files            23      25      +2     
  Lines          1341    1396     +55     
==========================================
+ Hits            590     645     +55     
  Misses          751     751
nijel commented 7 years ago

Is it possible to test email sending as well? Otherwise it looks good.

Thought I'm not really sure if we want notifications for all reports on the devel list, maybe it would be better to have some summary mails or create separate list for these notifications.

devenbansod commented 7 years ago

We could have a cron job running once a week. The cron job would run a shell which sends emails for the reports added in the previous week. (We can may be limit number of reports in a mail to five/ten to prevent a very long mail and instead send them in parts). What do you think?

I am okay with having a separate list for a trial and then we can decide later based on whether it is receiving too many email notifications per week.

nijel commented 7 years ago

I think we can start with code you've written on separate list, let's see how that works.

And what about the tests?

devenbansod commented 7 years ago

Yes. I will take a look on how we can test sending emails.

devenbansod commented 7 years ago

Hi @nijel, added the tests for sending email notifications.

nijel commented 7 years ago

Merged, thanks for your contribution!