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

DO NOT MERGE fixes #6226 - if only one option for required field on new host page (ex. installation media), then automatically select it #1512

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
4 participants
@isratrade
Copy link
Member

isratrade commented Jun 15, 2014

No description provided.

@domcleal

This comment has been minimized.

Copy link
Contributor

domcleal commented Jun 16, 2014

I've got to admit, I quite liked the change in behaviour when blank_or_inherit_f was introduced. The main reason was for host edit rather than for new hosts, as people often found it confusing when they'd edit a host to look at which attributes were currently set and it would select the only option available - even when the attribute was actually nil.

Maybe the behaviour should be different based on new_record?, so it's automatically selected for new hosts, but when editing you can see nil fields.

(edit: taxonomy and puppet master fields were a common problem, think the taxonomy ones still are)

@ohadlevy

This comment has been minimized.

Copy link
Member

ohadlevy commented Jun 16, 2014

👍 for new_record

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Jun 16, 2014

@domcleal, will add restriction to new_record? but the the original behavior from blank_or_inherit_f only affects hostgroups, not hosts. This PR only affects hosts

inherited_value ||= _("no value")
_("Inherit parent (%s)") % inherited_value
else
return true if f.object.new_record? || collection.nil?

This comment has been minimized.

Copy link
@isratrade

isratrade Jun 16, 2014

Author Member

rebased after adding check if f.object.new_record?

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Jun 17, 2014

[test]

@domcleal

This comment has been minimized.

Copy link
Contributor

domcleal commented Jun 19, 2014

The bulk of blank_or_inherit_f did indeed affect host groups only, but for hosts it would always return 'true' so for hosts it always ensured there was a blank entry.

The behaviour in the current commit doesn't look like it solves the problem I mentioned. Shouldn't it be !f.object.new_record? so when editing a host you always get a blank entry, and the conditional for collection.size is only reached on new hosts?

The PR's "do not merge" too - why, and what's the plan? Does it need extending to other fields?

@isratrade isratrade changed the title DO NOT MERGE fixes #6226 - if only one option for required field on new host page (ex. installation media), then automatically select it fixes #6226 - if only one option for required field on new host page (ex. installation media), then automatically select it Jun 26, 2014

isratrade added some commits Jun 15, 2014

fixes #6226 - if only one option for required field on new host page …
…(ex. installation media), then automatically select it
fixes #6401 - change include blank option to 'No environments' or 'No…
… subnets', etc if there are no select options

              change include blank option to 'Select environment' or 'Select subnet' if required field and more than one option
@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Jun 30, 2014

I'm not sure what in this PR would cause the integration failure on realm edit page that I fixed on PR #1547.

@@ -9,6 +9,7 @@ class RealmTest < ActionDispatch::IntegrationTest
test "create new page" do
assert_new_button(realms_path,"New Realm",new_realm_path)
fill_in "realm_name", :with => "EXAMPLE.COM"
select "FreeIPA", :from => "realm_realm_type"

This comment has been minimized.

Copy link
@isratrade

isratrade Jun 30, 2014

Author Member

I fixed the realm test issue, but PR #1547 won't hurt to merge. I still don't know why it gave my a routing error for 'myrealm.net' when the issue was with the validation.

This comment has been minimized.

Copy link
@isratrade

isratrade Jun 30, 2014

Author Member

I figured it out. The No route matches [GET] "/realms/myrealm.net/edit" was on the friendly_id branch

if cnt == 1 && is_required?(f, attr)
false # auto-select option
elsif cnt > 0 && is_required?(f, attr)
blank_msg || (_("Select %s") % attr.to_s.humanize.downcase)

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 2, 2014

Contributor

Using humanize on model names won't translate them.. this is tricky, it may depend on #4503.

ditto below

This comment has been minimized.

Copy link
@isratrade

isratrade Jul 2, 2014

Author Member

I could use .gsub('_id', '') instead of .humanize, but it will do the nearly the same. I liked .humanize since it adds a space

 > 'smart_proxy_id'.humanize
 => "Smart proxy" 

verses

> 'smart_proxy_id'.gsub('_id','')
 => "smart_proxy"

It's just to generate a string for the 'blank message'. Why does this affect translations?

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 2, 2014

Contributor

The problem is that the noun doesn't get translated as it is... you want to be able to say "Choisir un environnement" rather than "Choisir un environment", and it touches upon an issue that's seen in the UI in a few other places, that concatenating strings like this isn't i18n-friendly (e.g. in that example, the gender of the noun varies, so doesn't make sense to translate "Select %s" to "Select un".)

Really I think these strings would need to be generated in full into locale/model_attributes.rb or similar so they can be translated in their entirety, instead of using humanize and concatenation.

@@ -149,10 +150,11 @@ def field(f, attr, options = {})
content_tag :div, :class => "form-group #{error.empty? ? "" : 'has-error'}",
:id => options.delete(:control_group_id) do

required_mark = '* '.html_safe if is_required?(f, attr)

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 2, 2014

Contributor

I wonder about putting this on the right hand side of the box and making it red (think they usually are)? Unsure..

This comment has been minimized.

Copy link
@isratrade

isratrade Jul 2, 2014

Author Member

@ohadlevy, your thoughts on this? either way is fine with me.

end

def include_blank_value(f, attr, collection = nil, blank_msg = nil, none_msg = nil)
cnt = collection.try(:count)

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 2, 2014

Contributor

.size preferred?

This comment has been minimized.

Copy link
@isratrade

isratrade Jul 2, 2014

Author Member

.size vs .length vs .count, I remember there are slight differences, but not totally sure. I can change to .size

This comment has been minimized.

Copy link
@domcleal

domcleal Jul 2, 2014

Contributor

.size is the more intelligent one IIRC

This comment has been minimized.

Copy link
@iNecas

iNecas Jul 26, 2014

Member

Also, could we use size or count instead of cnt variable name?

@domcleal

This comment has been minimized.

Copy link
Contributor

domcleal commented Jul 2, 2014

I've not tested much with single-value options, but I think the idea is good, will do more soon. Talking of tests, some small tests of those application helpers may be useful and quite easy.

There are a couple of UI issues mentioned - one might be hard (i18n), but has come up in a few places, and the other may be worth asking Kyle or someone about, if that's easy.

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Jul 2, 2014

good idea, I will write Kyle

cnt = collection.try(:count)
if cnt == 1 && is_required?(f, attr)
false # auto-select option
elsif cnt > 0 && is_required?(f, attr)

This comment has been minimized.

Copy link
@iNecas

iNecas Jul 26, 2014

Member

if cnt is blank (for example when the collection is nil), you will get "undefined method >' for nil:NilClass", as I did when testing this PR

cnt = collection.try(:count) || 0

seemed to do the trick

@iNecas

This comment has been minimized.

Copy link
Member

iNecas commented Jul 26, 2014

It doesn't seem to work for domain -> subnet association: my test case: I have two domain -> subnet pairs, each associated with different location, when I change the location, I see the domain preselected, but in the subnets, I see "No Subnets", because the list of available subnets is not reflected by the fact, that the domain was preselected (and because I have no other domain to choose from, I can't even force to update the form with ajax to get the list of subnets after that).

I would expect the pre-selection happening on model, not view level, to avoid this kind of logical issues.

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Jul 28, 2014

@iNecas, did this happen on the New Host form after a domain was already selected and then you updated the location. I do recall a known ajax bug about this independent of this PR.

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Jul 28, 2014

@iNecas, can you confirm if this ajax issues between domain/subnets also happens on the 'develop' branch?

@iNecas

This comment has been minimized.

Copy link
Member

iNecas commented Jul 29, 2014

I've rebased against latest develop, I still see the same issue. Also, another good reason for not having this logic in the views is that the API would also deserve the same behavior: when I provision the host via hammer, I would expect specifying the org/location would determine the networks if possible.

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Jul 29, 2014

@iNecas, I entered a separate issues for the ajax bugs after location/organization is changed.
http://projects.theforeman.org/issues/6814

@iNecas

This comment has been minimized.

Copy link
Member

iNecas commented Jul 29, 2014

I'm afraid merging this PR makes http://projects.theforeman.org/issues/6814 quite serious issue (as there is not possibility to change the domain to fix the ajax loading), therefore it would be nice to address these two together.

Are there any plans to do something similar to the API?

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Jul 29, 2014

@iNecas, we will hold off on merging this until issue #6814 is fixed.

@isratrade isratrade changed the title fixes #6226 - if only one option for required field on new host page (ex. installation media), then automatically select it DO NOT MERGE fixes #6226 - if only one option for required field on new host page (ex. installation media), then automatically select it Jul 29, 2014

@isratrade

This comment has been minimized.

Copy link
Member Author

isratrade commented Oct 1, 2014

closing. will re-open when issue #6814 is fixed.

@isratrade isratrade closed this Oct 1, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.