Skip to content

[Reporting] Fix TypeError with pending job info - #43924

Merged
tsullivan merged 7 commits into
elastic:masterfrom
tsullivan:tests/reporting-listing
Aug 28, 2019
Merged

[Reporting] Fix TypeError with pending job info#43924
tsullivan merged 7 commits into
elastic:masterfrom
tsullivan:tests/reporting-listing

Conversation

@tsullivan

@tsullivan tsullivan commented Aug 23, 2019

Copy link
Copy Markdown
Member

Closes #42484 #42895

@tsullivan tsullivan added review release_note:fix zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead v8.0.0 Team:Stack Services v7.4.0 labels Aug 23, 2019
@elasticmachine

elasticmachine commented Aug 23, 2019

Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-stack-services

@elasticmachine

elasticmachine commented Aug 23, 2019

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

@elasticmachine

elasticmachine commented Aug 26, 2019

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

tsullivan
tsullivan commented Aug 26, 2019
View reviewed changes
Comment thread x-pack/legacy/plugins/reporting/public/components/report_info_button.tsx

const jobType = info.jobtype || NA;

// TODO queue method (clicked UI, watcher, etc)

@tsullivan tsullivan Aug 26, 2019

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I removed this TODO because it's been talked about before, but never seemed essential

@elasticmachine

elasticmachine commented Aug 27, 2019

Copy link
Copy Markdown
Contributor

💚 Build Succeeded

nreese
nreese approved these changes Aug 27, 2019
View reviewed changes

@nreese nreese left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes look good. Thanks for adding tests.

Only thing I noticed is that ReportInfoButton component contains several strings that are not wrapped in i18n translations like "unknown", "n/a", "Width: ${width} x Height: ${height}", "Job Info", all JobInfoMap titles, and "Unable to fetch report info".

lgtm
code review

@tsullivan

tsullivan commented Aug 28, 2019

Copy link
Copy Markdown
Member Author

Only thing I noticed is that ReportInfoButton component contains several strings that are not wrapped in i18n translations like "unknown", "n/a", "Width: ${width} x Height: ${height}", "Job Info", all JobInfoMap titles, and "Unable to fetch report info".

Good call @nreese! This has come up before in Reporting code: #42873 That issue has come up once in a Stack Services sync as something that could be tackled by a non-expert in Reporting code.

Things move fast!

@tsullivan
tsullivan merged commit 1a0dc65 into elastic:master Aug 28, 2019
@tsullivan
tsullivan deleted the tests/reporting-listing branch August 28, 2019 05:45
@tsullivan tsullivan mentioned this pull request Aug 28, 2019
Merged
tsullivan added a commit to tsullivan/kibana that referenced this pull request Aug 28, 2019
@tsullivan
[Reporting] Fix TypeError with pending job info (elastic#43924)
c7f0272 
* fix typeerror and typescript warnings

* add test for report listing

* get a relevant snapshot to test with

* fix debug diff

* remove unnecessary change
tsullivan added a commit that referenced this pull request Aug 30, 2019
@tsullivan
[Reporting] Fix TypeError with pending job info (#43924) (#44218)
32540dc 
* fix typeerror and typescript warnings

* add test for report listing

* get a relevant snapshot to test with

* fix debug diff

* remove unnecessary change
@tsullivan tsullivan mentioned this pull request Sep 19, 2019
Merged
tsullivan added a commit to tsullivan/kibana that referenced this pull request Sep 19, 2019
@tsullivan
[Reporting] Fix TypeError with pending job info (elastic#43924)
6b591d1 
* fix typeerror and typescript warnings

* add test for report listing

* get a relevant snapshot to test with

* fix debug diff

* remove unnecessary change
tsullivan added a commit that referenced this pull request Sep 19, 2019
@tsullivan
[Reporting] Fix TypeError with pending job info (#43924) (#46179)
9ee6f28 
* fix typeerror and typescript warnings

* add test for report listing

* get a relevant snapshot to test with

* fix debug diff

* remove unnecessary change
@tsullivan tsullivan mentioned this pull request Nov 14, 2019
Closed
patrykkopycinski pushed a commit to patrykkopycinski/kibana that referenced this pull request May 6, 2026
@tsullivan
[Reporting] Fix TypeError with pending job info (elastic#43924)
ed2388f 
* fix typeerror and typescript warnings

* add test for report listing

* get a relevant snapshot to test with

* fix debug diff

* remove unnecessary change
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nreese nreese nreese approved these changes
@joelgriffith joelgriffith Awaiting requested review from joelgriffith

Assignees

No one assigned

Labels

release_note:fix review v7.4.0 v8.0.0 zDeprecated Feature:Reporting Use Reporting:Screenshot, Reporting:CSV, or Reporting:Framework instead

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Reporting/Tests] React component unit tests needed for report job listing

3 participants

@tsullivan @elasticmachine @nreese