Skip to:
Content

BuddyPress.org

Opened 4 years ago

Closed 4 years ago

#6535 closed enhancement (fixed)

Entering an invalid username when composing a message shows generic error

Reported by: r-a-y Owned by: r-a-y
Milestone: 2.4 Priority: normal
Severity: normal Version: 1.0
Component: Messages Keywords: has-patch commit
Cc:

Description

When an invalid username is entered when composing a message, a generic error message is shown:

Message was not sent. Please try again.

We should output a better error message for users.

I've attached a patch that changes messages_new_message() to add an $error_type parameter. When $error_type is 'wp_error', this uses the WP_Error API to return error messages.

This will allow us to grab error messages without having to parse out all invalid recipients again in bp_messages_action_create_message().

Attachments (2)

6535.01.patch (3.8 KB) - added by r-a-y 4 years ago.
6535.unittest.patch (987 bytes) - added by r-a-y 4 years ago.

Download all attachments as: .zip

Change History (9)

@r-a-y
4 years ago

#1 @DJPaul
4 years ago

  • Keywords needs-testing added

@r-a-y
4 years ago

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


4 years ago

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


4 years ago

#4 @boonebgorges
4 years ago

I think returning WP_Error messages for this kind of thing is generally a good thing. It stinks that we have to introduce this lame $error_type parameter :(

As for the question of whether BP_Messages_Message::send() should return WP_Error messages too: If we were building these systems from scratch, I'd probably lean yes. But given that all of our database-access methods tend to return false on failure, it'd be a large undertaking to fix them. Better, I think, would be to move business logic out of low-level functions like send() and into the controllers like messages_new_message(). For example, is there a reason why an if ( empty( $this->recipients ) ) clause belongs in the database access method? At a database level, there's nothing wrong with a message without recipients; it's only further up in the application that it's a problem. And when something goes wrong in one of these methods that is legitimately database-related - like a failed $wpdb->query() - $wpdb doesn't give us enough information to build an error object anyway.

#5 @r-a-y
4 years ago

Thanks for the feedback, boonebgorges.

I think returning WP_Error messages for this kind of thing is generally a good thing. It stinks that we have to introduce this lame $error_type parameter :(

Yeah, it does. I was thinking that this technique is something we could adopt in some of our older functions that need this type of error message feedback.

So okay to commit?


Good points regarding the following:

For example, is there a reason why an if ( empty( $this->recipients ) ) clause belongs in the database access method?

.

And when something goes wrong in one of these methods that is legitimately database-related - like a failed $wpdb->query() - $wpdb doesn't give us enough information to build an error object anyway.

#6 @boonebgorges
4 years ago

  • Keywords commit added; needs-testing removed

So okay to commit?

Yeah, let's go for it, and introduce the same $error_type pattern when absolutely necessary :)

#7 @r-a-y
4 years ago

  • Owner set to r-a-y
  • Resolution set to fixed
  • Status changed from new to closed

In 10286:

Messages: Return context-appropriate error messages for messages_new_message().

This commit introduces the error_type parameter in the
messages_new_message() function, which allows for us to return
descriptive error messages to the user when 'error_type' is set to
'wp_error'.

This most notably addresses an issue when entering an invalid username
when composing a new private message.

Commit also includes a unit test.

Fixes #6535.

Note: See TracTickets for help on using tickets.