varunKT001 / tomper-wear-ecommerce

E-commerce web-application for selling clothing essentials 😀
https://tomper-wear.netlify.app
MIT License
38 stars 48 forks source link

Existing password check added before changing password #44

Closed Siddhesh777 closed 2 years ago

Siddhesh777 commented 2 years ago

Issue reference: #41

Proposed changes: changed the /src/pages/profilePage/index.js & /src/context/user_context.js I have made the functionality to verify the existing password before letting the user to change the password.

Please delete options that are not relevant.

Checklist:

Additional info (if any):

netlify[bot] commented 2 years ago

👷 Deploy request for tomper-wear pending review. Visit the deploys page to approve it

🔨 Explore the source changes: 022cc050f15daaea147ff7353d4936980bff89ee

Siddhesh777 commented 2 years ago

hello @varunKT001 😅 are those changes fine?

varunKT001 commented 2 years ago

@Siddhesh777 I haven't checked it yet 😅 I've some sessions to organize so I'm preparing for that. Don't worry, as soon as I get time, I'll go through your changes 👍

varunKT001 commented 2 years ago

@Siddhesh777 Sorry for the delay 😅

I just checked your pull request. It's working fine, but still, some changes are needed:

Make these changes and tag me here 👍 If you need any help, ping me on the discord channel 👍

Siddhesh777 commented 2 years ago

hey @varunKT001 I have made suggested changes. kindly check.

varunKT001 commented 2 years ago

@Siddhesh777 You didn't re-wrote all the code to async-await, there's more left 🙂
image Don't worry about the catch block. Just return:

toast.error(`Error: ${error.message}`)

It will catch all the errors 👍

Also two more things:

Please look into all those things. Also, don't be in hurry, take your time thinking about what should be the correct flow of the code. Check everything twice before making a pull request. Don't write code just to get your pull request merged. It's also a pain to me to tell you about these small things every time. You only think why will I check things manually if I can see everything already messed up in Github itself 🙂

Siddhesh777 commented 2 years ago

@varunKT001 Sorry for the inconvenience. I am actually beginner in Web-development so I think I mess up the things. I have made suggested changes, you can check them. I would like to say I am learning so much from the program gssoc'22. Thanks!

varunKT001 commented 2 years ago

@Siddhesh777 No one is an expert 👍 I am just telling you, just look at all your changes twice and make sure that you have done everything that you have been told. Don't be in a hurry 👍

Also, I said directly convert the other part to async-await. No need to use another try-catch block. Since firebase's error will always contain a message property, the error will get handled properly in one try-catch block only.

function handleSubmitPassword() {
    ...
    ...
    setLoading(true);
    try {
      // eslint-disable-next-line
      const response_2 = await reauthenticateUser(exisitingPassword);
      if (newPassword.length < 6) {
        return toast.error('Password should be atleast 6 characters');
      }
      if (newPassword !== confirmNewPassword) {
        return toast.error('Passwords do not match');
      }
      const response_3 = await updateUserProfilePassword(confirmNewPassword);
      toast.success('Profile password changed successfully');
    } catch (error) {
      toast.error('Current password do not match');
    }
    setLoading(false);
};

Also, don't use return while writing the toast.error..., because if we return from the function, setLoading(false) won't execute and will cause a memory leak warning.

Siddhesh777 commented 2 years ago

@varunKT001 I have done the changes 🙂 Please check them. Even if it needs more corrections, let me know. I am seriously thankful to you😄 ,I am learning so much from this project.

varunKT001 commented 2 years ago

@Siddhesh777 Checked it, everything works fine 👍

One last change, as I said in a previous comment: In the catch block, instead of hardcoding the error message, use the following:

toast.error(`Error: ${error.message}`)

So that, we can see the original firebase error 👍

Do this last change 👍

Siddhesh777 commented 2 years ago

hey @varunKT001 😅 Did it !😅

varunKT001 commented 2 years ago

@Siddhesh777 It's good to merge now 😄

Thanks for having such patience. I'm happy to see you take all my suggestions calmly. I have seen many peoples (while I was at your stage) get frustrated if asked to make more changes again and again 😕

Thanks for contributing 🚀