Add new api call for BranchPermission #51

j0nathan33 · 2017-03-08T20:13:43Z

No description provided.

cdancy · 2017-03-08T21:05:47Z

src/main/java/com/cdancy/bitbucket/rest/domain/branch/BranchPermissionEnumType.java

+package com.cdancy.bitbucket.rest.domain.branch;
+
+public enum BranchPermissionEnumType {
+    fast_forward_only("Rewriting history", "fast-forward-only",


Can we do all CAPS for enum types (e.g. fast_forward_only to FAST_FORWARD_ONLY)?

cdancy · 2017-03-08T21:06:30Z

src/main/java/com/cdancy/bitbucket/rest/domain/branch/BranchPermissionEnumType.java

+    @Override
+    public String toString() {
+        return this.getApiName();
+    }


Is this necessary and/or being used anywhere?

Yes, it's need for update or create BranchPermission because Gson use toString() for conversion value before send to bitbucket

cdancy · 2017-03-08T21:09:04Z

src/main/java/com/cdancy/bitbucket/rest/domain/branch/Matcher.java

+        develop("development", "Development", "MODEL_BRANCH", "Branching model branch"),
+        master("production", "Production", "MODEL_BRANCH", "Branching model branch");
+
+


remove extra line break

cdancy · 2017-03-08T21:10:00Z

src/main/java/com/cdancy/bitbucket/rest/domain/branch/Matcher.java

+    public enum MatcherId {
+        release("RELEASE", "Release", "MODEL_CATEGORY", "Branching model category"),
+        develop("development", "Development", "MODEL_BRANCH", "Branching model branch"),
+        master("production", "Production", "MODEL_BRANCH", "Branching model branch");


Caps for enum names. Was it intentional to put RELEASE in call caps and then development not in all caps?

It's not error. In Bitbucket, the brancing model for release is uppercase and for develop/master is lowercase.

Fair enough. Can we also change the enum name to match the release name (e.g. RELEASE, DEVELOPMENT, MASTER)?

cdancy · 2017-03-08T21:13:13Z

src/main/java/com/cdancy/bitbucket/rest/features/BranchApi.java

+    @Path("/branch-permissions/2.0/projects/{project}/repos/{repo}/restrictions")
+    @Fallback(BitbucketFallbacks.BranchPermissionPageOnError.class)
+    @GET
+    BranchPermissionPage getBranchPermission(@PathParam("project") String project,


Lets change to listBranchPermission to keep in line with naming standards already set.

cdancy · 2017-03-08T21:14:22Z

src/main/java/com/cdancy/bitbucket/rest/features/BranchApi.java

@@ -114,4 +119,35 @@ Branch getDefault(@PathParam("project") String project,
    @GET
    BranchModel model(@PathParam("project") String project,
                      @PathParam("repo") String repo);
+
+    @Named("branch:get-BranchPermission")


Change to branch:list-branch-permission

cdancy · 2017-03-08T21:15:07Z

src/main/java/com/cdancy/bitbucket/rest/features/BranchApi.java

+                                             @Nullable @QueryParam("start") Integer start,
+                                             @Nullable @QueryParam("limit") Integer limit);
+
+    @Named("branch:update-BranchPermission")


Change to branch:update-branch-permission

cdancy · 2017-03-08T21:15:59Z

src/main/java/com/cdancy/bitbucket/rest/features/BranchApi.java

+                                   @PathParam("repo") String repo,
+                                   @BinderParam(BindToJsonPayload.class) List<BranchPermission> listBranchPermission);
+
+    @Named("branch:get-BranchPermission")


Change to branch:delete-branch-permission

cdancy · 2017-03-08T21:16:44Z

src/main/java/com/cdancy/bitbucket/rest/features/BranchApi.java

+    @Named("branch:update-BranchPermission")
+    @Documentation({"https://developer.atlassian.com/static/rest/bitbucket-server/4.14.1/bitbucket-ref-restriction-rest.html#idm45354011023456"})
+    @Path("/branch-permissions/2.0/projects/{project}/repos/{repo}/restrictions")
+    @Produces("application/vnd.atl.bitbucket.bulk+json")


Any particular reason to need to override the default json?

Yes because Bitbucket need for update. If, I don't put it, bitbucket return this error : "Can not deserialize instance of com.atlassian.stash.internal.repository.ref.restriction.rest.RestRestrictionRequest out of START_ARRAY token\n at [Source: com.atlassian.stash.internal.web.util.web.CountingServletInputStream@68f4c6d2; line: 1, column: 1]""

cdancy · 2017-03-08T21:17:22Z

src/main/java/com/cdancy/bitbucket/rest/features/BranchApi.java

+    @DELETE
+    boolean deleteBranchPermission(@PathParam("project") String project,
+                                   @PathParam("repo") String repo,
+                                   @PathParam("id") Long id);


Since id is not optional lets use long instead of boxed version.

cdancy · 2017-03-08T21:19:22Z

@j0nathan33 thanks for PR! Added some intial comments to address. Looks Ok so far.

FYI: I would need mock and integration tests prior to merging. If you need help writing these feel free to scream my way ;)

j0nathan33 · 2017-03-08T22:24:38Z

Ok, I will try to add some mock and integration tests :).

cdancy · 2017-03-09T16:48:25Z

src/main/java/com/cdancy/bitbucket/rest/domain/branch/BranchPermissionEnumType.java

+                return enumType;
+            }
+        }
+        throw new IllegalArgumentException("Value not Found");


Can we do something more descriptive? Maybe "Value " + apiName + " is not a legal BranchPermission"

ok, I change it

cdancy · 2017-03-09T22:18:36Z

src/main/java/com/cdancy/bitbucket/rest/features/BranchApi.java

+    @Named("branch:update-branch-permission")
+    @Documentation({"https://developer.atlassian.com/static/rest/bitbucket-server/4.14.1/bitbucket-ref-restriction-rest.html#idm45354011023456"})
+    @Path("/branch-permissions/2.0/projects/{project}/repos/{repo}/restrictions")
+    @Produces("application/vnd.atl.bitbucket.bulk+json")


Is this content-type required? Will the already defined @Produces(MediaType.APPLICATION_JSON) not work?

Looking at the documentation appears so. Ok lets keep it.

cdancy · 2017-03-09T22:28:27Z

@j0nathan33 looks like you left out accessKeys both to and from Bitbucket?

https://developer.atlassian.com/static/rest/bitbucket-server/4.14.1/bitbucket-ref-restriction-rest.html#idm45354011023456

j0nathan33 · 2017-03-10T13:44:00Z

Right now, it's not possible to add new attribute "accessKeys", because my bitbucket server have old version but I will try to add It

cdancy · 2017-03-10T14:03:00Z

@j0nathan33 what version of bitbucket are you on?

cdancy · 2017-03-10T14:03:20Z

Response looks like this:

{
    "id": 2,
    "type": "read-only",
    "matcher": {
        "id": "release*",
        "displayId": "release*",
        "type": {
            "id": "PATTERN",
            "name": "Pattern"
        },
        "active": true
    },
    "users": [
        {
            "name": "jcitizen",
            "emailAddress": "jane@example.com",
            "id": 101,
            "displayName": "Jane Citizen",
            "active": true,
            "slug": "jcitizen",
            "type": "NORMAL"
        }
    ],
    "groups": [
        "bitbucket-users"
    ],
    "accessKeys": [
        {
            "key": {
                "id": 1,
                "text": "ssh-rsa AAAAAA... me@atlassian.com",
                "label": "me@atlassian.com"
            }
        }
    ]
}

cdancy · 2017-03-10T14:04:01Z

While request looks like this:

{
    "type": "read-only",
    "matcher": {
        "id": "refs/tags/**",
        "displayId": "refs/tags/**",
        "type": {
            "id": "PATTERN",
            "name": "Pattern"
        },
        "active": true
    },
    "users": [
        "bman",
        "tstark",
        "hulk"
    ],
    "groups": [
        "bitbucket-users"
    ],
    "accessKeys": [
        1,
        2,
        3
    ]
}

cdancy · 2017-03-10T14:04:29Z

They look optional in both cases so to speak.

cdancy · 2017-03-13T13:52:08Z

@j0nathan33 integration tests pass as expected. Pulling this in as I'd like to get a release out today. As the endpoints don't require accessKeys to be present on request or response we can circle back at some later point and add them.

Thanks for the PR @j0nathan33!

j0nathan33 added 3 commits March 8, 2017 15:06

Add new api call for crud of BranchPermission

d5a49aa

Handle exception for branchPermission

f94a1b4

Fix build

f0ef0c8

cdancy self-assigned this Mar 8, 2017

cdancy added the enhancement label Mar 8, 2017

cdancy added this to the v0.0.14 milestone Mar 8, 2017

cdancy reviewed Mar 8, 2017

View reviewed changes

Edit code after codeReview

9d04a23

Add mock and integration test

6477d18

cdancy reviewed Mar 9, 2017

View reviewed changes

Jonathan Doire added 2 commits March 9, 2017 13:27

Edit error message after codeReview

0e29f43

Edit code after codeReview

15b504d

cdancy reviewed Mar 9, 2017

View reviewed changes

Merge branch 'master' into branch-branchPermission

7879388

cdancy merged commit 62af03e into cdancy:master Mar 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new api call for BranchPermission #51

Add new api call for BranchPermission #51

j0nathan33 commented Mar 8, 2017

cdancy Mar 8, 2017

cdancy Mar 8, 2017

j0nathan33 Mar 8, 2017

cdancy Mar 8, 2017

cdancy Mar 8, 2017

j0nathan33 Mar 8, 2017

cdancy Mar 8, 2017

cdancy Mar 8, 2017

cdancy Mar 8, 2017

cdancy Mar 8, 2017

cdancy Mar 8, 2017

cdancy Mar 8, 2017

j0nathan33 Mar 8, 2017

cdancy Mar 8, 2017

cdancy commented Mar 8, 2017

j0nathan33 commented Mar 8, 2017

cdancy Mar 9, 2017

j0nathan33 Mar 9, 2017

cdancy Mar 9, 2017

cdancy Mar 9, 2017

cdancy commented Mar 9, 2017

j0nathan33 commented Mar 10, 2017

cdancy commented Mar 10, 2017

cdancy commented Mar 10, 2017 •

edited

Loading

cdancy commented Mar 10, 2017

cdancy commented Mar 10, 2017

cdancy commented Mar 13, 2017

		develop("development", "Development", "MODEL_BRANCH", "Branching model branch"),
		master("production", "Production", "MODEL_BRANCH", "Branching model branch");

Add new api call for BranchPermission #51

Add new api call for BranchPermission #51

Conversation

j0nathan33 commented Mar 8, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdancy commented Mar 8, 2017

j0nathan33 commented Mar 8, 2017

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

cdancy commented Mar 9, 2017

j0nathan33 commented Mar 10, 2017

cdancy commented Mar 10, 2017

cdancy commented Mar 10, 2017 • edited Loading

cdancy commented Mar 10, 2017

cdancy commented Mar 10, 2017

cdancy commented Mar 13, 2017

cdancy commented Mar 10, 2017 •

edited

Loading