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

Improved error messages #89

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ItsAdityaKSingh
Copy link

fixes #88
Some error messages being thrown couldn't classify what was going wrong. I tried fixing it. Once fixed, here would reflect in the beacon app as well!

@@ -65,7 +65,7 @@ const resolvers = {
const { email, password } = credentials || {}; // unpack if available
const user = id ? await User.findById(id) : await User.findOne({ email });

if (!user) return new Error("User not found.");
if (!user) return new Error("No account registered with this email");
Copy link
Member

Choose a reason for hiding this comment

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

incorrect since not necessary user provided their email

Copy link
Author

@ItsAdityaKSingh ItsAdityaKSingh Jan 21, 2022

Choose a reason for hiding this comment

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

We are using a validator in the front-end to ensure no email having a null value is passed while logging in or signing up. Are you sure I should remove this change?

@@ -76,7 +76,7 @@ const resolvers = {

if (credentials) {
const valid = email === user.email && (await bcrypt.compare(password, user.password));
if (!valid) return new AuthenticationError("credentials don't match");
if (!valid) return new AuthenticationError("Enter a valid password");
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand the difference.

Copy link
Author

Choose a reason for hiding this comment

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

Wrong credentials could mean a wrong email or password or both. Giving a hint that it's just the wrong password here could prove useful.

@aadibajpai
Copy link
Member

Can the frontend not parse these errors and display them however they see fit?

@ItsAdityaKSingh
Copy link
Author

Can the frontend not parse these errors and display them however they see fit?

Yes, we could try that. But right now, the error returned from the backend is being shown on the screen. However, having these minute changes may help in bettering the backend too :)

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.

Unsure error messages
2 participants