Add 'click' functionality for Chart visualization

[ufedor]

What type of PR is this? (check all applicable)

• [ X ] Feature

Description

I've added a small improvement: click event reaction for Chart visualiztion, opening template-based url with clicked point data substitutions. All the work is done by adding regular 'plotly_click' event handler.

It can be configured from the Visualization Editor. At the bottom of General tab I've added next additional params:

• "Enable link" check box, enabling link functionality for Chart in general
• "Open in new tab" check box
• "URL Template" - works like a Data Labels templates

URL template

Every curve can be referenced using {{ @@x1 }} {{ @@y1 }} {{ @@x2 }} {{ @@y2 }} ... syntax:
axis with any curve number according to the Series config.

The first met curve X and Y values can be referenced by just{{ @@x }} {{ @@y }} syntax.
Any unresolved reference would be replaced with an empty string.

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

[susodapop]

I tested this out and it works quite well. Left a few comments for you.

Overall, can we make the link template to work like HTML links in the table visualisation? Although @@x1 works, it's more natural to use {{ column_name }}.

This is especially true because hovering over the chart doesn't display a preview of the URL that will be visited. Thoughts?

[ufedor]

Overall, can we make the link template to work like HTML links in the table visualisation? Although @@x1 works, it's more natural to use `{{ column_name }}.

I haven't found a reliable way to get value by column_name at the event handler point. ((
Sometimes columns could be named with Series, sometimes - no. With different templating for bars and for pie.
Honestly I'm just dont know how to achive this {{column_name}} mapping.
For other two comments - all ok. Let's have "" instead of "(nothing)" and have "click events".

[ufedor]

First check finished with error had found something not related to the pull request changes.
Is this pull request blocked with that check ?
What should be done next ?

[guidopetri]

@ufedor , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI?

We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that.

[codecov]

Codecov Report

Merging #5662 (d58a242) into master (b73d68f) will increase coverage by 0.06%.
Report is 6 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5662      +/-   ##
==========================================
+ Coverage   60.77%   60.84%   +0.06%     
==========================================
  Files         154      155       +1     
  Lines       12656    12714      +58     
  Branches     1721     1728       +7     
==========================================
+ Hits         7692     7736      +44     
- Misses       4739     4750      +11     
- Partials      225      228       +3     

see 4 files with indirect coverage changes

[justinclift]

@eradman @gabrieldutra Would either of you be up for reviewing this one?

It's been rebased, and is passing our CI tests. 😄

[eradman]

This seems to work nicely.

This adds two new keys in visualization.options

• enableLink (bool)
• linkFormat (text)

Look into adding viz-lib tests

[ufedor]

Hi guys.
Thanks for your support and assistance.
You are fast as lightning. I was just about to reply that, yes, this PR is really needed, just about to go and figure out how a rebase can be done, and now you have already done everything, and I feel like I'm fast as a sloth.

Is there anything else I can help with?

[justinclift]

@ufedor Cool. Yep, there is. Eric has reviewed this PR and asked about a few adjustments.

When you have time, please look over those and update things or discuss them further as makes sense. 😄

[ufedor]

Hi guys.
I've reviewed couple of changes.
Also I checked how tests are written in order to try to do similar ones in a single concept. I found that it uses an enzyme library that I have never worked with before. I didn’t work with tests at all, to be honest.
Can we skip creating tests in this case?
If we can’t move forward without tests, please tell me which component/test file have good enough tests to be a donor so that it would be easiest to adopt and use them to test my changes. I'll try to figure it out gradually.

[justinclift]

Looking at the test failures, at least some of them seem quite important to fix. For example, looking at the "backend-unit-tests" one, it has this:

   File "/app/redash/query_runner/databend.py", line 4, in <module>
    from databend_sqlalchemy import connector
  File "/usr/local/lib/python3.8/site-packages/databend_sqlalchemy/connector.py", line 11, in <module>
    from databend_py import Client
  File "/usr/local/lib/python3.8/site-packages/databend_py/__init__.py", line 1, in <module>
    from .client import Client
  File "/usr/local/lib/python3.8/site-packages/databend_py/client.py", line 4, in <module>
    from .connection import Connection
  File "/usr/local/lib/python3.8/site-packages/databend_py/connection.py", line 16, in <module>
    headers = {'Content-Type': 'application/json', 'User-Agent': sdk_info(), 'Accept': 'application/json',
  File "/usr/local/lib/python3.8/site-packages/databend_py/sdk_info.py", line 18, in sdk_info
    return f"{sdk_lan()}/{sdk_version()}"
  File "/usr/local/lib/python3.8/site-packages/databend_py/sdk_info.py", line 8, in sdk_version
    with open(version_py, encoding='utf-8') as f:
FileNotFoundError: [Errno 2] No such file or directory: '/usr/local/lib/python3.8/site-packages/databend_py/VERSION'

That "no such file or directory" seems pretty weird though. Maybe it was a bad install during the test run?

I'll re-run the failed tests, so we can hopefully figure out if it was just some weirdness with that test run, or if it was indeed showing a real problem. 😄

[justinclift]

Hmmm, that test failure doesn't appear to be caused by this PR. Looks like it's coming from something else.

Investigating...

[justinclift]

k. Fixed that failure that was causing our CI tests to fail for everyone. So, lets see what happens with the CI tests here now... 😄

[justinclift]

CI tests are all passing after all. 😄

@eradman What do you reckon?

[eradman]

@ufedor take a look at viz-lib/src/visualizations/chart/Editor/GeneralSettings.test.tsx

I don't think this needs extensive automated testing, but there should be a test that toggles the feature.

[ufedor]

@eradman , thank you for hint.
Finally checkbox toggle test - added, enableLink=true - inverted.

[eradman]

@ufedor looks good overall, but one test failure to investigate

[ufedor]

Please help me to update test snapshot to pass frontend-unit tests...

[eradman]

I'll look into this a bit later. This test is not failing for me locally.

[eradman]

It looks like my local tests pass because viz-lib/src/visualizations/chart/Editor/__snapshots__/GeneralSettings.test.tsx.snap was updated to reflect the new state set in the test.

Adding this regenerated file to this PR should fix the frontend test failure.

[eradman]

All good!