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

Duplicate device name validation #163

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

Duplicate device name validation #163

wants to merge 1 commit into from

Conversation

yoksanherlie
Copy link

@yoksanherlie yoksanherlie commented Oct 17, 2018

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Enhancement/Security (non-breaking change which is not noticeable to end users)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I created a feature branch and did not open a pull request from my master branch.
  • My code follows the code style of this project.
  • My change requires an update to the README and I have updated it accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • This is a complete change and doesn't leave the project in a bad state.
  • All new and existing tests pass.

@dbudwin
Copy link
Owner

dbudwin commented Oct 17, 2018

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.

Copy link
Owner

@dbudwin dbudwin left a 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);
Copy link
Owner

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.

@yoksanherlie
Copy link
Author

Oops, my bad! Sorry about that, still a newbie here :)
I'll try to fix it!
Just one question, should I create a new pull request? And also I messed up my master branch, I forgot to create a feature branch.

Thanks for the feedback btw!

@dbudwin
Copy link
Owner

dbudwin commented Oct 17, 2018

No worries, newbies are always welcome here. Don't worry about opening a new pull request, or that this is your master branch, those are easy enough to workaround. To make changes to your existing pull request, my preferred method is to use fixup! commits. If you're unfamiliar with fixup!'s, they're super awesome and are nice because it leaves a very clean Git history. Simply make your changes locally, then instead of making a new commit or amending your existing commit, do git commit --fixup <sha of your existing commit>. Then do git push --force-with-lease origin <branch name>. After you push, I will look at the changes you made in the fixup! commit. If I approve of the changes I'll tell you to squash them with a simple git command, or I'll suggest more changes which can be done with additional fixup! commits.

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.

@dbudwin
Copy link
Owner

dbudwin commented Oct 17, 2018

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 Rules in Laravel, but I do like your approach. Maybe you can do a review of that pull request and see if it's possible?

@yoksanherlie
Copy link
Author

yoksanherlie commented Oct 18, 2018

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:
Because, the HTML code will be really messy if we keep the current method.

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