./phpunit
Details
Diff Detail
- Repository
- rK kolab
- Branch
- dev/group-name
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 37049 Build 14768: arc lint + arc unit
Event Timeline
src/app/Http/Controllers/API/V4/GroupsController.php | ||
---|---|---|
293 | This is overwritten if GroupsController::validateGroupEmail doesn't fail, so it seems unnecessary. | |
308 | This overwrite errors if GroupsController::validateGroupEmail already failed. Either merge the errors or don't further validate after the first error. I would probably just integrate GroupsController::validateGroupEmail via a closure: https://laravel.com/docs/6.x/validation#using-closures | |
src/app/Rules/GroupName.php | ||
50 | I think that would be faster indeed as the entitlements join seems unnecessary for the uniqueness constraint. | |
src/database/migrations/2021_11_10_100000_add_group_name_column.php | ||
30 | Maybe add an index for the name field for the uniqueness checks? |
src/app/Http/Controllers/API/V4/GroupsController.php | ||
---|---|---|
308 | To fully validate a group name I need a domain. And we get the domain from an email address. So, I first validate the email address, and only if it is valid I do the "full validation". If email address is invalid I still want to do a "basic validation" on the name. | |
src/app/Rules/GroupName.php | ||
50 | It will not use an index, though. That's why I hesitate. | |
src/database/migrations/2021_11_10_100000_add_group_name_column.php | ||
30 | Name alone cannot be unique. An index on name + email does not really make sense to me. Maybe this table should have a domain or domain_id column, but it shouldn't be a stopper. |
src/app/Http/Controllers/API/V4/GroupsController.php | ||
---|---|---|
308 | Makes sense. It seems I have overlooked the array_merge since you're not actually overwriting $errors, so this should work. FWIW, the alternative I suggested wouldn't look much better anyways: $email = $request->input('email'); $members = $request->input('members'); $errors = []; list(, $domainName) = explode('@', $email); $rules = [ 'email' => [ function ($attribute, $value, $fail) { if ($error = GroupsController::validateGroupEmail($value, $owner)) { $fail($error); } }, ] 'name' => [ 'bail', 'required', 'string', //Validate the email for the $domainName below function ($attribute, $value, $fail) { if ($error = GroupsController::validateGroupEmail($email, $owner)) { $fail($error); } }, new GroupName($owner, $domainName), ] ]; $v = Validator::make($request->all(), $rules); if ($v->fails()) { $errors = $v->errors()->toArray(); } | |
src/database/migrations/2021_11_10_100000_add_group_name_column.php | ||
30 | The index on name would not be for uniqueness, but for lookup performance. The name + domain index would indeed be what we're after. I agree this is not a blocker. |