Page MenuHomePhabricator

CU 2.0: Add button to add all other IPs being used by a user to an investigation
Closed, ResolvedPublic3 Estimated Story PointsMar 20 2020

Description

Mock

image.png (1×2 px, 413 KB)

Acceptance criteria

  • There is a button underneath every username - Show all N IPs of this user
  • The button is blue when there are IPs being used by this user that are not already in the table
  • The button is gray if all the IPs used by the user are in the table already
    • The disabled button is only shown on hovering over the username cell (similar to T244816)
    • There is a message on hover telling them why it's disabled - All IPs being used by this user are already in the table
  • On click, all the other IP records of the same username are added to the table.

Event Timeline

Niharika triaged this task as Medium priority.Feb 11 2020, 12:54 AM
Niharika created this task.
Niharika set the point value for this task to 3.Feb 20 2020, 7:33 PM

@Niharika, @Prtksxna - I have a few more questions (the first is a bit long - sorry - but provides context relating to the performance of the Compare query):

  • There is a button underneath every username - Show all N IPs of this user

Querying for the Compare results has the potential to be expensive, as discussed in T245499. It's therefore necessary to limit the results (e.g. if a target has 200,000 changes in the CheckUser table, not all will be examined in a single query). There were several potential ways to limit the results, but the one we chose involves querying for each target's results separately and dividing the total limit by the number of targets, to get a limit-per-target. This allows us to (a) make sure we show at least some results for each target, and (b) tell the user which targets' results are incomplete, if any. (T245499#5941360 describes the benefits of this strategy further.)

To make this work well, we need to limit the number of targets that can be entered into the form, to ensure that the limit-per-target remains high and the number of separate queries remains low. Clicking the "Show all N IPs" button adds another IP target for each relevant IP, in addition to the currently-entered targets. If N is high, or several of these buttons are clicked, the number of targets may become unacceptably high (resulting in lots of queries and low limits-per-target). Can we come up with a strategy for limiting the number of targets, taking into account these buttons to add additional IPs?

  • The button is gray [disabled] if all the IPs used by the user are in the table already

Is it acceptable to have the button enabled until it is clicked, and disabled thereafter? Otherwise, we'd have to query on each page load for all IPs used by all users shown in the current page of the table, to check whether they're present.

  • On click, all the other IP records of the same username are added to the table.

Does this mean every username/IP/UA combination where IP is used by the user (registered and unregistered edits), or every IP/UA combination where the IP is used by the user (unregistered edits only)?

There is also the issue of adding the new target to the investigation. Considering that server requests are logged, we might have to either pre-generate a token with the new target (possibly expensive) or potentially do it via a POST request. The latter seems more reasonable and @dbarratt mentioned it is what he'll be doing with the filters.

Clicking the "Show all N IPs" button adds another IP target for each relevant IP, in addition to the currently-entered targets.

I'm confused, wouldn't "Show all IPs of this user" translates to adding only that user to the targets?

Clicking the "Show all N IPs" button adds another IP target for each relevant IP, in addition to the currently-entered targets.

I'm confused, wouldn't "Show all IPs of this user" translates to adding only that user to the targets?

This is correct. @Tchanders -- with this clarification, do you think we need to have a strategy for limiting targets still or is it alright to go ahead with what we have right now and circle back if we see the query performance being bad?

The button is gray [disabled] if all the IPs used by the user are in the table already

Is it acceptable to have the button enabled until it is clicked, and disabled thereafter? Otherwise, we'd have to query on each page load for all IPs used by all users shown in the current page of the table, to check whether they're present.

Walking through a scenario when IPs 1.1.1.1 and 1.1.1.2 are under investigation and the following turns up:

User A Show_all_3_IPs_of_this_user1.1.1.1
User A Show_all_3_IPs_of_this_user1.1.1.2
User B Show_all_5_IPs_of_this_user1.1.1.1

When the user clicks the Show all 3 IPs of this user button for the first record, User A is added as a target, the page refreshes and that button is disabled, correct? Is the button also disabled for the second record of User A?

On click, all the other IP records of the same username are added to the table.

Does this mean every username/IP/UA combination where IP is used by the user (registered and unregistered edits), or every IP/UA combination where the IP is used by the user (unregistered edits only)?

For the above example, the investigation acts as if User A was added to the initial input form. All IP/UA combinations of User A are now also displayed in the table.

This is correct. @Tchanders -- with this clarification, do you think we need to have a strategy for limiting targets still or is it alright to go ahead with what we have right now and circle back if we see the query performance being bad?

Adding only one target at a time is a lot simpler than I thought - thanks for clarifying @Niharika. I'm happy to circle back on this if you like, because it shouldn't detrimentally affect the performance of the main query. (If too many of these buttons are clicked, we will find that the limit per target decreases, increasing the likelihood of incomplete results; plus we'll be doing an extra (smaller) query for each additional target.)

When the user clicks the Show all 3 IPs of this user button for the first record, User A is added as a target, the page refreshes and that button is disabled, correct? Is the button also disabled for the second record of User A?

Yes - we can disable the button for any users that are already targets in the query.

For the above example, the investigation acts as if User A was added to the initial input form. All IP/UA combinations of User A are now also displayed in the table.

Ok.

When the user clicks the Show all 3 IPs of this user button for the first record, User A is added as a target, the page refreshes and that button is disabled, correct? Is the button also disabled for the second record of User A?

Yes - we can disable the button for any users that are already targets in the query.

Got it. I think what you are proposing here is the same as what @Prtksxna had in mind. Let's go ahead with that.

Change 576947 had a related patch set uploaded (by Tchanders; owner: Tchanders):
[mediawiki/extensions/CheckUser@master] WIP Add buttons to Special:Investigate for adding extra user targets

https://gerrit.wikimedia.org/r/576947

When the user clicks the Show all 3 IPs of this user button for the first record, User A is added as a target, the page refreshes and that button is disabled, correct? Is the button also disabled for the second record of User A?

Yes - we can disable the button for any users that are already targets in the query.

Got it. I think what you are proposing here is the same as what @Prtksxna had in mind. Let's go ahead with that.

The only other thing I had in mind was: if the person has used an IP that hasn't been used by any other person (I don't know how likely that is) then the button should be disabled because there aren't any other users on that IP.

Screenshot 2020-03-05 at 8.58.09 AM.png (194×2 px, 64 KB)

@Prtksxna Yes, we can disable the buttons in the IP column if the number of edits match (i.e. [1 edit] vs (1 from all users)). (Will link to this comment from T244816.)

There is also the issue of adding the new target to the investigation. Considering that server requests are logged, we might have to either pre-generate a token with the new target (possibly expensive) or potentially do it via a POST request. The latter seems more reasonable and @dbarratt mentioned it is what he'll be doing with the filters.

Yeah. Since these are buttons and not links, it makes sense to have it POST the form. In my mind the JavaScript would add the target to a hidden field (like AddTarget) or something. The form would do a POST and then a redirect with a new token (just like the filters).

ARamirez_WMF changed the subtype of this task from "Task" to "Deadline".
ARamirez_WMF changed Due Date from Mar 19 2020, 4:00 AM to Mar 25 2020, 4:00 AM.Mar 18 2020, 5:11 PM
Niharika changed Due Date from Mar 25 2020, 4:00 AM to Mar 20 2020, 4:00 AM.Mar 18 2020, 5:11 PM

Change 576947 merged by jenkins-bot:
[mediawiki/extensions/CheckUser@master] Add buttons to Special:Investigate for adding extra user targets

https://gerrit.wikimedia.org/r/576947

dom_walden added a subscriber: dom_walden.
  • There is a button underneath every username - Show all N IPs of this user

Unless it is an unregistered user.

We also don't show the N for number of IPs.

  • The button is blue when there are IPs being used by this user that are not already in the table
  • The button is gray if all the IPs used by the user are in the table already

Button is gray if the user is already part of the investigation. Button is blue if the user is not part of the investigation, even if technically all their IP addresses are currently in the table.

  • The disabled button is only shown on hovering over the username cell (similar to T244816)

Yep.

  • There is a message on hover telling them why it's disabled - All IPs being used by this user are already in the table

Yep.

  • On click, all the other IP records of the same username are added to the table.

With an investigation for user "Eve" and IP "1.2.3.4", if I click "show all IPs..." for user "Adam":

  • The SQL submitted to the DB is equivalent to starting a new investigation for "Eve", "Adam" and "1.2.3.4"
  • A new Special:InvestigateLog log entry is made for "Adam", with reason same as current investigation

Regarding filters:

  • If in the above example, "Adam" has already been filtered out, "Adam" is not added to the investigation (although a log entry for "Adam" is still made, even though we are not getting information for Adam)
  • If after submitting "Adam" I filter out "Adam", the SQL is the same as the original investigation

The token for the investigation is updated, so the new user added to the investigation is preserved across tabs.

  • If in the above example, "Adam" has already been filtered out, "Adam" is not added to the investigation (although a log entry for "Adam" is still made, even though we are not getting information for Adam)

This is a bug, but should be solved by T247281, after which "Adam" would not appear in the results, and hence would not be able to be added via a button.