Better message than "This change is ready for review" when patch stops being WIP
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	jhsoby
	Nov 8 2023, 12:35 PM

Description

Some of us have enabled the setting that makes every patch a WIP by default in Gerrit. When such a task changes from a WIP state to a normal state, wikibugs will make its first report about that change in IRC, but the description will just be "This change is ready for review", with no indication about what "this change" actually is (besides the link, of course).

Example from the #translatewiki channel just a few minutes ago:

<wikibugs> (CR) Jon Harald Søby: "This change is ready for review." [translatewiki] - https://gerrit.wikimedia.org/r/972813 (owner: Jon Harald Søby)

I think it would be better if, instead of the default "This change is ready for review", the bot would give the title/header of the change instead (in this example that would be [Most used] Add 'urlshortener-toolbox'.

Details

	Subject	Repo	Branch	Lines +/-
	Handle Un-WIP as if it was a new changeset	labs/tools/wikibugs2	master	+276 -112
	Handle Un-WIP as if it was a new changeset	labs/tools/wikibugs2	master	+276 -112

Customize query in gerrit

Related Objects

Mentioned Here: T239928: wikibugs2 should not post on IRC for Gerrit changes marked 'WIP'

Event Timeline

jhsoby created this task.Nov 8 2023, 12:35 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptNov 8 2023, 12:35 PM

Hej,

The last time I looked at it, the json provided by stream-events looked like this:

{
  "author": {
    "name": "Merlijn van Deen",
    "email": "valhallasw@arctus.nl",
    "username": "valhallasw"
  },
  "approvals": [
    {
      "type": "Verified",
      "description": "Verified",
      "value": "0"
    },
    {
      "type": "Code-Review",
      "description": "Code-Review",
      "value": "-1",
      "oldValue": "0"
    }
  ],
  "comment": "Patch Set 2: Code-Review-1\n\nThis change is ready for review.",
  "patchSet": {
    "number": 2,
    "revision": "21d76c9c833a4ba6f8ff68135f3ddff7ae3a8c30",
    "parents": [
      "1b9f07b57098cc9d7e50c8e714a56aa8d2170ae5"
    ],
    "ref": "refs/changes/46/429546/2",
    "uploader": {
      "name": "Merlijn van Deen",
      "email": "valhallasw@arctus.nl",
      "username": "valhallasw"
    },
    "createdOn": 1524911801,
    "author": {
      "name": "Merlijn van Deen",
      "email": "valhallasw@arctus.nl",
      "username": "valhallasw"
    },
    "kind": "REWORK",
    "sizeInsertions": 1,
    "sizeDeletions": 0
  },
  "change": {
    "project": "test/gerrit-ping",
    "branch": "master",
    "topic": "test-draft",
    "id": "I26e7bf2b2fbc55b307e67479d43722842219ab30",
    "number": 429546,
    "subject": "Testing draft changes for https://github.com/valhallasw/gerrit-reviewer-bot/issues/12#issuecomment-384482876",
    "owner": {
      "name": "Merlijn van Deen",
      "email": "valhallasw@arctus.nl",
      "username": "valhallasw"
    },
    "url": "https://gerrit.wikimedia.org/r/429546",
    "commitMessage": "Testing draft changes for https://github.com/valhallasw/gerrit-reviewer-bot/issues/12#issuecomment-384482876\n\nChange-Id: I26e7bf2b2fbc55b307e67479d43722842219ab30\n",
    "createdOn": 1524911699,
    "status": "NEW"
  },
  "project": "test/gerrit-ping",
  "refName": "refs/heads/master",
  "changeKey": {
    "id": "I26e7bf2b2fbc55b307e67479d43722842219ab30"
  },
  "type": "comment-added",
  "eventCreatedOn": 1530442670
}

I.e., it is indistinguishable from someone commenting (textually) "This change is ready for review".

But it's certainly possible to trigger on the phrase "This change is ready for review." and handle this differently.

The logic lives here: https://github.com/wikimedia/labs-tools-wikibugs2/blob/master/grrrrit.py#L67
- It could e.g. be handled as a separate "process_simple" event with "Ready" / "RfR" / ... instead of "CR".
Corresponding test should go in https://github.com/wikimedia/labs-tools-wikibugs2/blob/master/tests/test_json_events.py
with an (anonymized) json file in https://github.com/wikimedia/labs-tools-wikibugs2/tree/master/tests/data/stream_events

I've started a background job on toolforge to log some more of these events & to see if the format has maybe changed by now.

Having logged a WIP change, I'm actually a bit confused by the behaviour you're describing. From what I can see, there _is_ now a new event, but that event shouldn't be logged as it's not one of the supported events:

to-wip:

{
  "changer": {
    "name": "Merlijn van Deen",
    "email": "valhallasw@arctus.nl",
    "username": "valhallasw"
  },
  "patchSet": {
    "number": 2,
    "revision": "4009450198e0c9f568113f5d5141deadca48c258",
    "parents": [
      "b3053ae5642948f4f9828f2db03df5ae884e2617"
    ],
    "ref": "refs/changes/65/710665/2",
    "uploader": {
      "name": "Merlijn van Deen",
      "email": "valhallasw@arctus.nl",
      "username": "valhallasw"
    },
    "createdOn": 1628337667,
    "author": {
      "name": "Merlijn van Deen",
      "email": "valhallasw@arctus.nl",
      "username": "valhallasw"
    },
    "kind": "NO_CODE_CHANGE",
    "sizeInsertions": 29,
    "sizeDeletions": 0
  },
  "change": {
    "project": "labs/tools/wikibugs2",
    "branch": "master",
    "id": "Ic33e1dc3d56b841b8c5b4793f0076a99aab1d1ac",
    "number": 710665,
    "subject": "Add GitLab to Redis webhook",
    "owner": {
      "name": "Merlijn van Deen",
      "email": "valhallasw@arctus.nl",
      "username": "valhallasw"
    },
    "url": "https://gerrit.wikimedia.org/r/c/labs/tools/wikibugs2/+/710665",
    "commitMessage": "Add GitLab to Redis webhook\n\nThis doesn\\u0027t to any processing of the events, it simply forwards it to\nRedis. This also means we run less risk of GitLab retrying the webhook\nbecause of a timeout (after all, the code doesn\\u0027t really do anything...)\n\nBug: T288381\nChange-Id: Ic33e1dc3d56b841b8c5b4793f0076a99aab1d1ac\n",
    "createdOn": 1628337640,
    "status": "NEW",
    "wip": true
  },
  "project": "labs/tools/wikibugs2",
  "refName": "refs/heads/master",
  "changeKey": {
    "id": "Ic33e1dc3d56b841b8c5b4793f0076a99aab1d1ac"
  },
  "type": "wip-state-changed",
  "eventCreatedOn": 1699716607
}

to-ready:

{
  "changer": {
    "name": "Merlijn van Deen",
    "email": "valhallasw@arctus.nl",
    "username": "valhallasw"
  },
  "patchSet": {
    "number": 2,
    "revision": "4009450198e0c9f568113f5d5141deadca48c258",
    "parents": [
      "b3053ae5642948f4f9828f2db03df5ae884e2617"
    ],
    "ref": "refs/changes/65/710665/2",
    "uploader": {
      "name": "Merlijn van Deen",
      "email": "valhallasw@arctus.nl",
      "username": "valhallasw"
    },
    "createdOn": 1628337667,
    "author": {
      "name": "Merlijn van Deen",
      "email": "valhallasw@arctus.nl",
      "username": "valhallasw"
    },
    "kind": "NO_CODE_CHANGE",
    "sizeInsertions": 29,
    "sizeDeletions": 0
  },
  "change": {
    "project": "labs/tools/wikibugs2",
    "branch": "master",
    "id": "Ic33e1dc3d56b841b8c5b4793f0076a99aab1d1ac",
    "number": 710665,
    "subject": "Add GitLab to Redis webhook",
    "owner": {
      "name": "Merlijn van Deen",
      "email": "valhallasw@arctus.nl",
      "username": "valhallasw"
    },
    "url": "https://gerrit.wikimedia.org/r/c/labs/tools/wikibugs2/+/710665",
    "commitMessage": "Add GitLab to Redis webhook\n\nThis doesn\\u0027t to any processing of the events, it simply forwards it to\nRedis. This also means we run less risk of GitLab retrying the webhook\nbecause of a timeout (after all, the code doesn\\u0027t really do anything...)\n\nBug: T288381\nChange-Id: Ic33e1dc3d56b841b8c5b4793f0076a99aab1d1ac\n",
    "createdOn": 1628337640,
    "status": "NEW"
  },
  "project": "labs/tools/wikibugs2",
  "refName": "refs/heads/master",
  "changeKey": {
    "id": "Ic33e1dc3d56b841b8c5b4793f0076a99aab1d1ac"
  },
  "type": "wip-state-changed",
  "eventCreatedOn": 1699716611
}

I.e., two events with "type": "wip-state-changed", with event['change']['wip'] the only content change. I'm not seeing the comment being added?

This is using

; is there another (incompatible?) way to mark a change as 'draft'?

Did an online test with Jon and we were able to reproduce and log the issue. See attached (anonymized)

stream-events.jsonlines5 KBDownload

, resulting in the following IRC entry:

<+wikibugs>(CR) Jon Harald Søby: "This change is ready for review." [labs/tools/wikibugs2] - https://gerrit.wikimedia.org/r/975276 (owner: Jon Harald Søby)

Stream-events reports *both* a "wip-state-changed" *and* a "comment-added" event.

There are two possible paths:

a) correctly handle wip-state-changed, and ignore comment-added if exactly the string "This change is ready for review."
b) ignore wip-state-changed and special-case "This change is ready for review."

The problem of approach b) is that not all "Mark as ready" changes seem to leave a comment. Risk of a) is that someone literally posting "This change is ready for review." will be ignored.

Change 975400 had a related patch set uploaded (by Merlijn van Deen; author: Merlijn van Deen):

[labs/tools/wikibugs2@master] Handle Un-WIP as if it was a new changeset

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

gerritbot added a project: Patch-For-Review.Nov 18 2023, 2:46 PM

Took an initial stab at adding the scaffolding for changing the behavior in the WIP patchset above. Implementation still to be done (and open to pick up as I might not get to it).

Implementation wise my suggestion is to

Handle wip->ready for review as if a new patchset is pushed
Ignore the specific "This change is ready for review." message (and hope that no-one decides to just post that ;-))

	F41538082: afbeelding.png
	Nov 26 2023, 2:56 PM

	F41489335: afbeelding.png
	Nov 11 2023, 3:37 PM

Better message than "This change is ready for review" when patch stops being WIPClosed, ResolvedPublicActions

Description

Details

Related Objects

Event Timeline

Better message than "This change is ready for review" when patch stops being WIP
Closed, ResolvedPublic
Actions