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

Add authentication service along with user module #18

Merged
merged 8 commits into from
Jun 14, 2021

Conversation

codeAesthetic
Copy link
Contributor

  1. Add User module.
  2. Add Auth Service.
  3. Add .env example.
  4. Add env variables in CD.

1. Add User module.
2. Add .env example.
3. Add env variables in CD.
@github-actions
Copy link

🚀 Netlify deployed juntos-frontend as draft

https://60c206b88b805e29366ccf55--juntos-frontend.netlify.app

src/User/User.ts Outdated Show resolved Hide resolved
src/User/User.ts Outdated Show resolved Hide resolved
src/User/User.ts Outdated Show resolved Hide resolved
CONTRIBUTING.md Outdated Show resolved Hide resolved
src/User/User.ts Outdated Show resolved Hide resolved
src/User/User.ts Outdated Show resolved Hide resolved
@github-actions
Copy link

🚀 Netlify deployed juntos-frontend as draft

https://60c20f494c4554483d5c8130--juntos-frontend.netlify.app

Copy link
Contributor

@chirgjn chirgjn left a comment

Choose a reason for hiding this comment

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

Can you write some tests for these modules.

PS: You might need to mock firebase for these tests

@github-actions
Copy link

🚀 Netlify deployed juntos-frontend as draft

https://60c2fe7dce399cd8fba58214--juntos-frontend.netlify.app

Copy link
Contributor

@chirgjn chirgjn left a comment

Choose a reason for hiding this comment

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

Please address unresolved comments

Copy link
Contributor

@chirgjn chirgjn left a comment

Choose a reason for hiding this comment

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

There's some repetition in src/AuthService/AuthService.ts and src/User/User.ts. Can we move them to a src/modules folder so that the paths become src/modules/AuthService.ts and src/modules/User.ts.

src/AuthService/AuthService.test.ts Outdated Show resolved Hide resolved
src/AuthService/AuthService.test.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
1. Firebase mock written firebase user.
2. Add check if User exiting in auth methods.
@github-actions
Copy link

🚀 Netlify deployed juntos-frontend as draft

https://60c3259698bc8cde8dffbf69--juntos-frontend.netlify.app

@github-actions
Copy link

🚀 Netlify deployed juntos-frontend as draft

https://60c337a947d22c00aad2eec8--juntos-frontend.netlify.app

Copy link
Contributor

@chirgjn chirgjn left a comment

Choose a reason for hiding this comment

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

Can you add some tests for failure scenarios

src/modules/AuthService.ts Outdated Show resolved Hide resolved
src/modules/AuthService.ts Outdated Show resolved Hide resolved
src/modules/User.ts Outdated Show resolved Hide resolved
src/modules/User.ts Outdated Show resolved Hide resolved
@github-actions
Copy link

🚀 Netlify deployed juntos-frontend as draft

https://60c35e3be496c331e1a1f602--juntos-frontend.netlify.app

@github-actions
Copy link

🚀 Netlify deployed juntos-frontend as draft

https://60c35e895280723378c9ce88--juntos-frontend.netlify.app

src/modules/User.ts Outdated Show resolved Hide resolved
Comment on lines +12 to +13
signInWithEmailAndPassword: () =>
Promise.resolve({ user: firebaseUser }),
Copy link
Contributor

Choose a reason for hiding this comment

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

We also need to mock these functions as per: https://jestjs.io/docs/mock-functions#using-a-mock-function
So that we can check if the same was called in the respective wrapper with a check like:

// The first argument of the first call to the function was 0
expect(signInWithEmialAndPassword.mock.calls[0][0]).toBe(0);

Copy link
Contributor Author

@codeAesthetic codeAesthetic Jun 12, 2021

Choose a reason for hiding this comment

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

function that lies inside another function can't be mocked as you mentioned.
more details at following:-

  1. Spying on an imported function that calls another function in Jest,
  2. jest mock inner functions

Copy link
Contributor

Choose a reason for hiding this comment

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

These two examples are different from our case. Can you try something like:

const mockSignInWithEmailAndPassword = jest.fn(({email, password}) =>  Promise.resolve({ user: firebaseUser }));

jest.mock("firebase", () => ({
  ...
  return {
    auth: () => ({
        signInWithEmailAndPassword,
        ...
     })
  }
});

....

expect(mockSignInWithEmailAndPassword.mock.calls[0][0]).toBe("user@gmail.com");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mockSignInWithEmailAndPassword is simple mock function it can't test AuthService.ts file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put this on a codesandbox with the above approach or share some references

Copy link
Contributor Author

Choose a reason for hiding this comment

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

example you have given doesn't work so don't see the point of putting this in sandbox

Copy link
Contributor

Choose a reason for hiding this comment

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

The codesandbox would allow me to explain the mocking. Can you create one and post the link here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Jest is not fully supported in codesandbox: codesandbox/codesandbox-client#513

@github-actions
Copy link

🚀 Netlify deployed juntos-frontend as draft

https://60c4cd6e9d8db318f832401a--juntos-frontend.netlify.app

Copy link
Contributor

@chirgjn chirgjn left a comment

Choose a reason for hiding this comment

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

Please go through all un-resolved review comments

src/modules/User.ts Outdated Show resolved Hide resolved
src/modules/User.ts Outdated Show resolved Hide resolved
src/modules/User.ts Outdated Show resolved Hide resolved
Comment on lines +12 to +13
signInWithEmailAndPassword: () =>
Promise.resolve({ user: firebaseUser }),
Copy link
Contributor

Choose a reason for hiding this comment

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

These two examples are different from our case. Can you try something like:

const mockSignInWithEmailAndPassword = jest.fn(({email, password}) =>  Promise.resolve({ user: firebaseUser }));

jest.mock("firebase", () => ({
  ...
  return {
    auth: () => ({
        signInWithEmailAndPassword,
        ...
     })
  }
});

....

expect(mockSignInWithEmailAndPassword.mock.calls[0][0]).toBe("user@gmail.com");

@github-actions
Copy link

🚀 Netlify deployed juntos-frontend as draft

https://60c5c51776cd88335a64c6f0--juntos-frontend.netlify.app

@chirgjn chirgjn merged commit 187450f into main Jun 14, 2021
@chirgjn chirgjn deleted the feature/auth-service branch June 15, 2021 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants