1588: Bridge messages should not be sent for PRs in draft state #1469

lgxbslgx · 2023-02-09T17:48:28Z

Hi all,

This patch prevents the mailling bridge bot from sending emails when a PR is draft.
And the related test case is added.

Thanks for taking the time to review.

Best Regards,
-- Guoxiong

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace

Issue

SKARA-1588: Bridge messages should not be sent for PRs in draft state

Reviewers

Zhao Song (@zhaosongzs - Committer) ⚠️ Review applies to cdcfe380
Erik Joelsson (@erikj79 - Reviewer) ⚠️ Review applies to 4d50eaad

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/skara pull/1469/head:pull/1469
$ git checkout pull/1469

Update a local copy of the PR:
$ git checkout pull/1469
$ git pull https://git.openjdk.org/skara pull/1469/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1469

View PR using the GUI difftool:
$ git pr show -t 1469

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/skara/pull/1469.diff

bridgekeeper · 2023-02-09T17:48:37Z

👋 Welcome back gli! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

mlbridge · 2023-02-09T17:52:09Z

Webrevs

zhaosongzs

I think this patch could successfully help us postpone the sending of emails when the pr is in draft mode and some users will be happy to see this update.

kevinrushforth

Not sure whether this is sufficient. See my question below.

kevinrushforth · 2023-02-09T19:25:07Z

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBot.java

@@ -203,6 +203,7 @@ public List<WorkItem> getPeriodicItems() {

        List<PullRequest> prs = poller.updatedPullRequests();
        prs.stream()
+                .filter(pr -> !pr.isDraft())


Are you sure this is the right place to do this? The intention of this fix is to pause notifications for Draft PRs until it becomes "rfr" again, just like it currently does for PRs initially in Draft that become "rfr" for the first time. Will it do this or will it just lose the notifications permanently?

zhaosongzs · 2023-02-09T19:37:12Z

bots/mlbridge/src/test/java/org/openjdk/skara/bots/mlbridge/MailingListBridgeBotTests.java

+            // Make it as draft again.
+            pr.makeDraft();
+
+            // Add a new comment.
+            pr.addComment("This is a new comment");
+
+            // Run another archive pass.
+            TestBotRunner.runPeriodicItems(mlBot);
+
+            // The archive should not now contain the new comment.
+            Repository.materialize(archiveFolder.path(), archive.url(), "master");
+            assertEquals(2, archiveContainsCount(archiveFolder.path(), "RFR: 1234: This is a pull request"));
+            assertEquals(1, archiveContainsCount(archiveFolder.path(), "This is a comment"));
+            assertFalse(archiveContains(archiveFolder.path(), "This is a new comment"));
+
+            // Make it as not draft again.
+            pr.makeNotDraft();
+
+            // Run another archive pass.
+            TestBotRunner.runPeriodicItems(mlBot);
+
+            // The archive should now contain the new comment.
+            Repository.materialize(archiveFolder.path(), archive.url(), "master");
+            assertEquals(3, archiveContainsCount(archiveFolder.path(), "RFR: 1234: This is a pull request"));
+            assertEquals(1, archiveContainsCount(archiveFolder.path(), "This is a comment"));
+            assertEquals(1, archiveContainsCount(archiveFolder.path(), "This is a new comment"));


@kevinrushforth Hi Kevin, see Li's test case here. This pr was 'rfr' and he converted it to draft. When the pr was in draft mode, he added a comment 'This is a new comment' and run the bot. As expected, there is no email sent. After that, he converted the pr to 'rfr' again and run the bot. This time, he got the notification about the comment 'This is a new comment'. So we won't lose the notification permanently. It's just delayed.

OK, thanks.

erikj79

This will indeed pause emails while a PR is in draft state. When it gets back from draft, I believe everything that happened while in draft will be sent. That is certainly the easiest way to handle this and I'm good with this change. We will just have to see how it plays out in practice.

erikj79 · 2023-02-10T14:52:44Z

This will indeed pause emails while a PR is in draft state. When it gets back from draft, I believe everything that happened while in draft will be sent. That is certainly the easiest way to handle this and I'm good with this change. We will just have to see how it plays out in practice.

Thinking about this some more, this simple solution has a pretty big drawback. There is no actual synchronization between events. All bot actions are asynchronous. Imagine the following scenario.

User posts comment "I'm putting this PR in draft to work on the feedback"
User puts PR in draft
mlbridge bot processes PR, finds it in draft and does nothing
User removes draft state
mlbridge bot processes PR, finds new comment from 1 and posts it.

For this to work correctly, draft PRs still need to be processed by mlbridge, and any events up until the draft state change need to be processed as usual.

This is still possibly better than what we have, but I'm not sure. Opinions?

zhaosongzs · 2023-02-10T17:42:16Z

This will indeed pause emails while a PR is in draft state. When it gets back from draft, I believe everything that happened while in draft will be sent. That is certainly the easiest way to handle this and I'm good with this change. We will just have to see how it plays out in practice.

Thinking about this some more, this simple solution has a pretty big drawback. There is no actual synchronization between events. All bot actions are asynchronous. Imagine the following scenario.

User posts comment "I'm putting this PR in draft to work on the feedback"

User puts PR in draft

mlbridge bot processes PR, finds it in draft and does nothing

User removes draft state

mlbridge bot processes PR, finds new comment from 1 and posts it.

For this to work correctly, draft PRs still need to be processed by mlbridge, and any events up until the draft state change need to be processed as usual.

This is still possibly better than what we have, but I'm not sure. Opinions?

I'm not sure I understand it correctly. Is the problem that the notification 'I'm putting this PR in draft to work on the feedback' won't be sent until the user makes the PR out of draft mode? Maybe it's not a big problem, the window is not very big, just few minutes.

erikj79 · 2023-02-10T17:57:57Z

I'm not sure I understand it correctly. Is the problem that the notification 'I'm putting this PR in draft to work on the feedback' won't be sent until the user makes the PR out of draft mode? Maybe it's not a big problem, the window is not very big, just few minutes.

Yes, the problem is that the last comments or events before changing a PR to draft could be delayed until the PR is taken out of draft. The size of the window for this to happen is usually just a few minutes, but depending on the state of the bots, it could be much longer. My example scenario seems like a very plausible series of events however, a user comments about making it draft and then puts it in draft, more or less at the same time. In that case it's very likely that the last comment hits the window. Also delaying that particular message would be very confusing to users, and I can guarantee that if it happened, we would get a lot of complaints. A PR could be moved to draft for weeks, and suddenly receiving week old comments from before it was put in draft is pretty bad.

zhaosongzs · 2023-02-10T18:12:04Z

I'm not sure I understand it correctly. Is the problem that the notification 'I'm putting this PR in draft to work on the feedback' won't be sent until the user makes the PR out of draft mode? Maybe it's not a big problem, the window is not very big, just few minutes.

Yes, the problem is that the last comments or events before changing a PR to draft could be delayed until the PR is taken out of draft. The size of the window for this to happen is usually just a few minutes, but depending on the state of the bots, it could be much longer. My example scenario seems like a very plausible series of events however, a user comments about making it draft and then puts it in draft, more or less at the same time. In that case it's very likely that the last comment hits the window. Also delaying that particular message would be very confusing to users, and I can guarantee that if it happened, we would get a lot of complaints. A PR could be moved to draft for weeks, and suddenly receiving week old comments from before it was put in draft is pretty bad.

Ok, I think the solution is that we don't filter in MailingListBridgeBot#getPeriodicItems. Instead, in ArchiveWorkItem#run, we can check if the pr is in draft mode and record the time. This recorded time can then be used to filter the comments.

erikj79 · 2023-02-10T18:54:05Z

Ok, I think the solution is that we don't filter in MailingListBridgeBot#getPeriodicItems. Instead, in ArchiveWorkItem#run, we can check if the pr is in draft mode and record the time. This recorded time can then be used to filter the comments.

There should be events that we can query for with timestamps for when the state change happened. The check would be something like this: If pr.isDraft(), find last event that put it in draft and use the timestamp of that as filter for any email generation.

lgxbslgx · 2023-02-11T12:27:43Z

Thinking about this some more, this simple solution has a pretty big drawback. There is no actual synchronization between events. All bot actions are asynchronous. Imagine the following scenario.

User posts comment "I'm putting this PR in draft to work on the feedback"

User puts PR in draft

mlbridge bot processes PR, finds it in draft and does nothing

User removes draft state

mlbridge bot processes PR, finds new comment from 1 and posts it.

For this to work correctly, draft PRs still need to be processed by mlbridge, and any events up until the draft state change need to be processed as usual.

This is still possibly better than what we have, but I'm not sure. Opinions?

Good Caught. This is a case we need to solve.

I updated the code just now, which ignores the comments in method ArchiveWorkItem#ignoreComment
instead of filtering the draft PRs in method MailingListBridgeBot#getPeriodicItems.

In order to get the time when the PR was last marked as draft, I add a new API lastMarkedAsDraftTime to PullRequest
and implement it in the sub-classes. The related tests about this new case and the new API are also added.

zhaosongzs · 2023-02-13T18:11:49Z

Now, I could remember that my implementation of SKARA-1683 shares the same issue with Li's first version. I had previously mentioned in that pr "My current implementation has a bug. If a user force-pushes or rebases when the pr is in ready state, and then he converts the pr to draft state very quickly, in such a case, our pullRequestBot won't have enough time to send warning, and then user will never get the notification of this force-push or rebase.". I will fix this issue later this week .

erikj79

This looks pretty good, but see comments. Were you able to run the manual tests to verify the new methods on PullRequest?

erikj79 · 2023-02-13T19:03:08Z

bots/mlbridge/src/main/java/org/openjdk/skara/bots/mlbridge/ArchiveWorkItem.java

+        // If the pull request was converted to draft, the comments
+        // after the last converted time should be ignored.
+        if (pr.isDraft()) {
+            var lastDraftTime = pr.lastMarkedAsDraftTime();


This method gets called for every comment on the PR, every time ArchiveWorkItem is run for a PR. The result of pr.lastMarkedAsDraftTime() needs to be retrieved once and reused.

I move pr.lastMarkedAsDraftTime() to the method ArchiveWorkItem#run to avoid the duplicated invocation.

erikj79 · 2023-02-13T19:10:18Z

forge/src/main/java/org/openjdk/skara/forge/github/GitHubPullRequest.java

+                .map(JSONValue::asObject)
+                .filter(obj -> obj.contains("event"))
+                .filter(obj -> obj.get("event").asString().equals("convert_to_draft"))
+                .reduce((a, b) -> b)


Are you sure of the order events are returned in? I couldn't find a definition of the order. Maybe safer to map to date and reduce to the highest one?

I can't find the related document too. I tried the rest api to get the related data and found the order personally. Now I adjust the code to find the highest one to avoid the API result changes in the future.

erikj79 · 2023-02-13T19:12:14Z

forge/src/main/java/org/openjdk/skara/forge/gitlab/GitLabMergeRequest.java

+                .map(JSONValue::asObject)
+                .filter(obj -> obj.get("system").asBoolean())
+                .filter(obj -> draftMessage.equals(obj.get("body").asString()))
+                .reduce((a, b) -> a)


Same here as for GitHub, maybe reduce to highest date just to be safe?

Same as above and also adjust it.

…ust the API implementation.

lgxbslgx · 2023-02-14T08:57:35Z

Were you able to run the manual tests to verify the new methods on PullRequest?

I had run the manual tests locally. Everything looks fine.

erikj79 · 2023-02-14T14:08:59Z

forge/src/main/java/org/openjdk/skara/forge/github/GitHubPullRequest.java

+                .filter(obj -> obj.contains("event"))
+                .filter(obj -> obj.get("event").asString().equals("convert_to_draft"))
+                .map(obj -> ZonedDateTime.parse(obj.get("created_at").asString()))
+                .reduce((a, b) -> a.isBefore(b) ? b : a);


I think this could be expressed as max().

openjdk · 2023-02-14T16:49:43Z

@lgxbslgx This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

🔍 One or more changes in this pull request modifies files in areas of the source code that often require two reviewers. Please consider if this is the case for this pull request, and if so, await a second reviewer to approve this pull request before you integrate it.

After integration, the commit message for the final commit will be:

1588: Bridge messages should not be sent for PRs in draft state

Reviewed-by: zsong, erikj

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

lgxbslgx · 2023-02-14T20:40:20Z

It seems a new bug:

When a PR has become ready and then another PR, of which code conflicts with the former one, was integrated, the former PR remains ready. But actually, it should have a merge-conflict label and should not be ready.

This PR-1469 (SKARA-1588) has became ready and then PR-1472 (SKARA-1817) was integrated. This PR is still ready now. But actually, this PR should have a merge-conflict label and should not be ready (the ready label should be removed and the related comment aboud conflict should be added).

I suspect it is because PR bot have not been triggered in this PR (since this PR has not been updated). This comment can update this PR and can verify my idea.

# Conflicts: # forge/src/test/java/org/openjdk/skara/forge/gitlab/GitLabRestApiTest.java

lgxbslgx · 2023-02-14T21:18:16Z

I suspect it is because PR bot have not been triggered in this PR (since this PR has not been updated). This comment can update this PR and can verify my idea.

This suspicion looks wrong now.

lgxbslgx · 2023-02-14T21:23:19Z

It seems a new bug:

When a PR has become ready and then another PR, of which code conflicts with the former one, was integrated, the former PR remains ready. But actually, it should have a merge-conflict label and should not be ready.

This PR-1469 (SKARA-1588) has became ready and then PR-1472 (SKARA-1817) was integrated. This PR is still ready now. But actually, this PR should have a merge-conflict label and should not be ready (the ready label should be removed and the related comment aboud conflict should be added).

I suspect it is because PR bot have not been triggered in this PR (since this PR has not been updated). This comment can update this PR and can verify my idea.

Filed https://bugs.openjdk.org/browse/SKARA-1820 to record it.

lgxbslgx · 2023-02-15T07:19:43Z

/integrate

openjdk · 2023-02-15T07:20:10Z

Going to push as commit baa4fae.

openjdk · 2023-02-15T07:20:19Z

@lgxbslgx Pushed as commit baa4fae.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

SKARA-1588

cdcfe38

openjdk bot changed the title ~~SKARA-1588~~ 1588: Bridge messages should not be sent for PRs in draft state Feb 9, 2023

openjdk bot added the rfr label Feb 9, 2023

zhaosongzs approved these changes Feb 9, 2023

View reviewed changes

kevinrushforth reviewed Feb 9, 2023

View reviewed changes

zhaosongzs reviewed Feb 9, 2023

View reviewed changes

erikj79 reviewed Feb 9, 2023

View reviewed changes

Filter the comments in method ArchiveWorkItem#ignoreComment

1170427

erikj79 reviewed Feb 13, 2023

View reviewed changes

Move the last draft invocation out of the method 'ignoreComment'. Adj…

2d139bc

…ust the API implementation.

erikj79 reviewed Feb 14, 2023

View reviewed changes

Use max method instead of reduce

4d50eaa

erikj79 approved these changes Feb 14, 2023

View reviewed changes

openjdk bot added the ready label Feb 14, 2023

Merge branch 'master' into SKARA-1588

73c3a7b

# Conflicts: # forge/src/test/java/org/openjdk/skara/forge/gitlab/GitLabRestApiTest.java

openjdk bot added the integrated label Feb 15, 2023

openjdk bot closed this Feb 15, 2023

openjdk bot removed ready rfr labels Feb 15, 2023

lgxbslgx deleted the SKARA-1588 branch February 15, 2023 07:24

lgxbslgx mentioned this pull request Mar 15, 2023

1828: RFR emails still be sent when PR is in draft state #1484

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

1588: Bridge messages should not be sent for PRs in draft state #1469

1588: Bridge messages should not be sent for PRs in draft state #1469

lgxbslgx commented Feb 9, 2023 •

edited by openjdk bot

bridgekeeper bot commented Feb 9, 2023

mlbridge bot commented Feb 9, 2023 •

edited

zhaosongzs left a comment

kevinrushforth left a comment

kevinrushforth Feb 9, 2023

zhaosongzs Feb 9, 2023

kevinrushforth Feb 9, 2023

erikj79 left a comment

erikj79 commented Feb 10, 2023 •

edited

zhaosongzs commented Feb 10, 2023

erikj79 commented Feb 10, 2023

zhaosongzs commented Feb 10, 2023

erikj79 commented Feb 10, 2023

lgxbslgx commented Feb 11, 2023

zhaosongzs commented Feb 13, 2023

erikj79 left a comment

erikj79 Feb 13, 2023

lgxbslgx Feb 14, 2023

erikj79 Feb 13, 2023

lgxbslgx Feb 14, 2023

erikj79 Feb 13, 2023

lgxbslgx Feb 14, 2023

lgxbslgx commented Feb 14, 2023

erikj79 Feb 14, 2023

lgxbslgx Feb 14, 2023

openjdk bot commented Feb 14, 2023 •

edited

lgxbslgx commented Feb 14, 2023 •

edited

lgxbslgx commented Feb 14, 2023

lgxbslgx commented Feb 14, 2023

lgxbslgx commented Feb 15, 2023

openjdk bot commented Feb 15, 2023

openjdk bot commented Feb 15, 2023

1588: Bridge messages should not be sent for PRs in draft state #1469

1588: Bridge messages should not be sent for PRs in draft state #1469

Conversation

lgxbslgx commented Feb 9, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Feb 9, 2023

mlbridge bot commented Feb 9, 2023 • edited

Webrevs

zhaosongzs left a comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikj79 left a comment

Choose a reason for hiding this comment

erikj79 commented Feb 10, 2023 • edited

zhaosongzs commented Feb 10, 2023

erikj79 commented Feb 10, 2023

zhaosongzs commented Feb 10, 2023

erikj79 commented Feb 10, 2023

lgxbslgx commented Feb 11, 2023

zhaosongzs commented Feb 13, 2023

erikj79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgxbslgx commented Feb 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openjdk bot commented Feb 14, 2023 • edited

lgxbslgx commented Feb 14, 2023 • edited

lgxbslgx commented Feb 14, 2023

lgxbslgx commented Feb 14, 2023

lgxbslgx commented Feb 15, 2023

openjdk bot commented Feb 15, 2023

openjdk bot commented Feb 15, 2023

lgxbslgx commented Feb 9, 2023 •

edited by openjdk bot

mlbridge bot commented Feb 9, 2023 •

edited

erikj79 commented Feb 10, 2023 •

edited

openjdk bot commented Feb 14, 2023 •

edited

lgxbslgx commented Feb 14, 2023 •

edited