Moodle
  1. Moodle
  2. MDL-37993

Quiz completion options - passing grade, all attempts used

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: 2.4.1
    • Fix Version/s: DEV backlog
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide
      1. You need a course, with two students, s1 and s2.
      2. Put student s1 in group g1.
      3. Create a quiz which allows 2 attempts, with a closing date, overall grade = last attempt.
      4. Turn on automatic completion, and both completion options 'Require grade to pass' and 'no more attempts possible'.
      5. Add two questions to the quiz, each worth 1 mark, and the maximum grade for the quiz 100.
      6. Go to the gradebook, and set the passing grade for the quiz to 70%.
      7. As s2, complete the quiz once, getting 100%.
      8. Verify that the Quiz has automatically been marked complete on the course page.
        # As s1, complete the quiz and get q1 right, and q2 wrong 50%. Verify that the quiz is not marked complete.
        # As s1, complete the quiz a second time, getting get q1 right, and q2 wrong 50%. Verify that the quiz is marked complete, since you have used all attempts.
      9. As admin, go to the quiz and add a group override for group g1, allowing 3 attempts.
      10. Use the completion report to verify that the quiz is no longer complete for s1.
      11. Go to the edit quiz page, and change the maximum mark for q1 to 9 points and save. (So now s1's score is 90%).
      12. Verify that the quiz is now complete for s1.
      13. Go to the grade settings, and change the passing grade for the quiz to 95%.
      14. Verify that the quiz is no longer complete for s1.
      15. Edit the Q2 in the quiz, and change the scoring so that the response s1 gave is now considered correct.
      16. Regrade the quiz (so s1's score becomes 100%), and verify that it is now considered complete for s1.
      Show
      You need a course, with two students, s1 and s2. Put student s1 in group g1. Create a quiz which allows 2 attempts, with a closing date, overall grade = last attempt. Turn on automatic completion, and both completion options 'Require grade to pass' and 'no more attempts possible'. Add two questions to the quiz, each worth 1 mark, and the maximum grade for the quiz 100. Go to the gradebook, and set the passing grade for the quiz to 70%. As s2, complete the quiz once, getting 100%. Verify that the Quiz has automatically been marked complete on the course page. # As s1, complete the quiz and get q1 right, and q2 wrong 50%. Verify that the quiz is not marked complete. # As s1, complete the quiz a second time, getting get q1 right, and q2 wrong 50%. Verify that the quiz is marked complete, since you have used all attempts. As admin, go to the quiz and add a group override for group g1, allowing 3 attempts. Use the completion report to verify that the quiz is no longer complete for s1. Go to the edit quiz page, and change the maximum mark for q1 to 9 points and save. (So now s1's score is 90%). Verify that the quiz is now complete for s1. Go to the grade settings, and change the passing grade for the quiz to 95%. Verify that the quiz is no longer complete for s1. Edit the Q2 in the quiz, and change the scoring so that the response s1 gave is now considered correct. Regrade the quiz (so s1's score becomes 100%), and verify that it is now considered complete for s1.
    • Affected Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      47773

      Description

      This patch, which borrows heavily from mod/scorm/, adds completion options to Quiz similar to what is available in scorm and some other modules. Namely, one can have the quiz marked complete when a specified score passing grade is achieved, or when all attempts are exhausted.

        Issue Links

          Activity

          Hide
          Ray Morris added a comment -

          85% of the code is for "module specific conditions", not specific to either of the two conditions, and
          80% of the remainder is basically copy/paste duplicated for the two conditions, so that's why I made one
          issue for "module specific conditions" rather than artificially splitting it out to two issues. (If I had
          split it, which of the two conditions would get the bulk of the code, the "module specific conditions" code?)

          Show
          Ray Morris added a comment - 85% of the code is for "module specific conditions", not specific to either of the two conditions, and 80% of the remainder is basically copy/paste duplicated for the two conditions, so that's why I made one issue for "module specific conditions" rather than artificially splitting it out to two issues. (If I had split it, which of the two conditions would get the bulk of the code, the "module specific conditions" code?)
          Hide
          Tim Hunt added a comment -

          I guess this is useful funcitonality, but there is some way to go before this code is good enough to include in Moodle.

          Some review comments:

          1. Upgrade code should do each bit in a separate if ($oldversion < 2013020801) { block. That copes better if an error occurs in the middle of the upgrade. (Basically: use XMLDB to generate the code, and don't mess with it.)

          2. Do not require_once($CFG->libdir . '/gradelib.php'); at the top of mod/quiz/lib.php. Only include it where necessary. (Why? Peformance.)

          3. If you are going to copy and paste huge chunks of code from SCORM, please fix the comments to remove the word SCORM.

          4. You have not implemented backup and restore for the new quiz settings. (It is on the checklist at http://docs.moodle.org/dev/Activity_completion_API#Custom_completion_rules)

          5. You should run codechecker before asking a human to peer-review your code.

          6. Currently quiz_get_completion_stat is hiddeously inefficient. How many DB queries does it do? How many are necessary?

          Those are some basic points. I am sure there are more.

          I hope I have not been to harsh. I am sure you can appreciate that we need to ensure that code going into the standard Moodle release is hight quality, and will not cause performance problems.

          Show
          Tim Hunt added a comment - I guess this is useful funcitonality, but there is some way to go before this code is good enough to include in Moodle. Some review comments: 1. Upgrade code should do each bit in a separate if ($oldversion < 2013020801) { block. That copes better if an error occurs in the middle of the upgrade. (Basically: use XMLDB to generate the code, and don't mess with it.) 2. Do not require_once($CFG->libdir . '/gradelib.php'); at the top of mod/quiz/lib.php. Only include it where necessary. (Why? Peformance.) 3. If you are going to copy and paste huge chunks of code from SCORM, please fix the comments to remove the word SCORM. 4. You have not implemented backup and restore for the new quiz settings. (It is on the checklist at http://docs.moodle.org/dev/Activity_completion_API#Custom_completion_rules ) 5. You should run codechecker before asking a human to peer-review your code. 6. Currently quiz_get_completion_stat is hiddeously inefficient. How many DB queries does it do? How many are necessary? Those are some basic points. I am sure there are more. I hope I have not been to harsh. I am sure you can appreciate that we need to ensure that code going into the standard Moodle release is hight quality, and will not cause performance problems.
          Hide
          Ray Morris added a comment - - edited

          Tim, thank you very much for your comments. I've done 1-5 of the six items you mentioned. I've run the code checker and cleaned up the items it mentioned, edited the comment that said "quiz scorm" where it should have said "quiz", etc.

          I have a question regarding #6 in your list:

          > Currently quiz_get_completion_stat is hiddeously inefficient.
          > How many DB queries does it do? How many are necessary?

          There are exactly two queries in the new code itself, both fast get_record* calls with tight WHERE clauses. I don't see those two get_record being done any more efficiently, so I'm assuming you're looking at existing functions which this code calls and how many queries those pre-existing functions do.

          There are about 10 queries needed to create() the quiz object if the attempts parameter is enabled, or 5 queries to grade_get_grades if that's enabled.
          My thinking is that because this code is within Quiz, we can skip the 10 queries to instantiate the quiz object and and replace those 10 queries with 1.

          However, since grades isn't our component, we should continue to use the grades API, even if we think we could right a more efficient query against the database directly. Do you concur? Keep using the grades API, but write direct, efficient query for the quiz tables to get the maximum attempts allowed?

          PS - I did see that instantiating the quiz object just before the if() was dumb. That's been moved down a line.

          • I hope I have not been to harsh.

          You're tougher than the people who have looked at my other 8-10 patches to Moodle but you're nothing like Torvalds! I'm really glad I don't have to patch the kernel often.

          I'll push another patch when #6 is handled based on your comments, or if you get busy and don't
          have time to reply I'll push you one Thursday based on what I think you're wanting to see.

          Show
          Ray Morris added a comment - - edited Tim, thank you very much for your comments. I've done 1-5 of the six items you mentioned. I've run the code checker and cleaned up the items it mentioned, edited the comment that said "quiz scorm" where it should have said "quiz", etc. I have a question regarding #6 in your list: > Currently quiz_get_completion_stat is hiddeously inefficient. > How many DB queries does it do? How many are necessary? There are exactly two queries in the new code itself, both fast get_record* calls with tight WHERE clauses. I don't see those two get_record being done any more efficiently, so I'm assuming you're looking at existing functions which this code calls and how many queries those pre-existing functions do. There are about 10 queries needed to create() the quiz object if the attempts parameter is enabled, or 5 queries to grade_get_grades if that's enabled. My thinking is that because this code is within Quiz, we can skip the 10 queries to instantiate the quiz object and and replace those 10 queries with 1. However, since grades isn't our component, we should continue to use the grades API, even if we think we could right a more efficient query against the database directly. Do you concur? Keep using the grades API, but write direct, efficient query for the quiz tables to get the maximum attempts allowed? PS - I did see that instantiating the quiz object just before the if() was dumb. That's been moved down a line. I hope I have not been to harsh. You're tougher than the people who have looked at my other 8-10 patches to Moodle but you're nothing like Torvalds! I'm really glad I don't have to patch the kernel often. I'll push another patch when #6 is handled based on your comments, or if you get busy and don't have time to reply I'll push you one Thursday based on what I think you're wanting to see.
          Hide
          Tim Hunt added a comment -

          Sorry, a couple more:

          7. Several of the lang strings seems excessively verbose, and not necessarily in the most user-friedly language. E.g. 'Student attempted the maximum number of times' could be 'All available attempts completed'. Ideally, find some teachers to run the wording past. (You could post in the quiz forum: https://moodle.org/mod/forum/view.php?id=737)

          8. The completionscorerequired variable name is confusing. Or at least it confused me just now. It sounds to me like it should be the same as the core feature to require a grade. Also, in Moodle code, we use the word grade, not score. Would completionminimumgrade be a better name?

          OK, so back to your original question: ... oh dear, the more I look at quiz_get_completion_state the more problems I see ...

          The $DB->get_record_sql( "SELECT COUNT AS howmany should be a $DB->count_records.

          But acutally, is just number of attempts the right thing to be testing. The alternative would be to use the quiz_access_manager::is_finished result, which you might describe in the UI as "No more attempts allowed".

          The variable name $quizrec is not used anywhere else in the quiz code. It would be more idiomatic to call it $quiz.

          Is it really correct to return early during the $quizrec->completionattemptsexhausted test? Surely that prevents the $quizrec->completionscorerequired from running, even when they may give a different result.

          Code that does

          if ($condition) {
              call_function(true);
          } else {
              call_function(false);
          }
          

          is silly. It should be

          call_function($condition);
          

          Actually, I don't understand the need to add completionscorerequired at all. What requirement do you have that the exitsing COMPLETION_COMPLETE_PASS / COMPLETION_COMPLETE_FAIL states do not cover?

          Show
          Tim Hunt added a comment - Sorry, a couple more: 7. Several of the lang strings seems excessively verbose, and not necessarily in the most user-friedly language. E.g. 'Student attempted the maximum number of times' could be 'All available attempts completed'. Ideally, find some teachers to run the wording past. (You could post in the quiz forum: https://moodle.org/mod/forum/view.php?id=737 ) 8. The completionscorerequired variable name is confusing. Or at least it confused me just now. It sounds to me like it should be the same as the core feature to require a grade. Also, in Moodle code, we use the word grade, not score. Would completionminimumgrade be a better name? OK, so back to your original question: ... oh dear, the more I look at quiz_get_completion_state the more problems I see ... The $DB->get_record_sql( "SELECT COUNT AS howmany should be a $DB->count_records. But acutally, is just number of attempts the right thing to be testing. The alternative would be to use the quiz_access_manager::is_finished result, which you might describe in the UI as "No more attempts allowed". The variable name $quizrec is not used anywhere else in the quiz code. It would be more idiomatic to call it $quiz. Is it really correct to return early during the $quizrec->completionattemptsexhausted test? Surely that prevents the $quizrec->completionscorerequired from running, even when they may give a different result. Code that does if ($condition) { call_function( true ); } else { call_function( false ); } is silly. It should be call_function($condition); Actually, I don't understand the need to add completionscorerequired at all. What requirement do you have that the exitsing COMPLETION_COMPLETE_PASS / COMPLETION_COMPLETE_FAIL states do not cover?
          Hide
          Ray Morris added a comment - - edited

          Thank you again for your extensive and humbling comments.
          I've implemented most of what you suggested. I never did see an answer as
          to what you meant on six. I'll go ahead and ditch the expensive quiz object
          while keeping the grades API call and I'm sure you'll let me know if you think
          that's a bad idea.

          Most of the rest was just me assuming that the code already in scorm was either
          a) good (and tested) or b) worth being consistent with. Your suggestions on naming,
          etc. probably are better, so I've implemented those.

          Is this not atrocious?:

                  return completion_info::aggregate_completion_states($type, $result, 
                      ($quiz->completionattemptsexhausted && ($attempts >= $quizobj->get_num_attempts_allowed())));
          

          > is silly. It should be call_function($condition);

          In general I would agree. When both the function call and the $condition are each a hundred characters long and you've got parentheses five levels deep, as is the case here, I find the if () construction much more readable. To me, the condensed version, shown above, is atrocious. It's your module, though. If you prefer parentheses five levels deep in one monster statement, I'm more than happy to oblige. Absolutely no problem, if that's what you prefer.

          > Actually, I don't understand the need to add completionscorerequired at all.
          > What requirement do you have that the exitsing COMPLETION_COMPLETE_PASS /
          > COMPLETION_COMPLETE_FAIL

          There is a possible improvement by going in that general direction, but first let me answer your question.

          Keep in mind COMPLETION_COMPLETE_PASS is the state AFTER completion. Those aren't conditions for whether it's marked complete.
          You said you can see how marking it complete upon "is_finished" is useful.
          Okay, marking it complete when they use up all attempts is useful. Let's say you
          use that setting. Then a student uses just one attempt and aces it, scores 100% on the first attempt.
          The student almost surely will not make other attempts, and obviously has completed it.
          Without "complete on ___ grade", that user doesn't get COMPLETION_COMPLETE_PASS because there's nothing to say he's completed at all.
          You can't get much more complete on a quiz than acing it. Thus, a certain score on a quiz is completing it.

          Neither COMPLETION_COMPLETE_PASS not COMPLETION_COMPLETE_FAIL would be set without those conditions, because there would be no completion at all. Where pass/fail may apply, is that it may make logical sense (but not UI sense) to have the new setting be "complete upon pass" rather than "complete upon grade ___". I may be missing something, maybe there's a reason SCORM did complete on grade ___. If we don't assume Dan did that for a reason, I think from a strict computer science perspective "complete upon pass" would work. Users hate it because the new quiz and edit quiz forms don't let them set "grade to pass". Instead, that setting is off in several clicks deep in the navigation tree, in gradebook -> advanced or whatever it is. Users want to set up their quiz on the "Add a new quiz" page. Really, I think the BEST answer is "complete upon pass" but only if the "passing grade" can be set right there on the "new quiz" page. Again, that's assuming Dan didn't have a good reason for doing it the way they did in SCORM - there may be a reason we haven't thought of.

          Show
          Ray Morris added a comment - - edited Thank you again for your extensive and humbling comments. I've implemented most of what you suggested. I never did see an answer as to what you meant on six. I'll go ahead and ditch the expensive quiz object while keeping the grades API call and I'm sure you'll let me know if you think that's a bad idea. Most of the rest was just me assuming that the code already in scorm was either a) good (and tested) or b) worth being consistent with. Your suggestions on naming, etc. probably are better, so I've implemented those. Is this not atrocious?: return completion_info::aggregate_completion_states($type, $result, ($quiz->completionattemptsexhausted && ($attempts >= $quizobj->get_num_attempts_allowed()))); > is silly. It should be call_function($condition); In general I would agree. When both the function call and the $condition are each a hundred characters long and you've got parentheses five levels deep, as is the case here, I find the if () construction much more readable. To me, the condensed version, shown above, is atrocious. It's your module, though. If you prefer parentheses five levels deep in one monster statement, I'm more than happy to oblige. Absolutely no problem, if that's what you prefer. > Actually, I don't understand the need to add completionscorerequired at all. > What requirement do you have that the exitsing COMPLETION_COMPLETE_PASS / > COMPLETION_COMPLETE_FAIL There is a possible improvement by going in that general direction, but first let me answer your question. Keep in mind COMPLETION_COMPLETE_PASS is the state AFTER completion. Those aren't conditions for whether it's marked complete. You said you can see how marking it complete upon "is_finished" is useful. Okay, marking it complete when they use up all attempts is useful. Let's say you use that setting. Then a student uses just one attempt and aces it, scores 100% on the first attempt. The student almost surely will not make other attempts, and obviously has completed it. Without "complete on ___ grade", that user doesn't get COMPLETION_COMPLETE_PASS because there's nothing to say he's completed at all. You can't get much more complete on a quiz than acing it. Thus, a certain score on a quiz is completing it. Neither COMPLETION_COMPLETE_PASS not COMPLETION_COMPLETE_FAIL would be set without those conditions, because there would be no completion at all. Where pass/fail may apply, is that it may make logical sense (but not UI sense) to have the new setting be "complete upon pass" rather than "complete upon grade ___". I may be missing something, maybe there's a reason SCORM did complete on grade ___. If we don't assume Dan did that for a reason, I think from a strict computer science perspective "complete upon pass" would work. Users hate it because the new quiz and edit quiz forms don't let them set "grade to pass". Instead, that setting is off in several clicks deep in the navigation tree, in gradebook -> advanced or whatever it is. Users want to set up their quiz on the "Add a new quiz" page. Really, I think the BEST answer is "complete upon pass" but only if the "passing grade" can be set right there on the "new quiz" page. Again, that's assuming Dan didn't have a good reason for doing it the way they did in SCORM - there may be a reason we haven't thought of.
          Hide
          Tim Hunt added a comment -

          Most of SCORM is written by Dan Marsden, who is not stupid, but we all have bad days. (Git blame will show who wrote those bits of code you were copying and it was not Dan but it was people I would expect to write better code than this.)


          Yes, the example you give is atrocious, but you are already inside an if that shows that $quiz->completionattemptsexhausted is true, and when you delete that bit, it gets a lot nicer.

          If the line of code is too long, I would prefer using a named variable, like

          $allattemptsused = $attempts >= $quizobj->get_num_attempts_allowed();
          completion_info::aggregate_completion_states($type, $result, $allattemptsused);
          

          rather than an if with almost identical code in both branches.


          Just because the UI for setting an activity's passing grade is currently horrible, is not an excuse to add new functionality that is badly integrated with existing functionality. The best thing would be to fix MDL-13831, which would be a separate patch.

          I think we do need to worry about the UI for this. The help on the settings form says: "... If so, the activity will only be considered complete when ALL conditions are met." And what you are proposing naturally OR logic between the two new settings. So, the question is, how can we make that look on the quiz settings form, so that the UI is clear, and consistent with the help. I woudl acutally like to see a UI mock-up before seeing the code behind it. (There is a nice UI mockup tool in the More actions menu above.)


          I was discussing this with Sam Marshall, who build the completion system initially he was saying that this function is not too performance critical, so using the gradebook API is right. However, that is no reason to be proflogate with DB queries.

          Show
          Tim Hunt added a comment - Most of SCORM is written by Dan Marsden, who is not stupid, but we all have bad days. (Git blame will show who wrote those bits of code you were copying and it was not Dan but it was people I would expect to write better code than this.) Yes, the example you give is atrocious, but you are already inside an if that shows that $quiz->completionattemptsexhausted is true, and when you delete that bit, it gets a lot nicer. If the line of code is too long, I would prefer using a named variable, like $allattemptsused = $attempts >= $quizobj->get_num_attempts_allowed(); completion_info::aggregate_completion_states($type, $result, $allattemptsused); rather than an if with almost identical code in both branches. Just because the UI for setting an activity's passing grade is currently horrible, is not an excuse to add new functionality that is badly integrated with existing functionality. The best thing would be to fix MDL-13831 , which would be a separate patch. I think we do need to worry about the UI for this. The help on the settings form says: "... If so, the activity will only be considered complete when ALL conditions are met." And what you are proposing naturally OR logic between the two new settings. So, the question is, how can we make that look on the quiz settings form, so that the UI is clear, and consistent with the help. I woudl acutally like to see a UI mock-up before seeing the code behind it. (There is a nice UI mockup tool in the More actions menu above.) I was discussing this with Sam Marshall, who build the completion system initially he was saying that this function is not too performance critical, so using the gradebook API is right. However, that is no reason to be proflogate with DB queries.
          Hide
          Ray Morris added a comment - - edited

          I agree, the two settings naturally work well together.
          They also work separately, and we use them separately for other types of modules,
          but they fit very nicely together. I've attached a simple representation of
          my idea about how that could look, given that these two specifically work well together.

          I have two other (better?) generic UI designs I'll present in the tracker for generic
          Activity Completion. Allowing OR generically has been half completed for a long time.
          I'm awaiting an answer from Sam before finishing that. Not knowing if that will ever
          be done, I'm not sure we want to make it a blocker for this. I'll leave further discussion
          of that generic capability for the appropriate tracker.

          Show
          Ray Morris added a comment - - edited I agree, the two settings naturally work well together. They also work separately, and we use them separately for other types of modules, but they fit very nicely together. I've attached a simple representation of my idea about how that could look, given that these two specifically work well together. I have two other (better?) generic UI designs I'll present in the tracker for generic Activity Completion. Allowing OR generically has been half completed for a long time. I'm awaiting an answer from Sam before finishing that. Not knowing if that will ever be done, I'm not sure we want to make it a blocker for this. I'll leave further discussion of that generic capability for the appropriate tracker.
          Hide
          Tim Hunt added a comment -

          Generic OR would be good, but I agree that it should not be a blocker for this.

          Did you think about my quiz_access_manager::is_finished result (rather than just counting attempts). I still think it has merit.

          I don't think that 'passing grade' on its own makes sense, since the system already distinguishes COMPLETE_PASS and COMPLETE_FAIL, and that can be used in conditional activities. I think the passing grade thing is only relevant in combination with 'no more attempts'. How about UI like

          [ ] No more attempts possible ( [ ] or passing grade already reached ).

          With the second checkbox disabledIf on the first. You could even store this in a single DB column.

          I'm sorry this is proving so tricky. You started out thinking you had a nice easy win, porting some existing working functionality from SCORM to Quiz. I am afraid I do want to get this right, however. Perhaps SCORM can then incorporate our improvements

          Show
          Tim Hunt added a comment - Generic OR would be good, but I agree that it should not be a blocker for this. Did you think about my quiz_access_manager::is_finished result (rather than just counting attempts). I still think it has merit. I don't think that 'passing grade' on its own makes sense, since the system already distinguishes COMPLETE_PASS and COMPLETE_FAIL, and that can be used in conditional activities. I think the passing grade thing is only relevant in combination with 'no more attempts'. How about UI like [ ] No more attempts possible ( [ ] or passing grade already reached ). With the second checkbox disabledIf on the first. You could even store this in a single DB column. I'm sorry this is proving so tricky. You started out thinking you had a nice easy win, porting some existing working functionality from SCORM to Quiz. I am afraid I do want to get this right, however. Perhaps SCORM can then incorporate our improvements
          Hide
          Ray Morris added a comment -

          > Did you think about my quiz_access_manager::is_finished result (rather than just counting attempts).

          It recieves my approbation if you judge that it's not proflogate for the purpose.
          I'll make that change and push another commit with all of these changes later.

          > I don't think that 'passing grade' on its own makes sense, since the system already distinguishes
          > COMPLETE_PASS and COMPLETE_FAIL, and that can be used in conditional activities.

          In practice, conditional activities can handle that. Logically, it makes sense to say "it's not
          done until it's done right". So while I think "keep trying, it's complete when you pass" is logical
          on its own, we have no actual need for it to be separate.

          > [ ] No more attempts possible ( [ ] or passing grade already reached ).
          > With the second checkbox disabledIf on the first.

          Works for me.

          > You could even store this in a single DB column.

          That wouldn't be first normal form. Most of Moodle is third normal form or better.

          > ... I am afraid I do want to get this right, however.

          Thanks for sticking around long enough to get it right.

          Show
          Ray Morris added a comment - > Did you think about my quiz_access_manager::is_finished result (rather than just counting attempts). It recieves my approbation if you judge that it's not proflogate for the purpose. I'll make that change and push another commit with all of these changes later. > I don't think that 'passing grade' on its own makes sense, since the system already distinguishes > COMPLETE_PASS and COMPLETE_FAIL, and that can be used in conditional activities. In practice, conditional activities can handle that. Logically, it makes sense to say "it's not done until it's done right". So while I think "keep trying, it's complete when you pass" is logical on its own, we have no actual need for it to be separate. > [ ] No more attempts possible ( [ ] or passing grade already reached ). > With the second checkbox disabledIf on the first. Works for me. > You could even store this in a single DB column. That wouldn't be first normal form. Most of Moodle is third normal form or better. > ... I am afraid I do want to get this right, however. Thanks for sticking around long enough to get it right.
          Hide
          Tim Hunt added a comment -

          I think we are nearly there, and it is worth you pushing your latest commit, so I can see what it looks like now.

          On the question of one or two DB columns, it really depends whether you view

          [ ] No more attempts possible ( [ ] or passing grade already reached ).

          as two independent settings (we have said that they are not independent) or one cominded setting with three values:

          • No condition.
          • Complete when no more attempts possible.
          • Complete when no more attempts possible, or passing grade reached.

          I guess I see it as one setting, that appears in the UI as two checkboxes.

          Show
          Tim Hunt added a comment - I think we are nearly there, and it is worth you pushing your latest commit, so I can see what it looks like now. On the question of one or two DB columns, it really depends whether you view [ ] No more attempts possible ( [ ] or passing grade already reached ). as two independent settings (we have said that they are not independent) or one cominded setting with three values: No condition. Complete when no more attempts possible. Complete when no more attempts possible, or passing grade reached. I guess I see it as one setting, that appears in the UI as two checkboxes.
          Hide
          Ray Morris added a comment -

          Based on all that has been said above, I've pushed another commit to:
          https://github.com/MorrisR2/moodle/compare/MDL-37993-quiz-completion-options

          Show
          Ray Morris added a comment - Based on all that has been said above, I've pushed another commit to: https://github.com/MorrisR2/moodle/compare/MDL-37993-quiz-completion-options
          Hide
          Tim Hunt added a comment -

          I'm at the Dubin Moodle conference today and tomorrow, and then on holiday on Saturday, so just be warned that I may not get to this until next week. Sorry.

          Show
          Tim Hunt added a comment - I'm at the Dubin Moodle conference today and tomorrow, and then on holiday on Saturday, so just be warned that I may not get to this until next week. Sorry.
          Hide
          Ray Morris added a comment -

          Enjoy Dublin, Tim. My boss says it's about time to stop improving this particular code, so I probably couldn't work on it any further in the next few days anyway.

          Show
          Ray Morris added a comment - Enjoy Dublin, Tim. My boss says it's about time to stop improving this particular code, so I probably couldn't work on it any further in the next few days anyway.
          Hide
          Tim Hunt added a comment -

          I was just having another look at this, in the hope I could finish it off before the 2.5 code freeze, but I don't think I can.

          First, I made some changes that

          • Changed the database column definitions to be consistent with the other 'boolean' columns in the quiz table. Also added comments to explain what they do.
          • Tweaked the UI on the quiz settings form. I decided that logically the two options were independent. It could make sense to use either option on its own, or both together with OR logic.
          • Fixed the calls to update the completion state where necessary. There was at least on missing.

          However, I am still not sure this is right. For the explanation, see the changes I am about to make to the testing instructions.

          Show
          Tim Hunt added a comment - I was just having another look at this, in the hope I could finish it off before the 2.5 code freeze, but I don't think I can. First, I made some changes that Changed the database column definitions to be consistent with the other 'boolean' columns in the quiz table. Also added comments to explain what they do. Tweaked the UI on the quiz settings form. I decided that logically the two options were independent. It could make sense to use either option on its own, or both together with OR logic. Fixed the calls to update the completion state where necessary. There was at least on missing. However, I am still not sure this is right. For the explanation, see the changes I am about to make to the testing instructions.
          Hide
          Tim Hunt added a comment -

          Right, I think those testing instructions now contain all the edge cases we have to worry about. I have not tried it, but I bet that we fail several of those tests with the current code, and I do not intend to spend any more time on this now.

          Actually, give the complexity of those tests, I would really rather not see this submitted for integration until we have unit tests to verify all that. (I have to say, writing unit tests for it seems more fun that testing it by hand!)

          Of course, I could be wrong, and the current code might be good enough (if the core completion system is cleverer than I think it is). If anyone can confirm that, then we could submit this for integration.

          Show
          Tim Hunt added a comment - Right, I think those testing instructions now contain all the edge cases we have to worry about. I have not tried it, but I bet that we fail several of those tests with the current code, and I do not intend to spend any more time on this now. Actually, give the complexity of those tests, I would really rather not see this submitted for integration until we have unit tests to verify all that. (I have to say, writing unit tests for it seems more fun that testing it by hand!) Of course, I could be wrong, and the current code might be good enough (if the core completion system is cleverer than I think it is). If anyone can confirm that, then we could submit this for integration.
          Hide
          Tim Hunt added a comment -

          Sam, I added you as peer reviewer in the hope that you could give some clues as to how hard it will be to make all those tests pass.

          Show
          Tim Hunt added a comment - Sam, I added you as peer reviewer in the hope that you could give some clues as to how hard it will be to make all those tests pass.
          Hide
          Ray Morris added a comment -

          I've tested Tim's latest branch as per the testing instructions. The branch from my git has also been tested and
          is in use in production.

          The latest change from Tim's repository passes a integer course ID to a function which expects a course object:
          function quiz_delete_attempt($attempt, $quiz) {
          ...
          $modinfo = get_fast_modinfo($quiz->course);

          As suggested, changes to scoring, etc. do not retroactively affect completion. That is, a quiz that was completed does not become "not complete" when the passing score is changed. Some would say that's correct behavior.

          I notice that in other modules, some completion criteria changes are retroactive and some are not.
          Consider a student who takes a course, passing the quizzes, and receives a certificate indicating that they've
          completed the course. They certainly would not expect that two years later that completion would be revoked
          because the teacher decided to change what grade was considered passing. That's a strong argument that
          current behavior, "changing settings affects students who later complete the activity, and do not retroactively
          affect students who had already done it" is correct behavior.

          Has a policy decision been made that changing settings should be retroactive to students who have already done
          a course or activity in the past?

          Show
          Ray Morris added a comment - I've tested Tim's latest branch as per the testing instructions. The branch from my git has also been tested and is in use in production. The latest change from Tim's repository passes a integer course ID to a function which expects a course object: function quiz_delete_attempt($attempt, $quiz) { ... $modinfo = get_fast_modinfo($quiz->course); As suggested, changes to scoring, etc. do not retroactively affect completion. That is, a quiz that was completed does not become "not complete" when the passing score is changed. Some would say that's correct behavior. I notice that in other modules, some completion criteria changes are retroactive and some are not. Consider a student who takes a course, passing the quizzes, and receives a certificate indicating that they've completed the course. They certainly would not expect that two years later that completion would be revoked because the teacher decided to change what grade was considered passing. That's a strong argument that current behavior, "changing settings affects students who later complete the activity, and do not retroactively affect students who had already done it" is correct behavior. Has a policy decision been made that changing settings should be retroactive to students who have already done a course or activity in the past?
          Hide
          Tim Hunt added a comment -

          Sadly, there is a policy decision here that needs to be resolved, so this will have to go on the back burner for a bit. You may be interested in MDLSITE-2042. If you are interested in trying to settle this policy question, then starting a discussion in an appropriate forum on moodle.org would be a helpful contribution to moving this forwards. Sorry it is so complex.

          Show
          Tim Hunt added a comment - Sadly, there is a policy decision here that needs to be resolved, so this will have to go on the back burner for a bit. You may be interested in MDLSITE-2042 . If you are interested in trying to settle this policy question, then starting a discussion in an appropriate forum on moodle.org would be a helpful contribution to moving this forwards. Sorry it is so complex.
          Hide
          Ray Morris added a comment - - edited

          Discussion as suggested:
          https://moodle.org/mod/forum/view.php?id=55

          I'm not sure that this MDL requires discussion of changing Activity Completion's functionality in that way,
          but if you'd like to raise it now, so be it.

          Show
          Ray Morris added a comment - - edited Discussion as suggested: https://moodle.org/mod/forum/view.php?id=55 I'm not sure that this MDL requires discussion of changing Activity Completion's functionality in that way, but if you'd like to raise it now, so be it.
          Hide
          Sam Marshall added a comment -

          To add to Tim's comment now that I'm back - while this might not be working everywhere, it was originally intended that activity completion status updates dynamically.

          Example 1: If you just use 'on grade' completion, if a student does something and gets a grade, it will be marked complete. If the teacher then deletes their grade from gradebook, it will be marked incomplete again.

          Example 2: In forum, if you set it to be complete after making 3 posts, and the student makes 3 posts, it will be marked complete. But if the teacher then deletes one of the posts, it will be marked incomplete again.

          It could be argued that things should not be automatically 'un-completed' in this way (there are cases when both the above examples definitely seem appropriate, but it might be OK if the teacher had to manually un-complete them as well), but this would be a policy change (and also an API change as we should probably change the API to make that new assumption 'fixed in' i.e. remove the possibility of people uncompleting things, other than manually).

          Note that course completion may be different from activity completion in this regard, I'm not sure.

          Show
          Sam Marshall added a comment - To add to Tim's comment now that I'm back - while this might not be working everywhere, it was originally intended that activity completion status updates dynamically. Example 1: If you just use 'on grade' completion, if a student does something and gets a grade, it will be marked complete. If the teacher then deletes their grade from gradebook, it will be marked incomplete again. Example 2: In forum, if you set it to be complete after making 3 posts, and the student makes 3 posts, it will be marked complete. But if the teacher then deletes one of the posts, it will be marked incomplete again. It could be argued that things should not be automatically 'un-completed' in this way (there are cases when both the above examples definitely seem appropriate, but it might be OK if the teacher had to manually un-complete them as well), but this would be a policy change (and also an API change as we should probably change the API to make that new assumption 'fixed in' i.e. remove the possibility of people uncompleting things, other than manually). Note that course completion may be different from activity completion in this regard, I'm not sure.
          Hide
          Sam Marshall added a comment -

          Oops, just to note - if anybody actually tries out either of the two examples I gave, remember that you will need to log out and in again as the student before you 'see' the changes the teacher made. (Due to the way activity completion data is cached, which also could use fixing, you only immediately see changes that occur as a result of your own actions.)

          Show
          Sam Marshall added a comment - Oops, just to note - if anybody actually tries out either of the two examples I gave, remember that you will need to log out and in again as the student before you 'see' the changes the teacher made. (Due to the way activity completion data is cached, which also could use fixing, you only immediately see changes that occur as a result of your own actions.)
          Hide
          Ray Morris added a comment -

          Based on Sam's comments, it sounds like the policy decision was made, perhaps? I happen to think the policy is rather
          questionable, but if it's settled policy, it doesn't need to block this issue, correct?

          Under the principle of least surprise, I'd say that it's VERY surprising to arrange things so that students have no way of even knowing that they've suddenly been exposed to criminal sanctions, but that's a different ticket if it means changing settled policy.

          Show
          Ray Morris added a comment - Based on Sam's comments, it sounds like the policy decision was made, perhaps? I happen to think the policy is rather questionable, but if it's settled policy, it doesn't need to block this issue, correct? Under the principle of least surprise, I'd say that it's VERY surprising to arrange things so that students have no way of even knowing that they've suddenly been exposed to criminal sanctions, but that's a different ticket if it means changing settled policy.
          Hide
          Sam Marshall added a comment -

          Ray - There wasn't a policy decision - there was an original system design which was intended to work that way (I did all the original design and code for the completion system, so it's my fault). The original design worked that way, and it was accepted into Moodle working that way, but there wasn't a specific decision that this is the correct way it should work.

          In other words if there is a discussion about it and people come to the opinion that actually it would be better if things never get uncompleted (unless manually done by teacher), absolutely I would be happy to look at changing it. (Even if I don't have development time available, I could review code submitted by others to achieve the change.)

          So:

          1) Unless/until we have that discussion it is probably better not to make individual parts of the system work differently. (Albeit there might already be some inconsistencies.)

          2) That doesn't mean we should sleepwalk into doing things the way it works now purely because that's the way it works now. Maybe the way it works now is wrong. But if so it should be changed across the system.

          Show
          Sam Marshall added a comment - Ray - There wasn't a policy decision - there was an original system design which was intended to work that way (I did all the original design and code for the completion system, so it's my fault). The original design worked that way, and it was accepted into Moodle working that way, but there wasn't a specific decision that this is the correct way it should work. In other words if there is a discussion about it and people come to the opinion that actually it would be better if things never get uncompleted (unless manually done by teacher), absolutely I would be happy to look at changing it. (Even if I don't have development time available, I could review code submitted by others to achieve the change.) So: 1) Unless/until we have that discussion it is probably better not to make individual parts of the system work differently. (Albeit there might already be some inconsistencies.) 2) That doesn't mean we should sleepwalk into doing things the way it works now purely because that's the way it works now. Maybe the way it works now is wrong. But if so it should be changed across the system.
          Hide
          Ray Morris added a comment -

          I retract my comment "sounds like a policy decision has been made". Based on Sam's description of the original intent and
          based on testing the currently distributed code, here's what I found to be the case, which I think is reasonable:

          Changes to what a specific STUDENT does, like adding and removing forum posts, can affect their completion. That's reasonable - if you remove the post, you're indicating that student doesn't have the required number of qualifying posts.

          Changes to the gradebook etc settings such as passing grade, do not retroactively cause students who had already passed to fail. That is, their completion state does not change to "complete_failed". Again, that's also reasonable - the student was already done, and nothing has changed with that student.

          Here, we're looking at adding "mark complete when they have passed". We already have an established definition of "passed". That definition is outside of any specific module, it's not controlled by Quiz. I don't see any reason that just because we're reading whether or not they have passed we need to re-consider the definition of "passed".

          Perhaps we could re-consider it, but that discussion is orthogonal to using "passed" as a completion condition. If Activity Completion wants to make a change to make passed/failed retroactive upon changes to gradebook settings, Sam can do that and it's outside the scope of Quiz.

          Based on testing, the current definition of "passed" is "received a grade higher than the 'grade to pass' set at the time they received the grade". Changing "grade to pass" in gradebook does not change passed/failed status for people who already passed and failed.
          Do we actually have to discuss changing that longstanding definition before we can use the value? If so, that means recoding Activity Completion generally, because that's not handled by Quiz.

          Show
          Ray Morris added a comment - I retract my comment "sounds like a policy decision has been made". Based on Sam's description of the original intent and based on testing the currently distributed code, here's what I found to be the case, which I think is reasonable: Changes to what a specific STUDENT does, like adding and removing forum posts, can affect their completion. That's reasonable - if you remove the post, you're indicating that student doesn't have the required number of qualifying posts. Changes to the gradebook etc settings such as passing grade, do not retroactively cause students who had already passed to fail. That is, their completion state does not change to "complete_failed". Again, that's also reasonable - the student was already done, and nothing has changed with that student. Here, we're looking at adding "mark complete when they have passed". We already have an established definition of "passed". That definition is outside of any specific module, it's not controlled by Quiz. I don't see any reason that just because we're reading whether or not they have passed we need to re-consider the definition of "passed". Perhaps we could re-consider it, but that discussion is orthogonal to using "passed" as a completion condition. If Activity Completion wants to make a change to make passed/failed retroactive upon changes to gradebook settings, Sam can do that and it's outside the scope of Quiz. Based on testing, the current definition of "passed" is "received a grade higher than the 'grade to pass' set at the time they received the grade". Changing "grade to pass" in gradebook does not change passed/failed status for people who already passed and failed. Do we actually have to discuss changing that longstanding definition before we can use the value? If so, that means recoding Activity Completion generally, because that's not handled by Quiz.
          Hide
          Martin Dougiamas added a comment -

          Can someone please accurately define the policy decision to be made?

          Ideally a Moodle dev docs page with pros and cons for each possible decision. Then link to that from a new Policy issue. https://tracker.moodle.org/browse/MDL/component/12733

          Please also make that Policy issue a blocker for this one.

          Show
          Martin Dougiamas added a comment - Can someone please accurately define the policy decision to be made? Ideally a Moodle dev docs page with pros and cons for each possible decision. Then link to that from a new Policy issue. https://tracker.moodle.org/browse/MDL/component/12733 Please also make that Policy issue a blocker for this one.
          Hide
          Tim Hunt added a comment -

          Martin, it has been well explained both here and in the forum threads https://moodle.org/mod/forum/discuss.php?d=226129. Sam Marshall's comments above give a particularly clear summary.

          Show
          Tim Hunt added a comment - Martin, it has been well explained both here and in the forum threads https://moodle.org/mod/forum/discuss.php?d=226129 . Sam Marshall's comments above give a particularly clear summary.
          Hide
          Sam Marshall added a comment -

          Ray: The thing about changing the 'pass' grade threshold and it not updating is a bug in the current system. Under the current system where automatically calculated grades are dynamically recalculated when data changes, this logically ought to change the grades from complete-failed to complete-passed (if you reduce the grade so that the student now scored enough).

          If you do something that causes the system to automatically recalculate completion, it will change the state for these users.

          There are a number of settings in a similar position (i.e. logically, changing the setting should recalculate things, but because that was difficult to implement or we just didn't notice, it does not). This is probably a potential argument for changing the system, although there are issues on both sides and including the complete-passed/failed states complicates it further (suppose you can't automatically go from complete to incomplete, how about from complete-passed to failed, or from failed to passed).

          Show
          Sam Marshall added a comment - Ray: The thing about changing the 'pass' grade threshold and it not updating is a bug in the current system. Under the current system where automatically calculated grades are dynamically recalculated when data changes, this logically ought to change the grades from complete-failed to complete-passed (if you reduce the grade so that the student now scored enough). If you do something that causes the system to automatically recalculate completion, it will change the state for these users. There are a number of settings in a similar position (i.e. logically, changing the setting should recalculate things, but because that was difficult to implement or we just didn't notice, it does not). This is probably a potential argument for changing the system, although there are issues on both sides and including the complete-passed/failed states complicates it further (suppose you can't automatically go from complete to incomplete, how about from complete-passed to failed, or from failed to passed).
          Hide
          Ray Morris added a comment -
          Show
          Ray Morris added a comment - As Per Martin's request, this is the dev docs page: http://docs.moodle.org/dev/Policy_-_Retroactive_effects_of_completion_settings The policy issue is here: https://tracker.moodle.org/browse/MDL-39624
          Hide
          Ray Morris added a comment -

          Policy: should past completions by retroactively affected by settings changes?

          Show
          Ray Morris added a comment - Policy: should past completions by retroactively affected by settings changes?
          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
          Ray Morris added a comment -

          Sam said
          > "It could be argued ... but this would be a policy change".
          ...
          > I sort of assume that Martin or somebody from HQ has to make the decision!

          It's been a few months and I don't see any indication that anyone from HQ is even considering making this change. Can we not proceed with completion continuing to work as it always has? Are we permanently on hold because one day Martin might possibly make a change?

          Show
          Ray Morris added a comment - Sam said > "It could be argued ... but this would be a policy change". ... > I sort of assume that Martin or somebody from HQ has to make the decision! It's been a few months and I don't see any indication that anyone from HQ is even considering making this change. Can we not proceed with completion continuing to work as it always has? Are we permanently on hold because one day Martin might possibly make a change?
          Hide
          Ray Morris added a comment -

          This issue is no longer blocked pending MDL-37993 because MDL-37993 has been decided and closed.
          Activity completion shall be final and not affected by future changes to settings, Martin Dougiamas decided.
          This code will comply with that policy.

          I plan to review this code in light of the many changes to Quiz over the last 13 months, rebase, and request peer review be reopened.

          Show
          Ray Morris added a comment - This issue is no longer blocked pending MDL-37993 because MDL-37993 has been decided and closed. Activity completion shall be final and not affected by future changes to settings, Martin Dougiamas decided. This code will comply with that policy. I plan to review this code in light of the many changes to Quiz over the last 13 months, rebase, and request peer review be reopened.
          Hide
          Martin Dougiamas added a comment -

          Typo above: MDL-39624

          Show
          Martin Dougiamas added a comment - Typo above: MDL-39624
          Hide
          Sam Marshall added a comment -

          OK, I suppose it doesn't hurt to have one bit of code behaving inconsistently, if it is consistent with a new policy. However there will need to be much larger changes made to implement that new policy consistently. I have filed issue MDL-44878 to cover this development, with my analysis of what needs changing/how it should work.

          Show
          Sam Marshall added a comment - OK, I suppose it doesn't hurt to have one bit of code behaving inconsistently, if it is consistent with a new policy. However there will need to be much larger changes made to implement that new policy consistently. I have filed issue MDL-44878 to cover this development, with my analysis of what needs changing/how it should work.

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated: