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

User update req.body check for Vercel deployments ▲ #1446

Merged
merged 12 commits into from
Aug 30, 2024

Conversation

RobAndrewHurst
Copy link
Contributor

@RobAndrewHurst RobAndrewHurst commented Aug 29, 2024

Description

On deployed instances the req.body always defaults to '{}'. Therefore the check that was applied on the user_update assign for the req.body would not resolve correctly because it would be truthy and then provide an undefined email address to the update query.

const update_user = (Object.keys(req.body || {}).length === 0 || req.body === undefined)
    ? { email: req.params.email }
    : req.body;

This is solved by adding in a check on if there are no keys in the object then it would resolve to falsy assigning the email from the params.

We also check if there is an email property on the update_user before executing the query. As if you provide a body with keys it will pass the initial check but still fail the query.

  • 🐛 Bug fix (non-breaking change which fixes an issue)

Note
You can also test this locally with the rewrites of a deployed instance by running vercel dev. It will ask you some setup questions but just point them to the instance.

@RobAndrewHurst RobAndrewHurst added the Bug A genuine bug. There must be some form of error exception to work with. label Aug 29, 2024
@RobAndrewHurst RobAndrewHurst self-assigned this Aug 29, 2024
@RobAndrewHurst
Copy link
Contributor Author

I think another thing to consider is we should return an error to the user if a user email was not provided in the params or the body. Because actually with this check if you provide a body with no email, but other keys the query will still fail.

@simon-leech
Copy link
Contributor

@RobAndrewHurst Agreed - do you want to push that before I review properly?

@RobAndrewHurst
Copy link
Contributor Author

@simon-leech Done.

Copy link

sonarcloud bot commented Aug 30, 2024

Copy link
Contributor

@simon-leech simon-leech left a comment

Choose a reason for hiding this comment

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

All working with me - tested both locally and on a deployed instance and working across all fields and roles updates 👍

Copy link
Member

@dbauszus-glx dbauszus-glx left a comment

Choose a reason for hiding this comment

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

@dbauszus-glx dbauszus-glx merged commit b954920 into GEOLYTIX:main Aug 30, 2024
5 checks passed
@RobAndrewHurst RobAndrewHurst deleted the user_update_vercel branch September 30, 2024 09:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug A genuine bug. There must be some form of error exception to work with. v4.x.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants