Fix disabled multibranch build notification#26113
Fix disabled multibranch build notification#26113LakshyaBagani wants to merge 7 commits intojenkinsci:masterfrom
Conversation
…/github.com/LakshyaBagani/jenkins into Empty-list-in-menus-lead-to-endless-spinner
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where disabled multibranch projects incorrectly showed a "Build Scheduled" notification when users attempted to trigger builds on branch jobs, even though the builds were not actually scheduled.
Changes:
- Modified
isBuildable()to check if parent item (e.g., multibranch project) is disabled - Updated
doBuild()to return 409 CONFLICT instead of redirect when build scheduling fails - Changed JavaScript response validation from
rsp.oktorsp.status === 201for precise success detection
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| core/src/main/java/jenkins/model/ParameterizedJobMixIn.java | Added parent disabled check to isBuildable() and changed failed build response from redirect to 409 CONFLICT |
| core/src/main/resources/lib/hudson/project/configurable/configurable.js | Changed success detection from rsp.ok to explicit 201 status check |
| src/main/js/components/dropdowns/utils.js | Added defensive null check for subMenu (unclear relationship to PR purpose) |
| src/main/js/components/dropdowns/jumplists.js | Added null/array validation and subMenu safety checks (unclear relationship to PR purpose) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| menuItem, | ||
| Object.assign({}, Templates.dropdown(), { | ||
| content: generateDropdownItems(item.subMenu()), | ||
| content: generateDropdownItems(item.subMenu() || []), |
There was a problem hiding this comment.
The changes to this file (adding null/undefined checks for subMenu) don't appear to be related to the stated purpose of this PR, which is to fix disabled multibranch build notification. These defensive programming improvements should either be explained in the PR description or submitted as a separate PR to maintain clear change tracking and easier code review.
| ItemGroup<? extends Item> parentGroup = ((Job) this).getParent(); | ||
| if (parentGroup instanceof Item) { | ||
| Item parent = (Item) parentGroup; | ||
| if (parent instanceof ParameterizedJob && ((ParameterizedJob) parent).isDisabled()) { |
There was a problem hiding this comment.
The instanceof check for ParameterizedJob without a rawtype cast warning suppression may cause compilation warnings. Since ParameterizedJob is a generic interface, consider using a rawtype check or adding appropriate type safety annotations. Additionally, this check only verifies the immediate parent - consider whether checking all ancestors in the hierarchy would be more robust, or add a comment explaining why only the immediate parent is checked.
| if (parent instanceof ParameterizedJob && ((ParameterizedJob) parent).isDisabled()) { | |
| // Only the immediate parent is checked; higher-level ancestors are not modeled here. | |
| if (parent instanceof ParameterizedJob<?,?> && ((ParameterizedJob<?,?>) parent).isDisabled()) { |
| default boolean isBuildable() { | ||
| return !isDisabled() && !((Job) this).isHoldOffBuildUntilSave(); | ||
| if (isDisabled() || ((Job) this).isHoldOffBuildUntilSave()) { | ||
| return false; | ||
| } | ||
| ItemGroup<? extends Item> parentGroup = ((Job) this).getParent(); | ||
| if (parentGroup instanceof Item) { | ||
| Item parent = (Item) parentGroup; | ||
| if (parent instanceof ParameterizedJob && ((ParameterizedJob) parent).isDisabled()) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; |
There was a problem hiding this comment.
Consider adding an automated test for the new behavior where a job with a disabled parent (e.g., multibranch project) is not buildable. The existing ParameterizedJobMixInTest.java already has tests for disabled projects, and a similar test could be added to verify the fix for disabled parent projects. This would provide regression protection for this specific bug fix.
…isabled When trying to trigger a build of a branch job in a disabled multibranch project, the UI showed 'Build Scheduled' even though the build was not actually scheduled (queue returned null, causing a 302 redirect). Changes: - Updated isBuildable() to check if parent ParameterizedJob is disabled - Changed doBuild() to return 409 CONFLICT when schedule2() returns null - Fixed JavaScript to only treat HTTP 201 as success (not 302 redirects) Fixes issue where disabled multibranch parent allows branch builds to appear scheduled when they are actually rejected.
56c1ee4 to
29371cc
Compare
|
Hi @Copilot AI, Thanks for the review. I removed the changes to Why they were included: Why they were removed: Current state:
|
Fixes #16678
Description
When trying to trigger a build of a branch job in a disabled multibranch project, the UI incorrectly showed "Build Scheduled" notification even though the build was not actually scheduled. The queue rejected the build (returned null), but the code sent a 302 redirect which the JavaScript treated as success.
Changes
isBuildable()method: Now checks if the parentParameterizedJob(e.g., multibranch project) is disabled before allowing buildsdoBuild(): Changed from returning 302 redirect to throwing 409 CONFLICT whenschedule2()returns nullrsp.ok(which includes 302) to checkingrsp.status === 201specificallyTesting done
Manual testing performed:
Tested on: Jenkins 2.541-SNAPSHOT
Proposed changelog entries
Proposed changelog category
/label bug
Proposed upgrade guidelines
N/A
Submitter checklist
@Restrictedor have@since TODOJavadocs, as appropriate.@Deprecated(since = "TODO")or@Deprecated(forRemoval = true, since = "TODO"), if applicable.evalto ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mawinter69 @timja @daniel-beck