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

Using content-disposition pkg for non-US font titles in reporting #30895

Merged
merged 6 commits into from
Feb 21, 2019
Merged

Using content-disposition pkg for non-US font titles in reporting #30895

merged 6 commits into from
Feb 21, 2019

Conversation

joelgriffith
Copy link
Contributor

Uses the content-disposition pkg found by @spalger to properly handle filename's with non-ASCII characters. Also did a quick TS-ify of the problematic module.

@tsullivan tsullivan added v7.0.0 (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 v7.2.0 labels Feb 13, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@spalger
Copy link
Contributor

spalger commented Feb 13, 2019

Failures look legitimate

@elasticmachine
Copy link
Contributor

💔 Build Failed

@joelgriffith
Copy link
Contributor Author

Retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@joelgriffith joelgriffith requested review from a team as code owners February 14, 2019 18:22
@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

package.json Outdated Show resolved Hide resolved
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@joelgriffith
Copy link
Contributor Author

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joelgriffith
Copy link
Contributor Author

:dank:

@@ -287,6 +286,7 @@
"@types/joi": "^13.4.2",
"@types/jquery": "^3.3.6",
"@types/js-yaml": "^3.11.1",
"@types/json5": "^0.0.30",
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change automatically done by yarn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, likely just wasn't properly alphabetized before?

Copy link
Member

Choose a reason for hiding this comment

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

might as well leave it in, it'll just end up as an unrelated change in some other PR :D

};
}

return function getDocumentPayload(doc) {
const { status, output, jobtype: jobType, payload: { title } = {} } = doc._source;
return function getDocumentPayload(doc: any) {
Copy link
Member

Choose a reason for hiding this comment

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

After #31188 goes in, we can replace the any with a type :D

Copy link
Member

@tsullivan tsullivan 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 good code improvement, and a good fix for the bug. I had a minor-ish comment about the clarity in the code around how we get the filename

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan
Copy link
Member

The code change looks great, but can you add some steps for a quick manual verification?

@joelgriffith
Copy link
Contributor Author

@tsullivan to repro (on master) do:

  • Create a dashboard with non-ascii characters in the title: 平仮名 for instance (or emoji's).
  • Start a report of said dashboard.
  • Report fails, and Kibana logs an HTTP error.

Copy link
Member

@tsullivan tsullivan left a comment

Choose a reason for hiding this comment

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

LGTM

@joelgriffith
Copy link
Contributor Author

Will work on backporting this into 6.7 and 7.0

@weltenwort
Copy link
Member

Has this not been backported to 7.x on purpose?

@lukasolson
Copy link
Member

Correct me if I'm wrong, but it looks like this was first available in 7.3, not 7.0 or 7.2 as labeled here previously.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v6.7.0 v7.3.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants