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

Issue 150 store token from server in local storage #155

Conversation

ErikaKK
Copy link
Contributor

@ErikaKK ErikaKK commented Jul 21, 2024

Change Summary

image
  • Store token to local storage when sign in in sign-in.tsx page.
  • Changed fake API in poster page index.tsx, got task list from db API using axios interceptor in /lib/api.ts
  • Moved and removed some poster page files bc they were not very organized before
  • Add API call in poster page new-task.tsx, got error response
    image

image

Change Form

Fill this up (NA if not available). If a certain criteria is not met, can you please give a reason.

  • The pull request title has an issue number
  • The change works by "Smoke testing" or quick testing
  • The change has tests
  • The change has documentation

Other Information

  • Bc the design of db login table only has username field instead of email or mobile. I changed the login field label in sign-in.tsx

Related issue

Copy link

vercel bot commented Jul 21, 2024

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

Name Status Preview Comments Updated (UTC)
penni ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 23, 2024 5:26am

@ErikaKK ErikaKK requested review from SodaVolcano and yunho7687 July 21, 2024 15:15
Copy link
Collaborator

@SodaVolcano SodaVolcano left a comment

Choose a reason for hiding this comment

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

Have some console.log that should be removed probably, but looks pretty good!

@@ -61,7 +72,7 @@ export default function SignIn({ account }: { account: string }) {
<SingleLineInput
type="text"
name="account"
label="Email or mobile"
label="Username"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Replace with Email or mobile, "username" is not usually associated with emails or mobile but actual user names like SodaVolcano, can be confusing

client/src/pages/sign-in.tsx Outdated Show resolved Hide resolved
client/src/pages/sign-in.tsx Outdated Show resolved Hide resolved
client/src/lib/api.ts Outdated Show resolved Hide resolved
client/src/pages/poster/index.tsx Outdated Show resolved Hide resolved
client/src/pages/poster/index.tsx Outdated Show resolved Hide resolved
client/src/pages/poster/index.tsx Outdated Show resolved Hide resolved
ErikaKK added 2 commits July 22, 2024 03:04
…issue-150-Store_token_from_server_in_local_storage_RAM_or_cookie_idk_-_Sign_in_and_Sign_up
const LOGIN_URL = LocalBaseURL.concat("/app/login/");

const handleLogin = async (username: string, password: string) => {
const response = await axios.post(LOGIN_URL, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use the api in the lib?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote this so I use it. It works well

@ErikaKK
Copy link
Contributor Author

ErikaKK commented Jul 22, 2024

To satisfy current fields in db, have to use username to login. Needs to change later

Copy link
Collaborator

@SodaVolcano SodaVolcano left a comment

Choose a reason for hiding this comment

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

Looks pretty good, remove the validation if you want (can always go back to a previous commit if we need the regex pattern again)

Comment on lines 36 to 44
const isEmail = /^[a-zA-Z\._'\d-]+@[a-zA-Z]+(\.[a-zA-Z]+)+$/.test(
formData.account,
);
const isPhone = /^[0-9]{10}$/.test(formData.account);

if (!isEmail && !isPhone) {
setErrorMessage("Invalid email or phone number. Please try again.");
return;
}
// if (!isEmail && !isPhone) {
// setErrorMessage("Invalid email or phone number. Please try again.");
// return;
// }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Feel free to remove validation if you think it should be done only on the server side. I guess we have to send the details to the server anyway so it doesn't matter if we also validate it there as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok thanks now they only use username to login. I'll change later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now the login API uses username in the payload. I'll change later

@CodeWithTheDoctor
Copy link

I wasn't sure how to test this so can't review it.

What I tried: Opened localhost:3000, enetered my email and name, clicked "Finish", saw a confirmation on the console page.

But token doesn't appear in the localstorage?

image

Let me know if there's something else I should be looking for!

@ErikaKK
Copy link
Contributor Author

ErikaKK commented Jul 22, 2024

I wasn't sure how to test this so can't review it.

What I tried: Opened localhost:3000, enetered my email and name, clicked "Finish", saw a confirmation on the console page.

But token doesn't appear in the localstorage?

image

Let me know if there's something else I should be looking for!

Someone else is working on this signup page #134
My team made the signin page. You can access http://localhost:3000/signin-test and click register to signup a user with

email: "user1@example.com",
        password: "user123abc@#",
        username: "user1",

Then go to http://localhost:3000/sign-in. You have to use the username in the Email or mobile field to login bc that's how the api is designed. and then you can see the token in local storage
image

@ErikaKK
Copy link
Contributor Author

ErikaKK commented Jul 22, 2024

@CodeWithTheDoctor

Copy link
Member

@yunho7687 yunho7687 left a comment

Choose a reason for hiding this comment

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

please fix the conflict : )

…issue-150-Store_token_from_server_in_local_storage_RAM_or_cookie_idk_-_Sign_in_and_Sign_up

The changes in #110 made signin page unable to submit so I discarded these changes
…M_or_cookie_idk_-_Sign_in_and_Sign_up' and 'main' of https://github.com/codersforcauses/penni into issue-150-Store_token_from_server_in_local_storage_RAM_or_cookie_idk_-_Sign_in_and_Sign_up
@ErikaKK ErikaKK merged commit 0ebf7ff into main Jul 23, 2024
7 checks passed
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.

Store token from server in local storage (RAM or cookie idk) - Sign in and Sign up
5 participants