Skip to:
Content

BuddyPress.org

Opened 6 months ago

Closed 6 months ago

Last modified 5 months ago

#8676 closed defect (bug) (fixed)

Database Errors for multiple notifications

Reported by: mike80222's profile mike80222 Owned by: imath's profile imath
Milestone: 10.3.0 Priority: normal
Severity: normal Version: 10.0.0
Component: Toolbar & Notifications Keywords: has-patch
Cc:

Description

Wordpress 5.9.2
BuddyPress 10.2.0 Legacy or Nouveau
Either 2022 or 2020 Themes.

(1) Send two private messages to someone.

(2) That person clicks on one of the notifications, and the error happens
Sometimes it happens if the recipient just refreshes their profile (and hence the notifications)

Note: There need to be multiple private message notifications. If there’s just one notification the error doesn’t appear to happen.

More info in: https://buddypress.org/support/topic/database-errors/#post-323944

This is the way the error looks with Nouveau:

[31-Mar-2022 17:41:07 UTC] WordPress database error You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near ') AND user_id = 2 AND component_name = 'messages' AND component_action = 'new...' at line 1 for query UPDATE wp_bp_notifications SET is_new = 0 WHERE item_id IN () AND user_id = 2 AND component_name = 'messages' AND component_action = 'new_message' made by do_action('wp_ajax_messages_get_thread_messages'), WP_Hook->do_action, WP_Hook->apply_filters, bp_nouveau_ajax_get_thread_messages, bp_thread_the_message, BP_Messages_Thread_Template->the_message, do_action('thread_loop_start'), WP_Hook->do_action, WP_Hook->apply_filters, bp_messages_screen_conversation_mark_notifications, bp_notifications_mark_notifications_by_item_ids, BP_Notifications_Notification::update_id_list

Change History (15)

This ticket was mentioned in PR #14 on buddypress/buddypress by adiloztaser.


6 months ago

  • Keywords has-patch added

#2 @oztaser
6 months ago

  • Component changed from (not sure) to Toolbar & Notifications
  • Milestone Awaiting Review deleted
  • Version set to 10.0.0

Hi there,

Welcome to BuddyPress Trac! Thanks for the report.

I think the reason of this error is fetching new_message notifications from other threads instead of displayed one.

Here is my debug data to see how we find empty $message_ids array:

<?php
$unread_message_ids_from_notifications = array( 4, 5, 6 );
$message_ids_from_thread                           = array( 1, 2, 3 );

$message_ids = array_intersect( $unread_message_ids_from_notifications, $message_ids_from_thread );
print_r( $message_ids );

#3 @imath
6 months ago

  • Keywords needs-unit-tests added
  • Milestone set to 10.3.0

hi @mike80222, thanks a lot for your report 😍.
Hi @oztaser thanks a lot for your patch 💪.

I'm going to look at it asap, in the meantime could you include one or more unit tests to show a failure without the patch which is resolved once the patch is applied 🙏?

#4 @imath
6 months ago

I wasn't able to replicate. When I send more than one Private message to a user, there's only one screen notification displayed into the WP Admin Bar, see below:

https://cldup.com/py3bQh-CfA.png

If I click on it, everything is fine for me.

#5 @mike80222
6 months ago

@imath I was talking about in the user's profile, not in the admin bar. Sorry I wasn't clear about that.

#6 @oztaser
6 months ago

Hi @imath,

To see this PHP warning, send a some user private messages from two different user (that means two thread) and open the single view page one of the threads. https://buddypress.test/members/test2/messages/view/5/

As I said my previous comment we are fetching other thread message ids in thread_loop_start action hook to mark message notifications as read but these message ids are not belongs to current thread so we're using empty array.

I don't know how to write unit test for this situation I think we need to write some test for BP_Notifications_Notification::update_id_list function.

#7 @mike80222
6 months ago

@imath, @oztaser,

I believe you should also be able to cause the database error by sending two messages from a single user, with the two steps I originally mentioned -- but bearing in mind that in (2) I meant that the recipient clicked on the notification in their *profile* not in the admin bar at the top.

#8 @imath
6 months ago

Hi thanks a lot to you both for the complementary explanation. I’m looking at it asap 👍

#9 @imath
6 months ago

I've been able to reproduce. I think this case is happening because at the time we're trying to get all notifications, the one corresponding to the displayed thread has already been marked as read. So we get the one that is left out of the 2 notifications and this one doesn't belong to the thread.

@oztaser I agree your PR is fixing the issue and we could write a unit test out of what I just described above.

But I believe we need to understand why "the one corresponding to the displayed thread has already been marked as read" before the 'thread_loop_start' is fired. Because this means we could avoid 2 superfluous queries.

#10 @imath
6 months ago

Bingo > https://github.com/buddypress/buddypress/blob/master/src/bp-messages/actions/view.php#L51|L58

So I believe a better fix is to return early from bp_messages_screen_conversation_mark_notifications() if we're viewing a single message. Maybe by using a more precise condition at line 228, something like:

if ( ! bp_is_my_profile() || ( bp_is_current_action( 'view' ) && bp_action_variable( 0 ) ) ) {
	return;
}

@oztaser could you have a look at it and if you have the same diagnostic I have (then we don't need unit test), can you update the PR accordingly ?

Last edited 6 months ago by imath (previous) (diff)

#11 @oztaser
6 months ago

  • Keywords needs-unit-tests removed

Hi @imath,

Thanks for review and suggestion. I've updated the PR. This new approach make more sense to me as well.

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


6 months ago

#13 @imath
6 months ago

In 13269:

Avoid 2 superfluous private message notification queries

When viewing the single view of the thread, the potential notifications about it are marked as read into the messages_action_conversation() function. We don't need to do it again when looping into the thread messages.

This was revealed by a database error which was appearing when a user who had received more than 1 message from two different threads was directly viewing the single view of one of the threads.

Props mike80222, oztaser
Closes https://github.com/buddypress/buddypress/pull/14
See #8676 (trunk)

#14 @imath
6 months ago

  • Owner set to imath
  • Resolution set to fixed
  • Status changed from new to closed

In 13270:

Avoid 2 superfluous private message notification queries

When viewing the single view of the thread, the potential notifications about it are marked as read into the messages_action_conversation() function. We don't need to do it again when looping into the thread messages.

This was revealed by a database error which was appearing when a user who had received more than 1 message from two different threads was directly viewing the single view of one of the threads.

Props mike80222, oztaser

Fixes #8676 (10.0 branch)

This ticket was mentioned in Slack in #buddypress by imath. View the logs.


5 months ago

Note: See TracTickets for help on using tickets.