dml exception for null questionid on restore

[almanesa]

I have used embed filter in lesson pages, when I tried to restore a deleted lesson activity with question attempts, got this exception: null value in column "questionid" violates not-null constraint below changes, solved this issue for me:

[timhunt]

Thanks for reporting this, and for working out a fix that solves the problem.

I am just trying to work out why that fix is correct - or if it is. At any rate it would be good to add a Unit test which reproduces the bug and verifies the fix.

[almanesa]

Thank you for this good plugin. The problem is when we restore a deleted (backup of) activity with embedded questions, the backup just includes the question attempts and not the questions, while restore_questions_attempt_data_trait assumes that there should be questions for restore in the backup. so call to "get_mapping('question' ..." would fail. I just prepared mapping with existing question id to prevent that.

[almanesa]

Actually, this fix (hack) may not work for all scenarios. It would be better if restore_questions_attempt_data_trait (from core) didn't assume that questions are always included in the same backup along with question attempts,

[timhunt]

Yes. that is the danger. I think the fix works in some cases, not others. (Hence my desire to add unit tests for a range of cases, to go with any fix.)

[timhunt]

OK, so we already have a test like https://github.com/moodleou/moodle-report_embedquestion/blob/main/tests/backup_test.php#L72, but that is duplicating the activity, which does not include user-data, so is different from what we are talking about here.

[almanesa]

I've added a test for this, you can check pull request #12 .

[timhunt]

Oh! we are working in parallel. I just did a test of a single-activity backup and restore, which also shows the bug. I will merge them.

I am currently looking at a fix which looks like

    public function process_question_attempt($data) {
        // We need to override the trait method. It we are restoring a
        // single-activity backup, then the questions won't be included
        // (because the questions always come from course context).
        // In this case, it is quite likely that the right thing to do
        // is to link to the same questionid.
        if (!$this->get_mapping('question', $data['questionid'])) {
            $this->set_mapping('question', $data['questionid'], $data['questionid']);
        }
        $this->trait_process_question_attempt($data);
    }

but it is not working. It is getting late here. I may finish this tomorrow.

[timhunt]

Actually, I think I am there. Just manually mergeing in your test. Will close your pull request. but don't worry.

[almanesa]

Thanks.