[SECURITY_SOLUTION][ENDPOINT] Trusted Apps List page Empty State when no trusted apps exist #87252
Conversation
Still needs improvements
…ed-apps-empty-state
paul-tavares
added
v8.0.0
release_note:skip
x-pack/plugins/security_solution/public/management/pages/trusted_apps/store/middleware.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/public/management/pages/trusted_apps/store/middleware.ts
Outdated
Show resolved
Hide resolved
| payload: { type: 'LoadingResourceState', previousState: currentEntriesExistState }, | ||
| }); | ||
|
|
||
| let doTheyExist: boolean; |
There was a problem hiding this comment.
🤣 Love that guy
...ugins/security_solution/public/management/pages/trusted_apps/view/trusted_apps_page.test.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| http.get.mockImplementation(async (...args) => { | ||
| const path = (args[0] as unknown) as string; | ||
| // @ts-ignore |
There was a problem hiding this comment.
why, and how annoying would it be to not do this
There was a problem hiding this comment.
Yeah, I might need to bring in a type guard function to check args. I found that worked while I investigated this ts error else where. (FYI: the problem is that the interface (HttpHandler) is an interface with multiple calling signatures)
| if (path === TRUSTED_APPS_LIST_API) { | ||
| return { | ||
| data: [getFakeTrustedApp()], | ||
| total: 50, // << Should be a value large enough to fulfill two pages |
There was a problem hiding this comment.
first, 🔥 for the comment explaining the number chosen.
second,
const per_page = httpOptions?.query?.per_page ?? 20;
return {
//...
total: per_page*2, // need to fill two full pages
per_page: per_page,
}perhaps? but purely subjective
| }); | ||
|
|
||
| beforeEach(() => { | ||
| // @ts-ignore |
There was a problem hiding this comment.
🎅 sees you when you're coding. He knows when you're @ts-ignore
There was a problem hiding this comment.
🤣
YOU MUST do all code reviews on my PRs. I'm rolling over here.
| if (path === TRUSTED_APPS_LIST_API) { | ||
| const { page, per_page: perPage } = options.query as { page: number; per_page: number }; | ||
|
|
||
| if (page === 1 && perPage === 1) { |
There was a problem hiding this comment.
Again, not a big fan of putting too much fake logic in these mocks. Probably better patterns to accomplish this
There was a problem hiding this comment.
Not sure there is a better way here. We call the same API twice - one with "normal" arguments for populating the list, and another one to check if entries exist. This check here is to ensure we can identify both of them and are able to control the response for each in the test case.
There was a problem hiding this comment.
I should mention: there is another way to mock the responses without having the logic, and that's to use .mockResolvedValueOnce() for each sequential API call that is made. This felt more brittle to me because if any http.get() call is inserted at some point in the future, the test would break, so I opted for mocking the entire implementation in order to have the ability to inspect the call's arguments
|
@pzl thanks for the review - great feedback 👍 |
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: |
Summary
Displays an Empty Prompt on the Trusted Apps List page when there are no trusted applications.
Checklist