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.

Sign up for GitHub

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

Jump to bottom

Tenant options instead of free text #690

Merged
merged 1 commit into from
Mar 22, 2017
Merged

Tenant options instead of free text #690

merged 1 commit into from
Mar 22, 2017

Conversation

yaacov
Copy link
Member

@yaacov yaacov commented Mar 15, 2017 •
edited
Loading

Description

Compute > Containers > Providers - pick a provider - toolbar Monitoring > Ad hoc Metrics

Make _system and _ops easier to discover. Make the tenant html input element a select instead of type-ahead free text input.

(*) maybe using nice labels "System" and "Operators", or anything more descriptive)

Screenshot
gifrecord_2017-03-16_133854

@yaacov
Copy link
Member Author

yaacov commented Mar 15, 2017

@miq-bot add_label compute/containers

@simon3z @zeari @himdel please review

@yaacov yaacov force-pushed the tenant-combobox branch 6 times, most recently from 78ba391 to 0f4ab04 Compare March 15, 2017 12:37
@simon3z
Copy link
Contributor

simon3z commented Mar 15, 2017

@yaacov can we have an hard-coded table to have nice strings for "System" (_system) and "Operations" (_ops)?

@yaacov
Copy link
Member Author

yaacov commented Mar 15, 2017

@yaacov can we have an hard-coded table to have nice strings for "System" (_system) and "Operations" (_ops)?

@simon3z Yes we can :-) I am on it

@yaacov yaacov force-pushed the tenant-combobox branch 3 times, most recently from 96bec3f to 0612c9b Compare March 15, 2017 15:20
@yaacov
Copy link
Member Author

yaacov commented Mar 15, 2017

@yaacov can we have an hard-coded table to have nice strings for "System" (_system) and "Operations" (_ops)?

@simon3z done :-) updated screenshot in description.

@yaacov yaacov force-pushed the tenant-combobox branch 3 times, most recently from 32892f1 to e43c434 Compare March 15, 2017 15:56
simon3z
simon3z reviewed Mar 15, 2017
View reviewed changes
spec/javascripts/fixtures/json/container_live_dashboard_tenant_response.json Outdated
"tenants":
[
{"label": "System", "value": "_system"},
{"label": "Ops", "value": "_ops"},
Copy link
Contributor

Choose a reason for hiding this comment

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

@yaacov I think we can call this "Operations".

@yaacov
Copy link
Member Author

yaacov commented Mar 15, 2017

@yaacov I think we can call this "Operations".

@simon3z 👍 done

simon3z
simon3z reviewed Mar 15, 2017
View reviewed changes
app/services/hawkular_proxy_service.rb Outdated
def humanize(id)
special_cases = {
"_system" => "System",
"_ops" => "Operations"
Copy link
Contributor

@simon3z simon3z Mar 15, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@yaacov isn't this duplicating what you have in container_live_dashboard_tenant_response.json? Is there a way to unify this so we won't have multiple places to update?

Update: my mistake... that's a spec file sorry.

simon3z
simon3z reviewed Mar 15, 2017
View reviewed changes
app/services/hawkular_proxy_service.rb Outdated
"_ops" => "Operations"
}

if special_cases.key?(id)
Copy link
Contributor

Choose a reason for hiding this comment

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

@yaacov why all this complex logic. If it's in the special_cases (rename and make it a constant) then use that, otherwise just use the plain hawkular id.

Copy link
Member Author

Choose a reason for hiding this comment

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

@simon3z 👍 yes, it was just .humenize at the beginning, and then things escalated a little :-)
I'll fix it tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

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

@simon3z Done :-)

@yaacov yaacov force-pushed the tenant-combobox branch 2 times, most recently from c4759b7 to 995b599 Compare March 16, 2017 07:20
simon3z
simon3z reviewed Mar 16, 2017
View reviewed changes
app/services/hawkular_proxy_service.rb Outdated
@@ -1,6 +1,15 @@
class HawkularProxyService
include UiServiceMixin

MAX_LEN = 25
SPECIAL_CASES = {
Copy link
Contributor

Choose a reason for hiding this comment

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

@yaacov I think should be more meaningful of "special cases" (think of other engineers that are not familiar with what you're doing... special cases of what?)

Copy link
Member Author

Choose a reason for hiding this comment

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

@simon3z 👍 right ! changed to tenant_label_special_cases

@yaacov yaacov closed this Mar 16, 2017
@yaacov yaacov reopened this Mar 16, 2017
@yaacov yaacov mentioned this pull request Mar 19, 2017
Closed
@yaacov yaacov force-pushed the tenant-combobox branch 2 times, most recently from e5ad13c to d9e1118 Compare March 22, 2017 12:06
@yaacov yaacov mentioned this pull request Mar 22, 2017
Merged
@yaacov
Copy link
Member Author

yaacov commented Mar 22, 2017

@simon3z @cben PTAL ,
@martinpovolny asked that we LGTM this , to make it easy for @h-kataria @mzazrivec to look and merge it ( @himdel that knows this code, is on PTO until the end of the week )

@cben
Copy link
Contributor

cben commented Mar 22, 2017

LGTM

@miq-bot
Copy link
Member

miq-bot commented Mar 22, 2017

This pull request is not mergeable. Please rebase and repush.

@h-kataria
Copy link
Contributor

@yaacov please rebase/repush.

@yaacov
Copy link
Member Author

yaacov commented Mar 22, 2017

@yaacov please rebase/repush.

@h-kataria 👍 conflicts resolved :-)

@yaacov yaacov mentioned this pull request Mar 22, 2017
Merged
@miq-bot
Copy link
Member

miq-bot commented Mar 22, 2017

Checked commit yaacov@a5c4944 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks good. 🍪

@h-kataria
Copy link
Contributor

@simon3z @yaacov can you set EUWE labels and bug/enhancement labels on this PR.

@h-kataria h-kataria added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 22, 2017
@h-kataria h-kataria merged commit 59dde9d into ManageIQ:master Mar 22, 2017
@yaacov
Copy link
Member Author

yaacov commented Mar 22, 2017

@simon3z @yaacov can you set EUWE labels and bug/enhancement labels on this PR.

@miq-bot add_label euwe/no enhancement

@miq-bot
Copy link
Member

miq-bot commented Mar 22, 2017

@yaacov Cannot apply the following label because they are not recognized: euwe/no enhancement

@yaacov
Copy link
Member Author

yaacov commented Mar 22, 2017

@miq-bot add_label euwe/no, enhancement

@yaacov yaacov mentioned this pull request Mar 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simon3z simon3z simon3z left review comments

Assignees

@h-kataria h-kataria

Labels
compute/containers enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@yaacov @simon3z @cben @miq-bot @h-kataria