Moodle
  1. Moodle
  2. MDL-6340

force unique/unseen questions in quiz retakes

    Details

    • Type: Improvement Improvement
    • Status: Waiting for integration review
    • Priority: Major Major
    • Resolution: Unresolved
    • 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 questoins.

      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 unliminted 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 questoins. 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 unliminted 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
            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
            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
            Shkarupa Alex added a comment -

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

            Show
            Shkarupa Alex added a comment - This patch for moodle 1.9. I'll make patch for moodle 2.0 in around 2 days
            Hide
            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
            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
            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
            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
            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
            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
            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
            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
            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
            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
            Oleg Sychev added a comment -

            Well, does question_variant_pseudorandom_no_repeats_strategy in Moodle 2.1 fixes this?

            Show
            Oleg Sychev added a comment - Well, does question_variant_pseudorandom_no_repeats_strategy in Moodle 2.1 fixes this?
            Hide
            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
            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
            Tim Hunt added a comment -

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

            Show
            Tim Hunt added a comment - Just removing the patch label. None of the patches here are for recent versions of Moodle.
            Hide
            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
            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
            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
            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
            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
            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
            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
            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
            Darya Beda added a comment - https://github.com/DaryaBeda/moodle/tree/MDL-6340
            Hide
            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
            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 added a comment -

            Fails against automated checks.

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

            More information about this report

            Show
            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
            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
            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
            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
            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
            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
            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
            Oleg Sychev added a comment -

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

            Show
            Oleg Sychev added a comment - Tim, we fixed problems you detected and several more, now that should be a solid fix.
            Hide
            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 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 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 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
            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
            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
            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
            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
            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
            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
            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
            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
            Tim Hunt added a comment -

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

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

            This is now ready for peer review.

            Show
            Tim Hunt added a comment - This is now ready for peer review.
            Hide
            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 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 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 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 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 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
            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
            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 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 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
            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
            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
            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
            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
            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
            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
            Tim Hunt added a comment -

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

            Show
            Tim Hunt added a comment - Great. Thanks Mahmound. I have rebased, and am submitting for integration.
            Hide
            Tim Hunt added a comment -
            Show
            Tim Hunt added a comment - Forum discussion here: https://moodle.org/mod/forum/discuss.php?d=310241
            Hide
            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
            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
            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
            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
            Tim Hunt added a comment -

            Rebased.

            Show
            Tim Hunt added a comment - Rebased.
            Hide
            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 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

              People

              • Votes:
                17 Vote for this issue
                Watchers:
                20 Start watching this issue

                Dates

                • Created:
                  Updated: