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

Sort using unix timestamp value #43162

Merged

Conversation

ycombinator
Copy link
Contributor

@ycombinator ycombinator commented Aug 12, 2019

Fixes #42472.

This PR attempts to fix the sorting for the "Last Modified" column in the Logstash Pipeline Management UI.

Pre-requisites for viewing the Logstash Pipeline Management UI

  • Use a Trial license.
  • Enable Security.

@ycombinator
Copy link
Contributor Author

ycombinator commented Aug 12, 2019

@justinkambic @cjcenizal It's been a while since I've touched UI code and it looks like EUI has advanced quite a bit since I was last using it. I feel like the fix I have in this PR should just work but it doesn't 😞.

So obviously there's more going on here that I can't figure out. I did notice a bunch of React proptype validation warnings (even before I made any changes for this PR), so maybe that has something to do with why this change isn't just working?

Could either of you take a look and point me in the right direction? Thanks!

@ycombinator ycombinator added Feature:Logstash Pipelines Logstash Pipeline UI related WIP Work in progress labels Aug 12, 2019
@elasticmachine
Copy link
Contributor

💔 Build Failed

@cjcenizal cjcenizal removed their request for review August 13, 2019 17:56
@ycombinator
Copy link
Contributor Author

Per @cjcenizal's suggestion, pinging @chandlerprall and @thompsongl for help (please see my previous comment). Thanks!

@thompsongl
Copy link
Contributor

@ycombinator Happy to help. I don't seem to have a local setup right now that let's me see this view, but it appears that you're using sortable correctly. What data type is lastModified (Date, timestamp, number, etc.)?

@ycombinator
Copy link
Contributor Author

Thanks @thompsongl!

What data type is lastModified (Date, timestamp, number, etc.)?

It's a moment object.

@thompsongl
Copy link
Contributor

thompsongl commented Aug 14, 2019

Is it possible to use the last_modified field directly and not have to go through Moment?

Screen Shot 2019-08-14 at 11 41 08 AM

Really, I'm just trying to discern whether lastModified.valueOf() returns a value that can be sorted simply.

@ycombinator
Copy link
Contributor Author

Really, I'm just trying to discern whether lastModified.valueOf() returns a value that can be sorted simply.

Yes, it is. It returns the milliseconds-since-epoch timestamp, so it's an integer.

@thompsongl
Copy link
Contributor

Ok. I'll have to get a local setup that will let me debug. Replicating that component config in a more basic environment works as I'd expect it to.

@justinkambic
Copy link
Contributor

FWIW I went so far as to create a contrived table/data on this page and was unable to produce the desired behavior. I can provide the code if it's helpful to anyone.

@thompsongl
Copy link
Contributor

Might be helpful, but, I don't think EuiInMemoryTable is the cause of the problem.

This is using the config and new sortable function in this PR and the dates are moment objects.

Kapture 2019-08-16 at 13 14 08

@justinkambic
Copy link
Contributor

I've added d9b2ae5 and 1d9d0ae to fix the sorting problem and update the branch with newer master, respectively. @chandlerprall do you think you'd be ok doing a review for us since @ycombinator and I have both committed at this point?

@ycombinator ycombinator changed the title [WIP] Sort using unix timestamp value Sort using unix timestamp value Sep 13, 2019
@ycombinator ycombinator removed the WIP Work in progress label Sep 13, 2019
Copy link
Contributor Author

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

I played around with the latest changes and they LGTM. I'm able to sort all columns, including the Last Modified one, as expected. Thanks @justinkambic and @chandlerprall!

@chandlerprall, we'd still like your review on this for reasons @justinkambic mentioned in his comment.

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@ycombinator
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM

Copy link
Contributor

@justinkambic justinkambic left a comment

Choose a reason for hiding this comment

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

I made sure that localized headings translated alright.

LGTM too!

@ycombinator ycombinator merged commit c1d6a47 into elastic:master Sep 17, 2019
ycombinator added a commit to ycombinator/kibana that referenced this pull request Sep 17, 2019
* Sort using unix timestamp value

* Extract internationalization from react components to function calls.

* Updating Jest snapshots for pipelines table component
rylnd added a commit to rylnd/kibana that referenced this pull request Sep 17, 2019
* master: (33 commits)
  [easy] Exclude __examples__ from coverage (elastic#45556)
  [DOCS] Update CCR links (elastic#44012)
  Use unique junit report filenames again (elastic#45897)
  [ftr/savedObjects] add simple saved object api client to ftr s… (elastic#45856)
  New visualization editor Lens (elastic#36437)
  Sort using unix timestamp value (elastic#43162)
  [APM] Use POST instead of implicit GET (elastic#45903)
  [Canvas] Converting workpad header components to typescript and adding i18n (elastic#45274)
  skip flaky test (elastic#45884)
  set IS_PIPELINE_JOB in intake jobs (elastic#45850)
  [Uptime] Fix/issue 48 integration popup closes after refresh (elastic#45759)
  [Logs UI] Support zoom by brushing in the log rate chart (elastic#45879)
  [DOCS] Changes name to host (elastic#45798)
  [ML] Add population job wizard test (elastic#45765)
  [skip-ci][Maps][File upload] Geojson indexing and styling docs (elastic#41394)
  remove setTimeoue for state change (elastic#45853)
  [Graph] Restructure folders and add readme (elastic#45782)
  [ML] Enhance job id error message (elastic#45349)
  [SIEM] Do not update state component when they did unmount (elastic#45847)
  [i18n] sync from 7.4 latest translations (elastic#45823)
  ...
ycombinator added a commit that referenced this pull request Sep 19, 2019
* Sort using unix timestamp value

* Extract internationalization from react components to function calls.

* Updating Jest snapshots for pipelines table component
ycombinator added a commit that referenced this pull request Sep 19, 2019
* Sort using unix timestamp value

* Extract internationalization from react components to function calls.

* Updating Jest snapshots for pipelines table component
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logstash Pipeline management UI sort not working as expected
5 participants