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.

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-37241] Support for query parameters in autocomplete #9959

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timja
Copy link
Member

@timja timja commented Nov 11, 2024

See JENKINS-37241.

Testing done

Verified that existing usages in core are unaffected:

  1. Creating a logger and having logger names suggested. This one is a good test as it doesn't have a descriptor behind it as well.
  2. Copy from in a new job

Created a change in Azure VM agents jenkinsci/azure-vm-agents-plugin#580 which uses the depends on functionality to access query parameters

Proposed changelog entries

  • Developer: Add support for @QueryParameter to the autocomplete component
  • Change autocomplete component to use POST for sending requests

Proposed upgrade guidelines

N/A

Submitter checklist

Desired reviewers

@mention

Before the changes are marked as ready-for-merge:

Maintainer checklist

// build query parameter line by figuring out what should be submitted
List<String> depends = buildFillDependencies(method, new ArrayList<>());
if (!depends.isEmpty()) {
attributes.put("fillDependsOn", String.join(" ", depends));
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about whether this should be a different attribute name, e.g. autocompleteFillDependsOn but I couldn't see when they would ever clash so I just left it as the same name

Comment on lines +66 to +77
const q = qs(e).addThis();
if (depends && depends.length > 0) {
depends.split(" ").forEach(
TryEach(function (n) {
q.nearBy(n);
}),
);
}

const queryString = q.toString();
const idx = queryString.indexOf("?");
const parameters = queryString.substring(idx + 1);
Copy link
Member Author

Choose a reason for hiding this comment

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

copied from hudson-behaviour.js for the doCheck functionality.

It uses query string by default and then if the method is post it converts to parameters, (which is the default)

I didn't see any reason for there to need to be an option for get, I'm not aware of it ever being used for the doChecks

@Wadeck Wadeck added the needs-security-review Awaiting review by a security team member label Nov 11, 2024
@timja timja added the web-ui The PR includes WebUI changes which may need special expertise label Nov 11, 2024
@timja timja requested review from daniel-beck and a team November 11, 2024 15:36
@timja timja requested a review from a team November 11, 2024 15:44
@timja timja added the rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted label Nov 11, 2024
@Kevin-CB
Copy link
Contributor

Sorry for the delay! I’ll get started on the security review now.

@Kevin-CB Kevin-CB added security-approved @jenkinsci/core-security-review reviewed this PR for security issues and removed needs-security-review Awaiting review by a security team member labels Nov 20, 2024
@janfaracik
Copy link
Contributor

Tested locally and LGTM. Seems there's quite a few competing components for this in Design Library, would be good for us to recommend one of them (and maybe dissuade usage of the others if they're not as functional).

@timja timja requested review from a team and removed request for a team November 23, 2024 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfe For changelog: Minor enhancement. use `major-rfe` for changes to be highlighted security-approved @jenkinsci/core-security-review reviewed this PR for security issues web-ui The PR includes WebUI changes which may need special expertise
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants