Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-73900] Un-inline JS in ModuleLocation/config.jelly and fix validation logic #319

Merged
merged 10 commits into from
Nov 5, 2024
+53 −20
Merged

[JENKINS-73900] Un-inline JS in ModuleLocation/config.jelly and fix validation logic #319

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c94d7ab
wip
shlomomdahan Oct 11, 2024
d3d8ef7
make subversion CSP compliant / fix existing form validation
shlomomdahan Oct 14, 2024
1895dc4
make subversion CSP compliant / fix existing form validation
shlomomdahan Oct 14, 2024
cc487b9
make subversion CSP compliant / fix existing form validation
shlomomdahan Oct 14, 2024
7fb59bd
address comments
shlomomdahan Oct 15, 2024
a574c02
address comments
shlomomdahan Oct 15, 2024
13bd3cf
Merge branch 'master' into JENKINS-73900
basil Oct 17, 2024
2e2de5f
simplify invisible entry
shlomomdahan Oct 28, 2024
80738a0
use specific class namesa to access elements
shlomomdahan Oct 28, 2024
1bd302a
Merge branch 'master' into JENKINS-73900
shlomomdahan Oct 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions src/main/java/hudson/scm/SubversionSCM.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2359,6 +2359,7 @@
@Deprecated
@RequirePOST
public FormValidation doCheckRemote(StaplerRequest req, @AncestorInPath AbstractProject context, @QueryParameter String value, @QueryParameter String credentialsId) {

Jenkins instance = Jenkins.getInstance();
if (instance != null) {
ModuleLocation.DescriptorImpl d = instance.getDescriptorByType(ModuleLocation.DescriptorImpl.class);
Expand Down Expand Up @@ -2545,13 +2546,15 @@
@RequirePOST
public FormValidation doCheckRevisionPropertiesSupported(@AncestorInPath Item context,
@QueryParameter String value,
@QueryParameter String credentialsId,
@QueryParameter String excludedRevprop) throws IOException, ServletException {
String v = Util.fixNull(value).trim();
@QueryParameter("svn.remote.loc") String remoteLocation,
@QueryParameter("svn.remote.cred") String credentialsId
shlomomdahan marked this conversation as resolved.
Show resolved Hide resolved
) throws IOException, ServletException {

String v = Util.fixNull(remoteLocation).trim();
if (v.length() == 0)
return FormValidation.ok();

String revprop = Util.fixNull(excludedRevprop).trim();
String revprop = Util.fixNull(value).trim();

Check warning on line 2557 in src/main/java/hudson/scm/SubversionSCM.java

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered line 

Line 2557 is not covered by tests
if (revprop.length() == 0)
return FormValidation.ok();

Expand Down Expand Up @@ -3200,10 +3203,10 @@
*/
@RequirePOST
public FormValidation doCheckRemote(/* TODO unused, delete */StaplerRequest req, @AncestorInPath Item context,
@QueryParameter String remote) {
@QueryParameter String value) {

// repository URL is required
String url = Util.fixEmptyAndTrim(remote);
String url = Util.fixEmptyAndTrim(value);
if (url == null) {
return FormValidation.error(Messages.SubversionSCM_doCheckRemote_required());
}
Expand All @@ -3228,7 +3231,7 @@
*/
@RequirePOST
public FormValidation doCheckCredentialsId(StaplerRequest req, @AncestorInPath Item context,
@QueryParameter String remote, @QueryParameter String value) {
@QueryParameter("remoteLocation") String remote, @QueryParameter String value) {
shlomomdahan marked this conversation as resolved.
Show resolved Hide resolved

// Test the connection only if we may use the credentials (cf. hudson.plugins.git.UserRemoteConfig.DescriptorImpl.doCheckUrl)
if (context == null && !Jenkins.get().hasPermission(Jenkins.ADMINISTER) ||
Expand Down
17 changes: 5 additions & 12 deletions src/main/resources/hudson/scm/SubversionSCM/ModuleLocation/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,22 +24,15 @@ THE SOFTWARE.

<?jelly escape-by-default='true'?>
<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form" xmlns:c="/lib/credentials">
<f:entry title="${%Repository URL}" field="remote" help="/scm/SubversionSCM/url-help">
<f:textbox checkMethod="post" id="svn.remote.loc" onchange="{ /* workaround for JENKINS-19124 */
var self=this.targetElement ? this.targetElement : this;
var r=findNextFormItem(self,'credentialsId');
r.onchange(r);
if (self===document.getElementById('svn.remote.loc')){
r=findNextFormItem(self,'excludedRevprop');
r.onchange(r);
}
self=null;
r=null;
}"/>

<f:entry title="${%Repository URL}" field="remoteLocation" help="/scm/SubversionSCM/url-help">
shlomomdahan marked this conversation as resolved.
Show resolved Hide resolved
<f:textbox checkUrl="${rootURL}/descriptorByName/hudson.scm.SubversionSCM%24ModuleLocation/checkRemote" checkDependsOn=""/>
</f:entry>

shlomomdahan marked this conversation as resolved.
Show resolved Hide resolved
<f:entry title="${%Credentials}" field="credentialsId">
<c:select checkMethod="post"/>
</f:entry>

<f:entry title="${%Local module directory}" field="local">
<f:textbox value="${instance!=null?instance.local:'.'}"/>
</f:entry>
Expand Down
15 changes: 14 additions & 1 deletion src/main/resources/hudson/scm/SubversionSCM/config.jelly
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,22 @@ THE SOFTWARE.
<f:entry title="${%Excluded Commit Messages}" field="excludedCommitMessages">
<f:textarea />
</f:entry>

<st:adjunct includes="hudson.scm.SubversionSCM.excludedRevprop-validation" />

<f:invisibleEntry>
<f:entry field="svn.remote.loc" >
shlomomdahan marked this conversation as resolved.
Show resolved Hide resolved
shlomomdahan marked this conversation as resolved.
Show resolved Hide resolved
<f:textbox />
</f:entry>
<f:entry field="svn.remote.cred" >
shlomomdahan marked this conversation as resolved.
Show resolved Hide resolved
<f:textbox />
</f:entry>
</f:invisibleEntry>

<f:entry title="${%Exclusion revprop name}" field="excludedRevprop">
<f:textbox checkMethod="post" checkUrl="'descriptorByName/hudson.scm.SubversionSCM/checkRevisionPropertiesSupported?value='+toValue(document.getElementById('svn.remote.loc'))+'&amp;credentialsId='+toValue(document.getElementById('svn.remote.cred'))+'&amp;excludedRevprop='+toValue(this)"/>
<f:textbox checkUrl="${rootURL}/descriptorByName/hudson.scm.SubversionSCM/checkRevisionPropertiesSupported" checkDependsOn="svn.remote.loc svn.remote.cred" />
shlomomdahan marked this conversation as resolved.
Show resolved Hide resolved
</f:entry>

<f:entry title="${%Filter changelog}" field="filterChangelog">
<f:checkbox />
</f:entry>
Expand Down
31 changes: 31 additions & 0 deletions src/main/resources/hudson/scm/SubversionSCM/excludedRevprop-validation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
Behaviour.specify("input[name='_.remoteLocation']", 'SubversionSCM.RemoteLocation', 0, function(element) {
element.addEventListener('blur', updateHiddenFields);
});

Behaviour.specify("select[name='_.credentialsId'][filldependson='remote']", 'SubversionSCM.CredentialsId', 0, function(element) {
element.addEventListener('change', updateHiddenFields);
});

function updateHiddenFields() {

var remoteLocationElement = document.querySelector("input[name='_.remoteLocation']");
var credentialsIdElement = document.querySelector("select[name='_.credentialsId'][filldependson='remote']");
var selectedOption = credentialsIdElement.options[credentialsIdElement.selectedIndex].value;


var remoteHidden = document.querySelector("input[name='_.svn.remote.loc']");
var credentialsHidden = document.querySelector("input[name='_.svn.remote.cred']");

if (remoteHidden) {
remoteHidden.value = remoteLocationElement.value;
}

if (credentialsHidden) {
credentialsHidden.value = selectedOption;
}

var revPropField = document.querySelector("input[name='_.excludedRevprop']");
if (revPropField) {
revPropField.dispatchEvent(new Event('change'));
}
Comment on lines +28 to +30
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to dispatch the event here? Is that supposed to retrigger the validation for excludedRevprop field when we change either remote or credentialsId? Is that how it worked before your change?

I don't like it personally, I'd prefer to have it triggered only when excludedRevprop loses focus. But disregard my comment if it worked like this before, who am I to judge :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is the way it was "supposed" to work before. Although it wasn't working correctly.

 if (self===document.getElementById('svn.remote.loc')){
                r=findNextFormItem(self,'excludedRevprop');
                r.onchange(r);
            }

I think it makes sense as we should validate the excluded revProp whenever remote or credentials changes as well as when excluded revProp itself changes

}