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

FIX suggestion of ip when subnet id is not the same as array index #34

Merged
merged 1 commit into from Apr 17, 2012

Conversation

tjikkun
Copy link

@tjikkun tjikkun commented Apr 16, 2012

No description provided.

@ohadlevy
Copy link
Member

this looks good, in which context would the error happen?

@ohadlevy
Copy link
Member

and also, is there an open bug about this? thanks!

@tjikkun
Copy link
Author

tjikkun commented Apr 16, 2012

New Host: Network tab. I select a domain, then a subnet. I expect an ip address to be autosugested, but without my patch I only get a javascript error:
drop_text is undefined

Our select box looks like:

<select onchange="subnet_selected(this);" name="host[subnet_id]" id="host_subnet_id"><option value=""></option>
<option value="3">kelder (10.255.9.0/24)</option></select>

and drop_tet is defined as

$(element).text().split("\n")[subnet_id]

subnet_id will be 3 in this case, but there is no 3th array index

I hope you can make sense out of this :)

@tjikkun
Copy link
Author

tjikkun commented Apr 16, 2012

No open bug that I know of, just something we ran into. Should I always file a bug report, or is just fixing it ok as well?

@ohadlevy
Copy link
Member

hmm.. I can't reproduce, which browser is it?

@tjikkun
Copy link
Author

tjikkun commented Apr 16, 2012

Firefox 11
To reproduce, create a few Subnets, 3 for example. Then delete the first two subnets. Then you should be able to reproduce.

@ohadlevy
Copy link
Member

OK, I want to try and reproduce to understand the root of the problem, thanks for the patch!

@tjikkun
Copy link
Author

tjikkun commented Apr 17, 2012

If it makes it easier to understand:
instead of:

-  var drop_text = $(element).text().split("\n")[subnet_id]
+  var drop_text = $(element).children(":selected").text();

I could also write:

-  var drop_text = $(element).text().split("\n")[subnet_id]
+  var drop_text = $(element).children("[value=" + subnet_id + "]").text();

The important thing is that if some subnets are removed, then you are missing some \n's to split on, so array indexing is off.

@ohadlevy
Copy link
Member

what I'm not getting, is why would the returned array is wrong?

@tjikkun
Copy link
Author

tjikkun commented Apr 17, 2012

Ok, another try:
we have this selectbox:

    <option value=""></option>
    <option value="1">choosing this option will work (10.255.8.0/24)</option>
    <option value="3">choosing this option will break (10.255.9.0/24)</option>
</select>```
what you see here is that we once had a subnet with id of 2, but it is removed.
Now the current code does
```var drop_text = $(element).text().split("\n")[subnet_id]```
$(element).text() will be:
 ```<empty line>
choosing this option will work (10.255.8.0/24)
choosing this option will break (10.255.9.0/24)

Now you split it, and you get the array:

1 => 'choosing this option will work (10.255.8.0/24)'
2 => 'choosing this option will break (10.255.9.0/24)'

but since 'value' of the last option is 3, subnet_id will be 3, and
var drop_text = $(element).text().split("\n")[subnet_id]
means we try to get index 3 from the array, which doesn't exist.
What you want is to get the text of the option where value==3, not the 3th in the list:
var drop_text = $(element).children("[value=" + subnet_id + "]").text();
Or even better, just get the text of the selected element, which is what we are trying to do anyway:
var drop_text = $(element).children(":selected").text();
This last one is what my patch does

ohadlevy added a commit that referenced this pull request Apr 17, 2012
FIX suggestion of ip when subnet id is not the same as array index
@ohadlevy ohadlevy merged commit dfcfbae into theforeman:develop Apr 17, 2012
h0jeZvgoxFepBQ2C pushed a commit to h0jeZvgoxFepBQ2C/foreman that referenced this pull request Jul 26, 2018
Gracefully handle the 'garbage in' of an empty .foreman file.
cfouant pushed a commit to cfouant/foreman that referenced this pull request Dec 18, 2018
Using older version of vagrant due to rsync issues
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants