[JENKINS-60738] Fix global configuration submission from UI

jenkinsci · May 2, 2023 · cfaa0c4 · cfaa0c4
1 parent 80dd71b
commit cfaa0c4
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 24 deletions.
diff --git a/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java b/src/main/java/org/jenkinsci/plugins/github/config/GitHubPluginConfig.java
@@ -4,6 +4,7 @@
 import com.google.common.base.Function;
 import com.google.common.base.Predicate;
 import com.google.common.base.Predicates;
+import hudson.BulkChange;
 import hudson.Extension;
 import hudson.Util;
 import hudson.XmlFile;
@@ -190,17 +191,36 @@ protected XmlFile getConfigFile() {
 
     @Override
     public boolean configure(StaplerRequest req, JSONObject json) throws FormException {
-        hookSecretConfigs = null; // form binding might omit empty lists
         try {
-            req.bindJSON(this, json);
+            BulkChange bc = new BulkChange(this);
+            try {
+                if (json.has("configs")) {
+                    setConfigs(req.bindJSONToList(GitHubServerConfig.class, json.getJSONObject("configs")));
+                } else {
+                    setConfigs(Collections.emptyList());
+                }
+                if (json.has("hookSecretConfigs")) {
+                    setHookSecretConfigs(req.bindJSONToList(HookSecretConfig.class,
+                        json.getJSONObject("hookSecretConfigs")));
+                } else {
+                    setHookSecretConfigs(Collections.emptyList());
+                }
+                if (json.optBoolean("isOverrideHookUrl", false) && (json.has("hookUrl"))) {
+                    setHookUrl(json.optString("hookUrl"));
+                } else {
+                    setHookUrl(null);
+                }
+                req.bindJSON(this, json);
+                clearRedundantCaches(configs);
+            } finally {
+                bc.commit();
+            }
         } catch (Exception e) {
             LOGGER.debug("Problem while submitting form for GitHub Plugin ({})", e.getMessage(), e);
             LOGGER.trace("GH form data: {}", json.toString());
             throw new FormException(
                     format("Malformed GitHub Plugin configuration (%s)", e.getMessage()), e, "github-configuration");
         }
-        save();
-        clearRedundantCaches(configs);
         return true;
     }
 

diff --git a/src/main/resources/org/jenkinsci/plugins/github/config/GitHubPluginConfig/config.groovy b/src/main/resources/org/jenkinsci/plugins/github/config/GitHubPluginConfig/config.groovy
@@ -27,10 +27,11 @@ f.section(title: descriptor.displayName) {
             f.entry(title: _("Override Hook URL")) {
                 g.blockWrapper {
                     f.optionalBlock(title: _("Specify another hook URL for GitHub configuration"),
+                            name: "isOverrideHookUrl",
                             inline: true,
                             checked: instance.isOverrideHookUrl()) {
                         f.entry(field: "hookUrl") {
-                            f.textbox(checkMethod: "post")
+                            f.textbox(checkMethod: "post", name: "hookUrl")
                         }
                     }
                 }

diff --git a/src/test/java/com/cloudbees/jenkins/GlobalConfigSubmitTest.java b/src/test/java/com/cloudbees/jenkins/GlobalConfigSubmitTest.java
@@ -3,7 +3,6 @@
 import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 import org.jenkinsci.plugins.github.GitHubPlugin;
-import org.junit.Ignore;
 import org.junit.Rule;
 import org.junit.Test;
 import org.jvnet.hudson.test.JenkinsRule;
@@ -14,17 +13,18 @@
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.startsWith;
 
 /**
  * Test Class for {@link GitHubPushTrigger}.
  *
  * @author Seiji Sogabe
  */
-@Ignore("Have troubles with memory consumption")
+//@Ignore("Have troubles with memory consumption")
 public class GlobalConfigSubmitTest {
 
-    public static final String OVERRIDE_HOOK_URL_CHECKBOX = "_.isOverrideHookUrl";
-    public static final String HOOK_URL_INPUT = "_.hookUrl";
+    public static final String OVERRIDE_HOOK_URL_CHECKBOX = "isOverrideHookUrl";
+    public static final String HOOK_URL_INPUT = "hookUrl";
 
     private static final String WEBHOOK_URL = "http://jenkinsci.example.com/jenkins/github-webhook/";
 
@@ -43,7 +43,7 @@ public void shouldSetHookUrl() throws Exception {
     }
 
     @Test
-    public void shouldNotSetHookUrl() throws Exception {
+    public void shouldResetHookUrlIfNotChecked() throws Exception {
         GitHubPlugin.configuration().setHookUrl(WEBHOOK_URL);
 
         HtmlForm form = globalConfig();
@@ -52,20 +52,7 @@ public void shouldNotSetHookUrl() throws Exception {
         form.getInputByName(HOOK_URL_INPUT).setValueAttribute("http://foo");
         jenkins.submit(form);
 
-        assertThat(GitHubPlugin.configuration().getHookUrl(), equalTo(new URL(WEBHOOK_URL)));
-    }
-
-    @Test
-    public void shouldNotOverrideAPreviousHookUrlIfNotChecked() throws Exception {
-        GitHubPlugin.configuration().setHookUrl(WEBHOOK_URL);
-
-        HtmlForm form = globalConfig();
-
-        form.getInputByName(OVERRIDE_HOOK_URL_CHECKBOX).setChecked(false);
-        form.getInputByName(HOOK_URL_INPUT).setValueAttribute("");
-        jenkins.submit(form);
-
-        assertThat(GitHubPlugin.configuration().getHookUrl(), equalTo(new URL(WEBHOOK_URL)));
+        assertThat(GitHubPlugin.configuration().getHookUrl().toString(), startsWith(jenkins.jenkins.getRootUrl()));
     }
 
     public HtmlForm globalConfig() throws IOException, SAXException {