Do not build accepted merge requests unless specified so in the confi…

…guration. The combination of state and action upon approval is "updated" and "approved". Therefore such combination cannot be excluded by using logical or. Therefore the meaning of the arguments to MergeRequestHookTriggerHandlerImpl changed, and tests were adjusted properly.
jenkinsci · Mar 13, 2018 · cf2e15d · cf2e15d
1 parent 80c3786
commit cf2e15d
Show file tree

Hide file tree

Showing 5 changed files with 107 additions and 6 deletions.
diff --git a/...dabsquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerFactory.java b/...dabsquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerFactory.java
@@ -32,7 +32,7 @@ public static MergeRequestHookTriggerHandler newMergeRequestHookTriggerHandler(b
     }
 
 	private static Set<Action> retrieveAllowedActions(boolean triggerOnApprovedMergeRequest) {
-		Set<Action> allowedActions = EnumSet.noneOf(Action.class);
+		Set<Action> allowedActions = EnumSet.of(Action.open, Action.update);
 		if (triggerOnApprovedMergeRequest)
 			allowedActions.add(Action.approved);
 		return allowedActions;

diff --git a/...om/dabsquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerImpl.java b/...om/dabsquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerImpl.java
@@ -38,7 +38,7 @@ class MergeRequestHookTriggerHandlerImpl extends AbstractWebHookTriggerHandler<M
 	private final Collection<Action> allowedActions;
 
     MergeRequestHookTriggerHandlerImpl(Collection<State> allowedStates,  boolean skipWorkInProgressMergeRequest) {
-    	this(allowedStates, EnumSet.noneOf(Action.class),skipWorkInProgressMergeRequest);
+    	this(allowedStates, EnumSet.allOf(Action.class),skipWorkInProgressMergeRequest);
     }
 
     MergeRequestHookTriggerHandlerImpl(Collection<State> allowedStates, Collection<Action> allowedActions, boolean skipWorkInProgressMergeRequest) {
@@ -159,7 +159,7 @@ private String getTargetBranchFromBuild(Run<?, ?> mergeBuild) {
 
 	private boolean isAllowedByConfig(MergeRequestObjectAttributes objectAttributes) {
 		return allowedStates.contains(objectAttributes.getState()) 
-        	|| allowedActions.contains(objectAttributes.getAction());
+        	&& allowedActions.contains(objectAttributes.getAction());
 	}    
 
     private boolean isNotSkipWorkInProgressMergeRequest(MergeRequestObjectAttributes objectAttributes) {

diff --git a/...absquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerImplTest.java b/...absquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerImplTest.java
@@ -121,11 +121,19 @@ public void mergeRequest_do_not_build_when_closed() throws IOException, Interrup
 
     @Test
     public void mergeRequest_build_when_approved() throws IOException, InterruptedException, GitAPIException, ExecutionException {
-        MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = new MergeRequestHookTriggerHandlerImpl(EnumSet.noneOf(State.class), EnumSet.of(Action.approved), false);
+        MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = new MergeRequestHookTriggerHandlerImpl(EnumSet.allOf(State.class), EnumSet.of(Action.approved), false);
         OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, Action.approved);
 
         assertThat(buildTriggered.isSignaled(), is(true));
     }
+
+    @Test
+    public void mergeRequest_do_not_build_when_when_approved() throws Exception {
+        MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = new MergeRequestHookTriggerHandlerImpl(EnumSet.allOf(State.class), EnumSet.of(Action.update), false);
+        OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, defaultMergeRequestObjectAttributes().withState(State.opened).withAction(Action.approved));
+
+        assertThat(buildTriggered.isSignaled(), is (false));
+    }
 
     @Test
     public void mergeRequest_build_only_when_approved_and_not_when_opened() throws IOException, InterruptedException, GitAPIException, ExecutionException {
@@ -140,14 +148,14 @@ public void mergeRequest_build_only_when_approved_and_not_when_updated() throws
 
 	private void mergeRequest_build_only_when_approved(Action action)
 			throws GitAPIException, IOException, InterruptedException {
-		MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = new MergeRequestHookTriggerHandlerImpl(EnumSet.noneOf(State.class), EnumSet.of(Action.approved), false);        
+		MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = new MergeRequestHookTriggerHandlerImpl(EnumSet.allOf(State.class), EnumSet.of(Action.approved), false);
 	    OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, action);
 
 	    assertThat(buildTriggered.isSignaled(), is(false));
 	}    
 
     private OneShotEvent doHandle(MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler, Action action) throws GitAPIException, IOException, InterruptedException {
-        return doHandle(mergeRequestHookTriggerHandler, defaultMergeRequestObjectAttributes().withState(State.opened).withAction(action));
+        return doHandle(mergeRequestHookTriggerHandler, defaultMergeRequestObjectAttributes().withAction(action));
     }
 
     private OneShotEvent doHandle(MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler, State state) throws GitAPIException, IOException, InterruptedException {
@@ -196,6 +204,8 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
 	private MergeRequestObjectAttributesBuilder defaultMergeRequestObjectAttributes() {
 		return mergeRequestObjectAttributes()
 		    .withIid(1)
+            .withAction(Action.update)
+            .withState(State.opened)
 		    .withTitle("test")
 		    .withTargetProjectId(1)
 		    .withSourceProjectId(1)

diff --git a/src/test/java/com/dabsquared/gitlabjenkins/webhook/build/MergeRequestBuildActionTest.java b/src/test/java/com/dabsquared/gitlabjenkins/webhook/build/MergeRequestBuildActionTest.java
@@ -100,6 +100,20 @@ public void skip_closedMR() throws IOException {
         verify(trigger, never()).onPost(any(MergeRequestHook.class));
     }
 
+    @Test
+    public void skip_approvedMR() throws IOException, ExecutionException, InterruptedException {
+        FreeStyleProject testProject = jenkins.createFreeStyleProject();
+        testProject.addTrigger(trigger);
+        testProject.setScm(new GitSCM(gitRepoUrl));
+        QueueTaskFuture<?> future = testProject.scheduleBuild2(0, new ParametersAction(new StringParameterValue("gitlabTargetBranch", "master")));
+        future.get();
+
+        exception.expect(HttpResponses.HttpResponseException.class);
+        new MergeRequestBuildAction(testProject, getJson("MergeRequestEvent_approvedMR.json"), null).execute(response);
+
+        verify(trigger, never()).onPost(any(MergeRequestHook.class));
+    }
+
     @Test
     public void skip_alreadyBuiltMR() throws IOException, ExecutionException, InterruptedException {
         FreeStyleProject testProject = jenkins.createFreeStyleProject();

diff --git a/...st/resources/com/dabsquared/gitlabjenkins/webhook/build/MergeRequestEvent_approvedMR.json b/...st/resources/com/dabsquared/gitlabjenkins/webhook/build/MergeRequestEvent_approvedMR.json
@@ -0,0 +1,77 @@
+{
+  "object_kind": "merge_request",
+  "user": {
+    "name": "Administrator",
+    "username": "root",
+    "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon"
+  },
+  "object_attributes": {
+    "id": 99,
+    "target_branch": "master",
+    "source_branch": "ms-viewport",
+    "source_project_id": 14,
+    "author_id": 51,
+    "assignee_id": 6,
+    "title": "MS-Viewport",
+    "created_at": "2013-12-03T17:23:34.123Z",
+    "updated_at": "2013-12-03T17:23:34.123Z",
+    "st_commits": null,
+    "st_diffs": null,
+    "milestone_id": null,
+    "state": "opened",
+    "merge_status": "unchecked",
+    "target_project_id": 14,
+    "iid": 1,
+    "description": "",
+    "source": {
+      "name": "Awesome Project",
+      "description": "Aut reprehenderit ut est.",
+      "web_url": "http://example.com/awesome_space/awesome_project",
+      "avatar_url": null,
+      "git_ssh_url": "git@example.com:awesome_space/awesome_project.git",
+      "git_http_url": "http://example.com/awesome_space/awesome_project.git",
+      "namespace": "Awesome Space",
+      "visibility_level": 20,
+      "path_with_namespace": "awesome_space/awesome_project",
+      "default_branch": "master",
+      "homepage": "http://example.com/awesome_space/awesome_project",
+      "url": "http://example.com/awesome_space/awesome_project.git",
+      "ssh_url": "git@example.com:awesome_space/awesome_project.git",
+      "http_url": "http://example.com/awesome_space/awesome_project.git"
+    },
+    "target": {
+      "name": "Awesome Project",
+      "description": "Aut reprehenderit ut est.",
+      "web_url": "http://example.com/awesome_space/awesome_project",
+      "avatar_url": null,
+      "git_ssh_url": "git@example.com:awesome_space/awesome_project.git",
+      "git_http_url": "http://example.com/awesome_space/awesome_project.git",
+      "namespace": "Awesome Space",
+      "visibility_level": 20,
+      "path_with_namespace": "awesome_space/awesome_project",
+      "default_branch": "master",
+      "homepage": "http://example.com/awesome_space/awesome_project",
+      "url": "http://example.com/awesome_space/awesome_project.git",
+      "ssh_url": "git@example.com:awesome_space/awesome_project.git",
+      "http_url": "http://example.com/awesome_space/awesome_project.git"
+    },
+    "last_commit": {
+      "id": "${commitSha1}",
+      "message": "fixed readme",
+      "timestamp": "2012-01-03T23:36:29+02:00",
+      "url": "http://example.com/awesome_space/awesome_project/commits/da1560886d4f094c3e6c9ef40349f7d38b5d27d7",
+      "author": {
+        "name": "GitLab dev user",
+        "email": "gitlabdev@dv6700.(none)"
+      }
+    },
+    "work_in_progress": false,
+    "url": "http://example.com/diaspora/merge_requests/1",
+    "action": "approved",
+    "assignee": {
+      "name": "User1",
+      "username": "user1",
+      "avatar_url": "http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=40\u0026d=identicon"
+    }
+  }
+}