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

[RFE] Allow customizing product title and brand image through settings #17773

Merged
merged 3 commits into from
Aug 6, 2018

Conversation

skateman
Copy link
Member

The product name is stored in a YAML file that is not very convenient to edit. There's a "constant" around the i18n.t call that retrieves the name, however it was not used globally. So first I replaced all the i18n.t calls with the constant and then added an option to retrieve the product name from Settings.product.name that is editable through the advanced settings in OPS UI. Also added the missing custom_login_logo and the new custom_brand keys to the settings.

@miq-bot add_reviewer @epwinchell
@miq-bot add_reviewer @martinpovolny

RFE BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1471301

@epwinchell
Copy link
Contributor

\cc @dclarizio

@skateman skateman changed the title Allow customizing product title and brand image through settings [RFE] Allow customizing product title and brand image through settings Jul 30, 2018
Copy link
Contributor

@epwinchell epwinchell left a comment

Choose a reason for hiding this comment

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

Tested. Looks fine.
georgeseal

:name => I18n.t("product.name"),
:description => "#{I18n.t("product.name")} Default Organization"
:name => Vmdb::Appliance.PRODUCT_NAME,
:description => "#{Vmdb::Appliance.PRODUCT_NAME} Default Organization"
Copy link
Member

Choose a reason for hiding this comment

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

This looks like a missing i18n not getting any better with this PR

ping @mzazrivec

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this part of the backend code creates objects in the embedded ansible instance and I'd say there's no point in applying i18n here.

Copy link
Member

Choose a reason for hiding this comment

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

@carbonin Can you just review this particular file just in case?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, this is fine 👍

@Fryguy Fryguy assigned Fryguy and unassigned mzazrivec Aug 1, 2018
@@ -974,6 +974,7 @@
:keep_archived_quotas: 6.months
:purge_window_size: 1000
:product:
:name: null
Copy link
Member

Choose a reason for hiding this comment

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

Just leave this blank

:name: 

Copy link
Member

Choose a reason for hiding this comment

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

Where will we put "Red Hat CloudForms" if this is the approach? Right now we create a config/settings/production.yml that is layered on settings.yml, but I think it's layered after settings.yml. If the user wants to "remove" that value, where do you think they will enter this value?

Copy link
Member Author

Choose a reason for hiding this comment

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

The productization of this string is solved with i18n.t here, so if this value is not set, it will behave as it behaves right now. Does this answer your question @Fryguy?

Copy link
Member

@Fryguy Fryguy left a comment

Choose a reason for hiding this comment

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

@miq-bot
Copy link
Member

miq-bot commented Aug 1, 2018

Checked commits skateman/manageiq@a08c049~...2d3b069 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 🏆

@@ -1001,6 +1002,8 @@
:console_proxy_port:
:start: 6000
:end: 7000
:custom_brand: false
:custom_login_logo: false
Copy link
Member

Choose a reason for hiding this comment

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

I notice this is added to the :server key as opposed to :product. Are we expecting a different custom brand per server?

Copy link
Member

Choose a reason for hiding this comment

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

What will these be used for? I don't see them elsewhere in the PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is already being used and missing, I added this for the sake of consistency.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was not really thinking about the location, added the missing and the needed key to the same place where the existing custom_logo was.

@Fryguy Fryguy merged commit 4e2609e into ManageIQ:master Aug 6, 2018
@Fryguy Fryguy deleted the product-title branch August 6, 2018 18:24
@Fryguy Fryguy added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 6, 2018
@skateman
Copy link
Member Author

@miq-bot add_label fine/yes, gaprindashvili/yes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants