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

Added validation around department_id in API patch request #13575

Merged
merged 9 commits into from
Sep 7, 2023

Conversation

marcusmoore
Copy link
Collaborator

Description

This PR simply adds the smallest amount of validation around the department_id field when sending an API PATCH request for a user. It was pointed out that sending

'department_id' => ['id' => 1]`

resulted in a success message but didn't actually a change. This is because the endpoint actually needs this format

'department_id' => 1

I just added a validation rule that says the department_id, if provided, needs to exist in the departments table.


I started to go down the path of testing validation for all inputs but decided to keep it simple and focused in this PR.

One thing I noticed is that even though our docs say two_factor_enrolled, and two_factor_optin can be updated via this endpoint those properties aren't in the User model's fillable array so changes are not applied. I think the docs should be updated to remove those fields right?

Type of change

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

@what-the-diff
Copy link

what-the-diff bot commented Sep 7, 2023

PR Summary

  • Enhancements to User Input Validations
    We've included a rule to validate the department_id provided while saving user-related details. This ensures that we maintain data integrity and the system works accurately.

  • Improved User Update Functionality Testing
    A new suite of tests has been added to evaluate the user update functionality via patch request. We're now rigorously testing various user attributes such as personal info (first name, last name, username, password, email), professional details (permissions, phone, job title, manager, and employee number), user preferences (notes), organization details (company, department, location, remote status, groups), and timeline details (VIP status, start and end date). This exhaustive testing will ensure a robust and error-free user update function.

@snipe
Copy link
Owner

snipe commented Sep 7, 2023

Yeah, I don't think we want those fields to be fillable

@snipe snipe merged commit 0683666 into snipe:develop Sep 7, 2023
@marcusmoore marcusmoore mentioned this pull request Sep 7, 2023
@marcusmoore marcusmoore deleted the fixes/improve-api-messaging branch September 7, 2023 23:42
@marcusmoore marcusmoore mentioned this pull request Sep 7, 2023
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.

2 participants