Correct field names for reports #14905

jntullo · 2017-04-26T18:48:11Z

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

When creating a report, the field names were not being stored properly in col_options because of how the UI was parsing the field. For example, the field Vm.miq_provision.miq_request-requester_name was being parsed as miq_request.v_approved_by instead of the correct miq_provision.miq_request.v_approved_by, causing the MiqExpression to never evaluate to true and therefore not styling the report correctly.

This PR adds in an instance method to MiqExpression::Field & MiqExpression::Tag that the UI can leverage (ManageIQ/manageiq-ui-classic#1170) to properly retrieve the column/field name.

@miq_bot add_label bug, reporting
@miq_bot assign @gtanzillo
cc: @imtayadeway

imtayadeway · 2017-04-26T21:49:14Z

lib/miq_expression/field.rb

@@ -18,6 +18,12 @@ def self.parse(field)
        parsed_params[:virtual_custom_column])
  end

+  def self.get_col_name(field)
+    return field.split('.').last.tr('-', '.') unless is_field?(field)


I'm curious as to why this is needed - could you add a test to demonstrate?

imtayadeway · 2017-04-26T21:52:38Z

lib/miq_expression/field.rb

+  def self.get_col_name(field)
+    return field.split('.').last.tr('-', '.') unless is_field?(field)
+    parsed_field = parse(field)
+    parsed_field.associations.push(parsed_field.column).join('.')


Using .push here I think will corrupt the associations data on the field object. That's OK here because the object is short-lived but might lead to some confusion if this changes....the non-destructive + [other] might be better?

Maybe we could add an instance method that encapsulates this presentation form of the field with a possibly better name than get_col_name (which I know is not yours 😉). Something like to_human could be consistent with other parts of MiqExpression (i.e. "this is the form that we want to present in the UI"), WDYT?

imtayadeway · 2017-04-27T17:43:03Z

spec/lib/miq_expression/field_spec.rb

@@ -77,6 +77,16 @@
    end
  end

+  describe '#parse_report_field' do
+    it 'returns the correct format for a non field' do
+      expect(MiqExpression::Field.parse_report_field('Vm.managed-environment')).to eq('managed.environment')


Ah! This is because it's a tag? I think you want MiqExpression.parse_field_or_tag for that 😁

imtayadeway · 2017-04-27T19:01:55Z

lib/miq_expression/tag.rb

@@ -19,6 +19,7 @@ def self.parse(field)

  def initialize(model, associations, column, managed = true)
    super(model, associations, column)
+    @managed = managed
    @namespace = "/#{managed ? MANAGED_NAMESPACE : USER_NAMESPACE}/#{column}"


I think namespace is not quite right - it includes more than the namespace. Could this be changed so that you're encapsulating this knowledge (which namespace to use)? namespace is its public API though, so I don't know how far reaching this would be

@imtayadeway hmm not sure what you mean. Do you mean taking the namespace and translating it into the report_column?

Sorry 😊 - I'd expect that namespace would return simply MANAGED_NAMESPACE or USER_NAMESPACE as the names suggest - currently it's doing way more than that which means you can't reuse namespace in your method below, and you have to grab the managed value to do the same logic with it again. I'd suggest we either:

a) Change #namespace to return the unmodified namespace, and give the thing it's currently doing a new name, or
b) Give the unmodified namespace a name (base_namespace, or similar) that both namespace and your new method can delegate to

Given the choice, I'd prefer a over b even though it may be more work ( 😀 ) because I think having a thing called namespace and things called MANAGED_NAMESPACE and USER_NAMESPACE being different concepts sort of introduces some cognitive dissonance for me. It seems like there's a missing concept that hasn't been named!

jntullo · 2017-04-28T14:40:40Z

lib/miq_expression/tag.rb

@@ -19,11 +20,12 @@ def self.parse(field)

  def initialize(model, associations, column, managed = true)
    super(model, associations, column)
-    @namespace = "/#{managed ? MANAGED_NAMESPACE : USER_NAMESPACE}/#{column}"
+    @namespace = managed ? MANAGED_NAMESPACE : USER_NAMESPACE
+    @path = "/#{namespace}/#{column}"


@imtayadeway what would be the proper name here? I chose path but...am not really sure (naming problems 😩 )

I think looking at your other comments (b) might be more appropriate here, at least for now?

jntullo · 2017-04-28T14:43:48Z

lib/miq_expression/tag.rb

  end

  def contains(value)
-    ids = model.find_tagged_with(:any => value, :ns => namespace).pluck(:id)
+    ids = model.find_tagged_with(:any => value, :ns => path).pluck(:id)


@imtayadeway want to get your thoughts here...I agree with the name change of namespace, but there are many references to namespace and even just ns that should be changed to make all of them align / the name change make sense. I think that is getting out of the scope of this issue. Should we leave them for now and refactor later to make all of the changes in a separate PR, or just do this namespace name change in its entirety in a separate PR?

I think as this is a bug I would do the simplest thing to get it to 💚 , perhaps let's look at naming in a follow up PR? :D

@imtayadeway agreed! thanks for the 👀

imtayadeway · 2017-04-28T15:13:17Z

lib/miq_expression/field.rb

@@ -77,6 +77,10 @@ def arel_attribute
    target.arel_attribute(column)
  end

+  def report_column


Although I like this name way better, I'm just thinking that these are used in other parts of the UI than reporting - anywhere there is an expression builder. I wonder if there could be a more generic name for this. @gtanzillo do you have any ideas?

miq-bot · 2017-05-22T15:21:34Z

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

miq-bot · 2017-05-22T20:18:02Z

Checked commits jntullo/manageiq@5fd2d55~...49905e5 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

chessbyte · 2017-06-02T03:58:13Z

@jntullo @gtanzillo @imtayadeway bump

gtanzillo

👍 LGTM

jntullo · 2017-06-07T20:40:50Z

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

Correct field names for reports (cherry picked from commit 8db3935) https://bugzilla.redhat.com/show_bug.cgi?id=1460761

simaishi · 2017-06-12T16:15:19Z

Fine backport details:

$ git log -1
commit 623ae1f1897c43fb5a5162657c0d905d718b6491
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Mon Jun 5 23:03:00 2017 -0400

    Merge pull request #14905 from jntullo/bz/report_styles
    
    Correct field names for reports
    (cherry picked from commit 8db3935e68455a29943f89db1f40506b1e7269e9)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1460761

simaishi · 2017-06-30T19:41:30Z

@jntullo There are conflicts backporting to Euwe in all 4 files... would you mind creating a separate PR for Euwe branch?

simaishi · 2017-07-07T19:32:48Z

Backported to Euwe via #15498

jntullo mentioned this pull request Apr 26, 2017

Parse the column/field name correctly in reports ManageIQ/manageiq-ui-classic#1170

Merged

chessbyte assigned gtanzillo Apr 26, 2017

chessbyte added bug core/reporting labels Apr 26, 2017

imtayadeway reviewed Apr 26, 2017

View reviewed changes

imtayadeway reviewed Apr 27, 2017

View reviewed changes

jntullo commented Apr 28, 2017

View reviewed changes

imtayadeway reviewed Apr 28, 2017

View reviewed changes

miq-bot added the unmergeable label May 22, 2017

Jillian Tullo added 3 commits May 22, 2017 15:30

update MiqExpression::Field to return col_name

5fd2d55

add parse_report_field specs

e2d893d

add instance method to field and tag

49905e5

miq-bot removed the unmergeable label May 22, 2017

gtanzillo approved these changes Jun 6, 2017

View reviewed changes

gtanzillo added this to the Sprint 62 Ending Jun 5, 2017 milestone Jun 6, 2017

gtanzillo merged commit 8db3935 into ManageIQ:master Jun 6, 2017

miq-bot added euwe/yes labels Jun 7, 2017

simaishi pushed a commit that referenced this pull request Jun 12, 2017

Merge pull request #14905 from jntullo/bz/report_styles

623ae1f

Correct field names for reports (cherry picked from commit 8db3935) https://bugzilla.redhat.com/show_bug.cgi?id=1460761

simaishi added fine/backported and removed fine/yes labels Jun 12, 2017

simaishi added the euwe/conflict label Jun 30, 2017

jntullo mentioned this pull request Jul 3, 2017

[EUWE] correct field names in reports #15498

Merged

simaishi removed the euwe/conflict label Jul 6, 2017

simaishi added euwe/backported and removed euwe/yes labels Jul 7, 2017

jntullo deleted the bz/report_styles branch November 28, 2017 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct field names for reports #14905

Correct field names for reports #14905

jntullo commented Apr 26, 2017 •

edited

Loading

imtayadeway Apr 26, 2017

imtayadeway Apr 26, 2017

imtayadeway Apr 26, 2017

imtayadeway Apr 27, 2017

imtayadeway Apr 27, 2017

jntullo Apr 27, 2017

imtayadeway Apr 27, 2017

jntullo Apr 28, 2017

imtayadeway Apr 28, 2017

jntullo Apr 28, 2017

imtayadeway Apr 28, 2017

jntullo Apr 28, 2017

imtayadeway Apr 28, 2017

miq-bot commented May 22, 2017

miq-bot commented May 22, 2017

chessbyte commented Jun 2, 2017

gtanzillo left a comment

jntullo commented Jun 7, 2017

simaishi commented Jun 12, 2017

simaishi commented Jun 30, 2017

simaishi commented Jul 7, 2017

Correct field names for reports #14905

Correct field names for reports #14905

Conversation

jntullo commented Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented May 22, 2017

miq-bot commented May 22, 2017

chessbyte commented Jun 2, 2017

gtanzillo left a comment

Choose a reason for hiding this comment

jntullo commented Jun 7, 2017

simaishi commented Jun 12, 2017

simaishi commented Jun 30, 2017

simaishi commented Jul 7, 2017

jntullo commented Apr 26, 2017 •

edited

Loading