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

[_] Chore/add test for share guest signup #1447

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

TamaraFinogina
Copy link
Contributor

@TamaraFinogina TamaraFinogina commented Jan 30, 2025

Description

  • Add tests for ShareGuestSingUpView
  • Removes parseUserSettingsEnsureKyberKeysAdded from ShareGuestSingUpView because doRegisterPreCreatedUser already runs parseUserSettingsEnsureKyberKeysAdded prior to outputting the result.

Related Issues

Related Pull Requests

Checklist

  • Changes have been tested locally.
  • Unit tests have been written or updated as necessary.
  • The code adheres to the repository's coding standards.
  • Relevant documentation has been added or updated.
  • No new warnings or errors have been introduced.
  • SonarCloud issues have been reviewed and addressed.
  • QA Passed

How Has This Been Tested?

Test for new user with Kyber keys
Test for old user without Kyber keys

Additional Notes

Copy link

vercel bot commented Jan 30, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
drive-web ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 31, 2025 11:43am

Copy link

cloudflare-workers-and-pages bot commented Jan 30, 2025

Deploying drive-web with  Cloudflare Pages  Cloudflare Pages

Latest commit: 36102fc
Status:⚡️  Build in progress...

View logs

@@ -158,9 +158,6 @@ function ShareGuestSingUpView(): JSX.Element {
const { email, password, token } = formData;
const { xUser, xToken, mnemonic } = await doRegisterPreCreatedUser(email, password, invitationId ?? '', token);

// TODO: Remove or modify this when the backend is updated to add kyber keys
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the backend ready then, right? If it is ready, then it looks good to me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@larryrider it's a duplication. doRegisterPreCreatedUser also runs a parser inside

package.json Outdated
@@ -66,7 +65,7 @@
"react-international-phone": "^4.3.0",
"react-live-chat-loader": "^2.8.2",
"react-loading-skeleton": "^2.2.0",
"react-pdf": "^7.7.3",
"react-pdf": "^9.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

have you checked that this not break the pdf preview?

package.json Outdated
Comment on lines 110 to 119
"resolutions": {
"react-scripts/webpack-dev-server/express/path-to-regexp": ">=0.1.12",
"react-scripts/workbox-webpack-plugin/workbox-build/rollup": ">=2.79.2",
"react-scripts/**/nth-check": ">=2.0.1",
"react-scripts/**/micromatch": ">=4.0.8",
"react-scripts/**/@babel/traverse": ">=7.23.2",
"react-scripts/**/follow-redirects": ">=1.15.6",
"@internxt/sdk/axios": ">=0.28.0",
"@internxt/inxt-js/axios": ">=0.28.0",
"@internxt/inxt-js/axios/follow-redirects": ">=1.15.6"
Copy link
Contributor

Choose a reason for hiding this comment

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

good one @TamaraFinogina 🥳

Comment on lines +37 to +44
vi.mock('app/core/services/local-storage.service', () => ({
default: {
get: vi.fn(),
clear: vi.fn(),
getUser: vi.fn(),
set: vi.fn(),
},
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Maybe at some point we should move the mocks to mocks files to avoid repeating them in different 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.

yeah, I was thinking about it, it's same mocks over and over (almost the same but still)

Comment on lines +335 to +352
const mockClearUser: UserSettings = {
uuid: 'mock-uuid',
email: '[email protected]',
privateKey: Buffer.from(decryptedPrivateKey).toString('base64'),
mnemonic: encryptedMockMnemonic,
userId: 'mock-userId',
name: 'mock-name',
lastname: 'mock-lastname',
username: 'mock-username',
bridgeUser: 'mock-bridgeUser',
bucket: 'mock-bucket',
backupsBucket: null,
root_folder_id: 0,
rootFolderId: 'mock-rootFolderId',
rootFolderUuid: undefined,
sharedWorkspace: false,
credit: 0,
publicKey: keys.ecc.publicKey,
Copy link
Contributor

Choose a reason for hiding this comment

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

for next times, maybe could extract the commont values to a one const and reuse it in the others

@@ -158,9 +158,6 @@ function ShareGuestSingUpView(): JSX.Element {
const { email, password, token } = formData;
const { xUser, xToken, mnemonic } = await doRegisterPreCreatedUser(email, password, invitationId ?? '', token);

// TODO: Remove or modify this when the backend is updated to add kyber keys
const parsedUser = parseUserSettingsEnsureKyberKeysAdded(xUser);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this parser no longer necessary? is it used somewhere or we could remove it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@CandelR it's also called inside doRegisterPreCreatedUser, so basically I called it twice

@TamaraFinogina TamaraFinogina marked this pull request as draft January 31, 2025 10:49
@TamaraFinogina TamaraFinogina marked this pull request as ready for review January 31, 2025 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants