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

WebUI: don't change casing of Auth Indicators values #331

Closed
wants to merge 2 commits into from

Conversation

pvomacka
Copy link

@pvomacka pvomacka commented Dec 13, 2016

All values were previously converted to lowercase which was not
coresponding with CLI behaviour. Now they stay as they are
inserted. I also have to change the strings to lowercase because
the otp and radius should be inserted as lowercase words.

https://fedorahosted.org/freeipa/ticket/6308

Copy link
Member

@pvoborni pvoborni left a comment

Choose a reason for hiding this comment

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

nack - regression in labels

"type_otp": _("Two factor authentication (password + OTP)"),
"type_password": _("Password"),
"type_radius": _("RADIUS"),
"type_radius": _("radius"),
Copy link
Member

Choose a reason for hiding this comment

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

Changing this shows then invalid lowercase value on IPA Server/Configuration page for auth types and also on user details page.

It would be better to create separate:

authindicator_otp: _("otp"),
authindicator_radius _("radius"),

And use those for auth indicators checkboxes to avoid such side effects.

Copy link
Member

Choose a reason for hiding this comment

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

or maybe even better, auth indicators should not use translated labels. So labels should be the same value as value and they can be hardcoded.

@@ -2509,6 +2509,9 @@ IPA.custom_checkboxes_widget = function(spec) {

var that = IPA.checkboxes_widget(spec);

that.set_value_to_lowercase = spec.set_value_to_lowercase === undefined
? true : spec.set_value_to_lowercase;
Copy link
Member

Choose a reason for hiding this comment

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

Would be better if the default behavior was 'no lower casing' -> change to false and then adjust usages of the widget.

Reasoning: no formatting seems more nutural

@pvoborni
Copy link
Member

LGTM (reading code).

Copy link
Member

@pvoborni pvoborni left a comment

Choose a reason for hiding this comment

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

aci.attributes_widget needs to have set_value_to_lowercase True

@@ -2509,6 +2509,8 @@ IPA.custom_checkboxes_widget = function(spec) {

var that = IPA.checkboxes_widget(spec);

that.set_value_to_lowercase = spec.set_value_to_lowercase || false;
Copy link
Member

Choose a reason for hiding this comment

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

When the default is changed, it needs to be changed also in the other widget where it was used: aci.attributes_widget

Without it, following command will break Web UI: ipa permission-mod AA_foo --attrs=businessCategory

Pavel Vomacka added 2 commits March 8, 2017 12:24
Add new attribute which keeps information whether each text added
using custom_checkbox_widget shoud be transformed to lowercase.

Part of: https://fedorahosted.org/freeipa/ticket/6308
All values were previously converted to lowercase which was not
coresponding with CLI behaviour. Now they stay as they are
inserted. I also have to change the strings to lowercase because
the otp and radius should be inserted as lowercase words.

https://fedorahosted.org/freeipa/ticket/6308
@pvoborni
Copy link
Member

pvoborni commented Mar 8, 2017

ACK but I've find out that the change is not enough because of existing bug. See pr #554

@pvoborni pvoborni added the ack Pull Request approved, can be merged label Mar 8, 2017
@ghost ghost added the pushed Pull Request has already been pushed label Mar 8, 2017
@ghost
Copy link

ghost commented Mar 8, 2017

master:

  • 0220fc8 WebUI: Allow disabling lowering text in custom_checkbox_widget
  • ad34510 WebUI: don't change casing of Auth Indicators values

@ghost ghost closed this Mar 8, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
2 participants