do we want to have less silly data?

I think so. You can find some sample values for these fields here https://github.com/elastic/kibana/blob/master/x-pack/plugins/endpoint/server/test_data/all_alerts_data.json.

maybe a comment explaining what this is/why it's needed

@paul-tavares just to follow up with the comment @peluja1012 left on your pr, this is the new route action/middleware approach we're taking, would like to hear your thoughts

switched for consistency purposes and to be able to pass props down if/when needed

why?

@kqualters-elastic needs to pass down props and we can keep consistency with the rest of the app

Good test, but the name actual seems a little ambiguous.

You can always do this later, but we're going to be removing POST support for this API. If you want to try changing it now, it probably will just require changing this to .get and then changing the the line below to query: paginationDataFromUrl(state) as HttpFetchQuery), instead of body: JSON.stringify(paginationDataFromUrl(state)), ... I'm fine with waiting until the next PR though, whatever you feel like.

We should move this file to the view/alerts folder, as hooks are associated with views. Also I think it might be better to rename the file to something like use_alerts_selector.ts because we will probably add more hooks in the future and I don't think we should keep them all in one file.

I think .toBe(true) applies better to this case because we expect isOnAlertPage to return either true or false.

We should update this now that you split up the tests.

We should update this now that you split up the tests.

Let's update this description 😉

You can probably keep the id's the same as they were before. To provide friendly names to the column headers you can use the EuiDataGridColumn display property. So you could do something like:

{
    id: 'event_type',
    display: i18n.translate(
        'xpack.endpoint.application.endpoint.alerts.eventType',
        {
          defaultMessage: 'Event Type',
        }
}

why as?

@oatkiller Cause I told him to 🙈

i think you need to call out / validate each value here. otherwise a malicious actor can give an unwitting nerd a url like somekibana.net/endpoint?badStuff=encodedHacks and then the nerd clicks the url and a request goes to kibana w/ unwitting nerd's creds but with whatever params were provided by malicious actor @dplumlee

@oatkiller We are doing pretty comprehensive validation on the backend FYI. So maybe not necessary? Always helps to have an extra layer, I s'pose.

I was also wondering this. I think once we start to grab data from the URL which is open to manipulation, we should do some sort of manipulation in order to avoid front-end code injections. I'm ok if we defer that to another Issue, but we should come up with a common approach.

I had mentioned this in my prior comment and just wanted to again make everyone aware - this will dispatch the action potentially BEFORE the component for the matched route is rendered. I think that is ok, but just wanted everyone to be aware of that behaviour.

Just FYI. This is actually a bug @parkiino fixed at one point (should just be the sagaReduxMiddleware variable that was created on line 52). That being said, its also going away since Candace refactored it to use Middleware and remove Sagas

Why use of local state and not redux store?

i dont know if we had a particular reason for not putting it in store beyond we didn't consider it part of the feature and it was just a side effect already built into the eui DataGrid component

We should all sync up on the values to use for page size. @parkiino and I are using [10, 20, 50], which I think we discussed with Bonnie.

Is this still used? should it be deleted?

you are updating the Alerts store every time the URL changes - is that intentional? or did you forget to use isOnAlertsList()?

Also, when the user leaves the alert list page, the store for it should be reset.

i suppose, only time the action is dispatched currently is if the app is already confirmed to be on the alerts page

The action is dispatched anytime the URL changes - so navigating to /management or /policy will trigger the action.

@paul-tavares We didn't want to force other teams to use this approach so we are storing the location in the alerts piece of the store. If all of us want to use this, we can store the location at the top level of the store. The idea is to update location every time the url changes.

I created an issue for clearing the state when navigating away https://github.com/elastic/endpoint-app-team/issues/183

@peluja1012 I actually thought we had agreed to use this new approach instead of usePageId() when we talked through the revised implementation around using location provided by react-router. At least I was planning on refactoring the Policy/Management pages to use userChangedUrl instead once this merged in.

Not sure if storing it globally would facilitate things though - since currently we're building our reducers/middleware to NOT be aware of global state. Accessing the location information would be an issue thus why each concern needs to store it, no?

@paul-tavares Oh that's nice. I didn't realize we were all moving forward with this approach. It would certainly be easier the way the code is laid out now to have each concern store the location. I don't know if I like the fact that we'll need a copied and pasted reducer clause to update the location in each concern though. I would rather have location be global. We could refactor the code to allow for a global location piece of state or create a reducer helper to handle updating the location in each concern. Either way, I'd like to do this work in a separate PR. This PR is currently blocking work on Alert Details and Alerts Filtering, so I would like to get it merged as soon as we can.

agreed 👍

(optional I would suggest you do a strict === as in state.location.pathname === '/alerts'