diff --git a/docs/implementation-examples.md b/docs/implementation-examples.md new file mode 100644 index 00000000..1d2acd55 --- /dev/null +++ b/docs/implementation-examples.md @@ -0,0 +1,113 @@ +## Implementation examples + +This document functions as a dev journal, documenting some changes made and +the corresponding code modifications. +It may serve as an inspiration for future feature implementation and bug +fixes. Especially for those that are not familiar with the project code, but I will be using +it as a cheatsheet all the time as well probably. + +### Adding a simple new field `theme` + +The user can choose a theme that should be saved in the database. +There are a few places where the code has to be modified and the field added +so that it works like expected. +The value can only be added by **update**, so we only need to +change one action. +#### Recap +* `UserTheme.php` create theme enum. +* `UserSubmitUpdateAction.php` add argument to `malformedRequestBodyChecker()` call. +* `UserValidator.php` add validation line in `validateUserUpdate()`. +* `UserAuthorizationChecker.php` add to the `$grantedUpdateKeys` in `isGrantedToUpdate()`. +* `UserUpdater.php` add to the `if(in_array())` in `updateUser()`. +* `UserData.php` add to the instance variables and constructor. +* `UserFinderRepository.php` add to the instance variable `$fields`. +* `UserUpdateProvider.php` add theme change to `$basicDataChanges` var in `userUpdateAuthorizationCases()`. +* `UserUpdateProvider.php` add theme change to request body and error message to response +in `invalidUserUpdateCases()`. + +#### Implementation + +* The field is stored as enum in the database, and it is possible that one day there +will be other themes than just light and dark, so it makes sense to create a php enum: + ```php + // Domain/User/Enum/UserTheme.php + enum UserTheme: string + { + case light = 'light'; + case dark = 'dark'; + } + ``` +* In the action class `UserSubmitUpdateAction.php`, `'theme'` has to be added to the + `malformedRequestBodyChecker` argument list, so it knows `theme` is an accepted value. +* After it passed the request body keys validator, the service function `updateUser()` +is called which first validates the values `validateUserUpdate()`. As it's a backed enum +the function `validateBackedEnum()` can be used: + ```php + // UserValidator.php validateUserUpdate() + // ... + if (array_key_exists('theme', $userValues)) { + $this->validator->validateBackedEnum( + $userValues['theme'], + UserTheme::class, + 'theme', + $validationResult + ); + } + ``` +* When value validation is done, authorization is tested with `isGrantedToUpdate()`. +It checks if authenticated user is allowed to change given field. The way it works +is by adding the field name to a `$grantedUpdateKeys` array if the permissions match. +`theme` has the same authorization rules as the other "general user data" so it's added +right below the list: + ```php + if (array_key_exists('theme', $userDataToUpdate)) { + $grantedUpdateKeys[] = 'theme'; + } + ``` +* The service function `updateUser()` creates an array of the fields that may be updated +to be certain that only the fields that are designed to be updated actually get changed: + ```php + if (in_array($column, ['first_name', '...', 'theme'], true)) { + $validUpdateData[$column] = $value; + } + ``` +* Now to be able to read this value and so the app knows that `theme` is now a user value, +add it to `UserData.php` instance variables and also the constructor. + ```php + class UserData implements \JsonSerializable + { + /* ... */ + public ?UserTheme $theme = null; + + public function __construct(array $userData = []) + { + $this->theme = $userData['theme'] ?? null ? UserTheme::tryFrom($userData['theme']) : null; + } + } + ``` +* The file that retrieves the user values from the database is the repository `UserFinderRepository.php` +and it retrieves only the requested values, not automatically all fields. The list of values that +should be selected are in an instance variable of this file `$fields`: + ```php + class UserFinderRepository + { + private array $fields = ['id', '...', 'theme',]; + // ... + } + ``` +#### Testing +* User theme value is an extremely simple, value therefore not much has to be done especially when +there are good existing tests. +The test itself doesn't even has to be changed, only the data provider that feeds it values +`UserUpdateProvider.php`: + ```php + public static function userUpdateAuthorizationCases(): array + { + // ... + $basicDataChanges = ['first_name' => 'NewFirstName', '...', 'theme' => 'dark']; + // ... + } + ``` +* There is a validation being made so an invalid value should be tested as well. For this the function +`invalidUserUpdateCases()` can be extended to also include `'theme' => 'invalid',` in the request body +and `['field' => 'theme', 'message' => 'theme not existing'],` in the response validation errors. \ No newline at end of file diff --git a/public/assets/general/ajax/submit-update-data.js b/public/assets/general/ajax/submit-update-data.js index f7da03f0..2f3226e8 100644 --- a/public/assets/general/ajax/submit-update-data.js +++ b/public/assets/general/ajax/submit-update-data.js @@ -8,7 +8,7 @@ import {handleFail, removeValidationErrorMessages} from "./ajax-util/fail-handle * On success validation errors are removed and response content returned * * @param {object} formFieldsAndValues {field: value} e.g. {[input.name]: input.value} - * @param {string} route after base path + * @param {string} route after base path e.g. clients/1 * @param {boolean|string} redirectToRouteIfUnauthenticated true or redirect route url after base path. * If true, the redirect url is the same as the given route * diff --git a/public/assets/general/dark-mode/dark-mode.js b/public/assets/general/dark-mode/dark-mode.js index 89858cac..324152c1 100644 --- a/public/assets/general/dark-mode/dark-mode.js +++ b/public/assets/general/dark-mode/dark-mode.js @@ -1,4 +1,7 @@ // Get the toggle switch element +import {submitUpdate} from "../ajax/submit-update-data.js?v=0.2.1"; +import {displayFlashMessage} from "../page-component/flash-message/flash-message.js?v=0.2.1"; + const toggleSwitch = document.querySelector('#dark-mode-toggle-checkbox'); // Retrieve the current theme from localStorage @@ -6,7 +9,7 @@ const currentTheme = localStorage.getItem('theme') ? localStorage.getItem('theme // Set the theme based on the stored value from localStorage if (currentTheme) { - // Set the data-theme attribute on the root element + // Set the data-theme attribute on the html element document.documentElement.setAttribute('data-theme', currentTheme); // Check the toggle switch if the current theme is 'dark' @@ -18,31 +21,30 @@ if (currentTheme) { // Add event listener to the toggle switch for theme switching toggleSwitch.addEventListener('change', switchTheme, false); -// Set the theme on initial load if there is a stored value in localStorage -if (currentTheme) { - // Set the data-theme attribute on the root element - document.documentElement.setAttribute('data-theme', currentTheme); - - // If the current theme is 'dark', set the data-theme attribute to 'dark' - if (currentTheme === 'dark') { - document.documentElement.setAttribute('data-theme', 'dark'); - } -} - /** * Handle theme switching with localstorage * * @param e */ function switchTheme(e) { + let theme; // Check the current theme and switch to the opposite theme if (document.documentElement.getAttribute('data-theme') === 'dark') { - document.documentElement.setAttribute('data-theme', 'light'); - localStorage.setItem('theme', 'light'); + theme = 'light'; } else { - document.documentElement.setAttribute('data-theme', 'dark'); - localStorage.setItem('theme', 'dark'); + theme = 'dark'; } + // Set html data-attribute and local storage entry + document.documentElement.setAttribute('data-theme', theme); + localStorage.setItem('theme', theme); + + // Make ajax call to change value in database + let userId = document.getElementById('user-id').value; + submitUpdate({theme: theme}, `users/${userId}`, true) + .then(r => {}) + .catch(r => { + displayFlashMessage('error', 'Failed to change the theme in the database.') + }); } diff --git a/public/assets/general/page-component/flash-message/flash-message.css b/public/assets/general/page-component/flash-message/flash-message.css index 7c9eb592..d4f28b35 100644 --- a/public/assets/general/page-component/flash-message/flash-message.css +++ b/public/assets/general/page-component/flash-message/flash-message.css @@ -177,6 +177,10 @@ /*width: 200px;*/ /*display: block;*/ } + .flash-message a{ + color: black; + font-weight: bold; + } .flash-close-btn { position: absolute; diff --git a/public/assets/user/read/user-update-contenteditable.js b/public/assets/user/read/user-update-contenteditable.js index 9df0e043..63c9f317 100644 --- a/public/assets/user/read/user-update-contenteditable.js +++ b/public/assets/user/read/user-update-contenteditable.js @@ -48,7 +48,7 @@ function saveUserValueAndDisableContentEditable(field) { submitUpdate( {[field.dataset.name]: field.textContent.trim()}, `users/${userId}`, - `users/${userId}` + true ).then(responseJson => { // Field disabled before save request and re enabled on error }).catch(responseJson => { diff --git a/resources/migrations/20230606142721_db_change_1316013665647f_42494524b.php b/resources/migrations/20230606142721_db_change_1316013665647f_42494524b.php new file mode 100644 index 00000000..e5af1c60 --- /dev/null +++ b/resources/migrations/20230606142721_db_change_1316013665647f_42494524b.php @@ -0,0 +1,40 @@ +table('user', [ + 'id' => false, + 'primary_key' => ['id'], + 'engine' => 'InnoDB', + 'encoding' => 'utf8mb4', + 'collation' => 'utf8mb4_unicode_ci', + 'comment' => '', + 'row_format' => 'DYNAMIC', + ]) + ->addColumn('theme', 'enum', [ + 'null' => true, + 'default' => 'light', + 'limit' => 5, + 'values' => ['light', 'dark'], + 'after' => 'password_hash', + ]) + ->changeColumn('updated_at', 'datetime', [ + 'null' => true, + 'default' => 'CURRENT_TIMESTAMP', + 'after' => 'theme', + ]) + ->changeColumn('created_at', 'datetime', [ + 'null' => true, + 'default' => 'CURRENT_TIMESTAMP', + 'after' => 'updated_at', + ]) + ->changeColumn('deleted_at', 'datetime', [ + 'null' => true, + 'default' => null, + 'after' => 'created_at', + ]) + ->save(); + } +} diff --git a/resources/schema/schema.php b/resources/schema/schema.php index e113dbc0..39074b23 100644 --- a/resources/schema/schema.php +++ b/resources/schema/schema.php @@ -1602,12 +1602,36 @@ 'IS_GENERATED' => 'NEVER', 'GENERATION_EXPRESSION' => NULL, ), + 'theme' => + array ( + 'TABLE_CATALOG' => 'def', + 'TABLE_NAME' => 'user', + 'COLUMN_NAME' => 'theme', + 'ORDINAL_POSITION' => 8, + 'COLUMN_DEFAULT' => '\'light\'', + 'IS_NULLABLE' => 'YES', + 'DATA_TYPE' => 'enum', + 'CHARACTER_MAXIMUM_LENGTH' => 5, + 'CHARACTER_OCTET_LENGTH' => 20, + 'NUMERIC_PRECISION' => NULL, + 'NUMERIC_SCALE' => NULL, + 'DATETIME_PRECISION' => NULL, + 'CHARACTER_SET_NAME' => 'utf8mb4', + 'COLLATION_NAME' => 'utf8mb4_unicode_ci', + 'COLUMN_TYPE' => 'enum(\'light\',\'dark\')', + 'COLUMN_KEY' => '', + 'EXTRA' => '', + 'PRIVILEGES' => 'select,insert,update,references', + 'COLUMN_COMMENT' => '', + 'IS_GENERATED' => 'NEVER', + 'GENERATION_EXPRESSION' => NULL, + ), 'updated_at' => array ( 'TABLE_CATALOG' => 'def', 'TABLE_NAME' => 'user', 'COLUMN_NAME' => 'updated_at', - 'ORDINAL_POSITION' => 8, + 'ORDINAL_POSITION' => 9, 'COLUMN_DEFAULT' => 'current_timestamp()', 'IS_NULLABLE' => 'YES', 'DATA_TYPE' => 'datetime', @@ -1631,7 +1655,7 @@ 'TABLE_CATALOG' => 'def', 'TABLE_NAME' => 'user', 'COLUMN_NAME' => 'created_at', - 'ORDINAL_POSITION' => 9, + 'ORDINAL_POSITION' => 10, 'COLUMN_DEFAULT' => 'current_timestamp()', 'IS_NULLABLE' => 'YES', 'DATA_TYPE' => 'datetime', @@ -1655,7 +1679,7 @@ 'TABLE_CATALOG' => 'def', 'TABLE_NAME' => 'user', 'COLUMN_NAME' => 'deleted_at', - 'ORDINAL_POSITION' => 10, + 'ORDINAL_POSITION' => 11, 'COLUMN_DEFAULT' => 'NULL', 'IS_NULLABLE' => 'YES', 'DATA_TYPE' => 'datetime', diff --git a/resources/schema/schema.sql b/resources/schema/schema.sql index 1011f981..a99064a0 100644 --- a/resources/schema/schema.sql +++ b/resources/schema/schema.sql @@ -70,6 +70,7 @@ CREATE TABLE `user` ( `status` enum('active','locked','unverified','suspended') DEFAULT 'unverified', `email` varchar(254) NOT NULL, `password_hash` varchar(300) NOT NULL, + `theme` enum('light','dark') DEFAULT 'light', `updated_at` datetime DEFAULT current_timestamp(), `created_at` datetime DEFAULT current_timestamp(), `deleted_at` datetime DEFAULT NULL, diff --git a/src/Application/Actions/Authentication/Submit/LoginSubmitAction.php b/src/Application/Actions/Authentication/Submit/LoginSubmitAction.php index 401c1101..deb11457 100644 --- a/src/Application/Actions/Authentication/Submit/LoginSubmitAction.php +++ b/src/Application/Actions/Authentication/Submit/LoginSubmitAction.php @@ -9,6 +9,7 @@ use App\Domain\Authentication\Service\LoginVerifier; use App\Domain\Factory\LoggerFactory; use App\Domain\Security\Exception\SecurityException; +use App\Domain\User\Service\UserFinder; use App\Domain\Validation\ValidationException; use Odan\Session\SessionInterface; use Psr\Http\Message\ResponseInterface as Response; @@ -26,6 +27,7 @@ public function __construct( private readonly LoginVerifier $loginVerifier, private readonly SessionInterface $session, private readonly MalformedRequestBodyChecker $malformedRequestBodyChecker, + private readonly UserFinder $userFinder, ) { $this->logger = $logger->addFileHandler('error.log')->createInstance('auth-login'); } @@ -57,12 +59,22 @@ public function __invoke(ServerRequest $request, Response $response): Response // Add success message to flash $flash->add('success', 'Login successful'); + // Check if user has enabled dark mode and if yes populate var + $themeQueryParams = []; + if ($theme = $this->userFinder->findUserById($userId)->theme) { + $themeQueryParams['theme'] = $theme->value; + } + // After register and login success, check if user should be redirected if (isset($queryParams['redirect'])) { - return $this->responder->redirectToUrl($response, $request->getQueryParams()['redirect']); + return $this->responder->redirectToUrl( + $response, + $request->getQueryParams()['redirect'], + $themeQueryParams + ); } - return $this->responder->redirectToRouteName($response, 'home-page'); + return $this->responder->redirectToRouteName($response, 'home-page', [], $themeQueryParams); } catch (ValidationException $ve) { return $this->responder->renderOnValidationError( $response, diff --git a/src/Application/Actions/User/Ajax/UserSubmitUpdateAction.php b/src/Application/Actions/User/Ajax/UserSubmitUpdateAction.php index d6a29db5..720865be 100644 --- a/src/Application/Actions/User/Ajax/UserSubmitUpdateAction.php +++ b/src/Application/Actions/User/Ajax/UserSubmitUpdateAction.php @@ -60,6 +60,9 @@ public function __invoke( 'email', 'status', 'user_role_id', + 'theme', + // When adding a new field also add it in updateUser(), validateUserUpdate(), isGrantedToUpdate(), + // UserFinderRepository->fields and don't forget testing ])) { try { $updated = $this->userUpdater->updateUser($userIdToChange, $userValuesToChange); diff --git a/src/Application/Middleware/PhpViewExtensionMiddleware.php b/src/Application/Middleware/PhpViewExtensionMiddleware.php index 351737ed..1d79f807 100644 --- a/src/Application/Middleware/PhpViewExtensionMiddleware.php +++ b/src/Application/Middleware/PhpViewExtensionMiddleware.php @@ -44,11 +44,11 @@ public function process( 'uri' => $request->getUri(), 'basePath' => $this->app->getBasePath(), 'route' => $this->app->getRouteCollector()->getRouteParser(), - 'currRouteName' => RouteContext::fromRequest($request)->getRoute()->getName(), + 'currRouteName' => RouteContext::fromRequest($request)->getRoute()?->getName(), 'flash' => $this->session->getFlash(), // Used for public values used by view like company email address 'config' => $this->publicSettings, - // Check if granted to read user that is different from the logged in one (+1) + // Check if granted to read user that is different then the authenticated user itself (+1) 'userListAuthorization' => $this->userAuthorizationChecker->isGrantedToRead( ($this->session->get('user_id') ?? 1) + 1 ), diff --git a/src/Domain/User/Authorization/UserAuthorizationChecker.php b/src/Domain/User/Authorization/UserAuthorizationChecker.php index ed3ff1b3..52bdd98e 100644 --- a/src/Domain/User/Authorization/UserAuthorizationChecker.php +++ b/src/Domain/User/Authorization/UserAuthorizationChecker.php @@ -177,6 +177,11 @@ public function isGrantedToUpdate( if (array_key_exists('password_hash', $userDataToUpdate)) { $grantedUpdateKeys[] = 'password_hash'; } + if (array_key_exists('theme', $userDataToUpdate)) { + $grantedUpdateKeys[] = 'theme'; + } + // If new basic data fiel is added, add it to provider userUpdateAuthorizationCases() $basicDataChanges + // and invalid value to provider invalidUserUpdateCases() // Things that only managing_advisor and higher privileged are allowed to do with users (and not if own profile) // If user is managing advisor we know by the parent if-statement that the user to change has not higher diff --git a/src/Domain/User/Data/UserData.php b/src/Domain/User/Data/UserData.php index f45351da..5da71af4 100644 --- a/src/Domain/User/Data/UserData.php +++ b/src/Domain/User/Data/UserData.php @@ -3,6 +3,7 @@ namespace App\Domain\User\Data; use App\Domain\User\Enum\UserStatus; +use App\Domain\User\Enum\UserTheme; /** * Class User also serving as DTO for simplicity reasons. More details on slim-api-example/issues/2 @@ -21,6 +22,7 @@ class UserData implements \JsonSerializable public ?string $password2; public ?string $passwordHash; public ?UserStatus $status = null; + public ?UserTheme $theme = null; public ?int $userRoleId = null; public ?\DateTimeImmutable $updatedAt; public ?\DateTimeImmutable $createdAt; @@ -30,8 +32,6 @@ class UserData implements \JsonSerializable * User constructor. * * @param array $userData - * - * @throws \Exception */ public function __construct(array $userData = []) { @@ -43,6 +43,7 @@ public function __construct(array $userData = []) $this->password = $userData['password'] ?? null; $this->password2 = $userData['password2'] ?? null; $this->passwordHash = $userData['password_hash'] ?? null; + $this->theme = $userData['theme'] ?? null ? UserTheme::tryFrom($userData['theme']) : null; $this->updatedAt = $userData['updated_at'] ?? null ? new \DateTimeImmutable($userData['updated_at']) : null; $this->createdAt = $userData['created_at'] ?? null ? new \DateTimeImmutable($userData['created_at']) : null; $this->status = $userData['status'] ?? null ? UserStatus::tryFrom($userData['status']) : null; diff --git a/src/Domain/User/Enum/UserStatus.php b/src/Domain/User/Enum/UserStatus.php index 5e1e914e..2906baaa 100644 --- a/src/Domain/User/Enum/UserStatus.php +++ b/src/Domain/User/Enum/UserStatus.php @@ -11,7 +11,7 @@ enum UserStatus: string { use EnumToArray; - // First letter uppercase and rest lowercase as names are used as labels in html form + // First letter uppercase and rest lowercase because these names are used as labels in html form as they are case Unverified = 'unverified'; // Default after registration case Active = 'active'; // Verified via token received in email case Locked = 'locked'; // Locked for security reasons, may be reactivated by account holder via email diff --git a/src/Domain/User/Enum/UserTheme.php b/src/Domain/User/Enum/UserTheme.php new file mode 100644 index 00000000..e2e032a6 --- /dev/null +++ b/src/Domain/User/Enum/UserTheme.php @@ -0,0 +1,9 @@ +userFinderRepository->findUserById((int)$id)); } diff --git a/src/Domain/User/Service/UserUpdater.php b/src/Domain/User/Service/UserUpdater.php index ffc63303..6a9cda46 100644 --- a/src/Domain/User/Service/UserUpdater.php +++ b/src/Domain/User/Service/UserUpdater.php @@ -52,6 +52,7 @@ public function updateUser(int $userIdToChange, array $userValues): bool 'email', 'status', 'user_role_id', + 'theme', ], true)) { $validUpdateData[$column] = $value; } else { diff --git a/src/Domain/User/Service/UserValidator.php b/src/Domain/User/Service/UserValidator.php index 83aeb4f3..183b98a8 100644 --- a/src/Domain/User/Service/UserValidator.php +++ b/src/Domain/User/Service/UserValidator.php @@ -4,6 +4,7 @@ use App\Domain\User\Data\UserData; use App\Domain\User\Enum\UserStatus; +use App\Domain\User\Enum\UserTheme; use App\Domain\Validation\ValidationException; use App\Domain\Validation\ValidationResult; use App\Domain\Validation\Validator; @@ -53,6 +54,14 @@ public function validateUserUpdate(int $userId, array $userValues): ValidationRe if (array_key_exists('status', $userValues)) { $this->validateUserStatus($userValues['status'], $validationResult, true); } + if (array_key_exists('theme', $userValues)) { + $this->validator->validateBackedEnum( + $userValues['theme'], + UserTheme::class, + 'theme', + $validationResult + ); + } if (array_key_exists('user_role_id', $userValues)) { $this->validateUserRoleId($userValues['user_role_id'], $validationResult, true); } diff --git a/src/Domain/Validation/Validator.php b/src/Domain/Validation/Validator.php index 6b3647aa..eea212c6 100644 --- a/src/Domain/Validation/Validator.php +++ b/src/Domain/Validation/Validator.php @@ -254,7 +254,7 @@ public function validateNumeric( * @template Enum * * @param \BackedEnum|string|null $value enum case or backed string value - * @param class-string $enum + * @param class-string $enum e.g. UserRole::class * @param string $fieldName * @param ValidationResult $validationResult * @param bool $required diff --git a/src/Infrastructure/User/UserFinderRepository.php b/src/Infrastructure/User/UserFinderRepository.php index 4b779846..421b709f 100644 --- a/src/Infrastructure/User/UserFinderRepository.php +++ b/src/Infrastructure/User/UserFinderRepository.php @@ -19,6 +19,7 @@ class UserFinderRepository 'status', 'updated_at', 'created_at', + 'theme', ]; public function __construct( diff --git a/templates/layout.html.php b/templates/layout.html.php index 264d2c75..95f5bb3e 100644 --- a/templates/layout.html.php +++ b/templates/layout.html.php @@ -52,9 +52,16 @@ <?= $title ?> diff --git a/templates/user/user-read.html.php b/templates/user/user-read.html.php index aee06b75..2629d162 100644 --- a/templates/user/user-read.html.php +++ b/templates/user/user-read.html.php @@ -81,7 +81,7 @@ class="contenteditable-edit-icon cursor-pointer" echo ""; } -?> + ?> @@ -91,11 +91,11 @@ class="contenteditable-edit-icon cursor-pointer" @@ -126,23 +126,26 @@ class="contenteditable-edit-icon cursor-pointer" -
-

Dark mode

-