PRfd - Merge PR and change PR status #8209

itaigilo · 2024-09-23T09:28:58Z

Closes #8176.

Change Description

Wire the "Merge PR" button to the API.
Wire the "Close PR" button to the API, and add a "Re-open PR" button.

Also, show PR details in Markdown.

github-actions · 2024-09-23T09:37:09Z

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions · 2024-09-23T09:38:35Z

E2E Test Results - Quickstart

itaigilo · 2024-09-23T12:40:38Z

webui/src/pages/repositories/repository/pulls/pulls.jsx

Some extra UI stuff for the List page,
Got into this PR, in order not to create another one in the PRs chain.

N-o-Z · 2024-09-23T15:02:34Z

@itaigilo Per discussion on the PRD - we do not support re-open ATM. This should be removed

itaigilo · 2024-09-23T15:05:50Z

@itaigilo Per discussion on the PRD - we do not support re-open ATM. This should be removed

Yeah I saw that -
But it felt weird, that after closing all the buttons disappear.

I don't mind removing it, but it's small piece of code that's already implemented.
Any good reason to not release it?

N-o-Z

Thanks!!
Please see comments

N-o-Z · 2024-09-23T15:04:03Z

webui/src/pages/repositories/repository/pulls/pullDetails.jsx

+    let [error, setError] = useState(null);
+
+    const mergePullRequest = async () => {
+        // TODO: this is not ideal - this should be handled in an atomic way


Let's open an issue for this and remove the comment as this is probably not something we're going to handle in the near future

N-o-Z · 2024-09-23T15:05:13Z

webui/src/pages/repositories/repository/pulls/pullDetails.jsx

+            await pullsAPI.update(repo.id, pull.id, {status: PullStatus.merged});
+            window.location.reload(); // TODO: replace with a more elegant solution
+        } catch (error) {
+            setError(`Failed to update pull-request status: ${error.message}`);


We should do something here - this is an inconsistent state which can lead to strange behavior

Yeah you're right.

I think we should:

Log this error somehow and monitor it, so we can see how often this occurs.

Have two extra retries.

Does this make sense, or do you have other suggestions?

N-o-Z · 2024-09-23T15:06:11Z

webui/src/pages/repositories/repository/pulls/pullDetails.jsx

+        setLoading(true);
+        try {
+            await pullsAPI.update(repo.id, pull.id, {status});
+            window.location.reload(); // TODO: replace with a more elegant solution


Same as previous comment (also please use our TODO convention)

Why this is an inconsistent state?

N-o-Z · 2024-09-23T15:08:06Z

webui/src/pages/repositories/repository/pulls/pullDetails.jsx

+                            }
+                        </Button>
+                    }
+                    {isPullOpen() &&


This condition is not enough - Merge should be also disabled if no changes (due to modifications of the branches, or if any of them no longer exists)

True -
Will be handled in a separate PR (or deferred).

N-o-Z · 2024-09-23T15:08:38Z

webui/src/pages/repositories/repository/pulls/pullDetails.jsx

+                                onClick={changePullStatus(isPullOpen() ? PullStatus.closed : PullStatus.open)}>
+                            {loading ?
+                                <span className="spinner-border spinner-border-sm text-light" role="status"/> :
+                                <>{isPullOpen() ? "Close" : "Re-open"} pull request</>


We don't support re-open

And also we shouldn't have a re-open button for Merged PRs

N-o-Z · 2024-09-23T15:11:27Z

@itaigilo Per discussion on the PRD - we do not support re-open ATM. This should be removed

Yeah I saw that - But it felt weird, that after closing all the buttons disappear.

I don't mind removing it, but it's small piece of code that's already implemented. Any good reason to not release it?

Yes - it complicates the state machine of the PRs. We want to reduce the scope as much as possible.
I think we can be good with graying out the buttons - we don't have to remove them
Whatever makes more sense to you

N-o-Z

Approved to unblock but please make sure to fix all comments before merging!

N-o-Z · 2024-09-23T23:14:45Z

webui/src/pages/repositories/repository/pulls/pullDetails.jsx

-        } catch (error) {
-            setError(`Failed to update pull-request status: ${error.message}`);
-            setLoading(false);
+        for (let i = 0; i < RETRIES_COUNT; i++) {


We are already importing https://www.npmjs.com/package/exponential-backoff so lets use it

This reverts commit a5566fa.

# Conflicts: # webui/src/lib/api/index.js # webui/src/pages/repositories/repository/pulls/pullDetails.jsx

itaigilo · 2024-09-24T09:46:25Z

All handled,
Except for the comments related to empty diffs, which will be handled in a separate PR.
Merging.

itaigilo added 8 commits September 22, 2024 16:05

Create PR page

d8e8bfb

Integrate List API

924a1ac

Fix Creation function

bb242d6

Extract BranchesSelection to a component

4e66153

Fix dates

8c2e3d3

Merge branch 'master' into feature/prfd-create-pr-page

feb5d83

Add Markdown support

4b1c71e

Merge PR and change PR status

f0a532e

itaigilo added exclude-changelog PR description should not be included in next release changelog pull-requests labels Sep 23, 2024

itaigilo requested review from N-o-Z and a team September 23, 2024 09:28

itaigilo changed the base branch from master to feature/prfd-create-pr-page September 23, 2024 10:52

Extra improvments for the List page

3f98ff4

itaigilo commented Sep 23, 2024

View reviewed changes

N-o-Z requested changes Sep 23, 2024

View reviewed changes

itaigilo added 9 commits September 23, 2024 19:04

Fix PR comments

64f6054

Change width

e43c551

Improve error handling

7b06cff

Update page width

0e4d0fd

Merge branch 'feature/prfd-create-pr-page' into prfd-merge-and-edit-pr

549701d

Add index to list items

60d33cb

Fix colors

1dde844

Add retries

bfc1580

Remove the option to Re-Open a PR

e5b0aef

itaigilo requested a review from N-o-Z September 23, 2024 18:00

N-o-Z approved these changes Sep 23, 2024

View reviewed changes

itaigilo added 5 commits September 24, 2024 11:16

Use exp-backoff

385af61

Fix error handling

7b30e29

Merge branch 'feature/prfd-create-pr-page' into prfd-merge-and-edit-pr

67369d9

Rename file

a5566fa

Fix PR comments

c58a88d

Base automatically changed from feature/prfd-create-pr-page to master September 24, 2024 09:27

itaigilo added 3 commits September 24, 2024 12:30

Revert "Rename file"

081ddf5

This reverts commit a5566fa.

Merge branch 'master' into prfd-merge-and-edit-pr

ea07559

# Conflicts: # webui/src/lib/api/index.js # webui/src/pages/repositories/repository/pulls/pullDetails.jsx

Rename file

e8f7f59

itaigilo enabled auto-merge (squash) September 24, 2024 09:47

itaigilo merged commit 06cd860 into master Sep 24, 2024
38 checks passed

itaigilo deleted the prfd-merge-and-edit-pr branch September 24, 2024 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PRfd - Merge PR and change PR status #8209

PRfd - Merge PR and change PR status #8209

itaigilo commented Sep 23, 2024

github-actions bot commented Sep 23, 2024

github-actions bot commented Sep 23, 2024

itaigilo Sep 23, 2024

N-o-Z commented Sep 23, 2024

itaigilo commented Sep 23, 2024

N-o-Z left a comment

N-o-Z Sep 23, 2024

itaigilo Sep 23, 2024

N-o-Z Sep 23, 2024

itaigilo Sep 23, 2024

N-o-Z Sep 23, 2024

itaigilo Sep 23, 2024

N-o-Z Sep 23, 2024

itaigilo Sep 24, 2024

N-o-Z Sep 23, 2024

N-o-Z Sep 23, 2024

N-o-Z commented Sep 23, 2024 •

edited

Loading

N-o-Z left a comment

N-o-Z Sep 23, 2024

itaigilo commented Sep 24, 2024

PRfd - Merge PR and change PR status #8209

PRfd - Merge PR and change PR status #8209

Conversation

itaigilo commented Sep 23, 2024

Change Description

github-actions bot commented Sep 23, 2024

E2E Test Results - DynamoDB Local - Local Block Adapter

github-actions bot commented Sep 23, 2024

E2E Test Results - Quickstart

Choose a reason for hiding this comment

N-o-Z commented Sep 23, 2024

itaigilo commented Sep 23, 2024

N-o-Z 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

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

N-o-Z commented Sep 23, 2024 • edited Loading

N-o-Z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

itaigilo commented Sep 24, 2024

N-o-Z commented Sep 23, 2024 •

edited

Loading