Fixed Conversations: recipient_count correct?

[HWS]

Please run the following query in your XenForo 1.1.3 database:

SELECT
xf_conversation_master.conversation_id,
xf_conversation_master.recipient_count,
count(xf_conversation_recipient.user_id) as 'howmany'
FROM
xf_conversation_master, xf_conversation_recipient
WHERE
xf_conversation_recipient.conversation_id = xf_conversation_master.conversation_id
GROUP BY xf_conversation_recipient.conversation_id

In our database the field "xf_conversation_master.recipient_count" shows incorrect data in many conversations. We found up to "6" recipient_count in conversations with just 2 people. Very often there is "3" when conversations only have 2 participients.

Can you confirm that this is a XenForo bug (race condition)?
If yes, how can we solve it?

Or is it just a weird coincidence with our database setup?

 

[Syndol]

Unless I am mistaken since it's almost 3 AM for me:
Whenever a reply is inserted into a conversation that includes a user that has "left" the conversation, that user state is then reverted to "active" so that they may view the new response.
The problem is that in order to switch back the user state, the insertConversationRecipient() function is called in function addConversationReplyToRecipients()
insertConversationRecipient() is designed to update a conversation user if one already exists which is great, however, it also increases the xf_conversation_master recipient count by one which is where the problem lies.

In Model/Conversation.php, in function insertConversationRecipient() (line 782)
Replace:

$db->query('
UPDATE xf_conversation_master SET
recipient_count = recipient_count + 1
WHERE conversation_id = ?
', $conversation['conversation_id']);

With (edited to show HWS's solution in order to prevent confusion):

To fix that bug, Syndols solution is almost correct.

The correct replacement code in Model_Conversation::insertConversationRecipient is:

if (!$existingRecipient || ($existingRecipient['recipient_state'] != 'deleted'))
{
$db->query('
UPDATE xf_conversation_master SET
recipient_count = recipient_count + 1
WHERE conversation_id = ?
', $conversation['conversation_id']);
}

 

[HWS]

Syndol, you're a hero.
This is a clear logical explanation.

Please XenForo, update this bug in your next version. Thx.

 

[HWS]

To fix that bug, Syndols solution is almost correct.

The correct replacement code in Model_Conversation::insertConversationRecipient is:

if (!$existingRecipient || ($existingRecipient['recipient_state'] != 'deleted'))
{
$db->query('
UPDATE xf_conversation_master SET
recipient_count = recipient_count + 1
WHERE conversation_id = ?
', $conversation['conversation_id']);
}

For the perfect solution we have to wait for the suggestion of XenForo developers.

 

[Hardcore]

Thanks guys. This bug has been bugging me.

 

[HWS]

Thanks guys. This bug has been bugging me.

Yes. It made some trouble. Thank to Syndol there is a temporary fix. However no official word by Xenforo until now.

 

[Jake Bunce]

Confirmed.

It could also be a situation where they forgot to decrement the count when a user leaves. They currently don't decrement. It depends on how they want to handle the maximum recipients. If a user leaves a conversation does that open up another slot for some one else?

But I think Syndol's solution is correct.

 

[HWS]

But I think Syndol's solution is correct.

No, with all due respect (and just to prevent problems if others use that replacement code), the correct solution is the solution we posted.

With Syndol's solution you'll get error messages because an important check isn't made.

We've implemented the solution at our large board since months without a problem now. So we can confirm it works as expected.

But thank you for recognizing this bug and answering and we all do hope that the official developers will release a perfect solution soon.

 

[Jake Bunce]

No, with all due respect, ours is the correct solution.

I was endorsing the idea of conditionally incrementing the counter. Your revised code does look correct.

 

[Syndol]

No, with all due respect (and just to prevent problems if others use that replacement code), the correct solution is the solution we posted.

I edited my post above to show HWS's solution in order to prevent confusion.

 

[luutruong]

wow..i modify as above but when i warn member...
i got

Undefined index: recipient_state
 
XenForo_Application::handlePhpError() in XenForo/Model/Conversation.php at line 782
XenForo_Model_Conversation->insertConversationRecipient() in XenForo/DataWriter/ConversationMaster.php at line 328
XenForo_DataWriter_ConversationMaster->_postSave() in XenForo/DataWriter.php at line 1385
XenForo_DataWriter->save() in XenForo/ControllerPublic/Member.php at line 736
XenForo_ControllerPublic_Member->actionWarn() in XenForo/FrontController.php at line 310
XenForo_FrontController->dispatch() in XenForo/FrontController.php at line 132
XenForo_FrontController->run() in /home/***/public_html/index.php at line 13

 

[HWS]

wow..i modify as above but when i warn member...
i got

Undefined index: recipient_state
 
XenForo_Application::handlePhpError() in XenForo/Model/Conversation.php at line 782
XenForo_Model_Conversation->insertConversationRecipient() in XenForo/DataWriter/ConversationMaster.php at line 328
XenForo_DataWriter_ConversationMaster->_postSave() in XenForo/DataWriter.php at line 1385

This happens if you use Syndols modification.
Use the one I posted and this error will go away.

 

[luutruong]

This happens if you use Syndols modification.
Use the one I posted and this error will go away.

oki..i tried

 

[Mike]

Fixed. Good catch.