[chart]Pivot Table Rendering Superset 1.0

[lilila]

I have an issue with pivot table I didn't have using version 0.38.

I use to display links (either links or images) in table cells. It always worked in simple Table and it is still working.
With the previous version of superset I was able to display images as well within Pivot Table (it was necessary to add escape = False in df.to_html() in superset/viz.py)

With the current version (1.0) if if print what is returned in viz.py I have :

    <tr>
      <th>3</th>
      <td><a href="https://www.allocine.fr/film/fichefilm_gen_cfilm=275832.html" target="_blank"><img src="http://fr.web.img4.acsta.net/pictures/20/08/05/09/21/0019416.jpg",  height=60></a></td>
      <td>263184</td>
      <td><a href="http://www.allocine.fr/film/fichefilm_gen_cfilm=275832.html" target="_blank">ANTEBELLUM</a></td>
    </tr>

If I look at what is rendered in the chart, I have the following

<tr>
      <th>3</th>
      <td></td>
      <td>263k</td>
      <td>ANTEBELLUM</td>
    </tr>

[junlincc]

Thank you Lilila for reporting the issue, we confirm that it has been resolved in latest master by #12564 and apache-superset/superset-ui#880. we tagged the fix v1.0.1, which will be available to users soon. 🙏
@mayurnewase thank you for your fix!

[lilila]

Dear @junlincc ,

I pulled the 1.0.1 version and I still have empty cells where I should see images in Pivot Table. Do you have any suggestion?
Best,
Lise

[junlincc]

Hi Lise, sorry we misunderstood the issue. the PR i mentioned didn't cover this case🤦🏾‍♀️. we are looking into it and will push a fix soon! I apologize. @lilila

[mayurnewase]

Hi @lilila thanks for raising this.
I have raised pr to handle image rendering(see apache-superset/superset-ui#954), but I think setting escape to False may not be a good idea for security reasons, so you might need to continue doing it manually.

To display text with links would be bit difficult to handle as it will require html parsing and finding correct text inside and setting it seperately along with innerHtml property.
If this is absolute requirement it can be done at the cost of number,date,null formatting,by applying following patch in superset-ui repository and linking that plugin to main repo.

diff --git a/plugins/legacy-plugin-chart-pivot-table/src/PivotTable.js b/plugins/legacy-plugin-chart-pivot-table/src/PivotTable.js
index ec822dbc..97cd62e9 100644
--- a/plugins/legacy-plugin-chart-pivot-table/src/PivotTable.js
+++ b/plugins/legacy-plugin-chart-pivot-table/src/PivotTable.js
@@ -117,7 +117,7 @@ function PivotTable(element, props) {
             dateRegex,
             dateFormatter,
           );
-          $(this)[0].textContent = textContent;
+          //$(this)[0].textContent = textContent;
           $(this).attr = attr;
         }        
       });

[mayurnewase]

@junlincc what do you think?

[ktmud]

My guess is, without escape=False, even <img> wouldn't render, textContent will just return <img ... >.

As this hack (manual modification of superset/viz.py) is not officially supported by Superset, I'd be hesitant to add more hacks to support it (or even categorize this as a regression).

If we want to properly support rendering (limited) HTML in Pivot Table cells, we should start thinking of migrating it to the new API or at least move the HTML rendering to the front-end.

[villebro]

@ktmud I did actually test this with escape=False and it does work with @mayurnewase 's modification. I agree it's problematic to maintain support for customizations that aren't relevant for the current vanilla codebase, but I'm ok making this change for now to unblock orgs that have this need as I don't see any direct downsides to it.

[ktmud]

I guess one downside is if the fix is not clean, more hacks would mean more future maintenance cost and wasted work (especially if we are rewriting this soon anyway).

Luckily, a complete fix seems to be fairly direct in this case: apache-superset/superset-ui#954 (comment) So I'm OK with pushing this forward.