Page MenuHomekolab.org

Groups: Display name
ClosedPublic

Authored by machniak on Fri, Nov 12, 2:29 PM.

Details

Reviewers
mollekopf
Group Reviewers
Restricted Project
Commits
rKacaad0dbc74a: Groups: Display name
Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

machniak requested review of this revision.Fri, Nov 12, 2:29 PM
machniak created this revision.
machniak updated this revision to Diff 8597.Fri, Nov 12, 4:25 PM
  • LDAP backend code improvements
mollekopf added inline comments.
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?
Maybe add a uniqueness contraint for name + email (so the uniqueness check in the role only additionally checks that you don't reuse the same name for any of your domains)?

mollekopf requested changes to this revision.Wed, Nov 17, 1:56 PM
This revision now requires changes to proceed.Wed, Nov 17, 1:56 PM
machniak added inline comments.Wed, Nov 17, 2:36 PM
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.

mollekopf added inline comments.Thu, Nov 18, 12:45 PM
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.

mollekopf accepted this revision.Thu, Nov 18, 12:45 PM
This revision is now accepted and ready to land.Thu, Nov 18, 12:45 PM
This revision was automatically updated to reflect the committed changes.