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

Moved login/logout logic into the useUser composable and updated references #12915

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

iamshobhraj
Copy link
Contributor

Summary

Login/logout and session logic is moved into useUser composable from action.js. Refactored vuex state into module level state. References to CORE_SET_SESSION is updated to use the setSession method from useUser composable.

References

fixes #12204

Reviewer guidance

after updating references to CORE_SET_SESSION in heartbeat.spec.js file, some tests still failed. I am unable to figure out why these tests are failing. I updated the mock file and implementated it as written in documentation.

@MisRob MisRob added the TODO: needs review Waiting for review label Dec 9, 2024

describe('HeartBeat', function () {
stubWindowLocation(beforeAll, afterAll);
// replace the real XHR object with the mock XHR object before each test
beforeEach(() => mock.setup());
beforeEach(() => useUser.mockImplementation(() => useUserMock()));
Copy link
Member

Choose a reason for hiding this comment

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

Is there a particular reason for mock.setup() not to be called anymore? I'm wondering if this is related to the failing tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the oversight. I’ve fixed this now.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

In the description you mentioned that the logic was moved from an actions.js file but there are no changes to any Vuex files included.

I also see that heartbeat.js may need to be updated to use the useUser module as it is still calling dispatch('setSession', ...) here.

I had a comment in the heartbeat.spec.js file re: the tests but I think that updating heartbeat.js might be the key

@iamshobhraj
Copy link
Contributor Author

In the description you mentioned that the logic was moved from an actions.js file but there are no changes to any Vuex files included.

@nucleogenesis Thanks for pointing that out. I had initially planned on removing the Vuex logic after successfully migrating the login and session logic to the new Composition API. However, I got stuck with the heartbeat.spec.js tests. I’ve now removed the login/logout and session logic from the Vuex files.

I also see that heartbeat.js may need to be updated to use the useUser module as it is still calling dispatch('setSession', ...) here.

I had a comment in the heartbeat.spec.js file re: the tests but I think that updating heartbeat.js might be the key

Regarding the heartbeat.js, I refactored the store.dispatch to use the setSession method, but the failing tests persist. I suspect the issue might be related to xhr-mock, but I’m unable to pinpoint the exact cause.

Please let me know if you have any suggestions.

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

I added one comment to the mock function for setSession and after some thinking and looking at the code locally I'm not sure precisely what the fix is, but I'm feeling it is very likely to do with that function altogether and it's differences to the real setSession.

Another thing I saw was that the kolibriLogin action is being dispatched a few places - such as the store.spec.js file

Thank you for your effort on this. I hope anything I've said can be helpful. I'll keep thinking about this and come back to it again tomorrow if I can

session, // Make session mutable for test scenarios

// Actions
setSession: jest.fn(({ session: newSession, clientNow }) => {
Copy link
Member

Choose a reason for hiding this comment

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

One of the failing tests re: not calling serverTime w/ clientNow may be fixed by calling it here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move login/logout logic into the useUser composable and update references
3 participants