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
Merged

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

Show file tree
Hide file tree
Changes from 7 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
18 changes: 10 additions & 8 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 @@ -2543,25 +2543,27 @@
* Validates the remote server supports custom revision properties
*/
@RequirePOST
public FormValidation doCheckRevisionPropertiesSupported(@AncestorInPath Item context,
public FormValidation doCheckExcludedRevprop(@AncestorInPath Item context,
@QueryParameter String value,
@QueryParameter String credentialsId,
@QueryParameter String excludedRevprop) throws IOException, ServletException {
String v = Util.fixNull(value).trim();
@QueryParameter String remoteLocation,
@QueryParameter String remoteCredentialsId
) 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();
if (revprop.length() == 0)
return FormValidation.ok();

// Test the connection only if we have admin permission
if (!Jenkins.getInstance().hasPermission(Jenkins.ADMINISTER))
return FormValidation.ok();

try {
SVNURL repoURL = SVNURL.parseURIDecoded(new EnvVars(EnvVars.masterEnvVars).expand(v));
StandardCredentials credentials = lookupCredentials(context, credentialsId, repoURL);
StandardCredentials credentials = lookupCredentials(context, remoteCredentialsId, repoURL);

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

View check run for this annotation

ci.jenkins.io / Code Coverage

Not covered lines 

Lines 2556-2566 are not covered by tests
SVNNodeKind node = null;
try {
node = checkRepositoryPath(context,repoURL, credentials);
Expand Down Expand Up @@ -3199,11 +3201,11 @@
* Validate the value for a remote (repository) location.
*/
@RequirePOST
public FormValidation doCheckRemote(/* TODO unused, delete */StaplerRequest req, @AncestorInPath Item context,

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

View check run for this annotation

ci.jenkins.io / Open Tasks Scanner

TODO 

NORMAL:
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 Down
14 changes: 3 additions & 11 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,14 @@ 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:textbox />
</f:entry>
<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
12 changes: 11 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,8 +56,18 @@ 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="remoteLocation" >
<f:textbox />
</f:entry>
<f:entry field="remoteCredentialsId" >
<f:textbox />
</f:entry>
</f:invisibleEntry>
shlomomdahan marked this conversation as resolved.
Show resolved Hide resolved
<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 />
</f:entry>
<f:entry title="${%Filter changelog}" field="filterChangelog">
<f:checkbox />
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='_.remote']", '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='_.remote']");
var credentialsIdElement = document.querySelector("select[name='_.credentialsId'][filldependson='remote']");
var selectedOption = credentialsIdElement.options[credentialsIdElement.selectedIndex].value;
Comment on lines +11 to +13
Copy link
Contributor

Choose a reason for hiding this comment

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

I expect this will fall apart if we try to define a second location in the configuration, because you selectors will return arrays of elements. Could you check how it behaves in that case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, disregard "arrays of elements", you're using querySelector, not querySelectorAll. Either way I'm concerned whether this is going to work when we try to define multiple locations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you're saying.

In this case, each time you add a module, the checkUrl and checkCredentials is correctly called for the corresponding module, however, the validation for the excludedRevprop only works on the module that was added first.

We can discuss this on the call this morning

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorted it out on a call. excludeRevProp validation previously has referenced credentialsId and remote URL or the first module that was defined. While this behavior seems wrong it is preserved here.



var remoteHidden = document.querySelector("input[name='_.remoteLocation']");
var credentialsHidden = document.querySelector("input[name='_.remoteCredentialsId']");
shlomomdahan marked this conversation as resolved.
Show resolved Hide resolved

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

}
Loading