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

Fixing checkbox serialization by jelly views #110

Merged
merged 1 commit into from Dec 12, 2016

Conversation

Projects
None yet
3 participants
@jeffersongirao

jeffersongirao commented Dec 9, 2016

Checkboxes in the PodTemplate were being reset to default state in the Views since it seems like Jelly expects the boolean primitive not the Boolean type.

@iocanel

This comment has been minimized.

Show comment
Hide comment
@iocanel

iocanel Dec 9, 2016

Looks good to me, but since I am no jelly expert, I'd like @carlossg opinion.

iocanel commented Dec 9, 2016

Looks good to me, but since I am no jelly expert, I'd like @carlossg opinion.

@carlossg

This comment has been minimized.

Show comment
Hide comment
@carlossg

carlossg Dec 11, 2016

@iocanel you had set them as Boolean for the pod inheritance, how does this change affect that?

carlossg commented Dec 11, 2016

@iocanel you had set them as Boolean for the pod inheritance, how does this change affect that?

@iocanel

This comment has been minimized.

Show comment
Hide comment
@iocanel

iocanel Dec 11, 2016

@carlossg: right! I think that the idea was to be able to check if a value has been set or not (checking against null), but I think that in the end of the day I added a default value in the constructor (which means that there was a bug there).

A solution for both cases would be add a second boolean field per boolean which marks if a value has been explicitly set.

Since this is a bug that was already there, I think that its fair to merge this and then fix the bug in a different pull request.

iocanel commented Dec 11, 2016

@carlossg: right! I think that the idea was to be able to check if a value has been set or not (checking against null), but I think that in the end of the day I added a default value in the constructor (which means that there was a bug there).

A solution for both cases would be add a second boolean field per boolean which marks if a value has been explicitly set.

Since this is a bug that was already there, I think that its fair to merge this and then fix the bug in a different pull request.

@carlossg carlossg merged commit 71c946a into jenkinsci:master Dec 12, 2016

1 check passed

Jenkins This pull request looks good
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment