BP_Notifications_Notification::get - error for meta_query

[shawfactor]

Team I have identified an error with BP_Notifications_Notification::get

When you do a meta query using the above static method the notifciation id is no longer avilable in the result as it won't cast as an integer

my code:

$meta_query = array(

array(

'key' => 'foo_key',
'compare' => 'NOT EXISTS',

)
);

$args = array(

'user_id' => $user_ids_array,
'meta_query' => $meta_query,
'per_page' => '1',
'order_by' => 'date_notified',
);

$notifications = BP_Notifications_Notification::get($args);

ithe issue saeems to be that it won't cats as an integer on line

$results[$key]->id = (int) $results[$key]->id;

[shawfactor]

actually the problem is caused by the join which creates two id fields in the result. The fix is quite easy.

Simply change the original select statement to

SELECT.

$select_sql = "SELECT n.*";

So that only the $bp->notifications->table_name fields are selected

[shawfactor]

actually fixing this also requires patching the method get_where_sql in the same class to ensure that the id field is never ambiguous, when you do a join with the meta table.

[espellcaste]

Bumping this to the next release.

[espellcaste]

I confirmed this issue, and I'm working on a possible solution.

When doing a meta_query, with the NOT EXISTS param, one can get the id columns from the bp_notifications and bp_notifications_meta tables. See the example below.

This query:
{{{php
BP_Notifications_Notification::get(

[

'user_id' => 1,
'meta_query' => [

[

'key' => 'foo',
'compare' => 'NOT EXISTS'

]

],

]

);
}}}
Will create this mysql query:
{{{mysql
SELECT

*

FROM

wp_bp_notifications n
LEFT JOIN wp_bp_notifications_meta ON (n.id = wp_bp_notifications_meta.notification_id

AND wp_bp_notifications_meta.meta_key = 'foo')

WHERE

user_id IN(5)
AND component_name IN('activity', 'blogs', 'friends', 'groups', 'members', 'messages', 'xprofile')
AND is_new = 1
AND(wp_bp_notifications_meta.notification_id IS NULL)

ORDER BY

date_notified

}}}

The result ends up with two id columns. Mysql is returning the one with NULL values.

This pull request adds a fix for that, plus unit tests.

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/8556

[espellcaste]

actually fixing this also requires patching the method get_where_sql in the same class to ensure that the id field is never ambiguous when you do a join with the meta table.

@shawfactor I found that not to be the case. After adding n.*, we don't get duplicate column ids anymore, so I haven't seen anything to patch related to this feedback.

changes about src/bp-activity/classes/class-bp-activity-activity.php + the corresponding Unit Test are not concerning the ticket and consist of a better way to write the same code (returning early if a condition is not matched instead of only doing something if the condition matches). So I think these changes should be removed from this PR.

1 - I'll keep the unit test since IT IS related to the task at hand (making sure the same issue is not present in the Activity component, hence the unit test). The other change was so minor that I don't see why we need to remove it out. But I'm happy to remove it if you feel strongly. I'm just not that _orthodox_ when it comes to small changes here and there ¯\_(ツ)_/¯

2 - @imath 🤔 I think this needs a bit more discussion due to its very narrow use case. Currently:

• The default request does not return data from the bp_notifications_meta table;
• No data is being used inside BuddyPress from the bp_notifications_meta table when using that request;
• The data is only available when using the meta_query param using the EXISTS value.
• When using the NOT EXISTS value, the data is null: there is no id (meta id), notification_id, meta_key, meta_value;

So it seems your suggestion is to:

• refactor the ::get method to return this data only when using the meta_query param using the EXISTS value.

Confirm?!

By the way, I'm happy to commit number 1 in a separate SVN commit since that's your main contention.

I confirm 🙂.

[shawfactor]

Does the proposed commit fix the underlying issue?

Ie being able to use not exists on a meta key when querying the notifications? This is vital as most other WordPress query apis support this and it is very useful if you want to send notifications to external apis

[espellcaste]

@shawfactor Yes! :)

[espellcaste]

In 13396:

Activity: adding test to confirm BP_Activity_Activity::get returns the proper data when using the meta_query argument.

See #8556

[espellcaste]

In 13397:

Notifications: returns the proper items when using the argument.

returns the notifications, together with the notification data, when the query is performed using the argument with the .

Closes ​https://github.com/buddypress/buddypress/pull/49
Fixes #8556