Moodle
  1. Moodle
  2. MDL-29854

Better exception handling when processing question responses

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1.2, 2.2
    • Fix Version/s: 2.1.3
    • Component/s: Questions, Quiz
    • Labels:
    • Rank:
      19379

      Description

      They may be many questions on a page, and if processing the response to one question causes an exception, it should not prevent the other responses being processed.

        Activity

        Hide
        Tim Hunt added a comment -

        Plan for implementing this:

        1. Introduce a new aggregate question_response_processing_exception class that can store multiple other exceptions.
        2. Change the question processing, so that if processing any of the question on a page throws an exception, then all the exceptions are captured in a question_response_processing_exception, but the results of processing the other questions are saved.
        3. If that gets thrown, make the quiz handle it by displaying a nice page to the student explaining the problem (after saving the update $quba).
        4. Think about whether it is possible to add a 'Try processing again' button on that page. (Might be possible with a lot of hidden form fields.)
        5. And make sure it is handled OK by question preview.
        Show
        Tim Hunt added a comment - Plan for implementing this: Introduce a new aggregate question_response_processing_exception class that can store multiple other exceptions. Change the question processing, so that if processing any of the question on a page throws an exception, then all the exceptions are captured in a question_response_processing_exception, but the results of processing the other questions are saved. If that gets thrown, make the quiz handle it by displaying a nice page to the student explaining the problem (after saving the update $quba). Think about whether it is possible to add a 'Try processing again' button on that page. (Might be possible with a lot of hidden form fields.) And make sure it is handled OK by question preview.
        Hide
        Tim Hunt added a comment -

        There is, of course, a potential nasty interaction with question_out_of_sequence_exceptions. There is no point re-trying them. No, it all works out OK.

        Show
        Tim Hunt added a comment - There is, of course, a potential nasty interaction with question_out_of_sequence_exceptions. There is no point re-trying them. No, it all works out OK.
        Hide
        Tim Hunt added a comment -

        We do, however, also need to deal with the ->finish_all_questions() method.

        Show
        Tim Hunt added a comment - We do, however, also need to deal with the ->finish_all_questions() method.
        Hide
        Tim Hunt added a comment -

        What about retrying submission including files?

        Show
        Tim Hunt added a comment - What about retrying submission including files?
        Hide
        Tim Hunt added a comment -

        Right, I think this code does what I want, but it does seem to add a lot of spaghetti in the name of better error handling.

        Show
        Tim Hunt added a comment - Right, I think this code does what I want, but it does seem to add a lot of spaghetti in the name of better error handling.
        Hide
        Dan Poltawski added a comment -

        I can't work out how to make a comment in the github interface...

        I think there is a missing end of a comment on line 694 of question/engine/lib.php

        Show
        Dan Poltawski added a comment - I can't work out how to make a comment in the github interface... I think there is a missing end of a comment on line 694 of question/engine/lib.php
        Hide
        Tim Hunt added a comment -

        Thanks dan. Commit amended.

        Show
        Tim Hunt added a comment - Thanks dan. Commit amended.
        Hide
        Tim Hunt added a comment -

        Submitting for integration. Fingers crossed.

        Show
        Tim Hunt added a comment - Submitting for integration. Fingers crossed.
        Hide
        Aparup Banerjee added a comment -

        Hi Tim, i've had a look through this:

        • mod/quiz/lang/en/quiz/php line 183 & 632 (minor note): the strings there are a partial strings. i suppose that alright for an english developer? but would this be really difficult for translators to handle? the context and meanings could become so convoluted. But i see its been done elsewhere hence just this minor note.
        • mod/quiz/processattempt.php lines 112-116 : just wondering if this contextual information around the question_procesing_exception required for recontructing the attempt would be better stored in the exception ?
        • mod/quiz/renderer.php :
          line 522 : phpdoc variable typo $qpa
          line 534 : perhaps the public function's parameter '$retryurl' should default to null?
        • question/engine/questionusage:
          line 531 : just a tad towards readability difficulty

        In overall, i'm not sure about this idea but so I'll just wait for some more (expert) feedback here first (Eloy had something to say to I heard). The whole need for the exception storing concept seems to indicate to me that there is a wide gray area between actual exceptions that need exceptional handling and normal program flow control.

        cheers,
        Apu

        Show
        Aparup Banerjee added a comment - Hi Tim, i've had a look through this: mod/quiz/lang/en/quiz/php line 183 & 632 (minor note): the strings there are a partial strings. i suppose that alright for an english developer? but would this be really difficult for translators to handle? the context and meanings could become so convoluted. But i see its been done elsewhere hence just this minor note. mod/quiz/processattempt.php lines 112-116 : just wondering if this contextual information around the question_procesing_exception required for recontructing the attempt would be better stored in the exception ? mod/quiz/renderer.php : line 522 : phpdoc variable typo $qpa line 534 : perhaps the public function's parameter '$retryurl' should default to null? question/engine/questionusage: line 531 : just a tad towards readability difficulty In overall, i'm not sure about this idea but so I'll just wait for some more (expert) feedback here first (Eloy had something to say to I heard). The whole need for the exception storing concept seems to indicate to me that there is a wide gray area between actual exceptions that need exceptional handling and normal program flow control. cheers, Apu
        Hide
        Pierre Pichet added a comment -

        I am not an expert, far from it but effectively I agree with
        Aparup comment
        "actual exceptions that need exceptional handling and normal program flow control"
        if we look at the following strings.

        $string['errormissingquestion'] = 'Error: The system is missing the question with id {$a}';
        277 278
        $string['errornotnumbers'] = 'Error - answers must be numeric';
        279

        However I think that Tim idea is a good one.

        As a further example, some questiontypes are more prone to errors, calculated being probably the most sensitive either the numerical values can give a bad numerical response i.e. the calculated formula give a bad value as divided by zero or the datasets data cannot be retrieved.

        Show
        Pierre Pichet added a comment - I am not an expert, far from it but effectively I agree with Aparup comment "actual exceptions that need exceptional handling and normal program flow control" if we look at the following strings. $string ['errormissingquestion'] = 'Error: The system is missing the question with id {$a}'; 277 278 $string ['errornotnumbers'] = 'Error - answers must be numeric'; 279 However I think that Tim idea is a good one. As a further example, some questiontypes are more prone to errors, calculated being probably the most sensitive either the numerical values can give a bad numerical response i.e. the calculated formula give a bad value as divided by zero or the datasets data cannot be retrieved.
        Hide
        Aparup Banerjee added a comment - - edited

        Thanks for the feedback , i wasn't trying to preclude anyone here lol

        I understand that this is a good way to deal with the errors for now (being near to freeze/release), its just that this sort of direction could lead to a lot more , as Tim said, spaghetti. maybe there should be a issue ready somewhere (for 2.3) to re-look into this.

        The exceptions are really for unforeseen situations in general and so they're there to broadly classify them and handle them generally. If we already know what could go wrong (with more analysis), then really we should be validating those inputs more (yep not easy). I realize that in the case of quiz, we likely don't want to be too friendly with GUI feedback by correcting inputs with validations etc (maybe validate but accept?). ultimately, improving the validations is definitely a step towards security (and accessibilty) and using the exceptions as control flow is a step toward lesser security but more broader range of 'unplanned' for inputs. Perhaps an issue to shift dealing with the complexity of handling these inputs from exceptions --> validations ?

        Show
        Aparup Banerjee added a comment - - edited Thanks for the feedback , i wasn't trying to preclude anyone here lol I understand that this is a good way to deal with the errors for now (being near to freeze/release), its just that this sort of direction could lead to a lot more , as Tim said, spaghetti. maybe there should be a issue ready somewhere (for 2.3) to re-look into this. The exceptions are really for unforeseen situations in general and so they're there to broadly classify them and handle them generally. If we already know what could go wrong (with more analysis), then really we should be validating those inputs more (yep not easy). I realize that in the case of quiz, we likely don't want to be too friendly with GUI feedback by correcting inputs with validations etc (maybe validate but accept?). ultimately, improving the validations is definitely a step towards security (and accessibilty) and using the exceptions as control flow is a step toward lesser security but more broader range of 'unplanned' for inputs. Perhaps an issue to shift dealing with the complexity of handling these inputs from exceptions --> validations ?
        Hide
        Tim Hunt added a comment -

        I will fix some of Aparup's comments now.

        Show
        Tim Hunt added a comment - I will fix some of Aparup's comments now.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Just for reference, we've been discussing this long @ HQ chat, and here there are some final notes (feel free to add/modify whatever):

        Link (interesting): http://moodle.org/mod/cvsadmin/view.php?conversationid=8645#c306606

        Possible solutions (and my votes about them as integrator):

        0) or this is rejected. This gets my +1

        1) or you change from that "abused-exceptions" to error-handling. This gets my +100

        2) or we accept your code. You are the maintainer and you know what are you doing (although later don't blame if your code becomes incompatible with future developments/handlings). This gets my -1. So you will need to convince another integrator to neutralize my vote.

        3) or the missing "interim" UNPROCESSED status gets implemented if possible, seems it would, at the same time, fix other problems. I don't know much about this but trust Tim 100% if he thinks is a viable way. This gets my +100 too.

        That is, thanks! Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Just for reference, we've been discussing this long @ HQ chat, and here there are some final notes (feel free to add/modify whatever): Link (interesting): http://moodle.org/mod/cvsadmin/view.php?conversationid=8645#c306606 Possible solutions (and my votes about them as integrator): 0) or this is rejected. This gets my +1 1) or you change from that "abused-exceptions" to error-handling. This gets my +100 2) or we accept your code. You are the maintainer and you know what are you doing (although later don't blame if your code becomes incompatible with future developments/handlings). This gets my -1. So you will need to convince another integrator to neutralize my vote. 3) or the missing "interim" UNPROCESSED status gets implemented if possible, seems it would, at the same time, fix other problems. I don't know much about this but trust Tim 100% if he thinks is a viable way. This gets my +100 too. That is, thanks! Ciao
        Hide
        Tim Hunt added a comment -

        OK, I will re-work this to do basically 0). I will just change the quiz to display a better explanation to students when any question processing fails. (That is, no data will be saved.)

        This is OK for the OU, because we normally use one question per page.

        Show
        Tim Hunt added a comment - OK, I will re-work this to do basically 0). I will just change the quiz to display a better explanation to students when any question processing fails. (That is, no data will be saved.) This is OK for the OU, because we normally use one question per page.
        Hide
        Tim Hunt added a comment -

        Is this patch any better?

        Show
        Tim Hunt added a comment - Is this patch any better?
        Hide
        Pierre Pichet added a comment - - edited

        I read th HQ discussion on exception handling and spaghetti cooking.

        However your first comment in the bug description is somehow surprising

        "Note, it is actually quite hard to get any of the standard question types to throw an exception. I have been testing this with ..(OU questiontypes)....which makes triggering exceptions easy".

        Can we conclude that these question types are not as stable as they should ?

        Most important problems are those generated when creating the question from the database data (lost, double records etc.).

        The student response should not create any bug or exception whatever he typed in the response field apart session handling (timing).
        Unless the saving attempt data ( i.e. the upper-lower case of calculated parameter) or the rebuilding of the adaptative question can generate something that cannot be handled without exception?

        Show
        Pierre Pichet added a comment - - edited I read th HQ discussion on exception handling and spaghetti cooking. However your first comment in the bug description is somehow surprising "Note, it is actually quite hard to get any of the standard question types to throw an exception. I have been testing this with ..(OU questiontypes)....which makes triggering exceptions easy". Can we conclude that these question types are not as stable as they should ? Most important problems are those generated when creating the question from the database data (lost, double records etc.). The student response should not create any bug or exception whatever he typed in the response field apart session handling (timing). Unless the saving attempt data ( i.e. the upper-lower case of calculated parameter) or the rebuilding of the adaptative question can generate something that cannot be handled without exception?
        Hide
        Tim Hunt added a comment -

        The one question type we have that can throw exceptions when processing student responses uses a SOAP web-service to delegate processing to another server. This can fail if the other server, or the network, is down.

        Show
        Tim Hunt added a comment - The one question type we have that can throw exceptions when processing student responses uses a SOAP web-service to delegate processing to another server. This can fail if the other server, or the network, is down.
        Hide
        Pierre Pichet added a comment -

        This is a clear case of legitimate exception use

        Your message
        "$string['errorprocessingresponses'] = 'An error occurred while processing your responses ({$a}). Click continue to return to the page you were on and try again.'"
        is effectively a a"more useful continue".

        Given that quiz can be grading items, any improvment on their stability is useful.

        Show
        Pierre Pichet added a comment - This is a clear case of legitimate exception use Your message "$string ['errorprocessingresponses'] = 'An error occurred while processing your responses ({$a}). Click continue to return to the page you were on and try again.'" is effectively a a"more useful continue". Given that quiz can be grading items, any improvment on their stability is useful.
        Hide
        Aparup Banerjee added a comment -

        Hi all,
        this patch has been integrated now is up for testing.

        ps:
        its been integrated only into master as its an improvement and theres only master branch.
        Tim, do we want to back-port this? This patch seems viable to be back-ported to me.

        Show
        Aparup Banerjee added a comment - Hi all, this patch has been integrated now is up for testing. ps: its been integrated only into master as its an improvement and theres only master branch. Tim, do we want to back-port this? This patch seems viable to be back-ported to me.
        Hide
        Tim Hunt added a comment -

        I did not think of that. The original planned change was much bigger, but you are right, this much simpler approach is suitable for back-porting. That would be very helpful for me, we could use it much sooner at the OU if it got into 2.1 branch.

        Please can you try cherry-picking to 2.1. Thanks.

        Show
        Tim Hunt added a comment - I did not think of that. The original planned change was much bigger, but you are right, this much simpler approach is suitable for back-porting. That would be very helpful for me, we could use it much sooner at the OU if it got into 2.1 branch. Please can you try cherry-picking to 2.1. Thanks.
        Hide
        Aparup Banerjee added a comment -

        picked and pushed :-D (Please test on 2.1 also)

        Show
        Aparup Banerjee added a comment - picked and pushed :-D (Please test on 2.1 also)
        Hide
        Aparup Banerjee added a comment -

        updated affects & fix versions

        Show
        Aparup Banerjee added a comment - updated affects & fix versions
        Hide
        Tim Hunt added a comment -

        Thanks Aparup.

        Show
        Tim Hunt added a comment - Thanks Aparup.
        Hide
        Rossiani Wijaya added a comment -

        Thank Apu for quickly integrating to 2.1.

        This is working great.

        Test passed.

        Show
        Rossiani Wijaya added a comment - Thank Apu for quickly integrating to 2.1. This is working great. Test passed.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Done, your delicious hacks have been sent upstream, many thanks!

        Closing as fixed, ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Done, your delicious hacks have been sent upstream, many thanks! Closing as fixed, ciao

          People

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

            Dates

            • Created:
              Updated:
              Resolved: