-
Notifications
You must be signed in to change notification settings - Fork 17
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
Duplicate device name validation #163
base: master
Are you sure you want to change the base?
Conversation
Thank you for your pull request. Could you please add some unit and UI (Laravel Dusk) tests to verify this functionality. Also, it appears two of the tests related to testing updating a device are broken. I'm assuming they are failing because when you edit a device and change another property (like the description) besides the name, and then save, it fails your validation check and thinks a device with that same name already exists. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to read up some on how Laravel's validation classes work, but this looks acceptable for the most part 👍 Like I mentioned in my other comment, please be sure to include tests. You can use Docker to run the tests for you if need be, see the Docker section in the README.
@@ -75,6 +83,12 @@ public function update(Request $request, Uuid $publicDeviceId): RedirectResponse | |||
} | |||
|
|||
$properties = $request->all(); | |||
$currentUserId = $request->user()->id; | |||
|
|||
$validator = $this->validateDuplicateName($properties, $currentUserId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This chunk of code appears to be identical to the one above when creating a device except for the verb in the flash message. Please consider making a private method with some parameter to prevent repeating this block of code.
Oops, my bad! Sorry about that, still a newbie here :) Thanks for the feedback btw! |
No worries, newbies are always welcome here. Don't worry about opening a new pull request, or that this is your https://fle.github.io/git-tip-keep-your-branch-clean-with-fixup-and-autosquash.html If you have any questions, or are not sure how to write tests, let me know and I can provide help. Luckily, there are a lot of them already in this codebase that you should be able to copy-paste and then modify. |
Just a heads up, another contributor submitted a pull request for this same issue just a few minutes ago: #164 I suggested they use rules like you do. Like I said, I'm not too familiar with |
Sorry for the late reply, just wanna ask one thing. Should we change the add and update functionality with ajax method? So that we can display the error message directly in the modal without closing the modal? Edit: |
Description
Create a validation logic when adding and updating device properties.
Motivation and Context
Disallow creating duplicate devices #162
How Has This Been Tested?
Not tested
Screenshots (if appropriate):
Types of changes
Checklist:
master
branch.