Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Adds support for multichoiceset quiz questions. #228

Merged
merged 4 commits into from
Aug 22, 2018

Conversation

caperneoignis
Copy link
Contributor

added an additional case statement to handle multichoiceset qtype. Will need to add a better default handler for future qtypes.

Description

  • adds an additional handling of one more question type.

Related Issues

  • didn't make a related issue.

PR Type

  • Fix

added an additional case statement to handle multichoiceset qtype. Will need to add a better default handler for future qtypes.
@caperneoignis
Copy link
Contributor Author

@ryansmith94 I added an updated qtype handling. I will work on the default handling of future question types later. This is just a quick addition for a qtype I am using and is missing. It should be handled as the multichoice. Since if it is correct or not is handled by Moodle and will display properly if different.

@ryasmi ryasmi self-requested a review August 21, 2018 14:13
Copy link
Member

@ryasmi ryasmi left a comment

Choose a reason for hiding this comment

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

I think we should probably create a test for this in tests/mod_quiz/attempt_submitted in case someone changes the way this works in future and forgets to add tests. For now it can just be a duplicate of the tests/mod_quiz/attempt_submitted/multichoice folder but replace uses of "multichoice" with "multichoiceset".

@caperneoignis
Copy link
Contributor Author

Okay I'll update. Do you want me to include the additional changes I have done to the multichoice statement? Since I am adding the correct response and potential answers to it or put that in other branch?

@caperneoignis
Copy link
Contributor Author

nm I will use seperate branch

@ryasmi
Copy link
Member

ryasmi commented Aug 21, 2018

Okay can this be closed in favour of #233?

@ryasmi ryasmi added fix feat and removed fix labels Aug 22, 2018
@caperneoignis
Copy link
Contributor Author

@ryansmith94 This branch is just merging the question type, not the answers or the other items, you said you may not want. So this is so the question type all or nothing can be used in the handler. While the other branch is adding the other items that may or maynot be merged.

@ryasmi
Copy link
Member

ryasmi commented Aug 22, 2018

Got it, thanks for clarifying 👍

@ryasmi
Copy link
Member

ryasmi commented Aug 22, 2018

Thanks for adding the tests @caperneoignis, just waiting to make sure the build passes before I merge this because I had to merge master into this PR 👍

@ryasmi ryasmi changed the title adding additional question type feat: Adds support for multichoiceset quiz questions. Aug 22, 2018
@ryasmi ryasmi merged commit 2da2e11 into xAPI-vle:master Aug 22, 2018
@HT2Bot
Copy link
Member

HT2Bot commented Aug 22, 2018

🎉 This PR is included in version 3.15.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants