| Project: | Organic groups |
| Version: | 6.x-2.x-dev |
| Component: | og_notifications |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | closed (fixed) |
Issue Summary
If the post is targeted to more than one OG group only subscription to Group Content type pushes it into notifications_queue table.
I've created two OG and posted a node targeted to both groups. I've subscribed an authenticated user first by thread where it failed and then by group content type where notification got pushed successfully into queue.
When you hit Save button on node editing form you are eventually taken to notification_queue($event) function inside nottifications.module. Problem is caused by INSERT..SELECT statement build there (precisely at line 446 $sql array representing query is passed to db_query from which I've pulled below SQL):
INSERT INTO notifications_queue (uid, destination, sid, module, eid, send_interval, send_method, cron, created, conditions)
SELECT DISTINCT s.uid, s.destination, s.sid, s.module, 87, s.send_interval, s.send_method, s.cron, 1240223643, s.conditions
FROM
notifications s
INNER JOIN
notifications_fields f ON s.sid = f.sid
LEFT JOIN
og_ancestry oga ON oga.nid = 13
WHERE
(s.status = 1) AND
(s.event_type = 'node') AND
(s.send_interval >= 0) AND
(s.uid <> 1) AND
((f.field = 'group' AND f.intval = oga.group_nid) OR
(f.field = 'nid' AND f.intval = 13) OR
(f.field = 'type' AND f.value = 'ogpost') OR
(f.field = 'author' AND f.intval = 1))
GROUP BY
s.uid, s.sid, s.module, s.send_interval, s.send_method, s.cron, s.conditions
HAVING
s.conditions = count(f.sid)Note:
Number 13 in above query refers to the node I've tested it on.
Basically SELECT statement is using condition in HAVING clause which produce empty resultset.
Conditions field for subscriptions by thread/author/tag is equal 1 in notifications table.
Now SELECT is doing left join on og_ancestry table (which keeps group ids related to a node). This results in having number of entries equal to number of groups the post was targeted to. Obvious...and count() function then properly return this number.
SELECT is using DISTINCT but the way the MySQL works leaves count() unaffected by this modifier.
Note:
You can use DISTINCT inside count() to count only distinct record if that was the goal here.
I've been trying to wrap my head around all this but my knowledge of Notificaitons module's internal logic is not enough to figure all the implications of conditions field used here.
My point is that the way it is used right now seems to be faulty. Conditions field inside notifications table seems to be always 1 (except if subscribed via Group Content type - 2) while count() inside HAVING will return number of groups.
Btw, can anyone shed some more light what the conditions field is used for?
I've tested this on clean install:
- Drupal 6.10
- Messaging 6.x-2.0-beta2
- Notifications 6.x-2.0-beta2
- Organic groups 6.x-2.0-rc1
- Token 6.x-1.11
- Views 6.x-2.5
- Vista and Linux
- PHP 5.2.9
- MySQL 5.1.11 and 5.0.58
Comments
#1
The conditions field is the number of conditions that subscription must match (= number of rows in notifications_fields). I've added descriptions to the schema btw.
Anyway you may be right about something being wrong with the query...
Could you post the result of the select statement? (without the INSERT part)
#2
If i run above SELECT against my database (just one post targeted in two groups and one user subscribed to this post) I got empty resultset.
If I run it having post targeted to one group i got following result (I've included count() result as a last column)
uid, destination, sid, module, 87, send_interval, send_method, cron, 1240223643, conditions, count(f.sid)
4, '', 21, 'notifications', 87, 0, 'phpmailer', 1, 1240223643, 1, 1
If I run it having post targeted to TWO groups I got empty resultset!
On the other hand commenting HAVING clause produced:
uid, destination, sid, module, 87, send_interval, send_method, cron, 1240223643, conditions, count(f.sid)
4, '', 21, 'notifications', 87, 0, 'phpmailer', 1, 1240223643, 1, 2
Running above (with HAVING still commented) for post targeted to 5 groups produced:
uid, destination, sid, module, 87, send_interval, send_method, cron, 1240223643, conditions, count(f.sid)
4, '', 21, 'notifications', 87, 0, 'phpmailer', 1, 1240223643, 1, 5
So If I run a query (no HAVING and GROUP BY) with fields from og_ancestry table to see what's really going on there:
SELECT DISTINCT oga.nid, oga.group_nid, s.uid, s.destination, s.sid, s.module, 87, s.send_interval, s.send_method, s.cron, 1240223643, s.conditions
FROM
notifications s
INNER JOIN
notifications_fields f ON s.sid = f.sid
LEFT JOIN
og_ancestry oga ON oga.nid = 13
WHERE
(s.status = 1) AND
(s.event_type = 'node') AND
(s.send_interval >= 0) AND
(s.uid <> 1) AND
((f.field = 'group' AND f.intval = oga.group_nid) OR
(f.field = 'nid' AND f.intval = 13) OR
(f.field = 'type' AND f.value = 'ogpost') OR
(f.field = 'author' AND f.intval = 1))
I got as suspected 5 rows each for different group (obviously grouping them (and using distinct) will compress them to only one row but count() will still pick 5)
uid, destination, sid, module, 87, send_interval, send_method, cron, 1240223643, conditions
13, 1, 4, '', 21, 'notifications', 87, 0, 'phpmailer', 1, 1240223643, 1
13, 6, 4, '', 21, 'notifications', 87, 0, 'phpmailer', 1, 1240223643, 1
13, 10, 4, '', 21, 'notifications', 87, 0, 'phpmailer', 1, 1240223643, 1
13, 14, 4, '', 21, 'notifications', 87, 0, 'phpmailer', 1, 1240223643, 1
13, 15, 4, '', 21, 'notifications', 87, 0, 'phpmailer', 1, 1240223643, 1
Jose if you are saying that conditions from notifications table must match number of rows in notifications_fields I don't see a way current JOIN won't let you to pull this number.
#3
Thank you, this is really useful.
I've been doing some research too and it seems the problem is that the join causes other condition fields (not group ones) to be repeated and counted multiple times, one for every group, caused by the join.
Curiously this issue causes the group subscriptions to work properly but all other ones to fail.
I think this patch should fix it and produce faster and cleaner queries. It produces query conditions like:
(f.field = 'group' AND f.intval IN (1,2)) // Being 1, 2 the group ids the node belongs to
#4
Jose I've applied your patch and I'v been just testing this for a while today...so far so good (against Notofications 6.x-2.0-beta3). The query looks a lot cleaner with only one JOIN. I think that you've nailed it.
#5
I think this makes the patch RTBC
#6
Committed. Cheers :)
#7
Automatically closed -- issue fixed for 2 weeks with no activity.