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.

Sign up for GitHub

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

Jump to bottom

Fix date formatting on server for CSV export #29977

Merged
merged 8 commits into from
Feb 6, 2019
Merged

Fix date formatting on server for CSV export #29977

merged 8 commits into from
Feb 6, 2019

Conversation

tsullivan
Copy link
Member

Summary

Closes #29463
Re-try #29781

Summarize your PR. If it involves visual changes include a screenshot or gif.

  • new createDateOnServerFormat field formatter type
    • This is pretty much a copy of createDateFormat, but this one treats the date as a value in UTC when it parses it

Checklist

  • Unit or functional tests were updated or added to match the most common scenarios
    • I'm still working on a test that generates a simple CSV and verifies the output. It might be a separate PR

@tsullivan tsullivan added review v7.0.0 (Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v6.6.1 labels Feb 4, 2019
@tsullivan
Copy link
Member Author

tsullivan commented Feb 4, 2019 •
edited
Loading

Reviewers: Let me know if you need to see a test added in this change. I think adding a new integration test that hits the CSV generation URL and then pulls the completed job out of ES might be the best option for now.

@tsullivan tsullivan force-pushed the fix/reporting-csv-tz-ii branch from 134f6c5 to 031846a Compare February 4, 2019 19:02
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

joelgriffith
joelgriffith suggested changes Feb 5, 2019
View reviewed changes
Copy link
Contributor

@joelgriffith joelgriffith left a comment

Choose a reason for hiding this comment

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

Found one stray console.log and a few questions (probably just my lack of understanding)

src/legacy/core_plugins/kibana/common/field_formats/types/date_server.js Outdated
}

if (date.isValid()) {
console.log({ date: date.format(this._memoizedPattern) });
Copy link
Contributor

Choose a reason for hiding this comment

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

Stray console?

src/legacy/core_plugins/kibana/common/field_formats/types/date_server.js
});
}

getParamDefaults() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't see this used elsewhere... is it an interface that this class implements?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, not sure. I copied most of this file from src/legacy/core_plugins/kibana/common/field_formats/types/date.js. All the format types seem to have that method.

Copy link
Member Author

Choose a reason for hiding this comment

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

Speaking of "copied from", there are unrelated differences in the new file: _memoizedConverter is declared in the constructor instead of in _convert

Copy link
Contributor

Choose a reason for hiding this comment

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

Coolio! Thanks for the clarifaction

x-pack/plugins/reporting/export_types/csv/server/execute_job.js Show resolved Hide resolved
@tsullivan tsullivan mentioned this pull request Feb 5, 2019
Merged
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@tsullivan
Copy link
Member Author

@joelgriffith ready for another look

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@joelgriffith
Copy link
Contributor

Nice, LGTM

@tsullivan tsullivan merged commit 4a148e6 into elastic:master Feb 6, 2019
@tsullivan tsullivan deleted the fix/reporting-csv-tz-ii branch February 6, 2019 16:42
tsullivan added a commit to tsullivan/kibana that referenced this pull request Feb 6, 2019
@tsullivan
Fix date formatting on server for CSV export (elastic#29977)
5175bb3 
* Fix date formatting on server for CSV export

* remove stray console.log

* allow async to act in parallel

* Log a warning when "Browser" is the timezone
@tsullivan tsullivan mentioned this pull request Feb 6, 2019
Merged
tsullivan added a commit to tsullivan/kibana that referenced this pull request Feb 6, 2019
@tsullivan
Fix date formatting on server for CSV export (elastic#29977)
53657e0 
* Fix date formatting on server for CSV export

* remove stray console.log

* allow async to act in parallel

* Log a warning when "Browser" is the timezone
This was referenced Feb 6, 2019
Merged
Closed
tsullivan added a commit to tsullivan/kibana that referenced this pull request Feb 6, 2019
@tsullivan
Fix date formatting on server for CSV export (elastic#29977)
3889b85 
* Fix date formatting on server for CSV export

* remove stray console.log

* allow async to act in parallel

* Log a warning when "Browser" is the timezone
@tsullivan tsullivan mentioned this pull request Feb 6, 2019
Closed
tsullivan added a commit to tsullivan/kibana that referenced this pull request Feb 6, 2019
@tsullivan
Fix date formatting on server for CSV export (elastic#29977)
1886df4 
* Fix date formatting on server for CSV export

* remove stray console.log

* allow async to act in parallel

* Log a warning when "Browser" is the timezone
tsullivan added a commit that referenced this pull request Feb 6, 2019
@tsullivan
Fix date formatting on server for CSV export (#29977) (#30264)
096dec4 
* Fix date formatting on server for CSV export

* remove stray console.log

* allow async to act in parallel

* Log a warning when "Browser" is the timezone
tsullivan added a commit that referenced this pull request Feb 7, 2019
@tsullivan
Fix date formatting on server for CSV export (#29977) (#30263)
2fe6401 
* Fix date formatting on server for CSV export

* remove stray console.log

* allow async to act in parallel

* Log a warning when "Browser" is the timezone
@bhavyarm
Copy link
Contributor

@tsullivan this is PR is failing in 6.6.1 BC1. Here is the regression bug: #31131

@tsullivan
Copy link
Member Author

@bhavyarm Apologies! This incorrectly has the 6.6.1 label. It is not a regression, and is fixed in 7.0+

@tsullivan
Copy link
Member Author

I fixed the version labels. The fix is for 7.0+

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joelgriffith joelgriffith joelgriffith approved these changes

@spalger spalger Awaiting requested review from spalger

@legrego legrego Awaiting requested review from legrego

@Bargs Bargs Awaiting requested review from Bargs

Assignees
No one assigned
Labels
(Deprecated) Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead review v7.0.0 v7.2.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tsullivan @elasticmachine @joelgriffith @bhavyarm