-
Notifications
You must be signed in to change notification settings - Fork 22
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
New feature: restricted time for participation for moodleoverflow forums #138
base: master
Are you sure you want to change the base?
Conversation
Please add two behat tests
Thank you! |
Assuming a teacher has already answered, should the comment link be available for students? |
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.
Review of the new functionality
db/install.xml
Outdated
@@ -28,7 +28,8 @@ | |||
<FIELD NAME="anonymous" TYPE="int" LENGTH="2" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/> | |||
<FIELD NAME="needsreview" TYPE="int" LENGTH="2" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/> | |||
<FIELD NAME="allowmultiplemarks" TYPE="int" LENGTH="1" NOTNULL="true" DEFAULT="0" SEQUENCE="false"/> | |||
<FIELD NAME="limitedanswer" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/> | |||
<FIELD NAME="la_starttime" TYPE="int" LENGTH="10" NOTNULL="false" SEQUENCE="false"/> |
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.
Add a default value (DEFAULT="0"), as later it is checked, if the time is 0 or not
locallib.php
Outdated
@@ -1138,7 +1140,7 @@ function moodleoverflow_print_post($post, $discussion, $moodleoverflow, $cm, $co | |||
$footer = '', $highlight = '', $postisread = null, | |||
$dummyifcantsee = true, $istracked = false, | |||
$iscomment = false, $usermapping = [], $level = 0, | |||
$multiplemarks = false, $limitedanswertime = 0) { | |||
$multiplemarks = false, $limitedanswersetting = 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.
The parameter is an std Class and should be null and not "0". Replace with ?stdClass $limitedanswersetting = null
locallib.php
Outdated
'limitedanswer' => 'student', ]; | ||
|
||
if ($limitedanswersetting->la_starttime != 0 || $limitedanswersetting->la_endtime != 0) { | ||
$limitedanswersetting->la_starttime ? $infolimited = " " . |
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.
Ternary Operators should only be used in simple, one-line statements
if (is_currently_time_limited($limitedanswersetting)) { | ||
// Tamaro: Capabilities are often more elegant than roles. Moreover, the code was very repetitive... | ||
// perfect for a function. | ||
if (!has_capability('mod/moodleoverflow:addinstance', $modulecontext)) { |
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.
There seems to be an error with the capability checking. Reproduce: Make limitedanswer on, then off, then on. As a teacher, I can not answer question although I should be able to.
['limitedanswerdate' => gmdate('d.m.Y H:i', $limitedanswersetting->la_endtime)]) : $infolimited .= ''; | ||
echo html_writer::div($infolimited, 'alert alert-warning', ['role' => 'alert']); | ||
} | ||
if (is_currently_time_limited($limitedanswersetting)) { |
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.
Writing another function is not really necessary, as part of the checks in the function were already checked before. Replace the function call with the part of the checks that have not been made so far.
locallib.php
Outdated
* @return void | ||
* @throws coding_exception | ||
*/ | ||
function render_limited_answer($textmuted, &$commands, $str, $infolimited, $role) { |
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.
textmuted, commands and str should be described as the use is not directly understandable
locallib.php
Outdated
if ($limitedanswersetting->la_starttime != 0 || $limitedanswersetting->la_endtime != 0) { | ||
$limitedanswersetting->la_starttime ? $infolimited = " " . | ||
get_string('limitedanswer_info_starttime', 'moodleoverflow', | ||
['limitedanswerdate' => gmdate('d.m.Y H:i', $limitedanswersetting->la_starttime)]) : $infolimited = ''; |
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.
There appears an error with the gmdate function, as it not converts from UTC to CEST. Because of that, the limited answers times appear wrong in moodleoverflow
#65