-
Notifications
You must be signed in to change notification settings - Fork 73
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
Fixes #20133: Technique editor allows id with only case difference, do an error, but still save technique #4079
Conversation
PR updated with a new commit |
PR updated with a new commit |
4a4765d
to
8f171e6
Compare
val techniques = techniqueRepository.getAll() | ||
techniques.keySet.map(_.name.value).contains(bundleName.value) | ||
techniques.values.map(_.name.toLowerCase).toList.contains(techniqueName.toLowerCase) |
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.
why do you need the toList ?
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.
techniques.values
produce Iterable[Technique]
and contains
do not apply on Iterable[A]
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.
you should advertise in the comment (at least) that it's a case insensitive comparision
isNameTaken = isTechniqueNameExist(technique.name) | ||
isIdTaken = isTechniqueIdExist(technique.bundleName) | ||
_ <- (isNameTaken, isIdTaken) match { | ||
case (true, true) => Failure(s"Technique name and ID must be unique. Name '${technique.name}' and ID '${technique.bundleName.value}' already used") |
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.
error message should state that the comparision is case insensitive
PR updated with a new commit |
This PR is not mergeable to upper versions. |
OK, squash merging this PR |
…o an error, but still save technique
8a9b9a3
to
196ae97
Compare
https://issues.rudder.io/issues/20133