From 8a6e96dc8ef98df1e733681825eafe413165a697 Mon Sep 17 00:00:00 2001 From: Sami Mazouz Date: Mon, 26 Feb 2024 17:43:45 +0100 Subject: [PATCH] feat: refactor nicknames extension --- extensions/nicknames/extend.php | 16 ++--- .../nicknames/src/AddNicknameValidation.php | 50 ------------- .../nicknames/src/Api/UserResourceFields.php | 54 ++++++++++++++ .../nicknames/src/SaveNicknameToDatabase.php | 40 ----------- .../tests/integration/api/EditUserTest.php | 37 ++++++++-- .../tests/integration/api/RegisterTest.php | 8 +-- .../src/Discussion/DiscussionValidator.php | 23 ------ framework/core/src/Group/GroupValidator.php | 20 ------ framework/core/src/Post/PostValidator.php | 22 ------ .../integration/extenders/ValidatorTest.php | 70 ++++++++++++++++--- 10 files changed, 158 insertions(+), 182 deletions(-) delete mode 100644 extensions/nicknames/src/AddNicknameValidation.php create mode 100644 extensions/nicknames/src/Api/UserResourceFields.php delete mode 100644 extensions/nicknames/src/SaveNicknameToDatabase.php delete mode 100644 framework/core/src/Discussion/DiscussionValidator.php delete mode 100644 framework/core/src/Group/GroupValidator.php delete mode 100644 framework/core/src/Post/PostValidator.php diff --git a/extensions/nicknames/extend.php b/extensions/nicknames/extend.php index c2c5239ec..224396816 100644 --- a/extensions/nicknames/extend.php +++ b/extensions/nicknames/extend.php @@ -9,9 +9,10 @@ namespace Flarum\Nicknames; -use Flarum\Api\Serializer\UserSerializer; +use Flarum\Api\Resource; use Flarum\Extend; use Flarum\Nicknames\Access\UserPolicy; +use Flarum\Nicknames\Api\UserResourceFields; use Flarum\Search\Database\DatabaseSearchDriver; use Flarum\User\Event\Saving; use Flarum\User\Search\UserSearcher; @@ -33,13 +34,9 @@ return [ (new Extend\User()) ->displayNameDriver('nickname', NicknameDriver::class), - (new Extend\Event()) - ->listen(Saving::class, SaveNicknameToDatabase::class), - - (new Extend\ApiSerializer(UserSerializer::class)) - ->attribute('canEditNickname', function (UserSerializer $serializer, User $user) { - return $serializer->getActor()->can('editNickname', $user); - }), + (new Extend\ApiResource(Resource\UserResource::class)) + ->fields(UserResourceFields::class) + ->field('username', UserResourceFields::username(...)), (new Extend\Settings()) ->default('flarum-nicknames.set_on_registration', true) @@ -50,9 +47,6 @@ return [ ->serializeToForum('setNicknameOnRegistration', 'flarum-nicknames.set_on_registration', 'boolval') ->serializeToForum('randomizeUsernameOnRegistration', 'flarum-nicknames.random_username', 'boolval'), - (new Extend\Validator(UserValidator::class)) - ->configure(AddNicknameValidation::class), - (new Extend\SearchDriver(DatabaseSearchDriver::class)) ->setFulltext(UserSearcher::class, NicknameFullTextFilter::class), diff --git a/extensions/nicknames/src/AddNicknameValidation.php b/extensions/nicknames/src/AddNicknameValidation.php deleted file mode 100644 index ed2350e30..000000000 --- a/extensions/nicknames/src/AddNicknameValidation.php +++ /dev/null @@ -1,50 +0,0 @@ -getUser() ? ','.$flarumValidator->getUser()->id : ''; - $rules = $validator->getRules(); - - $rules['nickname'] = [ - function ($attribute, $value, $fail) { - $regex = $this->settings->get('flarum-nicknames.regex'); - if ($regex && ! preg_match_all("/$regex/", $value)) { - $fail($this->translator->trans('flarum-nicknames.api.invalid_nickname_message')); - } - }, - 'min:'.$this->settings->get('flarum-nicknames.min'), - 'max:'.$this->settings->get('flarum-nicknames.max'), - 'nullable' - ]; - - if ($this->settings->get('flarum-nicknames.unique')) { - $rules['nickname'][] = 'unique:users,username'.$idSuffix; - $rules['nickname'][] = 'unique:users,nickname'.$idSuffix; - $rules['username'][] = 'unique:users,nickname'.$idSuffix; - } - - $validator->setRules($rules); - } -} diff --git a/extensions/nicknames/src/Api/UserResourceFields.php b/extensions/nicknames/src/Api/UserResourceFields.php new file mode 100644 index 000000000..4c7c96694 --- /dev/null +++ b/extensions/nicknames/src/Api/UserResourceFields.php @@ -0,0 +1,54 @@ +settings->get('flarum-nicknames.regex'); + + if(! empty($regex)) { + $regex = "/$regex/"; + } + + return [ + Schema\Str::make('nickname') + ->visible(false) + ->writable(function (User $user, Context $context) { + return $context->getActor()->can('editNickname', $user); + }) + ->nullable() + ->regex($regex ?? '', ! empty($regex)) + ->minLength($this->settings->get('flarum-nicknames.min')) + ->maxLength($this->settings->get('flarum-nicknames.max')) + ->unique('users', 'nickname', true, (bool) $this->settings->get('flarum-nicknames.unique')) + ->unique('users', 'username', true, (bool) $this->settings->get('flarum-nicknames.unique')) + ->validationMessages([ + 'nickname.regex' => $this->translator->trans('flarum-nicknames.api.invalid_nickname_message'), + ]) + ->set(function (User $user, ?string $nickname) { + $user->nickname = $user->username === $nickname ? null : $nickname; + }), + Schema\Boolean::make('canEditNickname') + ->get(fn (User $user, Context $context) => $context->getActor()->can('editNickname', $user)), + ]; + } + + public static function username(Schema\Str $field): Schema\Str + { + return $field->unique('users', 'nickname', true, (bool) resolve(SettingsRepositoryInterface::class)->get('flarum-nicknames.unique')); + } +} diff --git a/extensions/nicknames/src/SaveNicknameToDatabase.php b/extensions/nicknames/src/SaveNicknameToDatabase.php deleted file mode 100644 index 4a2cdb593..000000000 --- a/extensions/nicknames/src/SaveNicknameToDatabase.php +++ /dev/null @@ -1,40 +0,0 @@ -user; - $data = $event->data; - $actor = $event->actor; - $attributes = Arr::get($data, 'attributes', []); - - if (isset($attributes['nickname'])) { - $actor->assertCan('editNickname', $user); - - $nickname = $attributes['nickname']; - - // If the user sets their nickname back to the username - // set the nickname to null so that it just falls back to the username - $user->nickname = $user->username === $nickname ? null : $nickname; - } - } -} diff --git a/extensions/nicknames/tests/integration/api/EditUserTest.php b/extensions/nicknames/tests/integration/api/EditUserTest.php index 7c338f5c0..c2cc3253b 100644 --- a/extensions/nicknames/tests/integration/api/EditUserTest.php +++ b/extensions/nicknames/tests/integration/api/EditUserTest.php @@ -10,6 +10,7 @@ namespace Flarum\Nicknames\Tests\integration; use Flarum\Group\Group; +use Flarum\Locale\TranslatorInterface; use Flarum\Testing\integration\RetrievesAuthorizedUsers; use Flarum\Testing\integration\TestCase; use Flarum\User\User; @@ -45,7 +46,7 @@ class UpdateTest extends TestCase 'authenticatedAs' => 2, 'json' => [ 'data' => [ - 'type' => 'posts', + 'type' => 'users', 'attributes' => [ 'nickname' => 'new nickname', ], @@ -54,7 +55,7 @@ class UpdateTest extends TestCase ]) ); - $this->assertEquals(403, $response->getStatusCode()); + $this->assertEquals(403, $response->getStatusCode(), $response->getBody()->getContents()); } /** @@ -73,7 +74,7 @@ class UpdateTest extends TestCase 'authenticatedAs' => 2, 'json' => [ 'data' => [ - 'type' => 'posts', + 'type' => 'users', 'attributes' => [ 'nickname' => 'new nickname', ], @@ -82,8 +83,36 @@ class UpdateTest extends TestCase ]) ); - $this->assertEquals(200, $response->getStatusCode()); + $this->assertEquals(200, $response->getStatusCode(), $response->getBody()->getContents()); $this->assertEquals('new nickname', User::find(2)->nickname); } + + /** + * @test + */ + public function cant_edit_nickname_if_invalid_regex() + { + $this->setting('flarum-nicknames.set_on_registration', true); + $this->setting('flarum-nicknames.regex', '^[A-z]+$'); + + $response = $this->send( + $this->request('PATCH', '/api/users/2', [ + 'authenticatedAs' => 2, + 'json' => [ + 'data' => [ + 'type' => 'users', + 'attributes' => [ + 'nickname' => '007', + ], + ], + ], + ]) + ); + + $body = $response->getBody()->getContents(); + + $this->assertEquals(422, $response->getStatusCode(), $body); + $this->assertStringContainsString($this->app()->getContainer()->make(TranslatorInterface::class)->trans('flarum-nicknames.api.invalid_nickname_message'), $body); + } } diff --git a/extensions/nicknames/tests/integration/api/RegisterTest.php b/extensions/nicknames/tests/integration/api/RegisterTest.php index af050ab3f..fbf411e46 100644 --- a/extensions/nicknames/tests/integration/api/RegisterTest.php +++ b/extensions/nicknames/tests/integration/api/RegisterTest.php @@ -44,7 +44,7 @@ class RegisterTest extends TestCase ]) ); - $this->assertEquals(201, $response->getStatusCode()); + $this->assertEquals(201, $response->getStatusCode(), $response->getBody()->getContents()); /** @var User $user */ $user = User::where('username', 'test')->firstOrFail(); @@ -72,7 +72,7 @@ class RegisterTest extends TestCase ]) ); - $this->assertEquals(403, $response->getStatusCode()); + $this->assertEquals(403, $response->getStatusCode(), $response->getBody()->getContents()); } /** @@ -94,7 +94,7 @@ class RegisterTest extends TestCase ]) ); - $this->assertEquals(422, $response->getStatusCode()); + $this->assertEquals(422, $response->getStatusCode(), $response->getBody()->getContents()); } /** @@ -116,6 +116,6 @@ class RegisterTest extends TestCase ]) ); - $this->assertEquals(201, $response->getStatusCode()); + $this->assertEquals(201, $response->getStatusCode(), $response->getBody()->getContents()); } } diff --git a/framework/core/src/Discussion/DiscussionValidator.php b/framework/core/src/Discussion/DiscussionValidator.php deleted file mode 100644 index 48013e62d..000000000 --- a/framework/core/src/Discussion/DiscussionValidator.php +++ /dev/null @@ -1,23 +0,0 @@ - [ - 'required', - 'min:3', - 'max:80' - ] - ]; -} diff --git a/framework/core/src/Group/GroupValidator.php b/framework/core/src/Group/GroupValidator.php deleted file mode 100644 index 26a571d1e..000000000 --- a/framework/core/src/Group/GroupValidator.php +++ /dev/null @@ -1,20 +0,0 @@ - ['required'], - 'name_plural' => ['required'] - ]; -} diff --git a/framework/core/src/Post/PostValidator.php b/framework/core/src/Post/PostValidator.php deleted file mode 100644 index 2018d9e11..000000000 --- a/framework/core/src/Post/PostValidator.php +++ /dev/null @@ -1,22 +0,0 @@ - [ - 'required', - 'max:65535' - ] - ]; -} diff --git a/framework/core/tests/integration/extenders/ValidatorTest.php b/framework/core/tests/integration/extenders/ValidatorTest.php index ddb3b577c..cf83bd0d6 100644 --- a/framework/core/tests/integration/extenders/ValidatorTest.php +++ b/framework/core/tests/integration/extenders/ValidatorTest.php @@ -10,16 +10,16 @@ namespace Flarum\Tests\integration\extenders; use Flarum\Extend; -use Flarum\Group\GroupValidator; +use Flarum\Foundation\AbstractValidator; use Flarum\Testing\integration\TestCase; -use Flarum\User\UserValidator; +use Flarum\User\User; use Illuminate\Validation\ValidationException; class ValidatorTest extends TestCase { private function extendToRequireLongPassword() { - $this->extend((new Extend\Validator(UserValidator::class))->configure(function ($flarumValidator, $validator) { + $this->extend((new Extend\Validator(CustomUserValidator::class))->configure(function ($flarumValidator, $validator) { $validator->setRules([ 'password' => [ 'required', @@ -31,7 +31,7 @@ class ValidatorTest extends TestCase private function extendToRequireLongPasswordViaInvokableClass() { - $this->extend((new Extend\Validator(UserValidator::class))->configure(CustomValidatorClass::class)); + $this->extend((new Extend\Validator(CustomUserValidator::class))->configure(CustomValidatorClass::class)); } /** @@ -39,7 +39,7 @@ class ValidatorTest extends TestCase */ public function custom_validation_rule_does_not_exist_by_default() { - $this->app()->getContainer()->make(UserValidator::class)->assertValid(['password' => 'simplePassword']); + $this->app()->getContainer()->make(CustomUserValidator::class)->assertValid(['password' => 'simplePassword']); // If we have gotten this far, no validation exception has been thrown, so the test is succesful. $this->assertTrue(true); @@ -54,7 +54,7 @@ class ValidatorTest extends TestCase $this->expectException(ValidationException::class); - $this->app()->getContainer()->make(UserValidator::class)->assertValid(['password' => 'simplePassword']); + $this->app()->getContainer()->make(CustomUserValidator::class)->assertValid(['password' => 'simplePassword']); } /** @@ -66,7 +66,7 @@ class ValidatorTest extends TestCase $this->expectException(ValidationException::class); - $this->app()->getContainer()->make(UserValidator::class)->assertValid(['password' => 'simplePassword']); + $this->app()->getContainer()->make(CustomUserValidator::class)->assertValid(['password' => 'simplePassword']); } /** @@ -76,7 +76,7 @@ class ValidatorTest extends TestCase { $this->extendToRequireLongPassword(); - $this->app()->getContainer()->make(GroupValidator::class)->assertValid(['password' => 'simplePassword']); + $this->app()->getContainer()->make(CustomValidator::class)->assertValid(['password' => 'simplePassword']); // If we have gotten this far, no validation exception has been thrown, so the test is succesful. $this->assertTrue(true); @@ -95,3 +95,57 @@ class CustomValidatorClass ] + $validator->getRules()); } } + +class CustomUserValidator extends AbstractValidator +{ + protected ?User $user = null; + + public function getUser(): ?User + { + return $this->user; + } + + public function setUser(User $user): void + { + $this->user = $user; + } + + protected function getRules(): array + { + $idSuffix = $this->user ? ','.$this->user->id : ''; + + return [ + 'username' => [ + 'required', + 'regex:/^[a-z0-9_-]+$/i', + 'unique:users,username'.$idSuffix, + 'min:3', + 'max:30' + ], + 'email' => [ + 'required', + 'email:filter', + 'unique:users,email'.$idSuffix + ], + 'password' => [ + 'required', + 'min:8' + ] + ]; + } + + protected function getMessages(): array + { + return [ + 'username.regex' => $this->translator->trans('core.api.invalid_username_message') + ]; + } +} + +class CustomValidator extends AbstractValidator +{ + protected array $rules = [ + 'name_singular' => ['required'], + 'name_plural' => ['required'] + ]; +}