From 577d6e063b53d9a67fb74a07b8a36d89861a1379 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arnaud=20He=CC=81ritier?= Date: Fri, 11 Oct 2019 23:51:56 +0200 Subject: [PATCH] [JENKINS-59697] Better manage the form validation And make it more beautiful with Bootstrap --- .../advisor/AdvisorGlobalConfiguration.java | 37 ++++---- .../jenkins/plugins/advisor/BundleUpload.java | 49 +++++++--- .../advisor/casc/AdvisorRootConfigurator.java | 4 +- .../AdvisorGlobalConfiguration/index.jelly | 83 +++++++++++++--- .../lib/advisor/validateOnLoad.jelly | 95 ++++++++----------- .../plugins/advisor/BundleUploadTest.java | 3 +- 6 files changed, 169 insertions(+), 102 deletions(-) diff --git a/src/main/java/com/cloudbees/jenkins/plugins/advisor/AdvisorGlobalConfiguration.java b/src/main/java/com/cloudbees/jenkins/plugins/advisor/AdvisorGlobalConfiguration.java index 0e5a124..c4214b3 100644 --- a/src/main/java/com/cloudbees/jenkins/plugins/advisor/AdvisorGlobalConfiguration.java +++ b/src/main/java/com/cloudbees/jenkins/plugins/advisor/AdvisorGlobalConfiguration.java @@ -320,6 +320,13 @@ public String getDisplayName() { return Messages.Insights_DisplayName(); } + public FormValidation doCheckAcceptToS(@QueryParameter boolean value) { + if (!value) { + return FormValidation.error("Accepting our Terms and Conditions is mandatory to use this service."); + } + return FormValidation.ok(); + } + public FormValidation doCheckEmail(@QueryParameter String value) { String emailAddress = EmailUtil.fixEmptyAndTrimAllSpaces(value); @@ -382,14 +389,13 @@ public FormValidation doTestConnection(@QueryParameter("email") final String ema } // Used from validateOnLoad.jelly - public String connectionTest(String email) { + public String validateServerConnection() { AdvisorGlobalConfiguration config = AdvisorGlobalConfiguration.getInstance(); - if (!config.isAcceptToS()) { - return "tos-not-accepted"; + if(!config.isValid()){ + return "invalid-configuration"; } - try { - AdvisorClient advisorClient = new AdvisorClient(new Recipient(email)); + AdvisorClient advisorClient = new AdvisorClient(new Recipient(config.email)); advisorClient.doCheckHealth(); return "service-operational"; } catch (Exception e) { @@ -411,7 +417,7 @@ public FormValidation doTestSendEmail(@QueryParameter("email") final String emai AdvisorClient advisorClient = new AdvisorClient(new Recipient(email.trim())); advisorClient.doTestEmail(); - return FormValidation.ok("Sending email. Please check your inbox and filters."); + return FormValidation.ok("A request to send a test email from the server was done. Please check your inbox and filters."); } catch (Exception e) { return FormValidation.error("Client error : " + e.getMessage()); } @@ -419,15 +425,10 @@ public FormValidation doTestSendEmail(@QueryParameter("email") final String emai @Override public boolean configure(StaplerRequest req, JSONObject json) { + boolean acceptToS = json.getBoolean("acceptToS"); String email = json.getString("email"); String cc = json.getString("cc"); JSONObject advanced = json.getJSONObject("advanced"); - boolean acceptToS = json.getBoolean("acceptToS"); - - // Have to accept the Terms of Service to have a valid configuration - if (!acceptToS) { - return false; - } Set remove = new HashSet<>(); for (SupportAction.Selection s : req.bindJSONToList(SupportAction.Selection.class, advanced.get("components"))) { @@ -447,15 +448,17 @@ public boolean configure(StaplerRequest req, JSONObject json) { } try { - if (doCheckEmail(email).kind.equals(FormValidation.Kind.ERROR) || - doCheckCc(cc).kind.equals(FormValidation.Kind.ERROR)) { - return false; - } + return validate(acceptToS, email, cc); } catch (Exception e) { LOG.severe("Unexpected error while validating form: " + Functions.printThrowable(e)); return false; } - return true; + } + + public boolean validate(boolean acceptToS, String email, String cc) { + return !doCheckAcceptToS(acceptToS).kind.equals(FormValidation.Kind.ERROR) + && !doCheckEmail(email).kind.equals(FormValidation.Kind.ERROR) + && !doCheckCc(cc).kind.equals(FormValidation.Kind.ERROR); } } } diff --git a/src/main/java/com/cloudbees/jenkins/plugins/advisor/BundleUpload.java b/src/main/java/com/cloudbees/jenkins/plugins/advisor/BundleUpload.java index 35221d0..95e1e9c 100644 --- a/src/main/java/com/cloudbees/jenkins/plugins/advisor/BundleUpload.java +++ b/src/main/java/com/cloudbees/jenkins/plugins/advisor/BundleUpload.java @@ -17,7 +17,6 @@ import java.io.File; import java.io.FileOutputStream; -import java.text.SimpleDateFormat; import java.util.Calendar; import java.util.concurrent.TimeUnit; import java.util.logging.Level; @@ -38,6 +37,7 @@ public class BundleUpload extends AsyncPeriodicWork { private static final String COULD_NOT_SAVE_SUPPORT_BUNDLE = "ERROR: Could not save support bundle"; private static final String BUNDLE_DIR_DOES_NOT_EXIST = "Bundle root directory does not exist and could not be created"; + protected static final String BUNDLE_SUCCESSFULLY_UPLOADED = "Bundle uploaded"; private TaskListener task; public BundleUpload() { @@ -55,14 +55,16 @@ protected void execute(TaskListener listener) { if (!config.isPluginEnabled()) { log(Level.INFO, "Jenkins Health Advisor by CloudBees plugin disabled. Skipping bundle upload."); + updateLastBundleResult( + config, + createTimestampedWarnMessage("Jenkins Health Advisor by CloudBees plugin disabled. Skipping bundle upload.")); return; } if (!config.isValid()) { log(Level.INFO, "Invalid configuration. Skipping bundle upload."); - return; - } - if (!config.isAcceptToS()) { - log(Level.INFO, "Terms of conditions not accepted. Skipping bundle upload."); + updateLastBundleResult( + config, + createTimestampedWarnMessage("Invalid configuration. Skipping bundle upload.")); return; } @@ -72,7 +74,7 @@ protected void execute(TaskListener listener) { executeInternal(config.getEmail(), bundle, pluginVersion); } else { log(Level.SEVERE, UNABLE_TO_GENERATE_SUPPORT_BUNDLE); - config.setLastBundleResult(UNABLE_TO_GENERATE_SUPPORT_BUNDLE); + updateLastBundleResult(config, createTimestampedErrorMessage(UNABLE_TO_GENERATE_SUPPORT_BUNDLE)); } } @@ -82,7 +84,8 @@ private File generateBundle() { File bundleDir = SupportPlugin.getRootDirectory(); if (!bundleDir.exists() && !bundleDir.mkdirs()) { log(Level.SEVERE, String.format("%s %s", COULD_NOT_SAVE_SUPPORT_BUNDLE, BUNDLE_DIR_DOES_NOT_EXIST)); - config.setLastBundleResult(String.format("%s%n%s", COULD_NOT_SAVE_SUPPORT_BUNDLE, BUNDLE_DIR_DOES_NOT_EXIST)); + updateLastBundleResult(config, createTimestampedErrorMessage( + String.format("%s%n%s", COULD_NOT_SAVE_SUPPORT_BUNDLE, BUNDLE_DIR_DOES_NOT_EXIST))); return null; } @@ -93,7 +96,8 @@ private File generateBundle() { } } catch (Exception e) { logError(COULD_NOT_SAVE_SUPPORT_BUNDLE, e); - config.setLastBundleResult(String.format("%s%n%s", COULD_NOT_SAVE_SUPPORT_BUNDLE, e)); + updateLastBundleResult(config, + createTimestampedErrorMessage(String.format("%s%n%s", COULD_NOT_SAVE_SUPPORT_BUNDLE, e))); } return null; } @@ -106,17 +110,17 @@ private void executeInternal(String email, File file, String pluginVersion) { ClientResponse response = advisorClient .uploadFile(new ClientUploadRequest(Jenkins.get().getLegacyInstanceId(), file, config.getCc(), pluginVersion)); if (response.getCode() == 200) { - config.setLastBundleResult("Successfully uploaded a bundle at " + - new SimpleDateFormat("yyyy MM dd HH:mm:ss").format(Calendar.getInstance().getTime())); + updateLastBundleResult(config, createTimestampedMessage(BUNDLE_SUCCESSFULLY_UPLOADED)); } else { - config.setLastBundleResult("Bundle upload failed. Response code was: " + response.getCode() + ". " + - "Response message: " + response.getMessage()); + updateLastBundleResult(config, + createTimestampedErrorMessage("Bundle upload failed. Response code was: " + response.getCode() + ". " + + "Response message: " + response.getMessage())); } } catch (Exception e) { log(Level.SEVERE, "Issue while uploading file to bundle upload service: " + e.getMessage()); log(Level.FINEST, "Exception while uploading file to bundle upload service. Cause: " + ExceptionUtils.getStackTrace(e)); - config.setLastBundleResult("ERROR: Issue while uploading file to bundle upload service: " + e.getMessage()); + updateLastBundleResult(config, createTimestampedErrorMessage("Bundle upload failed: " + e.getMessage())); } } @@ -147,6 +151,25 @@ private void logError(String message, Throwable t) { LOG.log(Level.SEVERE, message, t); } + private String createTimestampedWarnMessage(String message) { + return "[WARN] " + createTimestampedMessage(message); + } + + private String createTimestampedErrorMessage(String message) { + return "[ERROR] " + createTimestampedMessage(message); + } + + private String createTimestampedMessage(String message) { + return String.format("%1$tF %1$tT - %2$s", + Calendar.getInstance().getTime(), + message); + } + + private void updateLastBundleResult(AdvisorGlobalConfiguration config, String message) { + config.setLastBundleResult(message); + config.save(); + } + /** * By default we wait a few minutes to allow support-core plugin time to generate a bundle first. * diff --git a/src/main/java/com/cloudbees/jenkins/plugins/advisor/casc/AdvisorRootConfigurator.java b/src/main/java/com/cloudbees/jenkins/plugins/advisor/casc/AdvisorRootConfigurator.java index ba8ab1a..f713006 100644 --- a/src/main/java/com/cloudbees/jenkins/plugins/advisor/casc/AdvisorRootConfigurator.java +++ b/src/main/java/com/cloudbees/jenkins/plugins/advisor/casc/AdvisorRootConfigurator.java @@ -87,7 +87,7 @@ protected AdvisorGlobalConfiguration instance(Mapping mapping, ConfigurationCont if (!acceptToS) { // In UI, if you don't accept the ToS, the configuration is not applied, so here we throw an exception throw new ConfiguratorException(this, - "Terms of Service for CloudBees Jenkins Advisor have to be accepted. Please, review the acceptToS field in the yaml file."); + "Terms of Service for CloudBees Jenkins Advisor have to be accepted. Please review the acceptToS field in the yaml file."); } AdvisorGlobalConfiguration insights = getTargetComponent(configurationContext); @@ -97,7 +97,7 @@ protected AdvisorGlobalConfiguration instance(Mapping mapping, ConfigurationCont descriptor.doCheckCc(cc).kind.equals(FormValidation.Kind.ERROR)) { // In UI, if the fields are invalid, the configuration is not applied, so here we throw an exception throw new ConfiguratorException(this, - "Invalid configuration for CloudBees Jenkins Advisor. Please, review the content of email and cc fields in the yaml file."); + "Invalid configuration for CloudBees Jenkins Advisor. Please review the content of email and cc fields in the yaml file."); } updateConfiguration(insights, email, cc, true, nagDisabled, excludedComponents); diff --git a/src/main/resources/com/cloudbees/jenkins/plugins/advisor/AdvisorGlobalConfiguration/index.jelly b/src/main/resources/com/cloudbees/jenkins/plugins/advisor/AdvisorGlobalConfiguration/index.jelly index 6f8aefd..2f2e08f 100644 --- a/src/main/resources/com/cloudbees/jenkins/plugins/advisor/AdvisorGlobalConfiguration/index.jelly +++ b/src/main/resources/com/cloudbees/jenkins/plugins/advisor/AdvisorGlobalConfiguration/index.jelly @@ -2,10 +2,23 @@ + + + + + + + +

- Jenkins Health Advisor by CloudBees Icon ${it.actionTitle}

@@ -19,13 +32,9 @@

As issues are detected in your master, Jenkins Health Advisor by CloudBees will send new reports to your email address.

- - -

- Last bundle upload: - ${it.lastBundleResult} -

- + + + @@ -38,9 +47,10 @@ our Terms and Conditions.

- - + + + @@ -50,10 +60,12 @@ + + description="You can optionally include a list of comma-separated emails to receive the report."> + @@ -96,9 +108,8 @@ ${%Reminder} - - + + @@ -107,6 +118,48 @@ + + + + + + + + + + + diff --git a/src/main/resources/lib/advisor/validateOnLoad.jelly b/src/main/resources/lib/advisor/validateOnLoad.jelly index 85eca86..291beae 100644 --- a/src/main/resources/lib/advisor/validateOnLoad.jelly +++ b/src/main/resources/lib/advisor/validateOnLoad.jelly @@ -1,63 +1,50 @@ - Check if Jenkins Health Advisor by CloudBees is correctly configured and the server is available. - - A valid email must be configured. - + Check if Jenkins Health Advisor by CloudBees the server is available. - -
- - - - - - - - - - - + + + + + + + + +
diff --git a/src/test/java/com/cloudbees/jenkins/plugins/advisor/BundleUploadTest.java b/src/test/java/com/cloudbees/jenkins/plugins/advisor/BundleUploadTest.java index 26b49b8..5162f43 100644 --- a/src/test/java/com/cloudbees/jenkins/plugins/advisor/BundleUploadTest.java +++ b/src/test/java/com/cloudbees/jenkins/plugins/advisor/BundleUploadTest.java @@ -16,6 +16,7 @@ import java.io.File; import java.util.concurrent.TimeUnit; +import static com.cloudbees.jenkins.plugins.advisor.BundleUpload.BUNDLE_SUCCESSFULLY_UPLOADED; import static com.github.tomakehurst.wiremock.client.WireMock.aResponse; import static com.github.tomakehurst.wiremock.client.WireMock.any; import static com.github.tomakehurst.wiremock.client.WireMock.anyRequestedFor; @@ -85,7 +86,7 @@ public void execute() throws Exception { .withHeader("Content-Type", WireMock.containing("multipart/form-data"))); // Refresh the configuration? - assertThat(config.getLastBundleResult(), containsString("Successfully uploaded a bundle")); + assertThat(config.getLastBundleResult(), containsString(BUNDLE_SUCCESSFULLY_UPLOADED)); } @Test