[JENKINS-68616] Enable field validation checks #316
Conversation
Regression in 1105 release. The `div` that was added to surround the `select` seems to prevent calls to the `attrs.checkMethod`. When using a GitHub branch source, an empty credentials field no longer displays the warning that a credential is recommended. Other uses of credentials seem to have the same behavior. Validation checks are no longer called. When the `div` is included in the page, the credential validation callback is not invoked. When the `div` is removed from the page, the credential validation callback is invoked. It is unclear to me why the addition of a `div` would change the validation behavior. Review the differences with white space changes suppressed so that it is clear the difference is only in the deletion of the `div` tags that wrapped the `select`. Revert one HTML `div` from "Minor improvements" This reverts part of commit 6dee9b8.
|
#287 for the record |
| @@ -107,11 +107,11 @@ | |||
| <input type="hidden" name="includeUser" value="${includeUser}"/> | |||
| </j:otherwise> | |||
| </j:choose> | |||
| <div class="form-group jenkins-form-item jenkins-form-item--tight"> | |||
| <f:select clazz="${attrs.clazz} credentials-select" field="${attrs.field}" | |||
There was a problem hiding this comment.
Hide whitespace to review
Agreed. Much clearer with white space diffs suppressed.
I agree that we should let this wait until @timja has had time to reply. The missing validation is unfriendly for the user but does not break functionality as far as I know. |
|
Looking |
|
All Greek to me. @MarkEWaite if you can confirm the last edit preserves the bug fix and general appearance, I can get a release cut. |
Confirmed that the fix the most recent change preserves the bug fix and the general appearance. |
There was a problem hiding this comment.
I support the merge and release of this fix, but I would prefer it if the Jira ticket be updated with an evaluation of the cause of the problem. This project does not have much of a culture of writing root cause analysis in bugs, so this is an optional comment, but I believe that preserving a clear and written analysis of the cause of the problem, including a description of how the new code addresses the problem (beyond just "fix spacing") would benefit everyone involved. This is optional, just for your consideration.
|
@MarkEWaite fix re-introduced the UI bug of elements touching each other. I didn’t attempt to track down a root cause, likely due to some tree building in hudson-behaviour.js which one of the classes was affecting. |
|
I understand, so Mark's change fixed one bug and created another, and commit 853ce58 fixed the newly-created bug. What still isn't clear to me, though, is whether the original hunk reverted by Mark in this PR was a logically valid usage of the core API (in which case the core implementation of that API is to blame and this PR is a workaround for an as-yet-unfiled core bug) or the original hunk reverted by Mark was an invalid usage of the core API (in which case the original hunk was incorrect and Mark's fix in this PR is a valid long-term solution). It is only this kind of rigorous analysis and follow-up that will prevent us from constantly playing whack-a-mole with these bugs. |
|
I’m unsure @janfaracik may know |
|
There is nothing wrong with being unsure when starting out, but the goal should be to become sure through analytical reasoning, regardless of whether or not one was the original author of the code. |
[JENKINS-68616] Enable field validation checks
Regression in 1105 release. The
divthat was added to surround theselectseems to prevent calls to theattrs.checkMethod.When using a GitHub branch source, an empty credentials field no longer displays the warning that a credential is recommended. Other uses of credentials seem to have the same behavior. Validation checks are no longer called.
When the
divis included in the page, the credential validation callback is not invoked. When thedivis removed from the page, the credential validation callback is invoked.It is unclear to me why the addition of a
divwould change the validation behavior.Review the differences with white space changes suppressed so that it is clear the difference is only in the deletion of the
divtags that wrapped theselect.Not sure how to create a viable test that confirms the fix is behaving correctly.
Revert one HTML
divfrom "Minor improvements"This reverts part of commit 6dee9b8.