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

Quiz completion options - passing grade, all attempts used

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.8
    • 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 passing grade" and "all available attempts completed"
      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.
      9.  As s1, complete the quiz and get q1 right, and q2 wrong 50%. Verify that the quiz is not marked complete.
      10. 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.
      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 passing grade" and "all available attempts completed" 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.
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_28_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-37993-quiz-completion-pass-attempts

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            raymor 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
            raymor 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
            timhunt 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
            timhunt 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
            raymor 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
            raymor 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
            timhunt 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
            timhunt 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
            raymor 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
            raymor 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
            timhunt 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
            timhunt 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
            raymor 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
            raymor 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
            timhunt 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
            timhunt 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
            raymor 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
            raymor 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
            timhunt 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
            timhunt 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
            raymor 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
            raymor 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
            timhunt 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
            timhunt 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
            raymor 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
            raymor 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
            timhunt 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
            timhunt 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
            timhunt 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
            timhunt 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
            timhunt 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
            timhunt 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
            raymor 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
            raymor 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
            timhunt 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
            timhunt 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
            raymor 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
            raymor 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
            quen 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
            quen 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
            quen 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
            quen 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
            raymor 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
            raymor 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
            quen 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
            quen 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
            raymor 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
            raymor 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
            dougiamas 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
            dougiamas 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
            timhunt 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
            timhunt 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
            quen 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
            quen 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
            raymor Ray Morris added a comment -
            Show
            raymor 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
            raymor Ray Morris added a comment -

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

            Show
            raymor Ray Morris added a comment - Policy: should past completions by retroactively affected by settings changes?
            Hide
            timhunt Tim Hunt added a comment -

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

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

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

            Typo above: MDL-39624

            Show
            dougiamas Martin Dougiamas added a comment - Typo above: MDL-39624
            Hide
            quen 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
            quen 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.
            Hide
            raymor Ray Morris added a comment -

            The code has been rebased and I've run through the testing instructions again.

            Show
            raymor Ray Morris added a comment - The code has been rebased and I've run through the testing instructions again.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-37993

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-37993 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-37993 -quiz-completion-pass-attempts to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3654 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3654/artifact/work/smurf.html
            Hide
            cibot CiBoT added a comment -

            Results for MDL-37993

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-37993 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-37993 -quiz-completion-pass-attempts to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/3655 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/3655/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            Thanks for coming back to this Ray, but I'm afraid this code is not right yet:

            1. You nee to follow the coding style everywhere, not just the things CiBoT notices.
            2. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-60e3ef4888f88976f619dd788dff385aR1827 - you should use MUST_EXIST here, rather than an if.
            3. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-875f0a44b3001f70eba77a8c8ec5a3e8R47 - is there a good reason why the two columns are defined differently?
            4. The two if blocks in upgrade.php must have different version numbers (00 and 01).
            5. get_context_instance is deprecated.
            6. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-68b0d1eefe47af1ab1ce7e0b874d703bR406 - given the agreen policy, why do we need code when an attempt is deleted?
            7. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-7bdec0b0e66c672fa95df0369e3c839eR599 - =& has not been necessary with objects since we moved to PHP5.
            8. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-7bdec0b0e66c672fa95df0369e3c839eR620 - why is one line !empty, and the other empty?
            9. Is it not feasible to write any unit tests for any of this code?
            Show
            timhunt Tim Hunt added a comment - Thanks for coming back to this Ray, but I'm afraid this code is not right yet: You nee to follow the coding style everywhere, not just the things CiBoT notices. Follow-on lines are indented 8 spaces, not 4. E.g. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-60e3ef4888f88976f619dd788dff385aR1839 but there are more. Should be no space inside the brackets, in an if () or other control strucutre. E.g. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-68b0d1eefe47af1ab1ce7e0b874d703bR1671 No spaces inside brackets in expressions. E.g. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-60e3ef4888f88976f619dd788dff385aR1855 but acutally those brackets should just be removed. No space after ! operator. E.g. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-60e3ef4888f88976f619dd788dff385aR1833 https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-60e3ef4888f88976f619dd788dff385aR1827 - you should use MUST_EXIST here, rather than an if. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-875f0a44b3001f70eba77a8c8ec5a3e8R47 - is there a good reason why the two columns are defined differently? The two if blocks in upgrade.php must have different version numbers (00 and 01). get_context_instance is deprecated. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-68b0d1eefe47af1ab1ce7e0b874d703bR406 - given the agreen policy, why do we need code when an attempt is deleted? https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-7bdec0b0e66c672fa95df0369e3c839eR599 - =& has not been necessary with objects since we moved to PHP5. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-7bdec0b0e66c672fa95df0369e3c839eR620 - why is one line !empty, and the other empty? Is it not feasible to write any unit tests for any of this code?
            Hide
            raymor Ray Morris added a comment -

            Wow Tim, you spotted a lot of stuff. I'm surprised and embarrassed. I was thinking that type of stuff had already been handled months ago. It seems I remembered wrong. I apologize for sending junk over for you to review. I'll take care of the issues right away. I may need to find a little help with unit tests, as I'm just now learning our two test suites.

            Show
            raymor Ray Morris added a comment - Wow Tim, you spotted a lot of stuff. I'm surprised and embarrassed. I was thinking that type of stuff had already been handled months ago. It seems I remembered wrong. I apologize for sending junk over for you to review. I'll take care of the issues right away. I may need to find a little help with unit tests, as I'm just now learning our two test suites.
            Hide
            timhunt Tim Hunt added a comment -

            Yes. I was surprised too. Oh well. Give me a yell if you want a hand with anything.

            Show
            timhunt Tim Hunt added a comment - Yes. I was surprised too. Oh well. Give me a yell if you want a hand with anything.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-37993

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-37993 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-37993 -quiz-completion-pass-attempts to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4137 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4137/artifact/work/smurf.html
            Hide
            raymor Ray Morris added a comment - - edited

            Unit tests have been added and everything Tim mentioned in https://tracker.moodle.org/browse/MDL-37993?focusedCommentId=292547&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-292547 has been handled. Also the logic has been slightly clarified.

            These are my very first from-scratch Behat tests, so it's possible some portion of them could be done better. (The "better" ways I saw all required javascript, which the feature itself does not require).

            Show
            raymor Ray Morris added a comment - - edited Unit tests have been added and everything Tim mentioned in https://tracker.moodle.org/browse/MDL-37993?focusedCommentId=292547&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-292547 has been handled. Also the logic has been slightly clarified. These are my very first from-scratch Behat tests, so it's possible some portion of them could be done better. (The "better" ways I saw all required javascript, which the feature itself does not require).
            Hide
            cibot CiBoT added a comment -

            Results for MDL-37993

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-37993 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-37993 -quiz-completion-pass-attempts to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4138 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4138/artifact/work/smurf.html
            Hide
            cibot CiBoT added a comment -

            Results for MDL-37993

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-37993 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-37993 -quiz-completion-pass-attempts to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4139 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4139/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            I think we are good to go. Thanks Ray.

            Show
            timhunt Tim Hunt added a comment - I think we are good to go. Thanks Ray.
            Hide
            cibot CiBoT added a comment -

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

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

            Thanks for working on this - it looks like it will be useful.

            I spotted a few problems that need fixing:

            1. has_capability('mod/quiz:ignoretimelimits', $context, null, false) in quiz_get_completion_state() - this will change the output depending on the user who triggers this function - this could be called by the student who attempted the quiz, or the manager who reset the completion data for the course - it should be passing the $userid

            2. $grade->grade >= $gradetopass - this is comparing the wrong thing. The gradebook could be setup to scale the grade etc. This really needs to call the grade_grade->is_passed function instead of trying to work it out.

            3. This is subjective - but I think using a group in the form for these settings makes it more confusing. IMO it would look better as 2 separate settings.

            4. I really don't like the javascript for the onChange attribute to the quickform element. It's barely valid syntax. (I think disabling the field is enough anyway).

            5. Maybe the second rule should be disabled if the allowed attempts field is set to unlimited (if this change is made, the help should mention it).

            6. You should bump the quiz version when you add upgrade steps.

            There are a few issues here so I'll reopen this one this week.

            Thanks!

            Show
            damyon Damyon Wiese added a comment - Thanks for working on this - it looks like it will be useful. I spotted a few problems that need fixing: 1. has_capability('mod/quiz:ignoretimelimits', $context, null, false) in quiz_get_completion_state() - this will change the output depending on the user who triggers this function - this could be called by the student who attempted the quiz, or the manager who reset the completion data for the course - it should be passing the $userid 2. $grade->grade >= $gradetopass - this is comparing the wrong thing. The gradebook could be setup to scale the grade etc. This really needs to call the grade_grade->is_passed function instead of trying to work it out. 3. This is subjective - but I think using a group in the form for these settings makes it more confusing. IMO it would look better as 2 separate settings. 4. I really don't like the javascript for the onChange attribute to the quickform element. It's barely valid syntax. (I think disabling the field is enough anyway). 5. Maybe the second rule should be disabled if the allowed attempts field is set to unlimited (if this change is made, the help should mention it). 6. You should bump the quiz version when you add upgrade steps. There are a few issues here so I'll reopen this one this week. Thanks!
            Hide
            cibot CiBoT added a comment -

            Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

            Show
            cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
            Hide
            raymor Ray Morris added a comment - - edited

            Damyon, I will take care of those things. #1 and #3 I'm not sure about.

            1. I'm not sure of this. Would changing that mean that manager's action of resetting the data would be prevented because the student isn't allowed to start a new attempt at this time? Can anyone else chime in on this (Tim?).

            3. That is indeed subjective. It was decided to use the UI we did because the two naturally go together as either they passed OR they aren't going to pass (they are finished either way). That's an OR, whereas other conditions are combined with AND, so separating them would make the UI confusing. Tim and I discussed this and agreed the current choice is best, at least until MDL-37899 is resolved. I promise to take another look at this after MDL-37899 is done.

            Show
            raymor Ray Morris added a comment - - edited Damyon, I will take care of those things. #1 and #3 I'm not sure about. 1. I'm not sure of this. Would changing that mean that manager's action of resetting the data would be prevented because the student isn't allowed to start a new attempt at this time? Can anyone else chime in on this (Tim?). 3. That is indeed subjective. It was decided to use the UI we did because the two naturally go together as either they passed OR they aren't going to pass (they are finished either way). That's an OR, whereas other conditions are combined with AND, so separating them would make the UI confusing. Tim and I discussed this and agreed the current choice is best, at least until MDL-37899 is resolved. I promise to take another look at this after MDL-37899 is done.
            Hide
            raymor Ray Morris added a comment -

            The points mentioned by Damyon have now been addressed (with the exception of the two discussed above). Thanks Damyon.

            Show
            raymor Ray Morris added a comment - The points mentioned by Damyon have now been addressed (with the exception of the two discussed above). Thanks Damyon.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-37993

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-37993 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-37993 -quiz-completion-pass-attempts to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4473 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4473/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            Sorry, this is still not there. (Though I still think it would be a nice feature to get implemented.)

            1. Damyon was right. This needs to be fixed.

            3. Ray is right. See above discussion.

            5. I disagree with Damyon. We implemented OR logic for a reason, because either setting on its own is useful, as well as the combination. Please can you undo the change Damyon requested.

            (I should probably find a less confrontational way to phrase those, but it is getting late.)

            New points:

            7. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-60e3ef4888f88976f619dd788dff385aR1810 - this variable is never used. Please remove.

            8. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-60e3ef4888f88976f619dd788dff385aR1842 - These two lines of code may be right, or they may be wrong. They are certainly completely incomprehensible. Please can you rewrite them so that they are self-evidently correct, and it is clearl what they are doing.

            Oh, I see (after about 10 minutes, and asking Sam Marshall who did not help, but who was a good rubber duck). It would be clearer if, at the start of the function (after getting $quiz) you did.

            if (!$quiz->completionattemptsexhausted && !$quiz->completionpass) {
                // No custom rules.
                return $type;
            }
            

            Then instead of the $attemptsused local variable, and $attemptsused = 1;, you can just return true; there.

            Same to eliminate the $passed local variable. (Basically I prefer guard clauses http://c2.com/cgi/wiki?GuardClause. I find they make the code easier to reason aboue.)


            Please note, I am on leave Friday 4th June, though I might check my email anyway, and so be able to advance this if it is in a state where it is obviously right for integration.

            Then I am hear next week, then I am on leave for two weeks (12th to 26th July) and I intend to actually go offline for those two weeks. Hopefully you are abot to work-around that.

            Show
            timhunt Tim Hunt added a comment - Sorry, this is still not there. (Though I still think it would be a nice feature to get implemented.) 1. Damyon was right. This needs to be fixed. 3. Ray is right. See above discussion. 5. I disagree with Damyon. We implemented OR logic for a reason, because either setting on its own is useful, as well as the combination. Please can you undo the change Damyon requested. (I should probably find a less confrontational way to phrase those, but it is getting late.) New points: 7. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-60e3ef4888f88976f619dd788dff385aR1810 - this variable is never used. Please remove. 8. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-60e3ef4888f88976f619dd788dff385aR1842 - These two lines of code may be right, or they may be wrong. They are certainly completely incomprehensible. Please can you rewrite them so that they are self-evidently correct, and it is clearl what they are doing. Oh, I see (after about 10 minutes, and asking Sam Marshall who did not help, but who was a good rubber duck). It would be clearer if, at the start of the function (after getting $quiz) you did. if (!$quiz->completionattemptsexhausted && !$quiz->completionpass) { // No custom rules. return $type; } Then instead of the $attemptsused local variable, and $attemptsused = 1;, you can just return true; there. Same to eliminate the $passed local variable. (Basically I prefer guard clauses http://c2.com/cgi/wiki?GuardClause . I find they make the code easier to reason aboue.) Please note, I am on leave Friday 4th June, though I might check my email anyway, and so be able to advance this if it is in a state where it is obviously right for integration. Then I am hear next week, then I am on leave for two weeks (12th to 26th July) and I intend to actually go offline for those two weeks. Hopefully you are abot to work-around that.
            Hide
            raymor Ray Morris added a comment -

            Thanks for all of the suggestions, which I've now applied. I will also apply most of them to mod_scorm, from whence the original version of this code was borrowed.

            Thanks for chiming in, Tim. Enjoy your July 4th, "Got Rid of the Americans" holiday. I for one appreciate the clarity of "Damyon was right. This needs to be fixed." The wording leaves no room for confusion.

            I noticed that our current GUI enforces the rule that the condition on line #1832, if ($quiz->completionpass), should always be true if execution reaches that line. Still, to avoid future bugs I'm leaning toward leaving the line in until the new completion condition UI lands with support for logic trees.

            Show
            raymor Ray Morris added a comment - Thanks for all of the suggestions, which I've now applied. I will also apply most of them to mod_scorm, from whence the original version of this code was borrowed. Thanks for chiming in, Tim. Enjoy your July 4th, "Got Rid of the Americans" holiday. I for one appreciate the clarity of "Damyon was right. This needs to be fixed." The wording leaves no room for confusion. I noticed that our current GUI enforces the rule that the condition on line #1832, if ($quiz->completionpass), should always be true if execution reaches that line. Still, to avoid future bugs I'm leaning toward leaving the line in until the new completion condition UI lands with support for logic trees.
            Hide
            damyon Damyon Wiese added a comment -

            Everything Tim said is fine by me - so when those changes are done this has my +1 too.

            Show
            damyon Damyon Wiese added a comment - Everything Tim said is fine by me - so when those changes are done this has my +1 too.
            Hide
            timhunt Tim Hunt added a comment -

            For the record, it is a 'Go to Yorkshire to watch the Tour de France, and have a family party' holiday.

            Show
            timhunt Tim Hunt added a comment - For the record, it is a 'Go to Yorkshire to watch the Tour de France, and have a family party' holiday.
            Hide
            cibot CiBoT added a comment -

            Results for MDL-37993

            • Remote repository: git://github.com/MorrisR2/moodle.git
            Show
            cibot CiBoT added a comment - Results for MDL-37993 Remote repository: git://github.com/MorrisR2/moodle.git Remote branch MDL-37993 -quiz-completion-pass-attempts to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/4585 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/4585/artifact/work/smurf.html
            Hide
            timhunt Tim Hunt added a comment -

            The has_capability call is still wrong. What matters here is whether the user who made this quiz attempt is exempt from the time-limit. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-60e3ef4888f88976f619dd788dff385aR1823

            Show
            timhunt Tim Hunt added a comment - The has_capability call is still wrong. What matters here is whether the user who made this quiz attempt is exempt from the time-limit. https://github.com/MorrisR2/moodle/compare/master...MDL-37993-quiz-completion-pass-attempts#diff-60e3ef4888f88976f619dd788dff385aR1823
            Hide
            raymor Ray Morris added a comment - - edited

            The has_capability() call has been corrected. Somehow that change got lost and didn't make it in the previous push.

            Show
            raymor Ray Morris added a comment - - edited The has_capability() call has been corrected. Somehow that change got lost and didn't make it in the previous push.
            Hide
            timhunt Tim Hunt added a comment -

            Yay! we got there at last. (& again )

            Show
            timhunt Tim Hunt added a comment - Yay! we got there at last. (& again )
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

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

            TIA and ciao

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

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

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

            Hello Ray,

            Sam asked me to look at this for behat. It will be nice if you can fix the following as well:

            1. As per coding guidelines after there should be only 1 Given and 1 when. http://docs.moodle.org/dev/Acceptance_testing#Writing_features (Point 4)
            2. You are using deprecated step "And I select "hiddenuntil" from "Advanced grade item options", Please replace this with

              And I set the field "Advanced grade item options" to "hiddenuntil"
              

            3. We discourage xpath use, as they are hard to decipher, but as you are looking for image without link, xpath is the only way to do this. Would be nice to replace it with following to make it more readable.

              And "//img[contains(@alt, 'Not completed: Test quiz name')]" "xpath_element" should exist in the "li.modtype_quiz" "css_element"
              And "//img[contains(@title,'Test quiz name') and @alt='Completed']" "xpath_element" should exist in the "Student 1" "table_row"
              

            Show
            rajeshtaneja Rajesh Taneja added a comment - - edited Hello Ray, Sam asked me to look at this for behat. It will be nice if you can fix the following as well: As per coding guidelines after there should be only 1 Given and 1 when. http://docs.moodle.org/dev/Acceptance_testing#Writing_features (Point 4) You are using deprecated step "And I select "hiddenuntil" from "Advanced grade item options", Please replace this with And I set the field "Advanced grade item options" to "hiddenuntil" We discourage xpath use, as they are hard to decipher, but as you are looking for image without link, xpath is the only way to do this. Would be nice to replace it with following to make it more readable. And "//img[contains(@alt, 'Not completed: Test quiz name')]" "xpath_element" should exist in the "li.modtype_quiz" "css_element" And "//img[contains(@title,'Test quiz name') and @alt='Completed']" "xpath_element" should exist in the "Student 1" "table_row"
            Hide
            raymor Ray Morris added a comment -

            The Behat tests have been updated in accordance with the comments Rajesh made.

            Show
            raymor Ray Morris added a comment - The Behat tests have been updated in accordance with the comments Rajesh made.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks for looking at those tests Raj and for fixing them up Ray.

            This has been integrated now. I did make an additional commit 3c94f90 as there was one remaining deprecated step in use that I fixed up.

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks for looking at those tests Raj and for fixing them up Ray. This has been integrated now. I did make an additional commit 3c94f90 as there was one remaining deprecated step in use that I fixed up.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Ray

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Ray
            Hide
            salvetore Michael de Raadt added a comment -

            I ran into an error after I enabled the new completion setting.

            Notice: Undefined offset: 4 in D:\xampp\htdocs\master_integration\mod\quiz\lib.php on line 1838
             
            Fatal error: Call to a member function is_passed() on a non-object in D:\xampp\htdocs\master_integration\mod\quiz\lib.php on line 1838
            

            I also thought the placement of the second checkbox was confusing. It abuts the first part of the condition so that it is unclear that it is related to the second. I will attach a screenshot.

            Show
            salvetore Michael de Raadt added a comment - I ran into an error after I enabled the new completion setting. Notice: Undefined offset: 4 in D:\xampp\htdocs\master_integration\mod\quiz\lib.php on line 1838   Fatal error: Call to a member function is_passed() on a non-object in D:\xampp\htdocs\master_integration\mod\quiz\lib.php on line 1838 I also thought the placement of the second checkbox was confusing. It abuts the first part of the condition so that it is unclear that it is related to the second. I will attach a screenshot.
            Hide
            raymor Ray Morris added a comment -

            Michael de Raadt the issues you brought up have been handled, thank you.

            Show
            raymor Ray Morris added a comment - Michael de Raadt the issues you brought up have been handled, thank you.
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks, Ray.

            Hopefully this can get back up for testing today, otherwise we can have another go next week.

            Over to Sam.

            Show
            salvetore Michael de Raadt added a comment - Thanks, Ray. Hopefully this can get back up for testing today, otherwise we can have another go next week. Over to Sam.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Sending back to testing as it sounds as thought these have been fixed

            Show
            samhemelryk Sam Hemelryk added a comment - Sending back to testing as it sounds as thought these have been fixed
            Hide
            salvetore Michael de Raadt added a comment -

            Restarting testing...

            Show
            salvetore Michael de Raadt added a comment - Restarting testing...
            Hide
            salvetore Michael de Raadt added a comment -

            Test result: Success!

            Show
            salvetore Michael de Raadt added a comment - Test result: Success!
            Hide
            poltawski Dan Poltawski added a comment -

            Thanks for your hard work - this issue is now part of Moodle!

            A good decision is based on knowledge and not on numbers.

            – Plato

            Show
            poltawski Dan Poltawski added a comment - Thanks for your hard work - this issue is now part of Moodle! A good decision is based on knowledge and not on numbers. – Plato
            Hide
            marycooch Mary Cooch added a comment -
            Show
            marycooch Mary Cooch added a comment - Just noting that these two new settings have been documented in https://docs.moodle.org/28/en/Activity_completion_settings and https://docs.moodle.org/28/en/Quiz_settings#Quiz_completion

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  10/Nov/14