internal > notifications > Bug (done?)

Output of index.php?go=triggerSendNotifications not useful/#7559

MoveBookmark

Summary

done?
Jan 18, 2009
100%
Jan 18, 2009 / ganesh
Jan 26, 2009 / ganesh
View previous 3 versions
ganesh
 

Attached files

No files uploaded
   
 
Currently, if I wget index.php?go=triggerSendNotifications I get a text-only output containing a localized version of the string "%s notifications sent" with possibile variations "One notification sent" and "No notifications sent".
IMHO, there are two problems:
  1. the reported number of notifications is wrong: the number output is count($PH->messages); but $PH->messages contains one FeedbackMessage object for each person who might have received a notification but did NOT get it because there was nothing to notify, plus one FeedbackWarning object for each error while sending the mail. (Unless I'm much mistaken, notifications actually sent are not even counted!)
  2. the output message is not suitable for a cron job: human are not expected to be reading this output, OTOH a script might need to parse it in order to propagate warnings to an administrator
I have prepared a fix to address the first problem, but in order to address the second problem I would like hear peoples opinion about how the message should be formatted. My opinions are:
  1. I would avoid localization and variations as they make non-human parsing more complicated
  2. In case of warnings, the message should contain the text of all warning messages (these messages may be localized, as long as a script is able to detect that they're warnings)
For example, I would stick with:
Notifications sent: %s // line always present
Warning: %s // one line for each warning if present
Comments?

Issue report

Minor
Always
Linux
v0.9
 

5 Comments

ganesh:pixtur said in #2211

20 months ago

Hi ganish,

thanks for addressing this problem. Your are totally right in all of your points. No human is going to read it, so giving a human readable response is useless.

I am not sure about printing the warnings, though, because it might have some security consideration like revealing usernames. Remember: TriggerNotifications is available anonymously.

How about something like this:
Notifications sent: %s
Warning: %s
# Please check errors.log.php for details.

Please consider to create a new topic of discussions like this, because too many comments will make the user-documenation unreadable.

How would you like to submit the patch? Svn?

  pixtur

ganesh:I agree

20 months ago

I totally forgot about anonymous access. I agree that there would be a security/privacy problem in writing full warnings on the output. It's a pity, because a cron job using wget from a separate machine would not be able to access error.log.php, but security is more important. Moreover warnings should occur very rarely.
Your proposed output sounds right to me. I should already have write access to the svn so I can submit the patch through it.

ganesh:

20 months ago

Implemented as described in SVN revision 469

pixtur:excellent, but...

20 months ago

Some hints to your modifications:
  1. Please use the coding conventions. Variables should be written with underlines, not CamelCase.
  2. I added a num_-prefix to make sure the variable is not an array, as it name would suggest.
  3. Try to add comments about return-values of functions.
Sorry I am so nitpicky... Besides this: Excellent.

  pixtur



ganesh:

20 months ago

Ok, I'll be more careful next time.

Add Comment