Buddypress 2.5.0 Private Messaging Bug: Can't add multiple users to message on compose screen

[markob17]

After clicking on a member's Private Message button to compose a new message to them, if you try to add another member to that same message it doesn't work anymore. It now just returns some code in the box instead off adding the additional user to the list. The weird code that shows instead of adding the member to the list is:

<span id="link-johnny" href="#"></span><img src="https://www.mywebsite.com/wp-content/uploads/avatars/3/21e7f535faf796dd8f4ad9d97ec8b0a6-bpthumb.jpg" style="width: 15px"> &nbsp; John Smith (johnny)

Before upgrading to Buddypress 2.5.0 this wasn't a problem, I could add other member to any email even those where I initiated via the private message button on a members profile.

For testing I:

Disabled all plugins
Changed to Default theme

To replicate this issue do the following:

visit a member's profile and click on the private message button
on the compose screen try and add another member to the email

[DJPaul]

I assume this worked in BuddyPress 2.4.

[sbrajesh]

Can confirm the issue. Yes, It works in 2.4
This line is returning empty object jQuery(settings.inputClass,tmp).closest('li') causing the error in jquery.autocompletefb.js

[imath]

could it be relative to #6939 ?

[r-a-y]

Looks like this was caused in r10447 due to changes in #6820.

[imath]

How come it's working for me ?

Is is specific to a theme or a template override ?

[r-a-y]

Attached patch does a few things:

• Passes JSHint. If we do not care about JSHint, ignore most of the changes.
• The most important change is to check to see if the current URL is using the ?r parameter. If it exists, we use the old selector, which should fix this issue. If it doesn't, we use the closest() selector so #6820 stays fixed.
• The URL querystring check could be improved. I just did a quick search for a decent JS URL querystring function ;)

[imath]

@r-a-y Thanks a lot for the JSHint cleaning :)

I think what's a bit too bad in playing in the area you chose is that the extra check will happen each time a new user will be added - it's not a big deal, though :) But we probably only need to do this check when document is ready

I suggest 2 other possibilities, both are making sure the bp_message_get_recipient_tabs() is building its html the same way the autocomplete does so that the layout is consistent. And differs as explained below:

• 6935.2.patch is using the messages_autocomplete_init_jsblock() function to run only once (when document is ready) a specific check and eventually reorganize the trouble with members/single/messages/compose.php.
• 6935.3.patch is from my point of view the correct fix if we had a dev-cycle above us as it edits the members/single/messages/compose.php the way it should be.

[r-a-y]

@imath - I like your patches way more than my hacky attempt! :)

I like 2.patch more because it doesn't touch the template part and fixes issues for those that have overriden compose.php in their themes. But, either patch works for me!

[imath]

Thanks for your feedback @r-a-y i agree we should probably use Javascript to preserve backcompat with themes overriding the compose template for 2.5.1.

I think we should improve it durint 2.6.

I've found another bug: a user could be added more than once. Then i remembered i was playing in this part with BuddyDrive to add multiple friends on load.

So .4.patch makes sure if there are multiple users added everything is fine too and prevents having the same user 2 times.

[DJPaul]

Since we changed the core JS in 2.5, I think we might as well just use 6935.3.patch and update the template correctly.

6935.2.patch *would* fix the problem for *everyone* today, but if 6935.3.patch was in 2.6, we'd break some sites (those that have overridden the compose.php template) when we remove this temporary JS fix. I don't want to fix this "forever" with the extra JS, either.

Pinging @boonebgorges for his thoughts as he worked on #6820

[DJPaul]

@imath 6935.4.patch changes a JS library that doesn't belong to us. I'd rather we go back and finish properly deprecating this buggy autocomplete script (like I half-did before v2.5).

[imath]

@DJPaul i understand, but it appears r10447 actually improved this library :) That's why we have the issue as @r-a-y noticed in #comment:5

So we could propably simply revert r10447 but if @boonebgorges improved it, there surely was a good reason. So that's why i've suggested patches to keep it.

So if we keep r10447, i really think we should do the 6935.4.patch, else reverting the commit should fix the issue.

[imath]

After some more thoughts - i think i totally agree with @DJPaul, we should simply revert. I'm going to test this to confirm it's fixing the issue.

FYI I've been working on a new UI for the messages screens, and i found it very handy to use the scripts we introduced for the mentions (atwho.js). The only thing to change is explaining the user to add an '@' before the username. We could probably do this early for 2.6 and finally deprecate this UI that is not great at all :)

[imath]

I confirm reverting is fixing the issue.

See 6935.revert-10447.patch

[boonebgorges]

The problem in #6820 was simply that <li><li></li></li> is not valid or semantically correct. As far as I know, there were no functional issues with the old way of doing things.

Given the acrobatics required to "fix" [10447], my vote is to simply revert it for 2.5.1. Let's just replace the whole UI for 2.6. See #4580. (I'd suggest we do *not* require the use of @ in this context, but that's a discussion for #4850.)

[hnla]

It's rock hard place. our templates wrong the script is wrong, to simply fix is hard without possibly upsetting established layouts.

<li><li></li></li> is wrong period (although I didn't actually spot that happening) we do start of with an empty 'li' element as the script builds that as these sorts of scripts like to.

To retain the 'fix' we could remove the templates hardcoded li elements:

<ul class="first acfb-holder">
 <?php bp_message_get_recipient_tabs(); ?>
 <input type="text" name="send-to-input" class="send-to-input" id="send-to-input" />
</ul>

In the script we change the .closest() to:

var x = jQuery(settings.inputClass,tmp).siblings('li').after(v);

This works however then throws up a similar error in having an unsupported element as child of a 'ul' in the input - wrapping input in li causes the original issue that the script gets confused over the hardcoded li elements, without minutely picking over the script not sure why this happens.

Think we have to go with the revert and 2.6 re-factor, update the template and re-write the script, and say sorry if it upsets any layouts.

[imath]

In 10633:

Messages: Make sure it is possible to add new users to a private conversation after the profile Private Message button has been clicked.

For the 2.5 branch we are reverting r10447, we will improve the autocomplete UI during next release.

Props r-a-y, imath for the acrobatics.
Props DJPaul, boonebgorges for their wisdom

See #6935 (branch 2.5)

[imath]

In 10634:

Messages: Make sure it is possible to add new users to a private conversation after the profile Private Message button has been clicked.

For the 2.5 branch we are reverting r10447, we will improve the autocomplete UI during next release.

Props r-a-y, imath for the acrobatics.
Props DJPaul, boonebgorges for their wisdom

Fixes #6935 (trunk)