Skip to content

Conversation

vuvanhieu143
Copy link

@vuvanhieu143 vuvanhieu143 commented Sep 16, 2025

Hi @timhunt ,

Could you please help to review those code changes. I try to separate it into several commits :

  1. In the first commit
  • I change all the back-end logic to support for two optional params: Course shortname/ question bank and add legacy support for existing embed code.
  • Adds support for user preference of default question bank.
  1. Second commit:
  • Add a new webservice so we can dynamically loading question bank in embed form.
  • Update JS logic to handle multiple cases where we loading question bank, question category or question id dynamically.
  • Add switch question bank into the embed form. The reason we can't re-use the existing code from core because the core code is hardcode with quizcmid and our logic is different than core.
  1. third commit : is just update unit test for webservice and behat test for integration test.
  2. Just update CI and changes.md

After we merged this, the failed automation test in this PR will passed:
moodleou/moodle-tiny_embedquestion#8

@gergelyrakoczi
Copy link

@vuvanhieu143 Fantastic job! Thank you very much for your fixes. I have tested your developments at TU Wien test instances with Moodle 5.0 see [1] All works fine now.
@timhunt may I also ask you to check and approve these fixes. We use the plugin on production Moodle 5.0 heavily in many courses. We get quite a few requests from teacher to fix this asap. Would be great to have the fixes published for all universities already using Moodel 5.0. Thank you!

[1]
01-10-2025_15-24-36

@danmarsden
Copy link

just noting this PR seems to fail the following upstrem unit tests:

  • tool_dataprivacy\metadata_registry_test::test_get_registry_metadata_count
  • core_privacy\privacy\provider_test::test_metadata_provider
  • core_privacy\privacy\provider_test::test_table_coverage

@vuvanhieu143
Copy link
Author

Thanks for raising the issue, @danmarsden. I’ll fix it now. It’s strange that it also caused another failure in a core plugin.

@danmarsden
Copy link

cool thanks for doing that! - I was hoping to get one of our team to dig into it further but great you've been able to sort it - I've just run that through the unit tests again on our side and all passing and handed over to the team to test the interface and will drop any further feedback here in case it's useful.

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.

3 participants