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

Socket Initialization on user login #1109

Merged
merged 1 commit into from
Jan 16, 2023
Merged

Socket Initialization on user login #1109

merged 1 commit into from
Jan 16, 2023

Conversation

CDimonaco
Copy link
Member

Description

Socket connection needs to be initialized only after user login.

How was this tested?

Automated tests

@CDimonaco CDimonaco added bug Something isn't working javascript Pull requests that update Javascript code labels Jan 13, 2023
@CDimonaco CDimonaco self-assigned this Jan 13, 2023
Copy link
Contributor

@dottorblaster dottorblaster left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -13,4 +13,4 @@ export const getLastExecutionByGroupID = (groupID) =>
wandaClient.get(`/api/checks/groups/${groupID}/executions/last`);

export const getCatalog = (env) =>
wandaClient.get(`/api/checks/catalog`, { params: env })
wandaClient.get(`/api/checks/catalog`, { params: env });
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the linter catch this?

Copy link
Member Author

Choose a reason for hiding this comment

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

It was a formatting issue, for some reason previous formatting tasks didn't catch 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

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

@CDimonaco can we investigate? are we checking the formatting?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes in the CI we have the correct step, I will investigate further.

Copy link
Member

@fabriziosestito fabriziosestito left a comment

Choose a reason for hiding this comment

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

Approved with comments

function* watchUserLoggedIn() {
yield all([
takeLatest('user/setUserAsLogged', initialDataFetch),
takeLatest('user/setUserAsLogged', setupSocketEvents),
Copy link
Member

Choose a reason for hiding this comment

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

what if this breaks?

Copy link
Member Author

Choose a reason for hiding this comment

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

The application will crash, it's an unrecoverable error (no sockets no frontend basically), for the initial data fetching the error handling is the same as all the api requests

@CDimonaco CDimonaco merged commit 79e34a2 into main Jan 16, 2023
@CDimonaco CDimonaco deleted the socket_init_on_login branch January 16, 2023 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working javascript Pull requests that update Javascript code
Development

Successfully merging this pull request may close these issues.

4 participants