-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
3678: Require timelog comment via settings #39
Conversation
@@ -59,12 +59,14 @@ function () { | |||
echo '<link rel="stylesheet" href="' . htmlspecialchars($timeTableStyle) . '"></link>'; | |||
$userId = htmlspecialchars(session('userdata.id'), ENT_QUOTES, 'UTF-8'); | |||
$ticketCacheExpiration = app()->make(Setting::class)->getSetting('itk-leantime-timetable.ticketCacheExpiration') ?: '1200'; | |||
$requireTimeRegistrationComment = app()->make(Setting::class)->getSetting('itk-leantime-timetable.requireTimeRegistrationComment') ?: '0'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is 0 a sane default? what does 0 mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should '0' be a int perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really any reason to save it as int, so i changed it to just use the provided string "1" or setting it to "0"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You seem to be casting the requireTimeRegistrationComment
a lot, can it be set to an int all around and not cast?
Link to ticket
#3678
Description
Timelog comment is not only required if the plugin setting for it is checked.
Screenshot of the result
N/A
Checklist
If your code does not pass all the requirements on the checklist you have to add a comment explaining why this change
should be exempt from the list.
Additional comments or questions
If you have any further comments or questions for the reviewer please add them here.