Moodle
  1. Moodle
  2. MDL-28655

Incorrect type of return value in qtype_calculated_qe2_attempt_updater

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1, 2.2
    • Fix Version/s: 2.1.2
    • Component/s: Questions
    • Labels:
    • Rank:
      18436

      Description

      Hello Tim

      while debugging my 1.9 -> 2.1 data migration, I noticed that under certain circumstances, qtype_calculated_qe2_attempt_updater::parse_response() returns an incorrect datatype (empty string, where an array of two strings is expected).

      This probably has no functional impact, however in debug mode it leads to numerous warning messages being triggered in the first line of response_summary() (same class). I found that a bit annoying, so I though I'd share it.

      The fix is trivial, see patch.

        Activity

        Hide
        Tim Hunt added a comment -

        Thanks for another useful fix.

        Since you are using git, would you like to think about getting a public git repository (like https://github.com/timhunt/moodle) and sharing your bug-fixes by pushing your changes as branches there, like I did with your fix to MDL-28652?

        If you could do that, it would make my life a lot easier. Therefore, if you need help getting started with that, I am happy to provide it.

        Once you do that, you should then click the Request peer review button at the top, and fill in all the branch information. Then, if all is well, I don't have to do all that to submit your change for integration.

        Doh! at the moment you are just a jira-user. To see the Submit for peer review button, you need to be a jira-developer. However, that should be easy to manage. If you are interested, let me know and I will make sure you get given more permissions. Thanks.

        Show
        Tim Hunt added a comment - Thanks for another useful fix. Since you are using git, would you like to think about getting a public git repository (like https://github.com/timhunt/moodle ) and sharing your bug-fixes by pushing your changes as branches there, like I did with your fix to MDL-28652 ? If you could do that, it would make my life a lot easier. Therefore, if you need help getting started with that, I am happy to provide it. Once you do that, you should then click the Request peer review button at the top, and fill in all the branch information. Then, if all is well, I don't have to do all that to submit your change for integration. Doh! at the moment you are just a jira-user. To see the Submit for peer review button, you need to be a jira-developer. However, that should be easy to manage. If you are interested, let me know and I will make sure you get given more permissions. Thanks.
        Hide
        Henning Bostelmann added a comment -

        OK, great. Yes, if that saves work, then it's a good idea. I have just set up the github repository. Actually I can see the Request Peer Review button on jira (maybe you have already changed the access rights). I will press the button in a moment.

        Show
        Henning Bostelmann added a comment - OK, great. Yes, if that saves work, then it's a good idea. I have just set up the github repository. Actually I can see the Request Peer Review button on jira (maybe you have already changed the access rights). I will press the button in a moment.
        Hide
        Tim Hunt added a comment -

        Great! Thanks. This is very close, but I am afraid I am going to be really picky and complain about two things:

        1. Moodle coding style wants a space after the comma (http://docs.moodle.org/dev/Coding_style#Numerically_indexed_arrays)

        2. And the standard for Moodle commit comments (which is not documented anywhere as far as I know) wants bug ID first, and some clue about what part of the code is affected near the start of the summary. So I would suggest something like

        MDL-28655 qtype_calculated question engine upgrade fix return value

        (and, for future reference, if you need to add more explanation to a commit comment, you should add a blank line before adding more explanation. git log --no-merges master is a good way to get a feel for the house style.)

        Are you able to do a git commit --amend to fix these two.

        Sorry to be so picky, but since this is your first commit, I hope it is helpful for me to teach you about Moodle best practice for contributions.

        Show
        Tim Hunt added a comment - Great! Thanks. This is very close, but I am afraid I am going to be really picky and complain about two things: 1. Moodle coding style wants a space after the comma ( http://docs.moodle.org/dev/Coding_style#Numerically_indexed_arrays ) 2. And the standard for Moodle commit comments (which is not documented anywhere as far as I know) wants bug ID first, and some clue about what part of the code is affected near the start of the summary. So I would suggest something like MDL-28655 qtype_calculated question engine upgrade fix return value (and, for future reference, if you need to add more explanation to a commit comment, you should add a blank line before adding more explanation. git log --no-merges master is a good way to get a feel for the house style.) Are you able to do a git commit --amend to fix these two. Sorry to be so picky, but since this is your first commit, I hope it is helpful for me to teach you about Moodle best practice for contributions.
        Hide
        Henning Bostelmann added a comment -

        No problem, I've amended the commit now.

        Show
        Henning Bostelmann added a comment - No problem, I've amended the commit now.
        Hide
        Tim Hunt added a comment -

        Brilliant, thanks! This should get integrated next week.

        Show
        Tim Hunt added a comment - Brilliant, thanks! This should get integrated next week.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated, thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
        Hide
        Rajesh Taneja added a comment -

        Can't be tested because of lack of data.
        Although checked the usage of "parse_response" and expected return value is array
        Passing it as it's not changing any major functionality.

        Thanks for fixing this Henning

        Show
        Rajesh Taneja added a comment - Can't be tested because of lack of data. Although checked the usage of "parse_response" and expected return value is array Passing it as it's not changing any major functionality. Thanks for fixing this Henning
        Hide
        Eloy Lafuente (stronk7) added a comment -

        This has been sent upstream and is now available in all git & cvs servers. Many thanks for the hard work!

        Show
        Eloy Lafuente (stronk7) added a comment - This has been sent upstream and is now available in all git & cvs servers. Many thanks for the hard work!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: