Skip to content

Commit

Permalink
GH-723 Improve parameter syncing
Browse files Browse the repository at this point in the history 
- Only create a new contextid if parameters were synced
- Carry-over old itemparameters to the new context and include them in
  the decision of which model should be the active one. E.g., if we get
  3 new models for questionid 1 but already have a different model
  locally with a higher status, this one should still be set as the
  active one.
  • Loading branch information
@davidszkiba
davidszkiba committed Nov 22, 2024
1 parent 1b448ef commit 6c6b145
Show file tree
Hide file tree
Showing 2 changed files with 224 additions and 36 deletions.
146 changes: 135 additions & 11 deletions classes/catquiz.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2585,10 +2585,41 @@ public static function get_item(int $contextid, int $componentid, string $compon
);
}

/**
* Retrieve an item together with its parameters.
*
* @param int $componentid The ID of the component.
* @param string $model The model of the item parameter
* @param int $contextid The context of the item parameter.
*
* @return ?stdClassThe record object retrieved from the database or null if not found.
*/
public function get_item_with_params(int $componentid, string $model, int $contextid): ?stdClass {
global $DB;

$sql = <<<SQL
SELECT *
FROM {local_catquiz_items} i
JOIN {local_catquiz_itemparams} ip ON ip.itemid = i.id
AND ip.contextid = :contextid
WHERE i.componentid = :componentid
AND ip.model = :model
SQL;
return $DB->get_record_sql(
$sql,
[
'componentid' => $componentid,
'model' => $model,
'contextid' => $contextid,
]
) ?: null;
}

/**
* Update an item record in the 'local_catquiz_items' table.
*
* @param stdClass $item The item object containing the updated data.
*
* @return void
*/
public static function update_item(stdClass $item): void {
Expand Down Expand Up @@ -2664,13 +2695,22 @@ public function move_items_to_new_context(int $newcontextid, ?int $oldcontextid)

// For each itemparam in the new context, get the one with the highest `status` value. If there a multiple, pick the first
// one.
// Problem: as it is now, we always select one of the new params.
// What if a param was not changed and should still be the active one?
// If "rasch" exists in both the old and new context, we ALWAYS take the new one.
// BUT: we want to include e.g. "raschbirnbaum" if it exists just in the old context.
$activeparams = [];
$itemparams = $DB->get_records(
'local_catquiz_itemparams',
['contextid' => $newcontextid],
'componentid, id, componentid, status'
);
foreach ($itemparams as $ip) {
$tocopy = [];
$toupdate = [];
$transaction = $DB->start_delegated_transaction();
$mergedparams = $this->merge_params($oldcontextid, $newcontextid);
foreach ($mergedparams as $ip) {
// Mark parameters we want to keep from the previous copy as 'to copy'.
if ($ip->contextid == $oldcontextid) {
$tocopy[$ip->componentid][] = $ip;
} else {
$toupdate[$ip->componentid][] = $ip;
}
if (
!array_key_exists($ip->componentid, $activeparams)
|| $activeparams[$ip->componentid]->status <= $ip->status
Expand All @@ -2684,13 +2724,23 @@ public function move_items_to_new_context(int $newcontextid, ?int $oldcontextid)
$items = $DB->get_records('local_catquiz_items', ['contextid' => $oldcontextid]);
foreach ($items as $item) {
$item->contextid = $newcontextid;
if (array_key_exists($item->componentid, $activeparams)) {
$ip = $activeparams[$item->componentid];
$item->activeparamid = $ip->id;
$ip->itemid = $item->id;
$DB->update_record('local_catquiz_itemparams', $ip);
// If we have no item param for this item, skip it.
if (!array_key_exists($item->componentid, $activeparams)) {
continue;
}
$activeparam = $activeparams[$item->componentid];
$activemodel = null;

// Update the activeparam property.
// If it is a newly synced param, make them point to each other.
if ($activeparam->contextid == $newcontextid) {
$item->activeparamid = $activeparam->id;
$activeparam->itemid = $item->id;
$DB->update_record('local_catquiz_itemparams', $activeparam);
$activemodel = $activeparam->model;
} else {
// Otherwise: Should we copy the param from the previous context?
// This could be more than one, or?
$copiedparam = $oldactiveparams[$item->activeparamid];
unset($copiedparam->id);
$copiedparam->contextid = $newcontextid;
Expand All @@ -2699,8 +2749,82 @@ public function move_items_to_new_context(int $newcontextid, ?int $oldcontextid)
$copiedparam->timemodified = $now;
$copiedparamid = $DB->insert_record('local_catquiz_itemparams', $copiedparam, true);
$item->activeparamid = $copiedparamid;
$activemodel = $copiedparam->model;
}
$DB->update_record('local_catquiz_items', $item);

// Now we copy the old non-active params.
foreach ($tocopy[$item->componentid] as $old) {
// Skip the parameter that has already been copied.
if ($old->model == $activemodel) {
continue;
}
$now = time();
$copied = $old;
$copied->timecreated = $now;
$copied->timemodified = $now;
$copied->contextid = $newcontextid;
$DB->insert_record('local_catquiz_itemparams', $copied);
}

// And now we set the correct itemid for the new, non-active params.
foreach ($toupdate[$item->componentid] as $upd) {
if ($upd->model == $activemodel) {
continue;
}
$upd->itemid = $item->id;
$upd->timemodified = time();
$DB->update_record('local_catquiz_itemparams', $upd);
}
}
$DB->commit_delegated_transaction($transaction);
}

/**
* Merges item parameters from two contexts.
*
* When a parameter exists in both contexts for the same itemid/model combination,
* the parameter from the new context is chosen. When it exists in only one context,
* it is included in the result. There can be at most one parameter per (itemid, model, contextid)
* combination.
*
* @param int $oldcontextid The ID of the old context
* @param int $newcontextid The ID of the new context
*
* @return array Array of merged item parameters
*/
public function merge_params(int $oldcontextid, int $newcontextid): array {
global $DB;

// First get all parameters from both contexts.
[$contextsql, $contextparams] = $DB->get_in_or_equal(
[$oldcontextid, $newcontextid],
SQL_PARAMS_NAMED
);

$params = $DB->get_records_select(
'local_catquiz_itemparams',
"contextid $contextsql",
$contextparams
);

// Group parameters by itemid and model.
$grouped = [];
foreach ($params as $param) {
$key = "{$param->componentid}_{$param->model}";

// If key doesn't exist, add this param.
if (!isset($grouped[$key])) {
$grouped[$key] = $param;
continue;
}

// If param is from new context, it always wins.
if ($param->contextid == $newcontextid) {
$grouped[$key] = $param;
}
}

return array_values($grouped);
}
}
114 changes: 89 additions & 25 deletions classes/external/client_fetch_parameters.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
use external_value;
use external_multiple_structure;
use local_catquiz\catcontext;
use local_catquiz\catquiz;
use local_catquiz\catscale;
use local_catquiz\data\dataapi;
use local_catquiz\local\model\model_item_param;
Expand All @@ -41,6 +42,12 @@
*/
class client_fetch_parameters extends external_api {

/**
* Internal helper to check if an item exists already in the new context.
* @var array
*/
private static array $existingitems = [];

/**
* Returns description of method parameters.
* @return external_function_parameters
Expand Down Expand Up @@ -91,6 +98,8 @@ public static function execute(string $scaleid) {
$warnings = [];
$stored = 0;
$errors = 0;
$newparams = [];
$changedparams = [];

$scale = $DB->get_record('local_catquiz_catscales', ['id' => $params['scaleid']], '*', MUST_EXIST);
if (!$scale) {
Expand All @@ -99,6 +108,7 @@ public static function execute(string $scaleid) {
if (!$scale->label) {
throw new \moodle_exception('scalehasnolabel', 'local_catquiz', '', $params['scaleid']);
}
$currentcontext = catscale::get_context_id($scale->id);

// Create a mapping of catscale labels to IDs.
$scalemapping = [];
Expand Down Expand Up @@ -192,6 +202,10 @@ public static function execute(string $scaleid) {
false
);

$repo = new catquiz();
$haschanged = false;
$transaction = $DB->start_delegated_transaction();

// Store the received parameters.
foreach ($result->parameters as $param) {
$questionid = $hashmap[$param->questionhash] ?? null;
Expand All @@ -200,7 +214,6 @@ public static function execute(string $scaleid) {
'item' => $param->questionhash,
'warning' => get_string('noquestionhashmatch', 'local_catquiz'),
];
$errors++;
continue;
}

Expand All @@ -209,7 +222,6 @@ public static function execute(string $scaleid) {
'item' => $param->questionhash,
'warning' => 'Missing scale label',
];
$errors++;
continue;
}

Expand All @@ -222,34 +234,52 @@ public static function execute(string $scaleid) {
(object) ['remotelabel' => $param->scalelabel]
),
];
$errors++;
continue;
}

try {
// If there is no entry yet for the given scale and component: insert with new contextid.
// Otherwise: update with new contextid.
$existing = $DB->get_record(
'local_catquiz_items',
['componentid' => $questionid, 'catscaleid' => $scalemapping[$param->scalelabel]]
// If there is no entry yet for the given scale and component:
// insert with new contextid. Otherwise: update with new
// contextid.
$existing = $repo->get_item_with_params(
$questionid,
$param->model,
$currentcontext
);

if ($existing) {
$itemrecord = $existing;
$itemrecord->contextid = $newcontext->id;
$itemid = $existing->id;
$DB->update_record('local_catquiz_items', $itemrecord);
} else {
$itemrecord = new \stdClass();
$itemrecord->componentid = $questionid;
$itemrecord->componentname = 'question';
$itemrecord->catscaleid = $scalemapping[$param->scalelabel];
$itemrecord->contextid = $newcontext->id;
$itemrecord->status = LOCAL_CATQUIZ_TESTITEM_STATUS_ACTIVE;
$itemid = $DB->insert_record('local_catquiz_items', $itemrecord);
// TODO: How to set the active model?
if (!$existing) {
// Insert only once per questionid.
if (!self::item_exists($questionid)) {
$itemrecord = new \stdClass();
$itemrecord->componentid = $questionid;
$itemrecord->componentname = 'question';
$itemrecord->catscaleid = $scalemapping[$param->scalelabel];
$itemrecord->contextid = $newcontext->id;
$itemrecord->status = LOCAL_CATQUIZ_TESTITEM_STATUS_ACTIVE;
$itemid = $DB->insert_record(
'local_catquiz_items',
$itemrecord
);
self::$existingitems[$questionid] = true;
}
$newparams[] = ['item' => $itemrecord, 'param' => $param];
}

// If it did not change, we can skip it.
$changed = $param->difficulty != $existing->difficulty
|| $param->discrimination != $existing->discrimination
|| $param->json != $existing->json
|| $param->status != $existing->status;

if (!$changed) {
continue;
}
$changedparams[] = ['old' => $existing, 'new' => $param];

// If we are here, at least one parameter changed and we will
// later have to commit the transaction.
$haschanged = true;

// Create and save the item parameter.
$record = (object)[
'itemid' => $itemid,
Expand All @@ -276,17 +306,51 @@ public static function execute(string $scaleid) {
}
}

// Only if we synced at least one parameter, commit the transaction to
// create a new context and update the item parameters.
if ($haschanged) {
$DB->commit_delegated_transaction($transaction);
}

$duration = microtime(true) - $starttime;

return [
'status' => $stored > 0,
'status' => $errors == 0,
'message' => $stored > 0
? get_string('fetchsuccess', 'local_catquiz', (object) ['num' => $stored, 'contextname' => $newcontext->name])
: get_string('fetchempty', 'local_catquiz'),
? get_string(
'fetchsuccess',
'local_catquiz',
(object) ['num' => $stored, 'contextname' => $newcontext->name]
)
: get_string('fetchempty', 'local_catquiz'),
'duration' => round($duration, 2),
'synced' => $stored,
'errors' => $errors,
'warnings' => $warnings,
];
}

/**
* Internal helper to check if there is an item for that question
*
* @param int $questionid The questionid
*
* @return bool
*/
private static function item_exists(int $questionid) {
global $DB;
if (array_key_exists($questionid, self::$existingitems)) {
return true;
}
// Insert only once per questionid.
$existsforquestion = $DB->record_exists(
'local_catquiz_items',
[
'componentid' => $questionid,
]
);

self::$existingitems[$questionid] = $existsforquestion;
return $existsforquestion;
}
}

0 comments on commit 6c6b145

Please sign in to comment.