#1427: Prevent job triggering for non-matching webhooks (#1720)

8ec69c7 introduced a change that does
not handle jobs created using Jenkins DSL properly, as DSL jobs that do
not specify `triggerOpenMergeRequestOnPush` have the
`triggerOpenMergeRequest` value passed into
`MergeRequestHookTriggerHandlerFactory` as `null`, with the above
change causing the value not to be handled as `never` where the plugin
previously treated `null` as `never`. This results in webhooks
triggering builds for DSL-created jobs even where the job is doing
actions that aren't valid for a Merge Request in that state (e.g.
triggering release builds even though the Merge Request hasn't been
merged). This change returns to handling an unspecified
`triggerOpenMergeRequest` value as being equivalent to specifying it as
`never`.

Co-authored-by: Kris Stern <[email protected]>

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

@@ -31,10 +31,16 @@ public static MergeRequestHookTriggerHandler newMergeRequestHookTriggerHandler(
             boolean cancelPendingBuildsOnUpdate) {
 
         TriggerConfigChain chain = new TriggerConfigChain();
-        chain.acceptOnlyIf(
+        chain.rejectUnless(
                         triggerOpenMergeRequest != TriggerOpenMergeRequest.never,
                         of(State.opened, State.updated),
                         of(Action.update))
+                .acceptIf(
+                        triggerOpenMergeRequest != TriggerOpenMergeRequest.never, of(State.updated), of(Action.update))
+                .acceptIf(
+                        triggerOpenMergeRequest != TriggerOpenMergeRequest.never && triggerOpenMergeRequest != null,
+                        of(State.opened),
+                        of(Action.update))
                 .acceptOnlyIf(triggerOnApprovedMergeRequest, null, of(Action.approved))
                 .acceptIf(triggerOnMergeRequest, of(State.opened, State.reopened), null)
                 .acceptIf(triggerOnAcceptedMergeRequest, null, of(Action.merge))

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

@@ -12,6 +12,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.assertNull;
 
+import com.dabsquared.gitlabjenkins.GitLabPushTrigger;
 import com.dabsquared.gitlabjenkins.gitlab.hook.model.Action;
 import com.dabsquared.gitlabjenkins.gitlab.hook.model.State;
 import com.dabsquared.gitlabjenkins.gitlab.hook.model.builder.generated.MergeRequestObjectAttributesBuilder;
@@ -501,6 +502,26 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
         jenkins.assertBuildStatusSuccess(jenkins.waitForCompletion(buildHolder.get()));
     }
 
+    @Test
+    public void
+            mergeRequest_skips_build_when_not_push_and_not_merge_request_and_accepted_and_trigger_open_merge_request_unspecified()
+                    throws Exception {
+        GitLabPushTrigger gitLabPushTrigger = new GitLabPushTrigger();
+        gitLabPushTrigger.setTriggerOnPush(false);
+        gitLabPushTrigger.setTriggerOnMergeRequest(false);
+        gitLabPushTrigger.setTriggerOnAcceptedMergeRequest(true);
+        gitLabPushTrigger.setBranchFilterType(BranchFilterType.All);
+
+        MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler =
+                MergeRequestHookTriggerHandlerFactory.newMergeRequestHookTriggerHandler(gitLabPushTrigger);
+        final AtomicReference<FreeStyleBuild> buildHolder = new AtomicReference<>();
+        OneShotEvent buildTriggered =
+                doHandle(mergeRequestHookTriggerHandler, State.opened, Action.update, buildHolder);
+
+        assertThat(buildTriggered.isSignaled(), is(false));
+        assertNull(buildHolder.get());
+    }
+
     private void do_not_build_for_state_when_nothing_enabled(State state) throws Exception {
         MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler =
                 withConfig().setTriggerOnMergeRequest(false).build();