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

Fixes #27903 - Do not init template-form-elements #7057

Conversation

m-bucher
Copy link
Contributor

fixes problem with not initialized counter and byte-size input elements
within dynamically added volumes of compute-resources in host-create

This fixed the error for me on https://github.com/theforeman/foreman_fog_proxmox, but I am open for better solutions 😉

@theforeman-bot
Copy link
Member

@m-bucher, the Redmine ticket used is for a different project than the one associated with this GitHub repository. Please either:

If changing the ticket number used, remember to update the PR title and the commit message (using git commit --amend).


This message was auto-generated by Foreman's prprocessor

@theforeman-bot
Copy link
Member

Issues: #27902

@theforeman-bot theforeman-bot added Needs testing Not yet reviewed Legacy JS PRs making changes in the legacy Javascript stack UI labels Sep 23, 2019
@m-bucher m-bucher force-pushed the fix_broken_spinner_in_compute_resource_volume branch from f8463ca to 567d00a Compare September 23, 2019 08:39
@m-bucher m-bucher changed the title Fixes #27902 - Do not init template-form-elements Fixes #27903 - Do not init template-form-elements Sep 23, 2019
@m-bucher m-bucher force-pushed the fix_broken_spinner_in_compute_resource_volume branch 3 times, most recently from 8165727 to 6922d7e Compare September 23, 2019 09:16
@ezr-ondrej
Copy link
Member

Looks good overall @m-bucher, I agree this probably should go in (I would only like to make sure, we are not depending on the form_template to be initialized somewhere, but I wouldn't guess so.
The problem you are facing seems similar to one, that has been fixed in #7023 though, could you try if that patch fixes your problem?

@ezr-ondrej
Copy link
Member

[test katello]

@ezr-ondrej
Copy link
Member

[test foreman]

@m-bucher
Copy link
Contributor Author

@ezr-ondrej , just tried the patch, but it did not fix the broken spinner-elements.

I agree, we should make sure that this does not break anything.
From what I saw (using git grep), counter_f and byte_size_f elements are only used for not-templated forms in foreman. So I guess we are save there.
I would have liked to add an implementation that uses byte_size_f to set disk-size in libvirt-volumes, which is currently just an input-field (probably because the spinners would not work 😉 ), but I am short on spare time at the moment.

However, I do not know if other plugins use these elements in form-templates.

Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Works and the implementation make sense, only the argument check is bit tricky. Could we improve it a bit?

@m-bucher m-bucher force-pushed the fix_broken_spinner_in_compute_resource_volume branch from 6922d7e to 6feaf19 Compare October 2, 2019 11:00
@m-bucher m-bucher force-pushed the fix_broken_spinner_in_compute_resource_volume branch 4 times, most recently from 12055f3 to 5dd656e Compare October 2, 2019 15:25
Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

The implementation looks good already, thanks for the event handling, looks better now!
Just one idea I came up with - bit more readable implementation of the parent checking. Let me know if you want to use it, if not, I am ok to merge as is :)

export function initByte() {
$('input.byte_spinner').each(function() {
export function initByte(selection = null) {
$('input.byte_spinner', selection).each(function() {
Copy link
Member

Choose a reason for hiding this comment

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

I believe that with this:

Suggested change
$('input.byte_spinner', selection).each(function() {
$('input.byte_spinner, selection).not('.form_template input.byte_spinner').each(function() {
// ....

we could leave out the if.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, I just added it.
eslint thinks the lines are too long, so it resulted in some code reformatting.

fixes problem with not initialized counter and byte-size input elements
within dynamically added volumes of compute-resources in host-create
@m-bucher m-bucher force-pushed the fix_broken_spinner_in_compute_resource_volume branch from 5dd656e to 9e29589 Compare October 2, 2019 22:11
Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

Thanks @m-bucher 👍 good job with the JS fight! 😍 👏

@ezr-ondrej ezr-ondrej merged commit f4cbc22 into theforeman:develop Oct 3, 2019
@m-bucher m-bucher deleted the fix_broken_spinner_in_compute_resource_volume branch October 4, 2019 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Legacy JS PRs making changes in the legacy Javascript stack Needs testing UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants