fix(config): Precise units for reservation reminders#1077
fix(config): Precise units for reservation reminders#1077JohnVillalovos merged 1 commit intoLibreBooking:developfrom
Conversation
belcirelk
force-pushed
the
fix/doc-start-time-units
branch
from
cfe6126 to
d3ec077
Compare
JohnVillalovos
left a comment
JohnVillalovos
left a comment
There was a problem hiding this comment.
Thanks @belcirelk
LGTM
There was a problem hiding this comment.
Pull request overview
This pull request updates configuration documentation for reservation reminder settings to clarify that the values must include time units (e.g., '15 minutes', '1 hours') rather than just numeric values. The changes aim to improve clarity for users configuring default reminder times.
Changes:
- Updated label and description text in
lib/Config/ConfigKeys.phpfor start and end reminder configuration - Updated comment documentation in
config/config.dist.phpfor start and end reminder settings
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| lib/Config/ConfigKeys.php | Updated label and description for RESERVATION_REMINDER_DEFAULT_START and RESERVATION_REMINDER_DEFAULT_END configuration keys to clarify format requirements |
| config/config.dist.php | Updated inline comments for default.start.reminder and default.end.reminder to include example format with units |
|
The main issue Copilot pointed out is that the code only supports "minutes", "hours", and "days". So "hour" doesn't work, unless the code is updated to support it. Easiest change is to update the example. I'm not worried about the minor capitalization issues. |
|
Ok, I will go see the code further. I assumed what was in ADVANCED-CONFIGURATION.rst was correct. So I will need to change this one too. Regarding the pull request, what is the cleanest way to do it, close this one and resubmit another one or just update the files and commit again? |
Just update the files and commit again, is usually the best way. |
JohnVillalovos
force-pushed
the
fix/doc-start-time-units
branch
from
7f8fa01 to
7000b4d
Compare
…s and days The documentation and examples were updated to show only valid values (minutes, hours, days).
JohnVillalovos
force-pushed
the
fix/doc-start-time-units
branch
from
7000b4d to
002543d
Compare
| Default reminder time before start (e.g., '15 minutes', '1 hours', '1 days'). | ||
|
|
||
| **reservation.default.end.reminder** | ||
| Default reminder time before end. |
There was a problem hiding this comment.
The advanced configuration docs update the start reminder examples, but the end reminder entry still doesn’t mention that a unit is required or provide a valid example. This looks inconsistent with the PR’s stated goal (start/end reminders require units); please update reservation.default.end.reminder to include the same format guidance/examples.
| Default reminder time before end. | |
| Default reminder time before end; must include a number and time unit (e.g., '15 minutes', '1 hours', '1 days'). |
|
|
||
| **reservation.default.start.reminder** | ||
| Default reminder time before start (e.g., '15 minutes', '1 hour'). | ||
| Default reminder time before start (e.g., '15 minutes', '1 hours', '1 days'). |
There was a problem hiding this comment.
The examples here use "1 hours" / "1 days", which reads like a grammatical mistake and may confuse users. Since the code only recognizes plural unit tokens (minutes/hours/days), consider using examples with values > 1 (e.g., '2 hours', '6 days') to stay both valid and grammatically natural.
| Default reminder time before start (e.g., '15 minutes', '1 hours', '1 days'). | |
| Default reminder time before start (e.g., '15 minutes', '2 hours', '6 days'). |
update configuration comments to specify that start/end
reminder times require units like hour/minute rather than just numbers.