Fix/merge request hooks behavior (#1319)

* Rename Action closed to close: State - closed, Action - close

* Fix logic for close and closed MR

* Fix logic for update action into MR and flag never

we set it on 1st place because the more important type of hook than flag approved or not.
Also update action often came with opened state.

* Update tests

* Fix merge handler tests with never rebuild feature

Signed-off-by: Brady Pratt <bpratt@redhat.com>

---------

Signed-off-by: Brady Pratt <bpratt@redhat.com>
Co-authored-by: Yaraslau Supanitski <yaraslau_supanitski@epam.com>
Co-authored-by: Bruno Verachten <gounthar@gmail.com>
Co-authored-by: Kris Stern <krisstern@outlook.com>

Author: 4 people

src/main/java/com/dabsquared/gitlabjenkins/gitlab/hook/model/Action.java

@@ -4,5 +4,5 @@
  * @author Robin Müller
  */
 public enum Action {
-    open, update, approved, merge, closed, reopen
+    open, update, approved, merge, close, reopen
 }

src/main/java/com/dabsquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerFactory.java

@@ -34,11 +34,12 @@ public static MergeRequestHookTriggerHandler newMergeRequestHookTriggerHandler(b
 
         TriggerConfigChain chain = new TriggerConfigChain();
         chain
+            .acceptOnlyIf(triggerOpenMergeRequest != TriggerOpenMergeRequest.never, of(State.opened, State.updated), of(Action.update))
             .acceptOnlyIf(triggerOnApprovedMergeRequest, null, of(Action.approved))
             .acceptIf(triggerOnMergeRequest, of(State.opened, State.reopened), null)
             .acceptIf(triggerOnAcceptedMergeRequest, null, of(Action.merge))
-            .acceptIf(triggerOnClosedMergeRequest, null, of(Action.closed))
-            .acceptIf(triggerOpenMergeRequest != TriggerOpenMergeRequest.never, of(State.updated), null)
+            .acceptIf(triggerOnClosedMergeRequest, null, of(Action.close))
+            .acceptIf(triggerOnClosedMergeRequest, of(State.closed), null)
         ;
 
         Set<String> labelsThatForcesBuildIfAddedSet = Stream.of(split(trimToEmpty(labelsThatForcesBuildIfAdded), ",")).collect(toSet());

src/test/java/com/dabsquared/gitlabjenkins/trigger/handler/merge/MergeRequestHookTriggerHandlerImplTest.java

@@ -80,13 +80,35 @@ public void mergeRequest_ciSkip() throws IOException, InterruptedException {
     }
 
     @Test
-    public void mergeRequest_build_when_opened() throws IOException, InterruptedException, GitAPIException, ExecutionException {
-        MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = withConfig().build();
+    public void mergeRequest_build_when_opened_with_source() throws IOException, InterruptedException, GitAPIException, ExecutionException {
+        MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = withConfig()
+            .setTriggerOpenMergeRequest(TriggerOpenMergeRequest.source)
+            .build();
+        OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, State.opened);
+
+        assertThat(buildTriggered.isSignaled(), is(true));
+    }
+
+    @Test
+    public void mergeRequest_build_when_opened_with_both() throws IOException, InterruptedException, GitAPIException, ExecutionException {
+        MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = withConfig()
+            .setTriggerOpenMergeRequest(TriggerOpenMergeRequest.source)
+            .build();
         OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, State.opened);
 
         assertThat(buildTriggered.isSignaled(), is(true));
     }
 
+    @Test
+    public void mergeRequest_build_when_opened_with_never() throws IOException, InterruptedException, GitAPIException, ExecutionException {
+        MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = withConfig()
+            .setTriggerOpenMergeRequest(TriggerOpenMergeRequest.never)
+            .build();
+        OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, State.opened, Action.update);
+
+        assertThat(buildTriggered.isSignaled(), is(false));
+    }
+
     @Test
     public void mergeRequest_build_when_reopened() throws IOException, InterruptedException, GitAPIException, ExecutionException {
         MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = withConfig()
@@ -100,6 +122,7 @@ public void mergeRequest_build_when_reopened() throws IOException, InterruptedEx
     public void mergeRequest_build_when_opened_with_approved_action_enabled() throws IOException, InterruptedException, GitAPIException, ExecutionException {
         MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = withConfig()
             .setTriggerOnApprovedMergeRequest(true)
+            .setTriggerOpenMergeRequest(TriggerOpenMergeRequest.source)
             .build();
         OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, State.opened);
 
@@ -133,7 +156,17 @@ public void mergeRequest_build_when_closed() throws IOException, InterruptedExce
         MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = withConfig()
             .setTriggerOnClosedMergeRequest(true)
             .build();
-        OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, State.closed, Action.closed);
+        OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, State.closed, Action.close);
+
+        assertThat(buildTriggered.isSignaled(), is(true));
+    }
+
+    @Test
+    public void mergeRequest_build_when_close() throws IOException, InterruptedException, GitAPIException, ExecutionException {
+        MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = withConfig()
+            .setTriggerOnClosedMergeRequest(true)
+            .build();
+        OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, Action.close);
 
         assertThat(buildTriggered.isSignaled(), is(true));
     }
@@ -144,7 +177,7 @@ public void mergeRequest_build_when_closed_with_actions_enabled() throws IOExcep
             .setTriggerOnClosedMergeRequest(true)
             .setTriggerOnApprovedMergeRequest(true)
             .build();
-        OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, State.closed, Action.closed);
+        OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, State.closed, Action.close);
 
         assertThat(buildTriggered.isSignaled(), is(true));
     }
@@ -205,7 +238,6 @@ public void mergeRequest_do_not_build_when_closed() throws IOException, Interrup
     @Test
     public void mergeRequest_do_not_build_for_updated_state_and_approved_action_when_both_not_enabled() throws IOException, InterruptedException, GitAPIException {
         MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = withConfig()
-            .setTriggerOpenMergeRequest(TriggerOpenMergeRequest.source)
             .build();
         OneShotEvent buildTriggered = doHandle(mergeRequestHookTriggerHandler, State.updated, Action.approved);
 
@@ -336,7 +368,7 @@ public void mergeRequest_build_only_when_state_modified()throws IOException, Int
         MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = withConfig()
             .setTriggerOnAcceptedMergeRequest(true)
             .setTriggerOnClosedMergeRequest(true)
-            .setTriggerOpenMergeRequest(TriggerOpenMergeRequest.never)
+            .setTriggerOpenMergeRequest(TriggerOpenMergeRequest.source)
             .build();
         Git.init().setDirectory(tmp.getRoot()).call();
         tmp.newFile("test");
@@ -483,7 +515,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
 	private MergeRequestObjectAttributesBuilder defaultMergeRequestObjectAttributes() {
 		return mergeRequestObjectAttributes()
 		    .withIid(1)
-            .withAction(Action.update)
+            .withAction(Action.open)
             .withState(State.opened)
 		    .withTitle("test")
 		    .withTargetProjectId(1)