fix: handle error when branch already exists #48

sinchang · 2019-01-16T06:46:51Z

fixes #16

sinchang · 2019-01-16T06:47:51Z

src/utils/errors.js

@@ -21,8 +21,16 @@ class UserNotFoundError extends AllContributorBotError {
    }
 }

+class ReferenceExistsError extends AllContributorBotError {
+    constructor() {
+        super(`A branch or Pull Request is already open.`)


error message any suggestions?

Perhaps Branch ${branch} already exists

^ so at least it indicates which branch.

codecov · 2019-01-16T06:48:05Z

Codecov Report

Merging #48 into master will increase coverage by 28.73%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master      #48       +/-   ##
===========================================
+ Coverage   30.35%   59.09%   +28.73%     
===========================================
  Files           3        4        +1     
  Lines          56      110       +54     
  Branches        8       11        +3     
===========================================
+ Hits           17       65       +48     
- Misses         37       42        +5     
- Partials        2        3        +1

Impacted Files	Coverage Δ
src/processIssueComment.js	`92.3% <100%> (ø)`
src/serverless-webhook.js	`0% <0%> (-65.39%)`	⬇️
src/serverless-health-check.js
src/index.js	`100% <0%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f72ee4a...2a6bfc6. Read the comment docs.

jakebolam · 2019-01-16T07:15:45Z

test/Repository/index.test.js

+            .reply(422)
+
+        const error = await rejectionOf(
+            repository.createPullRequestFromFiles({


When we are creating the pull request and the branch exists can we recover somehow?

E.g. delete the branch then create. Or Maybe have a unique I'd for the branch we are creating (using UUID) to avoid collisions?

I prefer the second suggestion.

Yeah I think so to.

Alternatively if there was a way to differentiate between 'you already have a branch open' and 'you already have a pull request open'. The pull request being checked first tho.

What are your thoughts on the best User Experience here?

If the branch exists, whether or not the pr is opened, we update the file on the existing branch.

Create a unique branch so that it is the easiest and the smallest to change

If the branch is already open, I think that the bot should be able to (re)-use it.
If the PR is already open then it shouldn't be doing anything unless it's changing/adding/removing something else.

To re-use the branch, we need to re get the original sha's / generate off the branch (not master/the default branch).

😅

Yes that may make the most sense

jakebolam · 2019-01-16T07:15:59Z

src/Repository/index.js

+            })
+        } catch (error) {
+            if (error.code === 422) {
+                throw new ReferenceExistsError()


sinchang · 2019-01-17T04:05:30Z

some cases:

branch not exists and pr not open - create branch and open pr
branch exists and pr not open - update branch and open pr
branch exists and pr open - update branch
branch exists and pr closed - update branch and create a new pr

still, need to add some tests.

jakebolam · 2019-01-17T07:25:51Z

@sinchang those cases look good. This one is hard, keep at it!

Berkmann18 · 2019-01-17T12:16:27Z

Here's one more case (although casi unlikely to occur):
5. branch doesn't exist but there's a pr open - move the pr to the correct branch

I think that's all possible cases unless we're missing a variable/factor?

Berkmann18 · 2019-01-17T12:18:02Z

src/Repository/index.js

-
-            return false
-        } catch (error) {
-            // branch already exists


What would be the advantage and reason for not handling situations where this.github.git.createRef fails and throws an error?

jakebolam · 2019-01-19T12:32:53Z

src/Repository/index.js

+                branchName,
+            })
+            return pullRequestURL
+        } catch (error) {


This catch and error thrown should be done in the createPullRequest method

jakebolam · 2019-01-19T15:35:29Z

src/Repository/index.js

+            return pullRequestURL
+        } catch (error) {
+            if (error.status === 422)
+                throw new AllContributorBotError('Pull request is already open')


Can we add a link?
I've updated the [pull request](INSERT_LINK_HERE) you have open.

I don't think this should error too, could we handle this as a reply case here: (https://github.com/all-contributors/all-contributors-bot/blob/master/src/processIssueComment.js#L65-L67) e.g. can throw the error, but catch it here

jakebolam · 2019-01-19T15:37:10Z

src/Repository/index.js

@@ -61,17 +71,17 @@ class Repository {
        return multipleFilesByPath
    }

-    async getHeadRef(defaultBranch) {
+    async getHeadRef(branchName) {
        const result = await this.github.git.getRef({


Instead of checking 404 above when called.

Can we throw an error here? (BranchAlreadyExists) and catch that

jakebolam · 2019-01-19T15:38:09Z

src/processIssueComment.js

+        await repository.getHeadRef(branchName)
+        repository.setBasedBranch(branchName)
+    } catch (error) {
+        if (error.status !== 404) throw error


See other comment and consider using here:
if (error instance of BranchAlreadyExists)

jakebolam · 2019-01-19T15:39:14Z

src/processIssueComment.js

    })
+
+    try {


This block should happen after fetching config IMO

This block should happen inside the processAddContributor block. (For future support of other actions)

jakebolam · 2019-01-19T15:44:03Z

test/index-e2e.test.js

@@ -273,4 +304,148 @@ describe('All Contributors app - End to end', () => {
        )
        expect(error instanceof Error).toBeTruthy()
    })
+
+    test('Happy path, add correct new contributor, but branch exists', async () => {


[nb] and pr closed

src/Repository/index.js

jakebolam

This is a really good solution @sinchang! Thanks for doing this.

Overall really happy with the approach!

Just some comments/feedback to make it easier for future developers to understand (please reply to my comments if you think they don't make sense).

jakebolam · 2019-01-19T17:11:45Z

src/Repository/index.js

+    setBasedBranch(branchName) {
+        this.basedBranch = branchName
+    }
+
    async getFile(filePath) {


I wonder how we can take the latest master file and overwrite the current branch :think: like a rebase

We probably shouldn't address this here. This is probably a whole ticket in itself. #62.
I'm interested in hearing how you would solve this problem tho.

jakebolam · 2019-01-23T11:59:59Z

@sinchang are you ok if I get this over the line?

sinchang · 2019-01-23T12:24:22Z

@jakebolam ok

jakebolam · 2019-01-26T23:19:18Z

@sinchang THIS IS AWESOME, THANK YOU

jakebolam · 2019-01-26T23:19:53Z

@all-contributors please add @sinchang for ideas

allcontributors · 2019-01-26T23:20:03Z

@jakebolam

I've put up a pull request to add @sinchang! 🎉

all-contributors-release-bot · 2019-01-27T00:30:52Z

🎉 This PR is included in version 1.0.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

sinchang commented Jan 16, 2019

View reviewed changes

jakebolam reviewed Jan 16, 2019

View reviewed changes

src/Repository/index.js Outdated

})

} catch (error) {

if (error.code === 422) {

throw new ReferenceExistsError()

Copy link

Contributor

jakebolam Jan 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

refactor

865b982

sinchang force-pushed the patch-1 branch from 2f683cd to 865b982 Compare January 17, 2019 03:51

Update createBranch method

33a231a

Berkmann18 reviewed Jan 17, 2019

View reviewed changes

sinchang added 2 commits January 18, 2019 10:57

Tweak

542b5a9

Add tests

b179e9b

Berkmann18 approved these changes Jan 18, 2019

View reviewed changes

jakebolam reviewed Jan 19, 2019

View reviewed changes

Merge branch 'master' into patch-1

eddf278

jakebolam reviewed Jan 19, 2019

View reviewed changes

src/Repository/index.js Outdated Show resolved Hide resolved

jakebolam reviewed Jan 19, 2019

View reviewed changes

src/Repository/index.js Outdated Show resolved Hide resolved

jakebolam requested changes Jan 19, 2019

View reviewed changes

jakebolam reviewed Jan 19, 2019

View reviewed changes

jakebolam mentioned this pull request Jan 21, 2019

In some cases, seeing two responses from the bot #79

Closed

Merge branch 'master' into patch-1

62f61d9

Fix conflicts

f00625d

jakebolam added 5 commits January 26, 2019 14:32

Merge remote-tracking branch 'parent/master' into patch-1

91e49bc

wip

f55efae

wip

2a6bfc6

Merge remote-tracking branch 'parent/master' into patch-1

a218994

lint

7378180

jakebolam approved these changes Jan 26, 2019

View reviewed changes

jakebolam merged commit ab0857b into all-contributors:master Jan 26, 2019

allcontributors bot mentioned this pull request Jan 26, 2019

docs: add sinchang as a contributor #108

Merged

all-contributors-release-bot added the released label Jan 27, 2019

sinchang deleted the patch-1 branch January 27, 2019 05:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle error when branch already exists #48

fix: handle error when branch already exists #48

sinchang commented Jan 16, 2019

sinchang Jan 16, 2019

jakebolam Jan 16, 2019

Berkmann18 Jan 16, 2019

codecov bot commented Jan 16, 2019 •

edited

Loading

jakebolam Jan 16, 2019 •

edited

Loading

sinchang Jan 16, 2019

jakebolam Jan 16, 2019

sinchang Jan 16, 2019

sinchang Jan 16, 2019

Berkmann18 Jan 16, 2019

jakebolam Jan 16, 2019

jakebolam Jan 16, 2019

sinchang commented Jan 17, 2019 •

edited

Loading

jakebolam commented Jan 17, 2019 •

edited

Loading

Berkmann18 commented Jan 17, 2019

Berkmann18 Jan 17, 2019

jakebolam Jan 19, 2019 •

edited

Loading

jakebolam Jan 19, 2019

jakebolam Jan 19, 2019

jakebolam Jan 19, 2019 •

edited

Loading

jakebolam Jan 19, 2019

jakebolam Jan 19, 2019

jakebolam Jan 19, 2019

jakebolam left a comment

jakebolam Jan 19, 2019

jakebolam Jan 19, 2019

jakebolam commented Jan 23, 2019

sinchang commented Jan 23, 2019

jakebolam commented Jan 26, 2019

jakebolam commented Jan 26, 2019

allcontributors bot commented Jan 26, 2019

all-contributors-release-bot commented Jan 27, 2019

fix: handle error when branch already exists #48

fix: handle error when branch already exists #48

Conversation

sinchang commented Jan 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Jan 16, 2019 • edited Loading

Codecov Report

jakebolam Jan 16, 2019 • edited Loading

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

sinchang commented Jan 17, 2019 • edited Loading

jakebolam commented Jan 17, 2019 • edited Loading

Berkmann18 commented Jan 17, 2019

Choose a reason for hiding this comment

jakebolam Jan 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebolam Jan 19, 2019 • edited Loading

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

jakebolam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakebolam commented Jan 23, 2019

sinchang commented Jan 23, 2019

jakebolam commented Jan 26, 2019

jakebolam commented Jan 26, 2019

allcontributors bot commented Jan 26, 2019

all-contributors-release-bot commented Jan 27, 2019

codecov bot commented Jan 16, 2019 •

edited

Loading

jakebolam Jan 16, 2019 •

edited

Loading

sinchang commented Jan 17, 2019 •

edited

Loading

jakebolam commented Jan 17, 2019 •

edited

Loading

jakebolam Jan 19, 2019 •

edited

Loading

jakebolam Jan 19, 2019 •

edited

Loading