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.

@timhunt
Copy link
Member

timhunt commented Oct 13, 2025

I see that codechecker has changed some rules, which is now breaking the build.

I think we should probably fix that in a separate pull request (soon). Let us try to get this one merged without extra complications.

Copy link
Member

@timhunt timhunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a lot of excellent work here. Let me start by saying that, becuase there are also some details which don't see quite right yet.

The main problems to solve are with the form fields used. Remember, you cannot trust anything coming from the client-side, even if it was one of your forms, unless you validate it server-side. So, that needs fixing.

And, related to this, some of the internal APIs are unnecessarily tangled.

Apart from that, I think it is minor things.

Good tests, which hopefully help with fixing things safely.

And, thank you for cleaning up some of the bits of code that are no longer needed in modern Moodle versions. (But, also thank you for not cleaning up too many things in this change which is already very big.)

@vuvanhieu143 vuvanhieu143 force-pushed the wiptest branch 2 times, most recently from 8fdc119 to 2e8bd61 Compare October 15, 2025 08:37
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