Moodle
  1. Moodle
  2. MDL-6340

force unique/unseen questions in quiz retakes

    Details

    • Type: Improvement Improvement
    • Status: Development in progress
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 1.8.2, 2.8
    • Fix Version/s: STABLE backlog
    • Component/s: Quiz
    • Labels:
      None
    • Environment:
      not sure, but probably all environments
    • Database:
      Any
    • Testing Instructions:
      Hide

      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
      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
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_28_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.

              People

              • Votes:
                5 Vote for this issue
                Watchers:
                11 Start watching this issue

                Dates

                • Created:
                  Updated: