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

Encode exported Query file paths #5070

Open
wants to merge 4 commits into
base: production
Choose a base branch
from
Open

Encode exported Query file paths #5070

wants to merge 4 commits into from

Conversation

melton-jason
Copy link
Contributor

Fixes #5069

Note:
The Export process still completely fails if the Query name contains / (the character is interpreted as part of the file path).

Checklist

  • Self-review the PR after opening it to make sure the changes look good
    and self-explanatory (or properly documented)
  • Add relevant issue to release milestone

Testing instructions

  • Save and export query with # in the query's name
  • Export the query as a CSV/KML (without selected any Query Results)
  • Ensure the file can be downloaded and the full query name is in the downloaded file's name
  • Save and export query with ? in the query's name
  • Export the query as a CSV/KML (without selected any Query Results)
  • Ensure the file can be downloaded and the full query name is in the downloaded file's name

For other special characters: @$%&+?\=;,":

  • Save a Query containing one or more of the characters
  • Ensure that Query export files can still be downloaded, the entire query name is present in the file name (special characters are either present in the download file name or replaced with _)

@melton-jason melton-jason requested review from a team July 4, 2024 01:13
@melton-jason melton-jason modified the milestones: 7.9.7, 7.9.x Jul 6, 2024
Copy link
Member

@maxpatiiuk maxpatiiuk left a comment

Choose a reason for hiding this comment

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

looks good - do we need to do this anywhere else? (i.e other notifications that have files)

@melton-jason
Copy link
Contributor Author

looks good - do we need to do this anywhere else? (i.e other notifications that have files)

@maxpatiiuk
We could do this for other notifications with files, but this was not strictly needed as all other file names can not be generated with these characters: their file names contain no user input.

Though yes, it's probably best we eliminate this bug in the other places before it becomes a problem. I'll push a fix for those files too.

Copy link

@Areyes42 Areyes42 left a comment

Choose a reason for hiding this comment

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

Testing instructions

  • Save and export query with # in the query's name
  • Export the query as a CSV/KML (without selected any Query Results)
  • Ensure the file can be downloaded and the full query name is in the downloaded file's name
  • Save and export query with ? in the query's name
  • Export the query as a CSV/KML (without selected any Query Results)
  • Ensure the file can be downloaded and the full query name is in the downloaded file's name

For other special characters: @$%&+?\=;,":

  • Save a Query containing one or more of the characters
  • Ensure that Query export files can still be downloaded, the entire query name is present in the file name (special characters are either present in the download file name or replaced with _)

I tested with various special characters, and they work with both unselected and selected queries.

One thing that I noticed unrelated to this PR — creating a CSV from selected query fields doesn't trigger the same notification download behavior. Is this expected?

Screen.Recording.2024-07-08.at.11.59.25.AM.mp4

@Areyes42 Areyes42 requested a review from a team July 8, 2024 17:31
Copy link
Collaborator

@lexiclevenger lexiclevenger left a comment

Choose a reason for hiding this comment

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

Testing instructions

  • Save and export query with # in the query's name
  • Export the query as a CSV/KML (without selected any Query Results)
  • Ensure the file can be downloaded and the full query name is in the downloaded file's name
  • Save and export query with ? in the query's name
  • Export the query as a CSV/KML (without selected any Query Results)
  • Ensure the file can be downloaded and the full query name is in the downloaded file's name

For other special characters: @$%&+?\=;,":

  • Save a Query containing one or more of the characters
  • Ensure that Query export files can still be downloaded, the entire query name is present in the file name (special characters are either present in the download file name or replaced with _)

Looks good! The file names are exported correctly.

@lexiclevenger
Copy link
Collaborator

One thing that I noticed unrelated to this PR — creating a CSV from selected query fields doesn't trigger the same notification download behavior. Is this expected?

@Areyes42 I think this is related to #5023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 📋Back Log
Development

Successfully merging this pull request may close these issues.

Exported queries with # or ? in name are not downloadable
6 participants