-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Behavioral Analytics] Introduce empty state for events tab #146685
[Behavioral Analytics] Introduce empty state for events tab #146685
Conversation
243fc36
to
081ddab
Compare
d98257d
to
2a83fc6
Compare
2a83fc6
to
b74d535
Compare
try { | ||
const eventsIndexExists = await analyticsEventsIndexExists(client, request.params.id); | ||
|
||
if (!eventsIndexExists) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: as this is the purpose of the API, I wouldn't expect it to throw an error if the index doesn't exist. It should be a 2xx response with a boolean flag on whether the index exists or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. It's going to be not REST-compliant then, since we try to call exists
in the scope of passed {collectionId}
. If it was events/exists/{collectionId}
, then it makes sense. Also, I wanted to be aligned with the Elasticsearch API, which returns 404 if the index doesn't exist and returns 200 with the payload when it exists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally 4xx / 5xx errors should be reserved for genuinely unexpected responses. For example I would expect a 404 error here if the analytics collection didn't exist. I wouldn't expect a 404 for when the purpose of this endpoint is check if events index is present. My reasoning stems from APM monitoring where this 4xx/5xx would indicate an issue but its an expected response. Hope that makes sense.
// analyticsEventsIndexExists: jest.fn(), | ||
// })); | ||
|
||
describe('delete analytics collection lib function', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: incorrect test name
}); | ||
|
||
describe('FetchAnalyticsCollectionsApiLogic', () => { | ||
it('calls the analytics collections list api', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: test name incorrect
...ications/analytics/components/analytics_collection_view/analytics_collection_events.test.tsx
Show resolved
Hide resolved
9997aa6
to
8812b8e
Compare
8812b8e
to
0518d2a
Compare
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @tutelaris |
Description
In order to proceed with the Behavioral Analytics introduction, it's required to introduce an empty state on the events tab, when there are no events, and remove this state once events start coming.
This PR is dedicated to introducing an empty state in the events tab.
Screen.Recording.2022-12-02.at.18.53.07.mov