Reactify configuration editing without changing stored setting formats#10509
Reactify configuration editing without changing stored setting formats#10509Montekkundan wants to merge 3 commits into
Conversation
github-actions
Bot
added
Language: PHP
|
Although they are currently (unfortunately) pervasively used in LORIS, https://react.dev/blog/2024/04/25/react-19-upgrade-guide#removed-proptypes-and-defaultprops |
github-actions
Bot
added
the
Module: dashboard
There was a problem hiding this comment.
Remove this file -- it should not have been committed, can you also add it to the gitignore?
| $setting['Value'] = ''; | ||
| } | ||
| } |
There was a problem hiding this comment.
If is not allow multiple and there are multiple settings it should throw this error:
throw new \ConfigurationException(
"Multiple config settings for setting that "
. "does not allow multiple"
);
| $setting['Value'] = ''; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing boolean check / conversion
if ($setting['DataType'] == "boolean") {
$settingValue = $setting['Value'] ?? "false";
if ($settingValue === "false") {
$setting['Value'] = false;
} else if ($settingValue === "true") {
$setting['Value'] = true;
} else {
$setting['Value'] = !empty($settingValue);
}
}
There was a problem hiding this comment.
This seems to be quite different than the version in @8471, it is missing loadResources for instance -- was that on purpose?
| /** | ||
| * Returns true if user has access to this endpoint. | ||
| * | ||
| * @param \User $user The user whose access is being checked | ||
| * | ||
| * @return bool | ||
| */ | ||
| #[\Override] | ||
| public function isAccessibleBy(\User $user) : bool | ||
| { | ||
| return $user->hasPermission('config'); | ||
| } |
There was a problem hiding this comment.
The permission is checked by the module entry in configuration.class.inc, so we don't need this here
| * | ||
| * @license http://www.gnu.org/licenses/gpl-3.0.txt GPLv3 | ||
| */ | ||
| class Setting extends \NDB_Page implements RequestHandlerInterface |
There was a problem hiding this comment.
Should this extend Endpoint instead of Page like it did in #8471? Since we don't really open the Settings page as a page? Let me know if there was a reason you did NDB_Page instead. If it is changed to Endpoint it will need
public function process(
ServerRequestInterface $request,
RequestHandlerInterface $handler
) : ResponseInterface {
return $handler->handle($request);
}from #8471
| }; | ||
|
|
||
| /** | ||
| * Intro text for the configuration page. |
There was a problem hiding this comment.
Can you update this comment to be the actual intro
| <p> | ||
| Please enter the various configuration variables into the fields below. | ||
| For information on how to configure LORIS, please refer to the Help | ||
| section and/or the Developer's guide. |
There was a problem hiding this comment.
| section and/or the Developer's guide. | |
| section and/or the Developer's guide. |
| function booleanRadioValue(value: ConfigValue): string { | ||
| if (value === '1' || value === 1) { | ||
| return '1'; | ||
| } | ||
| if (value === '0' || value === 0) { | ||
| return '0'; | ||
| } | ||
| if (value === true || value === 'true') { | ||
| return 'true'; | ||
| } | ||
| return 'false'; | ||
| } | ||
|
|
||
| /** | ||
| * Preserve legacy 1/0 boolean storage when a setting already uses it. | ||
| * | ||
| * @param {ConfigValue} value Stored boolean value | ||
| * @return {object} | ||
| */ | ||
| function booleanRadioOptions(value: ConfigValue): ConfigOptionMap { | ||
| if (value === '1' || value === '0' || value === 1 || value === 0) { | ||
| return {'1': 'Yes', '0': 'No'}; | ||
| } | ||
| return {'true': 'Yes', 'false': 'No'}; | ||
| } |
There was a problem hiding this comment.
Where are config values being moved from 0 / 1 to true / false? I think we should keep them as 0 / 1 for now, and revisit migrating to true / false later.
skarya22
added
the
State: Needs work
3 participants
Brief summary of changes
Testing instructions (if applicable)
/configuration/as a user with theconfigpermission and confirm the page loads as the React UI without the old Smarty form.true/false, then one that stores1/0, and confirm each keeps its existing storage format after saving.Link(s) to related issue(s)