-
Notifications
You must be signed in to change notification settings - Fork 1
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
ohm-525 Teacher Audit Log #21
base: master
Are you sure you want to change the base?
Conversation
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.
This looks like what was discussed yesterday and what the AC describe to me. Thanks @sketchings .
5301031
to
9d273c8
Compare
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.
Maybe change the "echo" at the end to reflect the correct table name. Otherwise seems to follow what we discussed.
assess2/AssessRecord.php
Outdated
@@ -180,6 +183,17 @@ public function saveRecord() { | |||
} | |||
$stm = $this->DBH->prepare($query); | |||
$stm->execute($qarr); | |||
if ($stm->rowCount()>0) { | |||
$result = TeacherAuditLog::addTracking( |
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.
This looks like it's going to record a teacher audit log even every time the assessment record is updated, which can happen without a grade change (like when feedback is added), and will also happen every time a student submits something, which shouldn't go in the teacher audit log.
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.
checking for $this->data['scoreoverride']==true
seems to work, and I also am recording score details
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.
With that, it looks like it's still going to record a log entry every time the assessment record is saved, if there is a scoreoverride set. Also, it shouldn't be necessary to decode the scoreddata - it was already decoded, and it doesn't even look like you're using it. The loadRecord call is also redundant - if we're saving the record, it's already loaded, and calling loadRecord again is causing it to next transactions and is triggering an error.
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.
TBH, I'm not 100% convinced this log entry is even useful, since the original grades are still present and visible when looking at the student's attempts, and it's obvious there's a grade override. Logging provides no additional information.
$aid, | ||
array('user_grades'=>$grades) | ||
); | ||
} |
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.
It looks like there's only logging for the "delete all attempts" clearing, but not for any of the individual attempt or question version clearing later in the file.
course/addquestionssave.php
Outdated
$cid, | ||
"Assessment Settings Change", | ||
$aid, | ||
$qarr |
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 source
It may be more useful to log what was changed, but that would be more difficult of course.
includes/TeacherAuditLog.php
Outdated
return false; | ||
} | ||
//always include calling file as source to metadata | ||
$metadata = ['source'=>basename(debug_backtrace()[0]['file'])]+$metadata; |
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.
Will the debug_backtrace
source be more useful than the $_SERVER['REQUEST_URI']
?
If we're trying to be able to say "this happened when you clicked on something on this page" I'm not sure if the innermost script is what we want to know.
b4a0c04
to
f1dab79
Compare
ohm-527 migration for data model ohm-527 change creation timestamp to created_at field ohm-527 match ENUM plural form ohm-527 update table name output ohm-528 initial logging class and testing pattern ohm-528 add findCourseAction and update/cleanup tests ohm-525 fix in memory sqlite db for tests ohm-525 use global userid ohm-525 add source file to all metadata ohm-525 Assessment setting changes logged ohm-525 update for mass updates not reqiring itemid ohm-525 Mass change assessment ohm-525 Unenroll with grade save ohm-525 Mass Assessment Date Change ohm-525 Clear Attempts, Question Settings Change, Assessment Settings Change ohm-525 Clear Attempts ohm-525 Clear Scores and Attempts ohm-525 update Clear Scores ohm-524 delete items ohm-525 initial Teacher Audit Log Report ohm-525 use blob for metadata ohm-525 update for code bugs ohm-525 record updated scores ohm-525 update Clear Scores, Clear Attempts. Change 'Grade Override' to 'Grade Change' ohm-525 mass change dates ohm-525 Change Grades Old Assessment ohm-525 record new assessment updates ohm-525 update include to TeacherAuditLog ohm-525 build assess2 for production ohm-525 loadRecord to get score data. Only record if this is a scoreoverride ohm-525 do not need to log teacher clearing their own scores ohm-525 clear attempts ohm-525 do not need to record for adding a new assessment ohm-525 clean up logged data ohm-525 record offline, forum and external grades from imas_grades ohm-525 must be admin to access Teacher Audit Log Report ohm-525 update breadcrumbs ohm-525 only pull imas_grades where appropriate ohm-525 count forums ohm-525 fix migrations ohm-525 add to actions file. Allow teacher audit log report to have no information. Add URI to tests ohm-525 add tracking grades. Add methods to report on count
d9f1c02
to
fc0177d
Compare
course/addquestions.php
Outdated
@@ -144,11 +153,31 @@ | |||
require_once('../includes/filehandler.php'); | |||
deleteallaidfiles($aid); | |||
if ($aver > 1) { | |||
$stm = $DBH->query("SELECT userid,score FROM imas_assessment_records WHERE assessmentid=:assessmentid"); |
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.
This is an invalid query: needs to use prepare
with an execute if using a placeholder
course/addquestions.php
Outdated
$stm = $DBH->prepare("DELETE FROM imas_assessment_records WHERE assessmentid=:assessmentid"); | ||
} else { | ||
$stm = $DBH->prepare("DELETE FROM imas_assessment_sessions WHERE assessmentid=:assessmentid"); | ||
$query = "SELECT userid, bestscores FROM imas_assessment_sessions WHERE assessmentid=$aid"; |
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.
This should probably be done using prepare
and execute, rather than sticking $aid directly into the query.
course/masschgdates.php
Outdated
} | ||
if (count($forumbasictoupdate)>0) { | ||
$placeholders = Sanitize::generateQueryPlaceholdersGrouped($forumbasictoupdate, 4); | ||
$query = "INSERT INTO imas_forums (id,startdate,enddate,avail) VALUES $placeholders "; | ||
$query .= "ON DUPLICATE KEY UPDATE startdate=VALUES(startdate),enddate=VALUES(enddate),avail=VALUES(avail)"; | ||
$stm = $DBH->prepare($query); | ||
$stm->execute($forumbasictoupdate); | ||
if ($stm->rowCount()>0) { | ||
$updated_settings = true; | ||
$metadata["Forums Basic"] = $forumbasictoupdate); |
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.
syntax error from the )
course/massexception.php
Outdated
} | ||
|
||
} else if (isset($_POST['forceclear'])) { | ||
//get old grades | ||
$stm = $DBH->prepare("SELECT userid, bestscores FROM imas_assessment_sessions WHERE userid=? AND assessmentid=$aid"); |
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.
This and a few lines down have a mix of named placeholders, ? placeholders, and directly injected parameters.
See parent task https://lumenlearning.atlassian.net/browse/OHM-525
DATA MODEL: