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

Bug 1480819: Fix labels for Scale Infrastructure Provider form #4896

Merged
merged 1 commit into from
Jan 8, 2019

Conversation

sseago
Copy link
Contributor

@sseago sseago commented Nov 8, 2018

fixes https://bugzilla.redhat.com/show_bug.cgi?id=1480819

This commit changes the labels on the Scale Infrastructure Provider form
from the user-unfriendly parameter names (i.e. "ControllerCount") to
more descriptive labels (i.e. "Number of Controller Hosts").

@sseago
Copy link
Contributor Author

sseago commented Nov 8, 2018

scale-provider-before
Before the fix

@sseago
Copy link
Contributor Author

sseago commented Nov 8, 2018

scaling-after
After

@sseago
Copy link
Contributor Author

sseago commented Nov 8, 2018

scaling-validation
This validation was already in place, which seems to address the additional call for "guidance" in the BZ, at least in part.

field_name = param_name.dup
field_name.sub!("Count", "")
field_name.sub!("::count", "")
"Number of #{field_name.underscore.humanize.titleize} Hosts"
Copy link
Member

Choose a reason for hiding this comment

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

I see this is not i18n. Should we i18n at least line 8?
Ping @mzazrivec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's one thing I wondered about, but it wasn't clear to me what's normally done here when the text is dynamic like this -- would we just translate the static part and just interpolate the dynamic text?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is the set of possible values of field_name finite? If so, it would be worth to resolve this with a case like:

case filed_name
when 'value1'
  _('Number of such and such hosts')
when 'value2'
  _('Number of such and such and such hosts')
...
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For OpenStack it's a finite list. I imagine it's similar for any other providers that use this screen. While this would resolve the translation problem, it would introduce a couple of potential downsides:

  1. It introduces provider-specific text in the UI (Ceph, Compute, Controller, etc.) -- I know that in the past there has been some resistance to this, but perhaps if it's only strings and not classnames or other logic it's fine here.
  2. Other provider teams would need to review this as well to see what field_name values will be needed for their own providers

In any case, I imagine if we do this we'll still want to use the current automated generation of field name from this PR as a fallback for any field name values that are not found in the list. With the fallback in place, I can ignore other providers and just include the OpenStack fields, so nothing breaks for other use cases -- then, in subsequent PRs the other provider teams can supply any missing fields.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mzazrivec Hi, any thoughts on @sseago's comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think doing

case filed_name
when 'value1'
  _('Number of openstack type1 hosts')
when 'value2'
  _('Number of openstack type2 hosts')
else
  _("Number of %{host_type} Hosts") % {:host_type => field_name.underscore.humanize.titleize}
...
end

is perfectly fine here.

fixes https://bugzilla.redhat.com/show_bug.cgi?id=1480819

This commit changes the labels on the Scale Infrastructure Provider form
from the user-unfriendly parameter names (i.e. "ControllerCount") to
more descriptive labels (i.e. "Number of Controller Hosts").
@miq-bot
Copy link
Member

miq-bot commented Jan 8, 2019

Checked commit sseago@24509ed with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍪

@martinpovolny martinpovolny merged commit f0c76f6 into ManageIQ:master Jan 8, 2019
@martinpovolny martinpovolny added this to the Sprint 103 Ending Jan 21, 2019 milestone Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants