Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#6063 closed enhancement (fixed)

Support meta query in bp_has_message_threads()

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

Description

Now that we have a meta table for messages, it would be nice to get meta queries working with bp_has_message_threads().

Attached patch does this.

Some notes:

  • In order to support 'meta_query' as an extra parameter, I changed BP_Messages_Thread::get_current_threads_for_user() / BP_Message_Box_Template to use one argument instead of multiple arguments.
  • I've refactored BP_Messages_Message::get_current_threads_for_user() so the SQL statement isn't duplicated in two spots.
  • Adds a unit test. If you analyze the test, there's a hint at getting a "Starred" box working. Might be better if this were a taxonomy instead of using meta, but it's just a test :)

Attachments (8)

6063.01.patch (18.0 KB) - added by r-a-y 5 years ago.
6063-assoc-array-args.diff (5.2 KB) - added by Mamaduka 5 years ago.
6063-assoc-array-args-tests.diff (1.8 KB) - added by Mamaduka 5 years ago.
6063-assoc-array-args.2.diff (10.2 KB) - added by Mamaduka 5 years ago.
6063-assoc-array-args-tests.2.diff (4.1 KB) - added by Mamaduka 5 years ago.
6063-inner-joins.diff (3.0 KB) - added by Mamaduka 5 years ago.
6063.sql-clauses.patch (3.1 KB) - added by r-a-y 5 years ago.
6063.meta-query.patch (10.0 KB) - added by r-a-y 5 years ago.

Download all attachments as: .zip

Change History (21)

@r-a-y
5 years ago

#1 @boonebgorges
5 years ago

The meta_query changes look solid to me!

I feel somewhat uneasy about all the refactoring of these methods, given the sad state of test coverage. (Are you surprised by this uneasiness? ;) ) Maybe we could at least get a test for each of the methods where you're switching to the array notation for params (with the @expectedDeprecated flag to ignore the _doing_it_wrong()) just to demonstrate that the old-form params are translated properly?

Otherwise yeah, let's do this for 2.2.

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


5 years ago

#3 @Mamaduka
5 years ago

Updated patch.

6063-assoc-array-args.2.diff includes changes for BP_Message_Box_Template as well, to use array of arguments.

#4 @Mamaduka
5 years ago

  • Keywords has-patch added

Added updated tests.

#5 @Mamaduka
5 years ago

6063-inner-joins.diff converts STRAIGH_JOINs to INNER JOINs for better compatibility with meta query. Tested with existing test for messages.

#6 @r-a-y
5 years ago

In 9302:

Tests: Add tests for BP_Messages_Thread::get_current_threads_for_user() and BP_Messages_Box_Template

Props Mamaduka.

See #6063.

#7 @Mamaduka
5 years ago

Thanks r-a-y for getting those patches in.

r9301 has wrong ticket reference, so I'll just leave it here.

Any suggestions recommendations regarding JOIN changes? Current patch only update original big SQL string, but we can also convert it and follow other WP/BP standards and use array of SQL statements, joins and clauses. Which I think should make code more readable.

/cc @boonebgorges

#8 @r-a-y
5 years ago

Yeah, I kinda botched the commit message for r9301 and also forgot to reference this ticket :(

The INNER JOIN change looks okay. I think combined with my changes from 01.patch, we should be good.

  • sql-clauses.patch includes the INNER JOIN addition and uses an array to form the SQL statement.
  • meta-query.patch includes the actual meta query additions + unit tests.

I'm very close to committing both, but I think we need more unit tests for meta query to demonstrate the JOIN / INNER JOIN issue.

Last edited 5 years ago by r-a-y (previous) (diff)

#9 @Mamaduka
5 years ago

r-a-y,

Any particular test cases in mind? I can get similar ones that we have in other place to cover different cases of meta queries.

#10 @r-a-y
5 years ago

In 9331:

Messages: Do not duplicate SQL statements in BP_Messages_Thread::get_current_threads_for_user()

Previously, to query the thread IDs and the total threads, two separate SQL
statements were generated. Both of these statements are almost identical.
Therefore, this commit uses an array to house the SQL clauses, which is
later adjusted in the total thread query.

See #6063.

#11 @r-a-y
5 years ago

In 9332:

Messages: Use INNER JOIN in BP_Messages_Thread::get_current_threads_for_user()

This commit uses INNER JOIN for compiling the SQL statement to better
prepare for meta queries with messages.

Props Mamaduka.

See #6063.

#12 @r-a-y
5 years ago

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

In 9335:

Messages: Support meta queries in bp_has_message_threads() stack.

Much like r6948, r6950, this commit adds support for meta queries, but for
the messages component.

This will allow plugin develoeprs to customize the fetching of message
threads based on data stored in the messagemeta table, using a
powerful syntax familiar from WP_Query.

Commit also adds unit tests for the 'meta_query' parameter in
bp_has_message_threads().

Fixes #6063.

#13 @r-a-y
5 years ago

boonebgorges, Mamaduka - Thanks for your feedback on this ticket. I've decided to commit this for 2.2. We can always iterate and add other unit tests should a bug arise.

I already have a "Starred" message box plugin in mind with these changes :)

Thanks again!

Note: See TracTickets for help on using tickets.