Descriptive custom attribute columns #14391

zeari · 2017-03-17T20:04:25Z

update of #11916
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1416220
Completed on the UI side by ManageIQ/manageiq-ui-classic#733
does version -> Labels: version in report columns.

@lpichler Please review
cc @simon3z

@miq-bot add_label euwe/yes, chargeback, providers/containers

simon3z · 2017-03-19T08:24:47Z

@zeari I added some comments on ManageIQ/manageiq-ui-classic#733 that are relevant here as well (adding the section in the field name, e.g. Custom Attribute: label : version and adding tests).

zeari · 2017-03-19T13:33:42Z

app/models/mixins/custom_attribute_mixin.rb

@@ -25,7 +25,7 @@ module CustomAttributeMixin
    end

    def self.custom_keys
-      CustomAttribute.where(:resource_type => base_class).distinct.pluck(:name).compact
+      CustomAttribute.where(:resource_type => base_class).distinct.pluck(:section, :name).compact
    end


@lpichler I couldnt find any other place in the code that uses this particular method:custom_keys other than _custom_details_for in miq_expression.rb changed below.
So i hope this is reasonable.

I didnt go into it too far but i think reporting doesnt take into account that there could be two custom attributes with the same name but with different section belonging to the same record.

zeari · 2017-03-19T13:57:57Z

@zeari I added some comments on ManageIQ/manageiq-ui-classic#733 that are relevant here as well (adding the section in the field name, e.g. Custom Attribute: label : version and adding tests).

@simon3z

Can CustomAttribute.section be nil? we need to take this into consideration since this change is across all of reporting
I think labels: version is better than Custom Attribute: label : version since 'CustomAttribute' doesnt mean much to the user

simon3z · 2017-03-20T09:00:42Z

Can CustomAttribute.section be nil? we need to take this into consideration since this change is across all of reporting

I don't think a section can be nil (I can't imagine what would mean). Anyway @gtanzillo and @blomquisg should have more information.

I think labels: version is better than Custom Attribute: label : version since 'CustomAttribute' doesnt mean much to the user

Yes, I don't think it we should expose "Custom Attribute".

LGTM 👍
@miq-bot assign gtanzillo

simon3z · 2017-03-20T09:01:11Z

@lpichler can you review this?

Fryguy · 2017-03-20T16:23:00Z

lib/miq_expression.rb

@@ -1172,11 +1172,11 @@ def self._custom_details_for(model, options)
    custom_attributes_details = []

    klass.custom_keys.each do |custom_key|
-      custom_detail_column = [model, CustomAttributeMixin::CUSTOM_ATTRIBUTES_PREFIX + custom_key].join("-")
-      custom_detail_name = custom_key
+      custom_detail_column = [model, CustomAttributeMixin::CUSTOM_ATTRIBUTES_PREFIX + custom_key[1]].join("-")


Instead of array indexing, it might be more readable to do

klass.custom_keys.each do |custom_section, custom_key|

and then use the named variables.

Fryguy · 2017-03-20T16:27:26Z

@lpichler Please also review with respect to reporting and how custom attributes can be filtered against.

Can CustomAttribute.section be nil? we need to take this into consideration since this change is across all of reporting

@zeari Yes, it can be nil. The key/value part (and maybe source) are the only columns that matter. Section is extra, but is not necessarily needed. In my mind I think of this table like an ini file, where the section is optional, and the source would be like the ini file name.

miq-bot · 2017-03-20T17:48:10Z

Checked commit zeari@a26fa1e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 👍

lpichler

👍 finalized with @zeari
tested:
-reporting & UI reporting definition
-reporting with filtering (expression editor)

reporting with filtering with field which are not listed in selected field.

zeari · 2017-03-20T18:09:07Z

@Fryguy We are letting go of section for now. I think this is ready for merge once travis completes.

simon3z

LGTM 👍
Ready for merge

@Fryguy @blomquisg @chessbyte @gtanzillo anyone around to merge this?
cc @chrispy1

…umns2 Descriptive custom attribute columns (cherry picked from commit ab3ce4a) https://bugzilla.redhat.com/show_bug.cgi?id=1434160

simaishi · 2017-03-20T20:51:59Z

Euwe backport details:

$ git log -1
commit 88c3b52d2dc28ed19e573e6bc79053398b909454
Author: Greg Blomquist <[email protected]>
Date:   Mon Mar 20 16:33:19 2017 -0400

    Merge pull request #14391 from zeari/descriptive_custom_attribute_columns2
    
    Descriptive custom attribute columns
    (cherry picked from commit ab3ce4a3302e4e49df94a6b263bdb251f59658aa)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1434160

zeari force-pushed the descriptive_custom_attribute_columns2 branch from 4bf9c25 to fe65569 Compare March 17, 2017 20:04

miq-bot added core/chargeback euwe/yes providers/containers labels Mar 17, 2017

zeari mentioned this pull request Mar 17, 2017

Add a 'Labels' prefix to custom attribute column in reports ManageIQ/manageiq-ui-classic#733

Merged

zeari force-pushed the descriptive_custom_attribute_columns2 branch 2 times, most recently from ee60b15 to bfa3077 Compare March 19, 2017 12:33

zeari commented Mar 19, 2017

View reviewed changes

zeari force-pushed the descriptive_custom_attribute_columns2 branch from bfa3077 to 0b1c0a3 Compare March 19, 2017 14:17

simon3z approved these changes Mar 20, 2017

View reviewed changes

zeari force-pushed the descriptive_custom_attribute_columns2 branch 2 times, most recently from 9e82858 to db66eba Compare March 20, 2017 11:42

chessbyte assigned gtanzillo Mar 20, 2017

chessbyte requested a review from lpichler March 20, 2017 13:14

Fryguy reviewed Mar 20, 2017

View reviewed changes

zeari force-pushed the descriptive_custom_attribute_columns2 branch 2 times, most recently from 26a8a53 to a26fa1e Compare March 20, 2017 17:43

change CustomAttribute columns display name in reports to 'Labels: name'

a26fa1e

lpichler approved these changes Mar 20, 2017

View reviewed changes

simon3z approved these changes Mar 20, 2017

View reviewed changes

blomquisg merged commit ab3ce4a into ManageIQ:master Mar 20, 2017

simaishi pushed a commit that referenced this pull request Mar 20, 2017

Merge pull request #14391 from zeari/descriptive_custom_attribute_col…

88c3b52

…umns2 Descriptive custom attribute columns (cherry picked from commit ab3ce4a) https://bugzilla.redhat.com/show_bug.cgi?id=1434160

simaishi added euwe/backported and removed euwe/yes labels Mar 20, 2017

This was referenced Apr 20, 2017

[WIP] Add sections to Virtual Custom Attributes #14823

Closed

Method for Human name of virtual custom attributes #14835

Merged

Generate virtual custom attributes with sections #14837

Merged

chessbyte added this to the Sprint 57 Ending Mar 27, 2017 milestone Jun 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Descriptive custom attribute columns #14391

Descriptive custom attribute columns #14391

zeari commented Mar 17, 2017 •

edited

Loading

simon3z commented Mar 19, 2017

zeari Mar 19, 2017 •

edited

Loading

zeari commented Mar 19, 2017 •

edited

Loading

simon3z commented Mar 20, 2017

simon3z commented Mar 20, 2017

Fryguy Mar 20, 2017

Fryguy commented Mar 20, 2017 •

edited

Loading

miq-bot commented Mar 20, 2017

lpichler left a comment

zeari commented Mar 20, 2017

simon3z left a comment

simaishi commented Mar 20, 2017

Descriptive custom attribute columns #14391

Descriptive custom attribute columns #14391

Conversation

zeari commented Mar 17, 2017 • edited Loading

simon3z commented Mar 19, 2017

zeari Mar 19, 2017 • edited Loading

Choose a reason for hiding this comment

zeari commented Mar 19, 2017 • edited Loading

simon3z commented Mar 20, 2017

simon3z commented Mar 20, 2017

Fryguy Mar 20, 2017

Choose a reason for hiding this comment

Fryguy commented Mar 20, 2017 • edited Loading

miq-bot commented Mar 20, 2017

lpichler left a comment

Choose a reason for hiding this comment

zeari commented Mar 20, 2017

simon3z left a comment

Choose a reason for hiding this comment

simaishi commented Mar 20, 2017

zeari commented Mar 17, 2017 •

edited

Loading

zeari Mar 19, 2017 •

edited

Loading

zeari commented Mar 19, 2017 •

edited

Loading

Fryguy commented Mar 20, 2017 •

edited

Loading