Manageiq-ui-classic: HTML in Container Build's textual summary table

[hstastna]

There is HTML code in the _Build Instances_ table in textual summary of selected _Container Build_.

Also the whole _Build Instances_ looks weird, it looks like there is not enough space for the info. IMHO, I would move the whole table to the left and change the width, or put info into the rows, not columns -> we would achieve more consistency with other tables and better visual experience.

Steps to reproduce:

1. _Compute > Containers > Container Builds_
2. Click on some Build in the list and see:
   

[himdel]

textual_group_build_instances uses collect_build_pods to get the row data,
which uses link_to_pod to create the link, and that outputs the html. I think that part is as it should be.

How it worked in the ruby version is that link_to returns html_safe strings, which would get automatically rendered as html by haml, as opposed to the non-html_safe strings which get escaped.

The whole table is a TextualMultilabel, which now uses the React SimpleTable component.
But, when the data is converted to json, we lose the html_safe flag, thus the JS component can't distinguish strings that are html_safe on ruby side (and thus should be rendered as html), from strings that are not and should be escaped, and escapes everything.

In SimpleTable, generateRows just does <TableCell {...props} key={${index}-${key}}>{row[key]}</TableCell> (see EDIT below, but same issue really), which will always escape strings, so we would need the row[key] value to be a react element instead of a string for this to work.

---

So... I think we may need to revert c04b811a2628c3a44862ca6e5142b24fff0ce80e, create a global (js) transform function to convert each row[4] to <div dangerouslySetInnerHTML={{ _html: originalValue }} />, add a way for TextualMultilabel to optionally pass a transform prop in locals, and pass that function name via that from ContainerBuildHelper::TextualSummary to the react code.

(That, or possibly allow overriding SimpleTable with another component, and create a ContainerBuildTable which wraps it and provides the transform directly.)

EDIT: correction SimpleTable used by textual summaries comes from react-ui-components and is not the same SimpleTable we have in ui-classic :(. So, no transform function is actually available, but it does seem to support objects with .value and .expandable and .link, in addition to text. So maybe we can add objects with .html too.

(So, apart from changing the link_to code to wrap the html in an extra hash, the rest of the fix is react-ui-components only.)

[himdel]

BTW second bug, the same screen also complains about:

Warning: Failed prop type: Invalid prop `labels[7]` of type `object` supplied to `SimpleTable`, expected `string`.

So, looks like we've lost support for {:value => _("Completion Timestamp"), :sortable => :desc} in TextualMultilabel labels as a label. (Cc @martinpovolny :))

[martinpovolny]

I do not want to support markup inside the fields (and dangerouslySetInnerHTML).

I'd rather pass structured data for a cell if needed (text, url). I am pretty sure we already do that in some of the TextualSummary classes.

I prefer to have a more strict structure of the data that is displayed rather that making it possible to render anything.

The issue is a result of a situation where someone was passing something "strange" in the data and did not write a test for it. I would like to keep things structured and explicit to make sure they do not break in the future.

[martinpovolny]

TableListView which is part of the TextuaSummary:

https://react-ui-components.surge.sh/?selectedKind=TextualSummary&selectedStory=TableListView&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybook%2Fstories%2Fstories-panel

supports links:

./table_list_view.jsx:  row.link ? clickableRow(row, i, colOrder, rowLabel, onClick) : simpleRow(row, i, colOrder)

But that is a link for the whole line. Not sure if that is applicable here.

MultiLinkTable:
https://react-ui-components.surge.sh/?selectedKind=TextualSummary&selectedStory=MultilinkTable&full=0&addons=1&stories=1&panelRight=0&addonPanel=storybook%2Fstories%2Fstories-panel

seems to support different links (or no links) for different columns.

So might be that all is needing is to switch from one component for displaying the group to another.

It's already some time since I did this, I do not remember exactly :-(

[himdel]

Makes sense,
SimpleTable does support value.link + value.value:

    } else if ((value != null) && (typeof value === 'object') && value.link) {
      // value with a link
      return <td onClick={e => onClick(value, e)} key={j}>{value.value}</td>;

So...

- link_to(xxx)
+ {:value => "something", :link => url_for_only_path(xxx)}

should do the trick :)

[himdel]

So, mixed results .. it works, but no longer looks anything like a link :(...

diff --git a/app/helpers/container_build_helper/textual_summary.rb b/app/helpers/container_build_helper/textual_summary.rb
index c5bd3b5d6..7222d3f44 100644
--- a/app/helpers/container_build_helper/textual_summary.rb
+++ b/app/helpers/container_build_helper/textual_summary.rb
@@ -81,10 +81,10 @@ module ContainerBuildHelper::TextualSummary

   def link_to_pod(container_group)
     if container_group
-      link_to(container_group.name,
-              :action     => 'show',
-              :controller => 'container_group',
-              :id         => container_group.id)
+      {:value => container_group.name,
+       :link  => url_for_only_path(:action     => 'show',
+                                   :controller => 'container_group',
+                                   :id         => container_group.id)}
     else
       _("No Pod")
     end

before:

after:

[hstastna]

So let's sum up this all:

TODO:

• [x] replace link_to by value.link + value.value on ruby side https://github.com/ManageIQ/manageiq-ui-classic/pull/6575 (merged)
• [x] make the link in the table displayed as a link (edit SimpleTable React UI component) (the PR)
• [x] add example to the storybook (react-ui-components) (the PR)
• [x] fix the warning, probably a proptype issue https://github.com/ManageIQ/react-ui-components/pull/164
• [x] make the _Build Instances_ table wide (+ sort instances in SQL) https://github.com/ManageIQ/manageiq-ui-classic/pull/6577 (merged)

[hstastna]

@miq-bot add_label bug

[martinpovolny]

To make a group wide, one of it's members need's to have the wide property set to true.

The code is here: https://github.com/ManageIQ/react-ui-components/blob/master/src/textual_summary/textual_row.jsx#L9

It was added here:
https://github.com/ManageIQ/react-ui-components/pull/42
but I don't see it used which is strange.

Also there's an unresolved comment about cursor shape in that PR what we might have to address.

PR to make the "Build Instances" wider using the wide property:
https://github.com/ManageIQ/manageiq-ui-classic/pull/6577

[hstastna]

After merging the last PR https://github.com/ManageIQ/react-ui-components/pull/163 the result will look like this:

[hstastna]

I'm closing the issue as all of the appropriate PRs are merged.