Skip to:
Content

BuddyPress.org

Opened 10 years ago

Closed 7 years ago

Last modified 7 years ago

#5911 closed enhancement (wontfix)

Improve user feedback in Group's invite screen.

Reported by: imath's profile imath Owned by: dcavins's profile dcavins
Milestone: Priority: normal
Severity: normal Version: 2.0
Component: Templates Keywords: needs-patch
Cc: dcavins

Description

Since 2.0, when "BP Theme Compat is on" selecting a friend from the checkbox list of group's invite screen is refreshing the page. It's fine, but there are two cases that are introducing potential comprehension difficulties for users.

If the friend i want to invite has been invited by another one, the page refreshes and nothing happens, the user can have the impression the checkbox was emptied for a reason the software is not informing of.
Same when, the friend has requested to join the group.

I think in BP Default, it's displaying a message in the 2nd case informing that inviting the user will make his request approved. For the first case, the user is added to the invite list.

So in a way, i think, in terms of user feedback BP Default has more functionalities than "BP Theme Compat". So i suggest the following patch to inform the user :

Attachments (4)

5911.patch (5.7 KB) - added by imath 10 years ago.
5911.02.patch (17.1 KB) - added by imath 10 years ago.
5911.03.patch (18.4 KB) - added by dcavins 10 years ago.
5911.04.patch (17.9 KB) - added by imath 10 years ago.

Download all attachments as: .zip

Change History (24)

@imath
10 years ago

#1 @boonebgorges
10 years ago

Thanks, imath, and good find. Why did you decide to fix it with these error messages, rather than by replicating the behavior of bp-default (inviting a user who has requested membership will approve his request; inviting a user who has already been invited will simply go through)? It seems to me that the behavior of bp-default is smoother here - if we can intelligently decide what the user intends, instead of throwing an error, it seems better to do so.

#2 @imath
10 years ago

Well, as the complete template part is reloading each time you click on a checkbox, it's the simplest way i've found ;)

Trying replicating BP Default behavior gave me this :
For example, let's say i have 2 friends A & B. A has already required a membership. I click on the checkbox, as the #friend-list selector doesn't exist you need to create it (which is not a problem) so the li element appear in the main-column with the message 'Sending an invitation will automatically add the member to the group'. If you click right away on the send invites button: it's ok, but if you click on checkbox B, then the template reloads and the li element for A is not there anymore. I guess the same kind of behavior would happen with the case of a friend already invited by another one as in BP Default it simply append the li element with no extra message.

I think we could store in a js var the 'response', then create an event to trigger from bp_filter_request() if it's an invite request and we could eventually catch this event and prepend the js var into the friends list.
But in the case of a friend already invited by another user we would need to leave uncheck the corresponding checkbox. Because if we want to be consistent and have it checked, then if the user changes his mind and click again on this checkbox, then the invite is deleted which in this case is not good because it means a user deleted the invite of another user.

I'm not sure to be clear:
A invited B
C invite B > the li element could be appended
but if the checkbox is checked and C uncheck it, then B is no more invited.
So A is sad :(

( maybe we could check for inviter_id before deleting an invite btw )

So, i found it weird to have a li element on the screen and the checkbox not checked. On the other hand i have no problem with the unchecked checkbox if a message is informing me why ;)

Last edited 10 years ago by imath (previous) (diff)

#3 @boonebgorges
10 years ago

If you click right away on the send invites button: it's ok, but if you click on checkbox B, then the template reloads and the li element for A is not there anymore.

I think we could store in a js var the 'response', then create an event to trigger from bp_filter_request() if it's an invite request and we could eventually catch this event and prepend the js var into the friends list.

Since we're currently rendering the entire template part on the server and sending the HTML as an AJAX response, it would be nice to fix the problem there, and not in JS.

I'd need to look more deeply into the template-building logic, but I think it should be possible to check the following when generating the markup for each user on each refresh:

  • Have we recorded an unsent, unconfirmed invitation from the logged-in user? If so, make sure there's an <li> for the user
  • Are there any other invites/requests in the system for this user that predate our unsent invite? If so, add a message to the template: "Sending an invitation to this user will automatically..."
  • Have we just unchecked a box? If so, be sure only to delete the invitation in the system that has the logged-in user as the inviter, *not* all invitations. (And if we have logic that prevents ore than one invitation from being sent to a single user for a given group, we should remove it - there's no reason I can think of why it shouldn't be possible for someone to receive more than one invite to a group from different people.)

I think it will probably take some juggling of our core template functions (like bp_group_has_invites()) to get this to work right, but I think that's OK. I don't see any insurmountable difficulties. What do you think?

#4 @imath
10 years ago

Sorry Boone, i'm hesitating, because i'm not sure i understand well :)

Have we recorded an unsent, unconfirmed invitation from the logged-in user? If so, make sure there's an <li> for the user

1/ That's what's built-in so far, i think. Am i correct ?

Are there any other invites/requests in the system for this user that predate our unsent invite? If so, add a message to the template: "Sending an invitation to this user will automatically..."

2/ The user you are talking about is the one the logged in user is trying to invite, right ?

Have we just unchecked a box? If so, be sure only to delete the invitation in the system that has the logged-in user as the inviter, *not* all invitations. (And if we have logic that prevents ore than one invitation from being sent to a single user for a given group, we should remove it - there's no reason I can think of why it shouldn't be possible for someone to receive more than one invite to a group from different people.)

3/ *not* all invitations I'm not sure to get it. I'm understanding that in wp_bp_groups_members there could be more than one row for a group id / user_id, one row for each inviter_id.

I think point 2/ would need an information about the checkboxes clicked to be sure to activate the right ones for the current group > meaning JS, i guess we could deal with this by storing data ( eg: group_id => array(list of ids clicked)) in bp-invite-extras cookie for instance and eventually use the ajax querystring filter to transport this data.

But point 3/ is making me think that you suggest to use 1 specific row for each inviter, which would resolve the problem of the checked checkbox and <li> elements displayed, i've just tested by commenting the groups_check_user_has_invite() in the function groups_invite_user(). In this case, all inviters have the checkbox and li element in the group's invite screen and the invited user only has 1 notification. Before the user accepts it the wp_bp_groups_members table looks like this :

id group_id user_id inviter_id
51 10 2 1
53 10 2 3

After he has accepted the invite, it looks like this

id group_id user_id inviter_id ... is_confirmed is_banned invite_sent
51 10 2 0 ... 1 0 1
53 10 2 3 ... 0 0 0
54 10 2 3 ... 0 0 0
56 10 2 1 ... 0 0 0

The group members count is ok, but each inviter keeps the invited li element in the group's invite screen. So i guess we could delete where is_confirmed != 1 once the user accepted the invite to solve this

But then, we would need to revisit groups_uninvite_user() function as it's a simple alias to the BP_Groups_Member::delete() method. Apparently removing an invite is the same as removing a member from the group.

In my example 2 invites for the same user generates 3 extra rows, so i guess it can be exponential with the number of invites a user can receive :)

Suddenly i have a doubt! Maybe i completely misunderstood your comment. Is it the case ?

#5 follow-up: @boonebgorges
10 years ago

Suddenly i have a doubt! Maybe i completely misunderstood your comment. Is it the case ?

No, I think you understand. But what I'm suggesting is that we make the necessary changes to groups_uninvite_user() and other functions that are necessary to support the workflow that we're going for here. I understand and agree with you that some of what we want to do is currently not possible given the way we store and retrieve multiple invitations; my proposal is that we work on making the underlying system more flexible.

In other words: your description of the problems makes sense, but instead of ending by saying "I guess we can't do this", I want to suggest that we say "is it feasible to make the necessary changes in BP". Does that make sense? :)

(A side note: I don't think this feature is extremely important, so if it's going to require huge amounts of work, it might not be worth pursuing right now, and we could go with a more conservative solution like what initiall suggested in the short term. However, the UX around group invitations is IMO quite terrible, so anything we can do to smooth it out would be most welcome.)

#6 in reply to: ↑ 5 @imath
10 years ago

Replying to boonebgorges:

No, I think you understand. But what I'm suggesting is that we make the necessary changes to groups_uninvite_user() and other functions that are necessary to support the workflow that we're going for here.

Nice, and yes it makes sense :) i'll explore this way a bit deeper because with the quick look at the code i've made, the necessary changes might not be so huge. Thanks a lot for your feedbacks.

#7 @imath
10 years ago

  • Keywords dev-feedback added

It seems to work fine. See 5911.02.patch.

I had to move the groups_accept_membership_request() function out of groups_invite_user() and put it in the groups_send_invites() function, as on the group's invite screen after a user click on the checkbox of a user who previously requested, it's too early IMHO to convert the membership request in a membership. We need to wait the user to send invites to be sure.

@imath
10 years ago

@dcavins
10 years ago

#8 @dcavins
10 years ago

Hi imath-

This is a great improvement to the invites system. One of the biggest problems the invitation system has is that the process isn't very transparent. Users don't know why some invitations disappear (when an invite already exists for that user) and that it's a two-step process (create draft/send).

I've tested your patch on my test installation and I'm getting the results I expected:

  • Multiple invitations can be extended from a group to a user, differentiated by the inviter id.
  • "Pending request" message appears when necessary.
  • Group members table gets cleaned up when the user joins the group.

I recently added a similar status message telling the user whether the invitation has been sent yet or not. I've included a similar message in the attached patch.

Thanks for working on this; group invitations are a great place to make improvements.

#9 @imath
10 years ago

Hi dcavins,

Thanks a lot for testing and for your feedback :)

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


10 years ago

#11 follow-up: @DJPaul
10 years ago

I'm not sure how close this change is to being completed; the ticket hasn't been updated in about 3 months. Is this ready for final testing, or should it be something we aim to focus on during the next dev cycle?

Last edited 10 years ago by DJPaul (previous) (diff)

#12 in reply to: ↑ 11 @imath
10 years ago

Replying to DJPaul:

I'm not sure how close this change is to being completed; the ticket hasn't been updated in about 3 months. Is this ready for final testing, or should it be something we aim to focus on during the next dev cycle?

In 5911.03.patch, dcavins is adding a message whether the invitation has been sent yet or not. It's interesting but it's also introducing some new UI considerations from my point of view. The css classes dcavins is using does not have any css matching rules in core css file.

I think for 2.2, we should at least improved the invitation process the way boonebgorges suggested and include the feedbacks BP-Default was providing and that was lost since BP Theme Compat.

in 5911.04.patch, i'm refreshing the patch, correcting some mistakes in unit tests and also edit the send-invites templates to avoid a blank output in case the user has no friends.

@imath
10 years ago

#13 @imath
10 years ago

  • Keywords early added
  • Milestone changed from 2.2 to 2.3

I'm a bit sad this ticket didn't make it so far. As 2.2-beta1 is there and the improvements here are playing with database, i think it's safer to review this early in next dev-cycle.

dcavins, i'd be happy if we could work together on this one :)

Last edited 10 years ago by imath (previous) (diff)

#14 @dcavins
10 years ago

Ah, imath, I thought this ticket made it in as well. For 2.3 I'm planning to build an invitation API, and it will definitely allow multiple invitations to an single member as you've laid out here.

And I think we can do a number of things to make the invitation page more usable, the first of which is better progress/error/status messages.

#15 @imath
10 years ago

  • Keywords early removed
  • Milestone changed from 2.3 to Future Release

I guess i've (we've) missed this one, once again :( Too late for 2.3, sorry.

#16 @dcavins
9 years ago

  • Cc dcavins added
  • Owner set to dcavins
  • Status changed from new to assigned

#17 @DJPaul
8 years ago

  • Component changed from Appearance - Template Parts to Templates

#18 @DJPaul
8 years ago

  • Keywords needs-patch added; has-patch dev-feedback removed

#19 @DJPaul
7 years ago

  • Milestone Future Release deleted
  • Resolution set to wontfix
  • Status changed from assigned to closed

Closing most tickets related to BP-Default and BP-Legacy, since the upcoming BP-Nouveau template pack (planned for 3.0) will make these redundant.

#20 @DJPaul
7 years ago

Closing most tickets related to BP-Default and BP-Legacy, since the upcoming BP-Nouveau template pack (planned for 3.0) will make these redundant.

Note: See TracTickets for help on using tickets.