Skip to content

Conversation

@bwalkerl
Copy link
Collaborator

@bwalkerl bwalkerl commented Sep 16, 2025

Adds mapping for:

  • page content
  • forum_posts
  • grade_grades
  • grade_grades_history
  • qtype_ddmatch_subquestions
  • assignfeedback_comments
  • assignsubmission_onlinetext
  • question_answers
  • qtype_essay_options

And some minor refactors for clarity, and hardcode 2 tables that have different 'format' column names.

Note that grade_grades takes content from many other sources (assign etc), so even if you fix the data there the base64 data may re-appear if the original field isn't fixed and is updated again.

For the view links:

  • The actual content for grade_grades is not properly visible in an edit field to an admin (there is one place it appears, but it doesn't accept pluginfiles). It is usually modified in a plugin instead, which we can't link back to directly.
  • grade_grades_history is not editable in the UI

It would be great if we could get some unit tests to cover mapping, but not sure if it's possible. It might be possible using some editor logic?

@Fragonite
Copy link
Contributor

Hey @bwalkerl,

Looks good so far. I am still testing on my end.

I ran into a few task errors like missing course id and broken pipe. I believe they are unrelated to your changes but testing is taking a bit longer than I expected.

Cheers.

@Fragonite
Copy link
Contributor

Record 325143 successfully migrated to pluginfile: question questiontext 15709862
Record 325144 successfully migrated to pluginfile: question questiontext 16089969
Record 325145 successfully migrated to pluginfile: question generalfeedback 16090504
... used 145776 dbqueries
... used 340.85465478897 seconds
Adhoc task failed: tool_encoded\task\migrate,Can't find data record in database table course. (SELECT id,category FROM {course} WHERE id = ?
[array (
  0 => '1909929',
)])
Debug info:
SELECT id,category FROM {course} WHERE id = ?
[array (
  0 => '1909929',
)]
Backtrace:
* line 1638 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
* line 200 of /lib/classes/context/course.php: call to moodle_database->get_record()
* line 188 of /admin/tool/encoded/classes/task/migrate.php: call to core\context\course::instance()
* line 112 of /admin/tool/encoded/classes/task/migrate.php: call to tool_encoded\task\migrate->convert_to_pluginfile()
* line 70 of /admin/tool/encoded/classes/task/migrate.php: call to tool_encoded\task\migrate->migrate_record()
* line 517 of /lib/classes/cron.php: call to tool_encoded\task\migrate->execute()
* line 348 of /lib/classes/cron.php: call to core\cron::run_inner_adhoc_task()
* line 148 of /admin/cli/adhoc_task.php: call to core\cron::run_adhoc_task()

@bwalkerl
Copy link
Collaborator Author

@Fragonite That's likely related to an old course as you said and not this patch, but yes the strictness for those should definitely be changed and a check for false added before continuing.

@dmitriim
Copy link
Member

@bwalkerl would be nice to add some automated tests to support this change.

@bwalkerl
Copy link
Collaborator Author

bwalkerl commented Sep 29, 2025

@dmitriim 100% agree - automated testing would make this so much easier, but I'm struggling to find ways to make this work. Do you have any ideas?

The data that's inserted into the database ends up being @@PLUGINFILE@@/image.png and whether that displays properly is entirely dependent on editor setup logic. The internals here are hard to follow, especially with draft files, so I haven't had any luck finding something that could be used to validate this in a test.

@bwalkerl
Copy link
Collaborator Author

bwalkerl commented Oct 8, 2025

After thinking things through, while I don't know of a way to cover the mapping with tests, covering the methods used in the helper class will go a long way to ensuring integrity. I've added a base and will flesh this out more when I get a chance.

@Fragonite
Copy link
Contributor

Hi @bwalkerl,

Looks pretty good to me. Clears 99% of base64 records on a large database (I believe many of the remaining are truncated and cannot be converted).

Cheers.

@Fragonite Fragonite merged commit 17e6c59 into MOODLE_401_STABLE Oct 23, 2025
27 checks passed
@Fragonite Fragonite deleted the new_mapping branch October 23, 2025 04:17
This was referenced Oct 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants