Page MenuHomePhabricator

Merge Special:InvestigateLog with Special:CheckUserLog
Closed, ResolvedPublic8 Estimated Story PointsOct 28 2020

Description

Goal

From the end user perspective, it makes sense to combine the two logs so that users are able to see all the checks being run in the same place. This also resolves the complexity of adding filters to the Special:InvestigateLog and having to account for the new indexes that might be needed.

Acceptance criteria

T266238

  • All log records from Special:InvestigateLog show up in Special:CheckuserLog
  • Note that the Special:Investigate log records keep their current format
  • All log records are filterable with the existing filters on Special:CheckuserLog

T266585

  • Link to the Special:Investigate form from the top of Special:CheckuserLog
  • Change the log button on Special:Investigate to link to Special:CheckuserLog
  • Remove SpecialInvestigateLog, InvestigateLogPager and InvestigateLogPagerFactory

Details

Due Date
Oct 28 2020, 4:00 AM

Event Timeline

The need for this arose from T256280: Update default tool links, in order to add links to the logs for a particular target.

Currently Special:CheckUserLog only displays checks from Special:CheckUser, and Special:InvestigateLog only displays checks from Special:Investigate.

@Prtksxna Which features should we bring over from Special:CheckUserLog?

Based on @cwylo's findings I suppose we'll have to have all the search functionality that CU has right now.

  • We'll need to OOUI-ify the form
  • The initiator and target radio is a bit confusing, especially since clicking initiator doesn't change the first text fields label

@Tchanders, are logs kept forever or do they follow the 90 day limit too?

@Prtksxna They also have a 90-day limit. They're stored in the same table as the CheckUser logs, just with a different type.

(Edited in light of @JJMC89's comment below.)

are logs kept forever or do they follow the 90 day limit too?

CU logs do not have a time limit. Are the investigate logs being handled differently?

@JJMC89 They're not being handled any differently.

Thanks @JJMC89 and @Tchanders 🙏🏽


I was looking at other Special pages for some widgets/patterns we might use.

  • We could use the same From date (and earlier) widget as Special:Log (and a few other places)
  • For the initiator/target I am thinking between two alternatives:
    1. Have separate single user select fields for both Initiator and Target. In Special:CheckUserLog you can't apply both but with this UI we'd no longer need to apply that restriction, unless there are other reasons to have it this way.
    2. Use something like the min/max byte size widget in Special:ProtectedPages. Instead of min and max we'd have initiator and target, and the text field would be a single user select.

What do you think, @Tchanders?

  1. Have separate single user select fields for both Initiator and Target. In Special:CheckUserLog you can't apply both but with this UI we'd no longer need to apply that restriction, unless there are other reasons to have it this way.

I think we should keep that restriction, to ensure the queries stay efficient, since there's no index on both the initiator and the target. However, looking at the indexes made me realise that even with this restriction we have a bit of a problem...

The indexes are:

CREATE INDEX /*i*/cul_user ON /*_*/cu_log (cul_user, cul_timestamp);
CREATE INDEX /*i*/cul_type_target ON /*_*/cu_log (cul_type,cul_target_id, cul_timestamp);
CREATE INDEX /*i*/cul_target_hex ON /*_*/cu_log (cul_target_hex, cul_timestamp);
CREATE INDEX /*i*/cul_range_start ON /*_*/cu_log (cul_range_start, cul_timestamp);
CREATE INDEX /*i*/cul_timestamp ON /*_*/cu_log (cul_timestamp);

That means we can have efficient queries filtering by:

  • initiator (but allowing for any type, including logs from Special:CheckUser checks, as well as checks with type 'investigate')
  • type (i.e. only checks with type 'investigate')
  • type and target ID (i.e. 'investigate' checks with a certain user target)
  • target IP address (i.e. the IP address if the target was in IP, but of any type)
  • target range start (i.e. the IP range if the target was an IP range, but of any type)
  • timestamp

This is where supporting both Special:CheckUserLog and Special:InvestigateLog becomes a bit tricky... The existing indexes are needed for Special:CheckUserLog, but Special:InvestigateLog would ideally have different indexes, starting with cul_type so we can always filter by type 'investigate'. However, adding more indexes adds cost, so I doubt we actually want to support two sets of indexes.

This makes me think it would probably be better to have one page after all - Special:CheckUserLog - that shows log entries of all types, including 'investigate'. We could link to the entries for a particular target from the tool links, which was the original motivation for this task. We could even add a filter for the type 'investigate' (note that the results would be sorted by target ID and then timestamp).

This makes me think it would probably be better to have one page after all - Special:CheckUserLog - that shows log entries of all types, including 'investigate'. We could link to the entries for a particular target from the tool links, which was the original motivation for this task.

+1 to having one log. It would also make it easier to review logs while CheckUser and Investigate are both available.

We could even add a filter for the type 'investigate' (note that the results would be sorted by target ID and then timestamp).

That sorting isn't ideal. I don't know any existing logs that are not sorted by timestamp.

This makes me think it would probably be better to have one page after all - Special:CheckUserLog - that shows log entries of all types, including 'investigate'. We could link to the entries for a particular target from the tool links, which was the original motivation for this task.

+1 to having one log. It would also make it easier to review logs while CheckUser and Investigate are both available.

Our intention when originally separating the two logs was to ensure that users wouldn't get confused on seeing logs from different tools show up together. Given the complexities, @Tchanders has outlined above, I am inclined to agree that it makes sense to combine the two logs together.
@Tchanders @JJMC89 I am thinking we can forego "Special:InvestigateLog" and show up all log entries in "Special:CheckUserLog" as long as we can clearly distinguish which logs are coming from which tool for the reader (@Prtksxna).
I think it is okay to not have a filter for the investigate type. I am not sure how useful it might be for users.

@Tchanders Can you also clarify the point about sorting? I thought we were sorting by timestamp followed by target (if multiple targets looked up at same time).

Niharika renamed this task from Make Special:InvestigateLog searchable to Merge Special:InvestigateLog with Special:CheckUserLog.Sep 26 2020, 9:09 AM
Niharika triaged this task as Medium priority.
Niharika updated the task description. (Show Details)
Niharika moved this task from Design to Onboarding Tasks on the Anti-Harassment board.

Summarizing a team discussion - we may want to split this into separate subtasks:

  • Remove filter on SpecialCheckUserLog that discards rows with type 'investigate' (so users can see all logs in one place right away)
  • Show all logs types on SpecialInvestigateLog, keeping formatting for 'investigate' type distinct (will involve merging parts of CheckUserLogPager into InvestigateLogPager)
  • Add filters to SpecialInvestigateLog
  • Replace SpecialCheckUserLog/CheckUserLogPager classes with SpecialInvestigateLog/InvestigateLogPager classes, including renaming the classes, URL, permissions, etc.
ARamirez_WMF changed the subtype of this task from "Task" to "Deadline".Oct 21 2020, 6:36 PM
ARamirez_WMF set Due Date to Oct 28 2020, 4:00 AM.

@Tchanders I just realized the acceptance criteria is missing a point: we should remove Special:InvestigateLog and redirect it to Special:CheckuserLog. Should I create another task to do that?

@Niharika I've made a second subtask to remove the code for Special:InvestigateLog and change the links that point to it.

We probably don't need a redirect, since the page is so new? Might just add an unnecessary maintenance burden...

Tchanders added a subscriber: dom_walden.

@dom_walden There's nothing extra to test here - it's just a parent task.

@dom_walden There's nothing extra to test here - it's just a parent task.

Thanks for letting me know. I have finished my testing of T267046, T266585 and T266238, so moving this on.