Defect #6541

Email notifications send to everybody

Added by Михаил Чулицкий about 7 years ago. Updated about 7 years ago.

Status:ClosedStart date:2010-09-30
Priority:HighDue date:
Assignee:Eric Davis% Done:

100%

Category:Email notifications
Target version:1.1.0
Resolution:Fixed Affected version:

Description

Email notifications send to everybody, regardless settings.


Related issues

Related to Redmine - Patch #6527: Recent migrations don't have the correct date Closed 2010-09-29
Related to Redmine - Defect #6549: Notification settings not migrated properly Closed 2010-09-30

Associated revisions

Revision 4247
Added by Eric Davis about 7 years ago

Change Project#notified_users to check for the 'all' notification option. #6541

The previous mail_notification? check would always pass since the
notifications where converted to strings and strings are always true.

Also changed Project#recipients to use #notified_users instead of duplicated
code.

Based on contribution by Felix Schäfer.

History

#1 Updated by Luc Heinrich about 7 years ago

Same here. I just updated to 1.0.2 and suddently email notifications are sent to ALL users :/

#2 Updated by Felix Schäfer about 7 years ago

Are you on 1.0.2 or latest trunk?

#3 Updated by Luc Heinrich about 7 years ago

Sorry, I wasn't very specific.

I first upgraded to latest trunk (currently r4229) and got this problem, so I downgraded to r4210 (which corresponds to 1.0.2) but still have the same problem: email notifications are sent to all users.

#4 Updated by Felix Schäfer about 7 years ago

  • Status changed from New to 7
  • Assignee set to Felix Schäfer

It seems the migration to the "new" messaging preferences resets the mail preferences to "any event in all my projects", or at least this is what I have observed so far. I'll have a look at the migration to see if there is a bug in there.

#5 Updated by Jean-Baptiste Barth about 7 years ago

I suspect migrations introduced in r4216 won't be easy to downgrade... Do users receive mail notifications they shouldn't be recipient of regarding projects privacy ? Or they just receive notification as if they chose it in "My account" page ?

#6 Updated by Felix Schäfer about 7 years ago

Jean-Baptiste Barth wrote:

I suspect migrations introduced in r4216 won't be easy to downgrade... Do users receive mail notifications they shouldn't be recipient of regarding projects privacy ? Or they just receive notification as if they chose it in "My account" page ?

The notifications options are set to "receive all mails for all events", and this is because the second migration of r4216 isn't run due to the wrong timestamp. JB, didn't you open a ticket for that? Could you make it a related to this one here?

Regarding that migration, there is another problem with it: it assumes true and false are stored as 1 and 0 in the DB, which isn't the case for sqlite, which stores t and f. Will submit a patch soon.

#7 Updated by Jean-Baptiste Barth about 7 years ago

Yes it's #6527. In fact they are run (at least on my laptop...) : with timestamped migrations, rails inserts a line for each migration in the internal table where it stores the migration (schema_something, I don't remember). Anyway, I'm not sure it will be easy to reverse after upgrading to a recent revision on trunk.

I let you investigate Felix (and thanks for the boolean thing, it makes me think of something on your doodle plugin I didn't report to you!)

#8 Updated by Felix Schäfer about 7 years ago

Could everyone facing this problem please post here what DB they are using, if 20100129193402 and 20100129193813 are in in the table schema_migrations, and if the values for users.mail_notifications are either 0 and 1, t and f or some meaningful_strings. Thanks.

#9 Updated by Felix Schäfer about 7 years ago

Jean-Baptiste Barth wrote:

Yes it's #6527. In fact they are run (at least on my laptop...) : with timestamped migrations, rails inserts a line for each migration in the internal table where it stores the migration (schema_something, I don't remember). Anyway, I'm not sure it will be easy to reverse after upgrading to a recent revision on trunk.

They are run here too, but the main problem is that the migration assumes the values from the boolean are 0 and 1, though at least on sqlite they are f and t.

#10 Updated by Luc Heinrich about 7 years ago

I have both 20100129193402 and 20100129193813 in the schema_migrations table.
As for the users.mail_notification, two users have all, one has none and all others have only_my_events.

#11 Updated by Felix Schäfer about 7 years ago

  • Assignee changed from Felix Schäfer to Eric Davis
  • Target version set to 1.0.3

Eric: seeing that you are toying around ;-) with the notification, I think you should review and commit this.

The fix is here: http://github.com/thegcat/redmine/commit/ea2642fe16dc717d33826555c4db525635a41cc5

It fixes two things: the migration to update the user.mail_notification column is fixed for sqlite, and Project s won't return all members but only those wishing to receive notifications for it.

Luc: you can re-upgrade and apply just this change http://github.com/thegcat/redmine/commit/ea2642fe16dc717d33826555c4db525635a41cc5#L0L390 , your DB is already up-to-date with the correct values, so that should correct the problem for you.

#12 Updated by Felix Schäfer about 7 years ago

  • Target version deleted (1.0.3)

Oh, forget the target version, I wasn't paying much attention, but seeing that the notification changes will not get into 1.0.3, the fix doesn't need either :-)

#13 Updated by Luc Heinrich about 7 years ago

I confirm that Felix' github patch fixes the problem for me.

#14 Updated by Felix Schäfer about 7 years ago

Anyone who made the update might want to have a look at http://www.redmine.org/issues/6549#note-1.

#15 Updated by Felix Schäfer about 7 years ago

Felix Schäfer wrote:

Regarding that migration, there is another problem with it: it assumes true and false are stored as 1 and 0 in the DB, which isn't the case for sqlite, which stores t and f. Will submit a patch soon.

Regarding that, from a completely fresh (i.e. nothing else than a rake db:migrate) sqlite DB:

sqlite> SELECT mail_notification FROM users;
t

So I can't say for sure other other sqlites don't convert true in a boolean field to a 1 in a varchar(255) field, but that one doesn't :-)

#16 Updated by christoph scheyk about 7 years ago

we still got this problem in version 4231-devel after an update yesterday. we are using mysql.
what can I do to fix it?

#17 Updated by Felix Schäfer about 7 years ago

christoph scheyk wrote:

we still got this problem in version 4231-devel after an update yesterday. we are using mysql.
what can I do to fix it?

The fix isn't in trunk yet. What I told Luc a few comments up should correct the problem too.

#18 Updated by Roland Discein about 7 years ago

Thank you, Felix!

#19 Updated by Henrik Ammer about 7 years ago

Does this relate to that I also see both the english and the swedish values in the select on the accountpage where they choose their setting for this?

#20 Updated by Felix Schäfer about 7 years ago

Henrik Jönsson wrote:

Does this relate to that I also see both the english and the swedish values in the select on the accountpage where they choose their setting for this?

Probably not all strings are translated yet, which happens rather often on trunk.

#21 Updated by Eric Davis about 7 years ago

  • Status changed from 7 to Closed
  • Target version set to 1.1.0
  • % Done changed from 0 to 100
  • Resolution set to Fixed

Fixed in r4246 with a modified version of Felix's patch.

#22 Updated by Eric Davis about 7 years ago

  • Status changed from Closed to Reopened

Whoops, wrong issue. #6549 was fixed in r4246. Still looking into this one.

#23 Updated by Eric Davis about 7 years ago

  • Status changed from Reopened to Closed
  • Priority changed from Normal to High

Fixed in r4247 with a bunch of tests to make sure this doesn't regress. Thanks for reporting the bug and finding the cause everyone.

Also available in: Atom PDF