Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-6340

force unique/unseen questions in quiz retakes

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.2, 2.8
    • Fix Version/s: 2.9
    • Component/s: Quiz
    • Database:
      Any
    • Testing Instructions:
      Hide

      This test deals with starting a quiz when there is randomisation. Both random selection of questions, and internal randomisation like with calculated questions.

      Random questions

      1. Create a category in the question bank containing 4 true-false questions. (Making the question texts A, B, C and D so you can easily tell them apart will make the rest of this easier.)
      2. Create a quiz that allows unlimited attempts.
      3. To that quiz add two random questions from the category you created, and make sure they are both on the same page.
      4. Preview the quiz. Click Start a new preview several times, and check you get a different set of two questions each time. (Well, it is random, so you may occasionally see the same as before.)
      5. Now attempt the quiz as a student.
      6. Note the questions you get in the first attempt (this should be a random selection).
      7. Submit that attempt, and start a second attempt. This time you should get the two question you did not see in the first attempt.
      8. Submit that attempt, and start a third attempt. This time you should get another random selection of two of the questions from all four.

      Internal randomisation

      1. Create a calculated question (e.g. {a} + {b}) https://docs.moodle.org/28/en/Calculated_question_type. Make all the datasets shared, and add 2 dataset items.
      2. Create a new quiz containing that calculated question.
      3. Attempt the quiz as a student.
      4. The values you get in the first attempt should be random.
      5. Finish that attempt and start a second one.
      6. This time, you should see the values you did not get the first time.

      Badly set up quiz.

      1. Create a quiz that tries to select 2 or more questions from the category that only contains the one question you created.
      2. Preview the quiz, and verify that you get an error.

      Shared datasets

      1. Create a second calculated (e.g. {a} - {b}) question in the same category as the first one. Use the same shared datasets as before.
      2. Create a new quiz containing both calculated questions on the same page.
      3. Attempt the quiz as a student.
      4. The values you get in both questions should be the same.
      5. Finish that attempt and start a second one.
      6. This time, you should see the set values you did not get the first time, but again the same values should be used in both questions.

      (Since this involves randomisation, ideally you should really do the all tests a few times, to verify that they are not passing just by chance the first time.)

      Show
      This test deals with starting a quiz when there is randomisation. Both random selection of questions, and internal randomisation like with calculated questions. Random questions Create a category in the question bank containing 4 true-false questions. (Making the question texts A, B, C and D so you can easily tell them apart will make the rest of this easier.) Create a quiz that allows unlimited attempts. To that quiz add two random questions from the category you created, and make sure they are both on the same page. Preview the quiz. Click Start a new preview several times, and check you get a different set of two questions each time. (Well, it is random, so you may occasionally see the same as before.) Now attempt the quiz as a student. Note the questions you get in the first attempt (this should be a random selection). Submit that attempt, and start a second attempt. This time you should get the two question you did not see in the first attempt. Submit that attempt, and start a third attempt. This time you should get another random selection of two of the questions from all four. Internal randomisation Create a calculated question (e.g. {a} + {b}) https://docs.moodle.org/28/en/Calculated_question_type . Make all the datasets shared, and add 2 dataset items. Create a new quiz containing that calculated question. Attempt the quiz as a student. The values you get in the first attempt should be random. Finish that attempt and start a second one. This time, you should see the values you did not get the first time. Badly set up quiz. Create a quiz that tries to select 2 or more questions from the category that only contains the one question you created. Preview the quiz, and verify that you get an error. Shared datasets Create a second calculated (e.g. {a} - {b}) question in the same category as the first one. Use the same shared datasets as before. Create a new quiz containing both calculated questions on the same page. Attempt the quiz as a student. The values you get in both questions should be the same. Finish that attempt and start a second one. This time, you should see the set values you did not get the first time, but again the same values should be used in both questions. (Since this involves randomisation, ideally you should really do the all tests a few times, to verify that they are not passing just by chance the first time.)
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_28_STABLE
    • Fixed Branches:
      MOODLE_29_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      When retaking a quiz, there is no way to force retakes to pull ONLY unseen questions from the pool, so that EVERY retake has completely unique/unseen questions in it. For example, I have a pool of 100 questions, and want to create a quiz with 25 questions in it. I want to give the student 4 chances at the quiz, and every time the student takes the quiz, he gets completely new questions, such that if he takes it 4 times, he will have seen all 100 questions. This would work well in conjunction with randomizing questions, which currently just pulls a random set for each retake, but allows for the same questions to randomly be pulled into each retake.

      This would be useful in the flashcard type of lesson too.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            oa_sychev Oleg Sychev added a comment -

            I think the correct way to implement this would be to add a new field to $cmoptions passed to create_session_and_responses like $cmoptions->excludedquestions

            Than a module (guided by it's logic and current settings) can deside which questions it would like to exclude from random question, while random question just follows it's hints.

            If finding suitable question is impossible, should it return an error or silently fallback on using excluded questions?

            Show
            oa_sychev Oleg Sychev added a comment - I think the correct way to implement this would be to add a new field to $cmoptions passed to create_session_and_responses like $cmoptions->excludedquestions Than a module (guided by it's logic and current settings) can deside which questions it would like to exclude from random question, while random question just follows it's hints. If finding suitable question is impossible, should it return an error or silently fallback on using excluded questions?
            Hide
            black Shkarupa Alex added a comment -

            This patch for moodle 1.9.
            I'll make patch for moodle 2.0 in around 2 days

            Show
            black Shkarupa Alex added a comment - This patch for moodle 1.9. I'll make patch for moodle 2.0 in around 2 days
            Hide
            timhunt Tim Hunt added a comment - - edited

            That looks pretty good. I have a bunch of minor complaints, and one serious one, number 6.

            > $oldattempts = array_keys(get_records_select( ...

            1. can we avoid doing this potentially expensive database query unless it is really necessary?

            2. The logic is wrong. The student may have attempted other quizzes, we are only interested in their attempts at this quiz, aren't we?

            3. The logic is wrong in another way. You need the quiz_attempt.uniqueid, not quiz_attempt.id in $oldattempts.

            > function get_usable_questions_from_category(...)

            4. I don't really like passing the whole of $cmoptions in here. I would rather leave $questionsinuse, and add an extra parameter, perhaps $avoidquestionsfromattempts

            5. Please use preg, not ereg.

            6. You are possibly using two or three database queries where previously there was one. This is bad for performance. Hmm. I see, it is more complex than that because of the $this->catrandoms caching. I guess the code now does the right thing, it is just not very clear.

            7. Good idea that get_usable_questions_from_category returns just the question ids, rather than objects with just an ->id field.

            So, what to do about 6?

            It would be nice to do everything in one database query. The following is my idea. I have not tested it, so it probably does not work. It also needs to be tested for performance, but the idea is worth pursuing.

            SELECT q.id, COUNT(qst.id) AS numprevioususes
             
            FROM mdl_question q
            LEFT JOIN mdl_question_states qst ON qst.answer LIKE CONCAT('random', q.id, '-%')
             
            WHERE q.category IN ($categorylist)
            AND q.parent = 0
            AND q.hidden = 0
            AND q.id NOT IN ($questionsinuse)
            AND qtype NOT IN ($this->excludedqtypes)
            AND qst.attempt IN ($avoidquestionsfromattempts)
            AND qst.seq_number = 0
             
            GROUP BY q.id
             
            ORDER BY numprevioususes ASC

            Do that query with get_records_sql_menu. That will give you an array of questionid => number of times that question has been used in previous attempts, in order of number of attempts.

            That way, you can use the first part of the array to get questions that have never been used before, then use the next bit to get questions that have been used once before, and then questions that have been used twice before, and so on.

            I hope all that makes sense. Thank you for working on this.

            Show
            timhunt Tim Hunt added a comment - - edited That looks pretty good. I have a bunch of minor complaints, and one serious one, number 6. > $oldattempts = array_keys(get_records_select( ... 1. can we avoid doing this potentially expensive database query unless it is really necessary? 2. The logic is wrong. The student may have attempted other quizzes, we are only interested in their attempts at this quiz, aren't we? 3. The logic is wrong in another way. You need the quiz_attempt.uniqueid, not quiz_attempt.id in $oldattempts. > function get_usable_questions_from_category(...) 4. I don't really like passing the whole of $cmoptions in here. I would rather leave $questionsinuse, and add an extra parameter, perhaps $avoidquestionsfromattempts 5. Please use preg, not ereg. 6. You are possibly using two or three database queries where previously there was one. This is bad for performance. Hmm. I see, it is more complex than that because of the $this->catrandoms caching. I guess the code now does the right thing, it is just not very clear. 7. Good idea that get_usable_questions_from_category returns just the question ids, rather than objects with just an ->id field. So, what to do about 6? It would be nice to do everything in one database query. The following is my idea. I have not tested it, so it probably does not work. It also needs to be tested for performance, but the idea is worth pursuing. SELECT q.id, COUNT (qst.id) AS numprevioususes   FROM mdl_question q LEFT JOIN mdl_question_states qst ON qst.answer LIKE CONCAT( 'random' , q.id, '-%' )   WHERE q.category IN ($categorylist) AND q.parent = 0 AND q.hidden = 0 AND q.id NOT IN ($questionsinuse) AND qtype NOT IN ($this->excludedqtypes) AND qst.attempt IN ($avoidquestionsfromattempts) AND qst.seq_number = 0   GROUP BY q.id   ORDER BY numprevioususes ASC Do that query with get_records_sql_menu. That will give you an array of questionid => number of times that question has been used in previous attempts, in order of number of attempts. That way, you can use the first part of the array to get questions that have never been used before, then use the next bit to get questions that have been used once before, and then questions that have been used twice before, and so on. I hope all that makes sense. Thank you for working on this.
            Hide
            black Shkarupa Alex added a comment -

            So, this is the next version of my patch.
            To Tim: i modified your SQL query a bit, because it caused an error when there were no attempts

            Show
            black Shkarupa Alex added a comment - So, this is the next version of my patch. To Tim: i modified your SQL query a bit, because it caused an error when there were no attempts
            Hide
            oa_sychev Oleg Sychev added a comment -

            Alex, some questions about you patch:
            1. Are you sure about using isset($avoidquestionsfromattempts) ? This is a function argument and it will probably always be set. Did you test it?
            2. You are probably forget to shuffle questions in case of no previous attempts exists.

            Tim, could you test new SQL query please? It works fine in MySQL. The changes where caused by NULLs returning by join in case no matching row exists, which caused MySQL to not return questions with 0 uses. So case with 0 uses require more attentions in testing.

            Also, would you like to have to returns in get_usable_questions.... (like in the last patch), or prefer setting $catrandoms and one return in the end?

            Show
            oa_sychev Oleg Sychev added a comment - Alex, some questions about you patch: 1. Are you sure about using isset($avoidquestionsfromattempts) ? This is a function argument and it will probably always be set. Did you test it? 2. You are probably forget to shuffle questions in case of no previous attempts exists. Tim, could you test new SQL query please? It works fine in MySQL. The changes where caused by NULLs returning by join in case no matching row exists, which caused MySQL to not return questions with 0 uses. So case with 0 uses require more attentions in testing. Also, would you like to have to returns in get_usable_questions.... (like in the last patch), or prefer setting $catrandoms and one return in the end?
            Hide
            black Shkarupa Alex added a comment -

            1) Yes, i'm sure. It works properly.
            2) You're right, i forgot. but now i corrected that

            Show
            black Shkarupa Alex added a comment - 1) Yes, i'm sure. It works properly. 2) You're right, i forgot. but now i corrected that
            Hide
            oa_sychev Oleg Sychev added a comment -

            Patch for Moodle 2.0 uploaded. Credits for patch upgrading goes to Sergei Bastrykin.

            Let's see how many more Moodle versions will go without that functionality...

            Show
            oa_sychev Oleg Sychev added a comment - Patch for Moodle 2.0 uploaded. Credits for patch upgrading goes to Sergei Bastrykin. Let's see how many more Moodle versions will go without that functionality...
            Hide
            oa_sychev Oleg Sychev added a comment -

            Well, does question_variant_pseudorandom_no_repeats_strategy in Moodle 2.1 fixes this?

            Show
            oa_sychev Oleg Sychev added a comment - Well, does question_variant_pseudorandom_no_repeats_strategy in Moodle 2.1 fixes this?
            Hide
            timhunt Tim Hunt added a comment -

            No, I don't think it does. It solves a different problem (synchronised datasets and similar functionality in our new 'varnumeric' question types.

            However, when I was doing the changes in Moodle 2.1, I did consider this issue, even though I did not solve it. This was one of the reasons I tried to un-tangle the way the random question type works.

            I think that it is now possible to fix this issue while only changing mod/quiz/startattempt.php (around lines 163-170).

            (Well, I don't know if we need to introduce an option for this, or whether we just always do it. If we need to introduce an option, then you would have to change other files, but actually, I see not need to make this optional. I think that we should always avoid repeats with previous attempts, if possible. If that is not possible, we should fall back to only avoiding repeats with the current attempt.)

            Show
            timhunt Tim Hunt added a comment - No, I don't think it does. It solves a different problem (synchronised datasets and similar functionality in our new 'varnumeric' question types. However, when I was doing the changes in Moodle 2.1, I did consider this issue, even though I did not solve it. This was one of the reasons I tried to un-tangle the way the random question type works. I think that it is now possible to fix this issue while only changing mod/quiz/startattempt.php (around lines 163-170). (Well, I don't know if we need to introduce an option for this, or whether we just always do it. If we need to introduce an option, then you would have to change other files, but actually, I see not need to make this optional. I think that we should always avoid repeats with previous attempts, if possible. If that is not possible, we should fall back to only avoiding repeats with the current attempt.)
            Hide
            timhunt Tim Hunt added a comment -

            Just removing the patch label. None of the patches here are for recent versions of Moodle.

            Show
            timhunt Tim Hunt added a comment - Just removing the patch label. None of the patches here are for recent versions of Moodle.
            Hide
            oa_sychev Oleg Sychev added a comment -

            And what chances there are if even they would be updated for more recent versions of Moodle? Judging by extra_question_fields code not much. I'm not going to update it many times (it's already was done from 1.9 to 2.0 with this code, and to no avail), so please note times when you really could look at results.

            Show
            oa_sychev Oleg Sychev added a comment - And what chances there are if even they would be updated for more recent versions of Moodle? Judging by extra_question_fields code not much. I'm not going to update it many times (it's already was done from 1.9 to 2.0 with this code, and to no avail), so please note times when you really could look at results.
            Hide
            timhunt Tim Hunt added a comment -

            This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

            For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

            Show
            timhunt Tim Hunt added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
            Hide
            beda1304 Darya Beda added a comment - - edited

            SQL query for moodle 2.8

            SELECT q.id , COUNT(qa.questionid) AS numprevioususes

            FROM {question} q
            LEFT JOIN {question_attempts} qa
            ON qa.id IN (SELECT qas.questionattemptid FROM {question_attempt_steps} qas
            WHERE qas.state = 'todo'
            AND qas.userid = {$userid} )
            AND q.id = qa.questionid

            WHERE q.category IN ({$categoryids})
            AND q.parent = 0
            AND q.hidden = 0
            AND qtype NOT IN ({$this->excludedqtypes})

            GROUP BY q.id
            ORDER BY numprevioususes ASC

            Show
            beda1304 Darya Beda added a comment - - edited SQL query for moodle 2.8 SELECT q.id , COUNT(qa.questionid) AS numprevioususes FROM {question} q LEFT JOIN {question_attempts} qa ON qa.id IN (SELECT qas.questionattemptid FROM {question_attempt_steps} qas WHERE qas.state = 'todo' AND qas.userid = {$userid} ) AND q.id = qa.questionid WHERE q.category IN ({$categoryids}) AND q.parent = 0 AND q.hidden = 0 AND qtype NOT IN ({$this->excludedqtypes}) GROUP BY q.id ORDER BY numprevioususes ASC
            Hide
            beda1304 Darya Beda added a comment - - edited

            Test 1.
            1. Create a one user
            2. Create new course
            3. Add 5 questions in the question bank with an indication in the text of the question numbers
            4. Create new quiz
            5. Add in quiz 2 random questions and 3 questions(1, 3, 5) from the question bank
            6. Make the quiz by created user
            7. In the quiz were the following questions: 1,2,3,4,5

            Test 2
            1. Create a one user
            2. Create new course
            3. Add 5 questions in the question bank with an indication in the text of the question numbers
            4. Create new quiz
            5. Add in quiz 5 random questions
            6. Make the quiz by created user
            7. In the quiz were the following questions: 1,2,3,4,5

            Test 3.
            1. Create a one user
            2. Create new course
            3. Add 5 questions in the question bank with an indication in the text of the question numbers
            4. Create new quiz
            5. Add in quiz 5 questions(1, 2, 3, 4, 5) from the question bank
            6. Make the quiz by created user
            7. In the quiz were the following questions: 1,2,3,4,5

            Test 4.
            1. Create a one user
            2. Create new course
            3. Add 10 questions in the question bank
            4. Create new quiz
            5. Add in quiz 5 random questions
            6. Make the quiz by one user twice
            7. Questions are not repeated in attempts

            Test 5.
            1. Create two users
            2. Create new course
            3. Add 10 questions in the question bank
            4. Create new quiz
            5. Add in quiz 5 random questions
            6. Make first attempt of the quiz by the first user
            7. Make first attempt of the quiz by the second user
            8. Questions from first attempt of the first user may be repeated with the questions from first attempt of the second user
            9. Make second attempt of the quiz again by the first user
            10. Make second attempt of the quiz again by the second user
            11. Questions from second attempt of the first user may be repeated with the questions from second attempt of the second user
            12. Questions are not repeated in the attempts of the first user
            13. Questions are not repeated in the attempts of the second user

            Show
            beda1304 Darya Beda added a comment - - edited Test 1. 1. Create a one user 2. Create new course 3. Add 5 questions in the question bank with an indication in the text of the question numbers 4. Create new quiz 5. Add in quiz 2 random questions and 3 questions(1, 3, 5) from the question bank 6. Make the quiz by created user 7. In the quiz were the following questions: 1,2,3,4,5 Test 2 1. Create a one user 2. Create new course 3. Add 5 questions in the question bank with an indication in the text of the question numbers 4. Create new quiz 5. Add in quiz 5 random questions 6. Make the quiz by created user 7. In the quiz were the following questions: 1,2,3,4,5 Test 3. 1. Create a one user 2. Create new course 3. Add 5 questions in the question bank with an indication in the text of the question numbers 4. Create new quiz 5. Add in quiz 5 questions(1, 2, 3, 4, 5) from the question bank 6. Make the quiz by created user 7. In the quiz were the following questions: 1,2,3,4,5 Test 4. 1. Create a one user 2. Create new course 3. Add 10 questions in the question bank 4. Create new quiz 5. Add in quiz 5 random questions 6. Make the quiz by one user twice 7. Questions are not repeated in attempts Test 5. 1. Create two users 2. Create new course 3. Add 10 questions in the question bank 4. Create new quiz 5. Add in quiz 5 random questions 6. Make first attempt of the quiz by the first user 7. Make first attempt of the quiz by the second user 8. Questions from first attempt of the first user may be repeated with the questions from first attempt of the second user 9. Make second attempt of the quiz again by the first user 10. Make second attempt of the quiz again by the second user 11. Questions from second attempt of the first user may be repeated with the questions from second attempt of the second user 12. Questions are not repeated in the attempts of the first user 13. Questions are not repeated in the attempts of the second user
            Show
            beda1304 Darya Beda added a comment - https://github.com/DaryaBeda/moodle/tree/MDL-6340
            Hide
            oa_sychev Oleg Sychev added a comment -

            I did not seems to be able to assign this to the right person, Darya Beda as she is first time developer and not in JIRA developer group

            Show
            oa_sychev Oleg Sychev added a comment - I did not seems to be able to assign this to the right person, Darya Beda as she is first time developer and not in JIRA developer group
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-6340 using repository: https://github.com/DaryaBeda/moodle

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-6340 using repository: https://github.com/DaryaBeda/moodle master [branch: https://github.com/DaryaBeda/moodle/tree/MDL-6340 | CI Job ] Info: the branch is based off moodle.git Info: base commit eb1dc9fab95451c758bde9a66d9fe0810653fc3b being used as initial commit. Error: The https://github.com/DaryaBeda/moodle/tree/MDL-6340 branch at https://github.com/DaryaBeda/moodle is very old (>60 days ago). Please rebase against current master. More information about this report
            Hide
            timhunt Tim Hunt added a comment -

            Thank you very much for working on this.

            While you fix may work well enough that it is usable in your local context, it will not work in all circumstances, and so is not yet good enough to go into the standard Moodle package.

            1. The SQL query does not return the correct results, for example:

            • Suppose there are two quizzes in a course. If a student gets question 123 in Quiz 1, then (after this change) they can never get the same question in Quiz 2. That is not the expected behaviour.
            • Suppose a user is a teach in Course A, and they manually grade an attempt at shared question 1234. Then they will never get question 1234 if it is used in a quiz in another course where they are a student.

            2. Have you done any performance measurements on that query with a large database? The tables being joined can be very, very big (and the way the JOIN has been written is in a form that is known to perform badly in MySQL Avoid SQL like 'IN (... subquery ...)' whenever possible.

            I think we need to assume this will perform badly, unless we see evidence that it is OK.

            3. The code is not idiomatic PHP. For example the way arrays are manipulated is very strange. Have a look at some other code that processes the results of database queries.

            4. Relying on global $USER in this code is not safe. (Imagine a script that pre-creates a quiz attempt for each user before an exam, to improve performance when students log in to start their attempts.)

            Show
            timhunt Tim Hunt added a comment - Thank you very much for working on this. While you fix may work well enough that it is usable in your local context, it will not work in all circumstances, and so is not yet good enough to go into the standard Moodle package. 1. The SQL query does not return the correct results, for example: Suppose there are two quizzes in a course. If a student gets question 123 in Quiz 1, then (after this change) they can never get the same question in Quiz 2. That is not the expected behaviour. Suppose a user is a teach in Course A, and they manually grade an attempt at shared question 1234. Then they will never get question 1234 if it is used in a quiz in another course where they are a student. 2. Have you done any performance measurements on that query with a large database? The tables being joined can be very, very big (and the way the JOIN has been written is in a form that is known to perform badly in MySQL Avoid SQL like 'IN (... subquery ...)' whenever possible. I think we need to assume this will perform badly, unless we see evidence that it is OK. 3. The code is not idiomatic PHP. For example the way arrays are manipulated is very strange. Have a look at some other code that processes the results of database queries. 4. Relying on global $USER in this code is not safe. (Imagine a script that pre-creates a quiz attempt for each user before an exam, to improve performance when students log in to start their attempts.)
            Hide
            oa_sychev Oleg Sychev added a comment -

            Tim, thank you for speedy review, but could you please elaborate on problem 2? It was you who originally supposed using complex SQL with 'IN (... subquery ...)' there first - https://tracker.moodle.org/browse/MDL-6340?focusedCommentId=76596&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-76596

            Did you now think you proposition was wrong and should be achieved by another way? Or you supposed adding an option to the random question whether to use this code, so the people who will have performance problems can turn it off? Or in what way this problem can be avoided?

            Show
            oa_sychev Oleg Sychev added a comment - Tim, thank you for speedy review, but could you please elaborate on problem 2? It was you who originally supposed using complex SQL with 'IN (... subquery ...)' there first - https://tracker.moodle.org/browse/MDL-6340?focusedCommentId=76596&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-76596 Did you now think you proposition was wrong and should be achieved by another way? Or you supposed adding an option to the random question whether to use this code, so the people who will have performance problems can turn it off? Or in what way this problem can be avoided?
            Hide
            timhunt Tim Hunt added a comment -

            My SQL in the comment you refer to does not use 'IN (... subquery ...)'. It just uses a few 'IN (... fixed list ...)' clauses. The latest code is not equivalent to what I proposed before.

            Also, solving the performance problem 2 is irrelevant until 1. is fixed, and the code works.

            Show
            timhunt Tim Hunt added a comment - My SQL in the comment you refer to does not use 'IN (... subquery ...)'. It just uses a few 'IN (... fixed list ...)' clauses. The latest code is not equivalent to what I proposed before. Also, solving the performance problem 2 is irrelevant until 1. is fixed, and the code works.
            Hide
            oa_sychev Oleg Sychev added a comment -

            Tim, we fixed problems you detected and several more, now that should be a solid fix.

            Show
            oa_sychev Oleg Sychev added a comment - Tim, we fixed problems you detected and several more, now that should be a solid fix.
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks with warnings.

            Checked MDL-6340 using repository: https://github.com/DaryaBeda/moodle

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks with warnings. Checked MDL-6340 using repository: https://github.com/DaryaBeda/moodle master (0 errors / 1 warnings) [branch: MDL-6340 | CI Job ] phplint (0/0) , php (0/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/1) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-6340 using repository: https://github.com/DaryaBeda/moodle

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-6340 using repository: https://github.com/DaryaBeda/moodle master (0 errors / 0 warnings) [branch: MDL-6340 | CI Job ] More information about this report
            Hide
            timhunt Tim Hunt added a comment - - edited

            I'm sorry it took me so long to look at this.

            This is looking much better. The basic idea of having an array where each question has a numprevioususes field, and to sort by that but otherwise shuffle, is good.

            Unfortunately there are still problems:

            1. The caching in $this->availablequestionsbycategory does not take note of the $questionsquiz list that was passed. So, if you call the function twice with different lists, you will be given the wrong results.

            2. The shuffling does work properly. usort is not a stable sort (http://php.net/manual/en/function.usort.php "If two members compare as equal, their relative order in the sorted array is undefined.") so shuffle then usort does not give the result one hopes for.

            3. The code violates an important design principle. There should be no mention of 'quiz' anywhere in 'question' code. In fact you are only passing a list of qaids. It is just the variable name that is wrong.

            4. The list of qaids is potentially very long. Passing around long lists of ints is not good design. We already have a better abstraction for hadling this, the qubaid_condition class. If you use qubaid_list, then the list of question-usage ids is going to be an order of magnitude smaller then the list of qaids. Better still would be to use the qubaid_join class.

            5. Is it really necessary to have two completely separate code-paths for the two cases? In the trivial case when there are no usages, just set numprevioususes to 0 for all questions and use the same code.

            6. The code should continue to use
            $questionids = question_bank::get_finder()->get_questions_from_categories in all cases, rather than duplicating the code from within that function to moodify it. Probably best to add a new method question_bank::get_finder()->get_questions_from_categories_with_usage_counts.

            7. Once we get a working implementation with good design, you need to format the SQL the way Moodle likes. See https://docs.moodle.org/dev/SQL_coding_style#Indentation

            I'm sorry, but this needs more work.

            Show
            timhunt Tim Hunt added a comment - - edited I'm sorry it took me so long to look at this. This is looking much better. The basic idea of having an array where each question has a numprevioususes field, and to sort by that but otherwise shuffle, is good. Unfortunately there are still problems: 1. The caching in $this->availablequestionsbycategory does not take note of the $questionsquiz list that was passed. So, if you call the function twice with different lists, you will be given the wrong results. 2. The shuffling does work properly. usort is not a stable sort ( http://php.net/manual/en/function.usort.php "If two members compare as equal, their relative order in the sorted array is undefined.") so shuffle then usort does not give the result one hopes for. 3. The code violates an important design principle. There should be no mention of 'quiz' anywhere in 'question' code. In fact you are only passing a list of qaids. It is just the variable name that is wrong. 4. The list of qaids is potentially very long. Passing around long lists of ints is not good design. We already have a better abstraction for hadling this, the qubaid_condition class. If you use qubaid_list, then the list of question-usage ids is going to be an order of magnitude smaller then the list of qaids. Better still would be to use the qubaid_join class. 5. Is it really necessary to have two completely separate code-paths for the two cases? In the trivial case when there are no usages, just set numprevioususes to 0 for all questions and use the same code. 6. The code should continue to use $questionids = question_bank::get_finder()->get_questions_from_categories in all cases, rather than duplicating the code from within that function to moodify it. Probably best to add a new method question_bank::get_finder()->get_questions_from_categories_with_usage_counts. 7. Once we get a working implementation with good design, you need to format the SQL the way Moodle likes. See https://docs.moodle.org/dev/SQL_coding_style#Indentation I'm sorry, but this needs more work.
            Hide
            timhunt Tim Hunt added a comment -

            Would anyone object if I took the latest patch, which is close but not quite there, and finished it off and got it ready for integration?

            I could use the finished version of this in my work on MDL-40992

            Show
            timhunt Tim Hunt added a comment - Would anyone object if I took the latest patch, which is close but not quite there, and finished it off and got it ready for integration? I could use the finished version of this in my work on MDL-40992
            Hide
            oa_sychev Oleg Sychev added a comment -

            Since Darya doesn't have time to fix porblems you found and this is important issue I would definitely not object.

            It may be somewhat hard to give all respects in commit message thought. First version of this code was written by me and Alex Shkarupa implementing Tim's idea back in 2009. Sergei Bastrykin updated patch in 2011 for Moodle 2.0. Latest upgrade was done by Darya Beda under my supervision.

            Show
            oa_sychev Oleg Sychev added a comment - Since Darya doesn't have time to fix porblems you found and this is important issue I would definitely not object. It may be somewhat hard to give all respects in commit message thought. First version of this code was written by me and Alex Shkarupa implementing Tim's idea back in 2009. Sergei Bastrykin updated patch in 2011 for Moodle 2.0. Latest upgrade was done by Darya Beda under my supervision.
            Hide
            timhunt Tim Hunt added a comment -

            Thanks Oleg. I am glad you said that, becuase I have been working on this for a lot of today

            What I have written is https://github.com/timhunt/moodle/compare/master...MDL-6340. Note, this is completely untested so far! I will make it actually work tomorrow. Also, I need to add unit tests in several places. However, I feel quite happy with the overall shape of the solution.

            Note, it is an intentional move to ignore the existing qtype_random code. I have a long-term goal of completely removing qtype_random from the code.

            In the end it turned out to be a complete re-implementation from scratch. However, I could not possibly have worked out this solution without having seen all the previout patches. I hope the way I have given credit in the commit comment is appropriate. (However, it is late here, so who knows what I just wrote. I will review it tomorrow, and if you have any suggestions, please let me know.)

            Show
            timhunt Tim Hunt added a comment - Thanks Oleg. I am glad you said that, becuase I have been working on this for a lot of today What I have written is https://github.com/timhunt/moodle/compare/master...MDL-6340 . Note, this is completely untested so far ! I will make it actually work tomorrow. Also, I need to add unit tests in several places. However, I feel quite happy with the overall shape of the solution. Note, it is an intentional move to ignore the existing qtype_random code. I have a long-term goal of completely removing qtype_random from the code. In the end it turned out to be a complete re-implementation from scratch. However, I could not possibly have worked out this solution without having seen all the previout patches. I hope the way I have given credit in the commit comment is appropriate. (However, it is late here, so who knows what I just wrote. I will review it tomorrow, and if you have any suggestions, please let me know.)
            Hide
            timhunt Tim Hunt added a comment -

            I just found MDL-45927, which was my issue for eliminating qtype_random, that this will help towards.

            Show
            timhunt Tim Hunt added a comment - I just found MDL-45927 , which was my issue for eliminating qtype_random, that this will help towards.
            Hide
            timhunt Tim Hunt added a comment -

            This is now ready for peer review.

            Show
            timhunt Tim Hunt added a comment - This is now ready for peer review.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git master (17 errors / 7 warnings) [branch: MDL-6340 | CI Job ] phplint (0/0) , php (13/7) , js (0/0) , css (0/0) , phpdoc (4/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git master (17 errors / 7 warnings) [branch: MDL-6340 | CI Job ] phplint (0/0) , php (13/7) , js (0/0) , css (0/0) , phpdoc (4/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git master (13 errors / 6 warnings) [branch: MDL-6340 | CI Job ] phplint (0/0) , php (13/6) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            timhunt Tim Hunt added a comment -

            Note, the long lines are all in a big table of data in a unit test. I am not going to line-wrap that.

            Show
            timhunt Tim Hunt added a comment - Note, the long lines are all in a big table of data in a unit test. I am not going to line-wrap that.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git master (16 errors / 6 warnings) [branch: MDL-6340 | CI Job ] phplint (0/0) , php (13/6) , js (0/0) , css (0/0) , phpdoc (3/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            mkassaei Mahmoud Kassaei added a comment -

            I just had a quick look at the chnages on https://github.com/timhunt/moodle/compare/master...MDL-6340 without testing it or running the unittests:

            Here are some comments:

            1. In qubaids_for_users_attempts contrcotor,the local variable $statuscondition is set but nit used.
            1. In least_used_strategy class line 93 the expression if ($maxvariants > 1000) { is used, I was wondering why 1000. This can be set as constant in the class or an admin setting.
            Show
            mkassaei Mahmoud Kassaei added a comment - I just had a quick look at the chnages on https://github.com/timhunt/moodle/compare/master...MDL-6340 without testing it or running the unittests: Here are some comments: In qubaids_for_users_attempts contrcotor,the local variable $statuscondition is set but nit used. In least_used_strategy class line 93 the expression if ($maxvariants > 1000) { is used, I was wondering why 1000. This can be set as constant in the class or an admin setting.
            Hide
            timhunt Tim Hunt added a comment -

            Thanks Mahmoud.

            The first of those was a real bug (even though that case is not yet used.)

            The second one was definitely not good code. It would have broken if anyone ever made 1000 quiz attempts. Having thought again, I found a better way to write the test, without a magic number.

            Both isses fixed, along with the automated check failures, as separate commits for now. I will rebase before submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Thanks Mahmoud. The first of those was a real bug (even though that case is not yet used.) The second one was definitely not good code. It would have broken if anyone ever made 1000 quiz attempts. Having thought again, I found a better way to write the test, without a magic number. Both isses fixed, along with the automated check failures, as separate commits for now. I will rebase before submitting for integration.
            Hide
            mkassaei Mahmoud Kassaei added a comment -

            That was a very quick fix. It looks fine now.

            1. The where-clause has been extenden and the unsued variable is gone
            2. This looks better than using an arbitrarry number.
            Show
            mkassaei Mahmoud Kassaei added a comment - That was a very quick fix. It looks fine now. The where-clause has been extenden and the unsued variable is gone This looks better than using an arbitrarry number.
            Hide
            timhunt Tim Hunt added a comment -

            Great. Thanks Mahmound. I have rebased, and am submitting for integration.

            Show
            timhunt Tim Hunt added a comment - Great. Thanks Mahmound. I have rebased, and am submitting for integration.
            Hide
            timhunt Tim Hunt added a comment -
            Show
            timhunt Tim Hunt added a comment - Forum discussion here: https://moodle.org/mod/forum/discuss.php?d=310241
            Hide
            itamart Itamar Tzadok added a comment -

            Insofar as this feature replaces current behavior rather than added as an option (default or otherwise) I strongly object. Reasons in the forum thread: https://moodle.org/mod/forum/discuss.php?d=310241.

            Show
            itamart Itamar Tzadok added a comment - Insofar as this feature replaces current behavior rather than added as an option (default or otherwise) I strongly object. Reasons in the forum thread: https://moodle.org/mod/forum/discuss.php?d=310241 .
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

            TIA and ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
            Hide
            timhunt Tim Hunt added a comment -

            Rebased.

            Show
            timhunt Tim Hunt added a comment - Rebased.
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git master (14 errors / 6 warnings) [branch: MDL-6340 | CI Job ] phplint (0/0) , php (13/6) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-6340 using repository: git://github.com/timhunt/moodle.git master (14 errors / 6 warnings) [branch: MDL-6340 | CI Job ] phplint (0/0) , php (13/6) , js (0/0) , css (0/0) , phpdoc (1/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            dmonllao David Monllaó added a comment -

            Thanks for working on this Tim. The code looks great, but I am concerned about many people against this change https://moodle.org/mod/forum/discuss.php?d=310241 and their valid reasons to justify the current behaviour as valid in some cases.

            I'm by default against new settings, IMO, most of the time, we end up adding new settings when we don't know what to do, just guessing what users want; in this case we know that there is a group of users that don't want this change and it is not compatible with what they are doing. Looking at the list of setting we have in mod_quiz and the questions engine, there is a big bunch of them that I have problems to understand what they are for, that is probably a personal issue I have though but I would understand a new option (when adding new random questions) to specify if I want the questions to be 100% random or prioritize the ones that the user has not seen.

            I'd like to have other other opinions than mine before deciding, your opinion here is very important as component lead; if you are completely opposed to a new setting IMO we need to let teachers know how they can continue using quiz as they have been using it as; for what it seems to me, seems quite reasonable that using an option named "add a random question" leads to random questions.

            Show
            dmonllao David Monllaó added a comment - Thanks for working on this Tim. The code looks great, but I am concerned about many people against this change https://moodle.org/mod/forum/discuss.php?d=310241 and their valid reasons to justify the current behaviour as valid in some cases. I'm by default against new settings, IMO, most of the time, we end up adding new settings when we don't know what to do, just guessing what users want; in this case we know that there is a group of users that don't want this change and it is not compatible with what they are doing. Looking at the list of setting we have in mod_quiz and the questions engine, there is a big bunch of them that I have problems to understand what they are for, that is probably a personal issue I have though but I would understand a new option (when adding new random questions) to specify if I want the questions to be 100% random or prioritize the ones that the user has not seen. I'd like to have other other opinions than mine before deciding, your opinion here is very important as component lead; if you are completely opposed to a new setting IMO we need to let teachers know how they can continue using quiz as they have been using it as; for what it seems to me, seems quite reasonable that using an option named "add a random question" leads to random questions.
            Hide
            jmvedrine Jean-Michel Vedrine added a comment - - edited

            Hello Davd,
            I will let Tim answer and I don't want to interfere with what will be decided for this issue as I have already (loudly but other have voiced a different opinion even more loudly ) voiced my opinion in the form thread.
            But there is something as a math teacher (and teaching statistics too !) that I absolutely need to answer : random with no replacement (the mode introduced by this change) IS ABSOLUTELY random and not "less random" (or "not 100% random") than random with replacement.
            And I can even bet with you that if you conduct a poll to ask people what they mean when you say "I draw 10 questions in a bank of 100 questions" or "I draw 10 balls in an urn containing 100 balls" that a neat majority will answer they mean draw without replacement

            Show
            jmvedrine Jean-Michel Vedrine added a comment - - edited Hello Davd, I will let Tim answer and I don't want to interfere with what will be decided for this issue as I have already (loudly but other have voiced a different opinion even more loudly ) voiced my opinion in the form thread. But there is something as a math teacher (and teaching statistics too !) that I absolutely need to answer : random with no replacement (the mode introduced by this change) IS ABSOLUTELY random and not "less random" (or "not 100% random") than random with replacement. And I can even bet with you that if you conduct a poll to ask people what they mean when you say "I draw 10 questions in a bank of 100 questions" or "I draw 10 balls in an urn containing 100 balls" that a neat majority will answer they mean draw without replacement
            Hide
            jmvedrine Jean-Michel Vedrine added a comment -

            Also I think we should ask all people wanting the introduction of a new setting for this to provide a clear UI explaining it to usual (not just math ) teachers. I will be delighted to read their answers.

            Show
            jmvedrine Jean-Michel Vedrine added a comment - Also I think we should ask all people wanting the introduction of a new setting for this to provide a clear UI explaining it to usual (not just math ) teachers. I will be delighted to read their answers.
            Hide
            timhunt Tim Hunt added a comment -

            Let me attempt to summarise the forum thread: (The following counts are rather quick. I am sorry if I mis-categorised anyone.)

            People in the forum thread in favour of the change proposed here:

            • Tim Hunt (+ Phil Butcher, Tim Lowe & others at the OU)
            • Emma Richardson
            • Joseph Thibault
            • Dawn Alderson
            • Marcus Green
            • Ray Morris
            • Jean-Michel Védrine
            • Gareth J Barnard
            • Douglas Broad
            • Joshua Bragg
            • Ralf Hilgenstock (+ "often asked by clients")
              (11 + friends)

            Note that the overlap between people in the forum thread, and people who have voted on this issue is surprisingly small. In the tracker votes there are 16 new names. Plus the person who originally reported the bug. Also, Oleg Sychev feels so strongly about this that he has worked on patches in the past, but has not voted, so I will count 1 for him.
            (+18)

            People in the forum who thought the change was good, but who thought their should also be an option for backwards compatibility:

            • Graham Moir
            • ryan sanders
            • Rick Jerz
            • Samuel Witzig
            • Jonathan Ariail
              (5)

            People in the forum who thought the change was bad, or bad without an option:

            • Itamar Tzadok
            • Tony Gardner-Medwin
              (2)

            There were only two reasons given for keeping (an option for) the old behaviour:

            1. Backwards compatibility on general principles.
            2. If students cannot easily tell whether they have seen all the possible questions yet, they will do more practice. Or, as I summarised it in the thread:

              Itamar's argument is not in favour of repeating quetsions.

              Itamar is argument is in favour of making it impossible for students to know whether they have seen all the questions that are available yet, and repeats are an acceptable price to pay for that. – Tim, https://moodle.org/mod/forum/discuss.php?d=310241#p1243200

              Or, as Itamar put it:

              This encourages students to do as many attempts as they possibly can in order to cover all possible questions for the final. – Itamar, https://moodle.org/mod/forum/discuss.php?d=310241#p1242878

            However, we know from the OU that students do not always respond to a learning design the way that teachers hope, so I asked people in favour of the old behaviour to look in their quiz reports an tell us whether this strategy was effective (https://moodle.org/mod/forum/discuss.php?d=310241#p1243200, https://moodle.org/mod/forum/discuss.php?d=310241#p1243591, https://moodle.org/mod/forum/discuss.php?d=310241#p1243666) but I never got a reply to that.

            I think most of the people asking for an option in the UI underestimate the the cost that adds ever afterwards, and many of them would never use it themselves. The quiz form is already too complicated, and we should be very, very reluctant to add any more, particularly for an internal implementation detail like this. It would be sufficient if this could be changed with a plugin. See this post (https://moodle.org/mod/forum/discuss.php?d=310241#p1243879) suggesting that Quiz Access Rule plugins (https://docs.moodle.org/dev/Quiz_access_rules) could be given a hook to control this. Itamar, the most vocal critic of the proposed change, seemed to accept that.

            Given all the above, my recommendation is:

            • Integrate the change as-is.
            • Later, if someone really needs it, they can implement the Quiz Acess Rule hook. I promise to give that favourable peer review if it comes up.
            Show
            timhunt Tim Hunt added a comment - Let me attempt to summarise the forum thread: (The following counts are rather quick. I am sorry if I mis-categorised anyone.) People in the forum thread in favour of the change proposed here: Tim Hunt (+ Phil Butcher, Tim Lowe & others at the OU) Emma Richardson Joseph Thibault Dawn Alderson Marcus Green Ray Morris Jean-Michel Védrine Gareth J Barnard Douglas Broad Joshua Bragg Ralf Hilgenstock (+ "often asked by clients") (11 + friends) Note that the overlap between people in the forum thread, and people who have voted on this issue is surprisingly small. In the tracker votes there are 16 new names. Plus the person who originally reported the bug. Also, Oleg Sychev feels so strongly about this that he has worked on patches in the past, but has not voted, so I will count 1 for him. (+18) People in the forum who thought the change was good, but who thought their should also be an option for backwards compatibility: Graham Moir ryan sanders Rick Jerz Samuel Witzig Jonathan Ariail (5) People in the forum who thought the change was bad, or bad without an option: Itamar Tzadok Tony Gardner-Medwin (2) There were only two reasons given for keeping (an option for) the old behaviour: Backwards compatibility on general principles. If students cannot easily tell whether they have seen all the possible questions yet, they will do more practice. Or, as I summarised it in the thread: Itamar's argument is not in favour of repeating quetsions. Itamar is argument is in favour of making it impossible for students to know whether they have seen all the questions that are available yet, and repeats are an acceptable price to pay for that. – Tim, https://moodle.org/mod/forum/discuss.php?d=310241#p1243200 Or, as Itamar put it: This encourages students to do as many attempts as they possibly can in order to cover all possible questions for the final. – Itamar, https://moodle.org/mod/forum/discuss.php?d=310241#p1242878 However, we know from the OU that students do not always respond to a learning design the way that teachers hope, so I asked people in favour of the old behaviour to look in their quiz reports an tell us whether this strategy was effective ( https://moodle.org/mod/forum/discuss.php?d=310241#p1243200 , https://moodle.org/mod/forum/discuss.php?d=310241#p1243591 , https://moodle.org/mod/forum/discuss.php?d=310241#p1243666 ) but I never got a reply to that. I think most of the people asking for an option in the UI underestimate the the cost that adds ever afterwards, and many of them would never use it themselves. The quiz form is already too complicated, and we should be very, very reluctant to add any more, particularly for an internal implementation detail like this. It would be sufficient if this could be changed with a plugin. See this post ( https://moodle.org/mod/forum/discuss.php?d=310241#p1243879 ) suggesting that Quiz Access Rule plugins ( https://docs.moodle.org/dev/Quiz_access_rules ) could be given a hook to control this. Itamar, the most vocal critic of the proposed change, seemed to accept that. Given all the above, my recommendation is: Integrate the change as-is. Later, if someone really needs it, they can implement the Quiz Acess Rule hook. I promise to give that favourable peer review if it comes up.
            Hide
            timhunt Tim Hunt added a comment -

            And, pointer to the summary posted in the forum https://moodle.org/mod/forum/discuss.php?d=310241#p1244553 so that people can check that I have been fair.

            Show
            timhunt Tim Hunt added a comment - And, pointer to the summary posted in the forum https://moodle.org/mod/forum/discuss.php?d=310241#p1244553 so that people can check that I have been fair.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            +1 as is

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - +1 as is
            Hide
            oa_sychev Oleg Sychev added a comment -

            Yes, Tim, you can count me as +1 as well.

            Thought I do not like idea of Quiz Access Rule given control on that, because that has nothing to do with Access control. One reason is that there are other modules that could use access control like quiz (advanced assignments and lessons are most obvious choices) so I hope some day they will be just Access Rules, not limited to the Quiz.

            Is anything actually preventing moving old random question type code to the 3rd party plugins database for people like Itamar to install and use? I guess it quite easy to make random behave like any other question in question creation/editing interface, explicitly adding random questions to the bank and from it to the quiz, selecting category on edit_random_form.

            Show
            oa_sychev Oleg Sychev added a comment - Yes, Tim, you can count me as +1 as well. Thought I do not like idea of Quiz Access Rule given control on that, because that has nothing to do with Access control. One reason is that there are other modules that could use access control like quiz (advanced assignments and lessons are most obvious choices) so I hope some day they will be just Access Rules, not limited to the Quiz. Is anything actually preventing moving old random question type code to the 3rd party plugins database for people like Itamar to install and use? I guess it quite easy to make random behave like any other question in question creation/editing interface, explicitly adding random questions to the bank and from it to the quiz, selecting category on edit_random_form.
            Hide
            timhunt Tim Hunt added a comment -

            Assigning back to me. (Presumably Itamar clicked the assign link on the wrong issue.)

            Show
            timhunt Tim Hunt added a comment - Assigning back to me. (Presumably Itamar clicked the assign link on the wrong issue.)
            Hide
            itamart Itamar Tzadok added a comment -

            This is a major change in behavior and the solution breaks instructional compatibility for no good reason. It has been suggested in the forum thread that the way to return to the current behavior is by hacking core code. Surely that is not an appropriate way. If integrated as is, the solution introduces a major known issue (bug) which needs to be recorded in the tracker and adressed as soon as possible.

            Oleg, I've also suggested that the current behavior can be reconstructed in a plugin if the solution implements a proper hook. I guess the only problem is that since this is a last minute change, there is no time to add a hook or anything else for that matter. But again, this could be treated as a known major issue and addressed as soon as possible after release.

            Btw, last time I suggested that doing something in a plugin would be easy, I received the following response:

            when you have a working patch, please show it to us for review. Until then, stop telling other people you think it would be easy. My opinion, as an experienced Moodle developer, is that it would be bloody hard to implement this in a generic way.

            Show
            itamart Itamar Tzadok added a comment - This is a major change in behavior and the solution breaks instructional compatibility for no good reason. It has been suggested in the forum thread that the way to return to the current behavior is by hacking core code. Surely that is not an appropriate way. If integrated as is, the solution introduces a major known issue (bug) which needs to be recorded in the tracker and adressed as soon as possible. Oleg, I've also suggested that the current behavior can be reconstructed in a plugin if the solution implements a proper hook. I guess the only problem is that since this is a last minute change, there is no time to add a hook or anything else for that matter. But again, this could be treated as a known major issue and addressed as soon as possible after release. Btw, last time I suggested that doing something in a plugin would be easy, I received the following response: when you have a working patch, please show it to us for review. Until then, stop telling other people you think it would be easy. My opinion, as an experienced Moodle developer, is that it would be bloody hard to implement this in a generic way.
            Hide
            itamart Itamar Tzadok added a comment -

            And sorry about the assign. The touchpad wasn't locked while I was adding a comment and it must have clicked by a slip of a finger.

            Show
            itamart Itamar Tzadok added a comment - And sorry about the assign. The touchpad wasn't locked while I was adding a comment and it must have clicked by a slip of a finger.
            Hide
            oa_sychev Oleg Sychev added a comment -

            Itmar, I know such responses. However, this issue has actual chances of been easy. I see no DB changes so additional fields used by random questions are still there. So existing question type is hook enought for you to re-create random question.

            To try this way you should basically make random questions viewed in the menu - by commenting out function menu_name() in question/type/random/questiontype.php edit_random_form is written already, thought it may miss option to enter (change) category. Try and see - whether you can create a separate random question, see it in the category and add to the quiz just as any other question. It may be possible right away, or there may be additional code hiding random questions from the question bank/quiz editing - it can either be commented out or you can rename random question when making it you plugin (e.g. "random_with_replacement") - renaming respectedly all classes. If this experiment will work, you can have random as a separate plugin with almost no work.

            Tim probably can tell more on possibility on that, but I hope you will consider making experiment with commenting out one function to be really easy.

            I'm sorry, having something easy doesn't means I have a time to do it right now. I working on releasing long-overdue 2.8 versions of two my own question types, each of them containing significantly more complex algorithmic changes than this issue (do you want to take part in the BETA testing?), and that doesn't count my other work. Due to my work shedule I and my students can address this issue in autumn if you want this, but the solution can be expected from November 2015 to February 2016.

            Show
            oa_sychev Oleg Sychev added a comment - Itmar, I know such responses. However, this issue has actual chances of been easy. I see no DB changes so additional fields used by random questions are still there. So existing question type is hook enought for you to re-create random question. To try this way you should basically make random questions viewed in the menu - by commenting out function menu_name() in question/type/random/questiontype.php edit_random_form is written already, thought it may miss option to enter (change) category. Try and see - whether you can create a separate random question, see it in the category and add to the quiz just as any other question. It may be possible right away, or there may be additional code hiding random questions from the question bank/quiz editing - it can either be commented out or you can rename random question when making it you plugin (e.g. "random_with_replacement") - renaming respectedly all classes. If this experiment will work, you can have random as a separate plugin with almost no work. Tim probably can tell more on possibility on that, but I hope you will consider making experiment with commenting out one function to be really easy. I'm sorry, having something easy doesn't means I have a time to do it right now. I working on releasing long-overdue 2.8 versions of two my own question types, each of them containing significantly more complex algorithmic changes than this issue (do you want to take part in the BETA testing?), and that doesn't count my other work. Due to my work shedule I and my students can address this issue in autumn if you want this, but the solution can be expected from November 2015 to February 2016.
            Hide
            dmonllao David Monllaó added a comment -

            Thanks all for the discussion and the work on this issue Tim, integrated to master.

            Show
            dmonllao David Monllaó added a comment - Thanks all for the discussion and the work on this issue Tim, integrated to master.
            Hide
            markn Mark Nelson added a comment -

            Thanks for all your hard work guys, and for Tim taking time to develop it. Works as expected.

            Show
            markn Mark Nelson added a comment - Thanks for all your hard work guys, and for Tim taking time to develop it. Works as expected.
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            Only because the sky is full of stars you may think that your brightness is not enough perceived.

            Let me tell you, you're wrong!

            We need sun-glasses whenever your awesome work helps Moodle to become better and better.

            Highly appreciated, closing this as fixed, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - Only because the sky is full of stars you may think that your brightness is not enough perceived. Let me tell you, you're wrong! We need sun-glasses whenever your awesome work helps Moodle to become better and better. Highly appreciated, closing this as fixed, ciao
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Hello Tim,

            I can see it is very well covered by unit test. Do you think it is a good candidate for QA?

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello Tim, I can see it is very well covered by unit test. Do you think it is a good candidate for QA?
            Hide
            timhunt Tim Hunt added a comment -

            I don't think this needs a MDLQA. No real UI changes. Just a change to an internal algorithm which, as you say, now has good unit tests.

            Show
            timhunt Tim Hunt added a comment - I don't think this needs a MDLQA. No real UI changes. Just a change to an internal algorithm which, as you say, now has good unit tests.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Tim

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Tim
            Hide
            marycooch Mary Cooch added a comment -

            Removing docs_required label as I added a short note to this effect https://docs.moodle.org/29/en/Building_Quiz#Adding_a_random_question and also linked to this tracker issue for further information if anyone needed it. Feel free anyone to add more documentation if necessary.

            Show
            marycooch Mary Cooch added a comment - Removing docs_required label as I added a short note to this effect https://docs.moodle.org/29/en/Building_Quiz#Adding_a_random_question and also linked to this tracker issue for further information if anyone needed it. Feel free anyone to add more documentation if necessary.
            Hide
            adamj Adam Jenkins added a comment -

            Hello all,

            Just a quick comment to show our appreciation for this work. Without a UI change, you may think this improvement went largely unnoticed... Well, we certainly noticed it here in Shizuoka and the work is most certainly very much appreciated. I administer a Moodle here in Japan where several teachers use practice quizzes incorporating randomization regularly. Many of these teachers have previously requested that this type of question selection algorithm be added. They were all incredibly grateful to hear that it has been added to 2.9.

            Thank you all very much!

            Show
            adamj Adam Jenkins added a comment - Hello all, Just a quick comment to show our appreciation for this work. Without a UI change, you may think this improvement went largely unnoticed... Well, we certainly noticed it here in Shizuoka and the work is most certainly very much appreciated. I administer a Moodle here in Japan where several teachers use practice quizzes incorporating randomization regularly. Many of these teachers have previously requested that this type of question selection algorithm be added. They were all incredibly grateful to hear that it has been added to 2.9. Thank you all very much!

              People

              • Votes:
                21 Vote for this issue
                Watchers:
                29 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/May/15