Separate activity WHERE conditions into new method

[johnjamesjacoby]

BP_Activity_Activity::get() is a fairly large method, and around half of it is from figuring out $where_conditions. We should be able to separate them out into a new method similar to Notifications without too much hassle.

[johnjamesjacoby]

​6271.01.patch separates out $where_conditions into a new get_where_sql() method. Tests still pass, and I haven't identified any breakage.

The new method is marked protected, but not really for any other reason than it does no good to call it from outside the class anyways.

[boonebgorges]

Agreed that get() is an overly large method, but I'm not sure I see the benefit of separating out the logic in just this way. In the case of Notifications, it's abstracted in one chunk because it's used in more than one place in the class. Could that potentially be the case as well for get_where_sql() (I haven't looked). In terms of sheer organization, it seems like we'd get more benefit from abstracting each specific WHERE clause into its own method, though much of this is done already (get_date_query_sql(), get_filter_sql(), etc).

I'm not necessarily against the proposed change, but I would like to be sure we're not refactoring with no benefit, especially if there's an alternative way to refactor that would be more beneficial in terms of testability and reusability.

[johnjamesjacoby]

We could also break up the individual conditions, which would make them easier to test.

Notifications does use this method in two places, but we will likely end up eliminating that extra count query to better match what's currently in the Activity component.

The addition of Notification meta in #6257 means it's under similar scrutinization, so we can apply our conclusion there as well.

[boonebgorges]

Notifications does use this method in two places, but we will likely end up eliminating that extra count query to better match what's currently in the Activity component.

Why?

[johnjamesjacoby]

Replying to boonebgorges:

Notifications does use this method in two places, but we will likely end up eliminating that extra count query to better match what's currently in the Activity component.

Why?

I haven't done a full comparison between the approaches, so I don't have a technical one. Ideally though, if there's no reason to be inconsistent, why would we want to be?

[DJPaul]

This has gone nowhere in a year; apart from probable staleness, it looks like we required a little more technical information about the desired change. I'm going to close this ticket for now.