Page MenuHomePhabricator

Rework AbuseFilterPager to allow more flexible sorting
Open, Needs TriagePublic

Description

To format Special:AbuseFilter we're currently extending TablePager class, which looks pretty and is quite handy for formatting results from a DB query. However, this approach brings some limits in sorting columns (which TablePager does using ORDER BY ). These are the currently identified ones:

  1. With T87862 we'll add to the table some data retrieved from cache instead of DB. They're correctly shown but can't be sorted at all.
  2. In T120563 we learned that in some particular cases, sorting by hit count may reveal too much info. This may also be addressed by overriding TablePager's method which builds the query adding some SQL tricks.
  3. In T52839 it's been proposed to make the table sortable without reloading the page. This would need a client-side sorting.

Although problem (2) may be worked-around and (3) is not a big deal, (1) is instead something uncomfortable to live with. In case we need a brand new system to solve it, then we'd better take into account the other issues as well.

Event Timeline

Well if you address #3, it'll take care of #2 and #1 naturally, wouldn't it? Then the only issue that remains would be the no-JS users, for whom I think it would be okay to completely disable sorting by hit count (if we really feel like it is "revealing too much") and also not enable sorting for the columns added in #1.

In the light of that analysis, I am inclined to either close this task as a dup of #3, or make it a parent task to all those three.

Yeah, but sorting client-side has troubles also for JS-users, since you need to take into account values not shown in the current page. And yes, my intention was to make this parent task but forgot to add it :)

If we were to go the JS route, then we should also use JS-based pagination.

Indeed, but I don't know how much JS could be handy here. BTW, thanks to @valerio.bozzolan I learned that to address (2) we may use ORDER BY af_hidden ASC, IF( af_hidden = 0, af_hit_count, NULL). This is however hard to achieve using IndexPager. We may get relatively close by overriding getIndexField and getExtraSortFields , but we still have a wrong order, and would need to override the sorting method as well, since in this case we don't want to put ASC or DESC next to every column (just the last one). There may also be other problems, my testing was only partial. My thought is that we'd better avoid overriding everything for what might be a temporary fix, and just wait for a more robust solution to solve everything at once without dirty tricks.

This will also be DB-specific; it may not work in Postgres or SQLite, for instance.

@Huji is right. Anyway we can also try to achieve this using the CASE structure instead of the IF function.

True, but since we're only using an IF and not some highly esoterical method, it should be easy to adapt. Cross-DB compatibility is IMHO the last of our problems for this task at the moment.

Here an example using the CASE structure that should be not DB-specific:

ORDER BY af_hidden ASC, CASE WHEN af_hidden = 0 THEN af_hit_count ELSE NULL END

Vvjjkkii renamed this task from Rework AbuseFilterPager to allow more flexible sorting to y1caaaaaaa.Jul 1 2018, 1:10 AM
Vvjjkkii triaged this task as High priority.
Vvjjkkii updated the task description. (Show Details)
Vvjjkkii removed subscribers: Huji, Aklapper.
Daimona renamed this task from y1caaaaaaa to Rework AbuseFilterPager to allow more flexible sorting.Jul 1 2018, 12:43 PM
Daimona raised the priority of this task from High to Needs Triage.
Daimona updated the task description. (Show Details)
Daimona added subscribers: Huji, Aklapper.