Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor the CSV export for CSP reasons #1453

Merged
merged 1 commit into from
May 24, 2022
Merged

Conversation

mgwalker
Copy link
Member

@mgwalker mgwalker commented May 5, 2022

Description

Tock's CSP policy disallows inline Javascript, so onclick="" attributes are forbidden. Those attributes are how CSV exports are triggered on the analytics page. The handler relies on a random pseudo ID that is generated at render time to identify the table being exported.

This PR removes the onclick attribute from the button, and adds data-csv, data-csv-id, and data-csv-name attributes. The data-csv attribute is just a flag that the button should act as a CSV exporter. data-csv-id and data-csv-name are populated with the pseudo ID and filename hint, respectively.

Then, after the DOM is loaded, this PR adds code that finds all button elements with a data-csv attribute. It iterates over them, pulling out their IDs and names, and adding a click event listener. It uses the ID and name to finally call the download_table_as_csv function after the button is cilcked.

Additional information

@mgwalker mgwalker requested a review from neilmb May 5, 2022 21:22
// Quick and simple export target #table_id into a csv
// inspired by https://stackoverflow.com/a/56370447
function download_table_as_csv(table_id, filename_prefix) {
var csv_string = table_to_csv(table_id);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Javascript changes from here down are where I ran prettier on the code. I tried to configure prettier to match the existing style instead of introducing a bunch of whitespace changes. It seems like most of the changes are whitespace alignment or semicolons.

@codecov-commenter
Copy link

Codecov Report

Merging #1453 (9f6fbda) into main (e6cfad2) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1453   +/-   ##
=======================================
  Coverage   90.11%   90.11%           
=======================================
  Files          51       51           
  Lines        2297     2297           
=======================================
  Hits         2070     2070           
  Misses        227      227           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e6cfad2...9f6fbda. Read the comment docs.

Copy link
Member

@neilmb neilmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a very nice solution to the CSP issue.

@mgwalker mgwalker merged commit 10d6098 into main May 24, 2022
@mgwalker mgwalker deleted the mgwalker/1418-csv-export branch May 24, 2022 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Export CSV functionality violates CSP policy
3 participants