Page MenuHomePhorge

Groups: Display name
ClosedPublic

Authored by machniak on Nov 12 2021, 2:29 PM.
Tags
None
Referenced Files
F11588301: D3014.id8645.diff
Thu, Mar 28, 7:48 PM
Unknown Object (File)
Feb 23 2024, 3:06 PM
Unknown Object (File)
Feb 10 2024, 1:43 PM
Unknown Object (File)
Feb 9 2024, 9:57 AM
Unknown Object (File)
Jan 24 2024, 6:49 AM
Unknown Object (File)
Jan 24 2024, 6:43 AM
Unknown Object (File)
Jan 24 2024, 3:25 AM
Unknown Object (File)
Jan 21 2024, 9:06 AM
Subscribers
Restricted Project

Details

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

./phpunit

Diff Detail

Repository
rK kolab
Branch
dev/group-name
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 37055
Build 14774: arc lint + arc unit

Event Timeline

machniak created this revision.
  • 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
49

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
29

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)?

This revision now requires changes to proceed.Nov 17 2021, 1:56 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
49

It will not use an index, though. That's why I hesitate.

src/database/migrations/2021_11_10_100000_add_group_name_column.php
29

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
29

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.

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