Moodle
  1. Moodle
  2. MDL-29141

After turning on show question text in the question bank, you can't turn it off again

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.4, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Questions
    • Labels:
      None
    • Testing Instructions:
      Hide

      1. Go to the question bank in any course that has questions.
      2. Tick the Show question text checkbox. Verify that
      a. the page reloads, and the question text appears, and
      b. the checkbox is now checked.
      3. Now uncheck the checkbox. Verify that
      a. the page reloads, and the question text is now hidden, and
      b. the checkbox is now unchecked.

      Show
      1. Go to the question bank in any course that has questions. 2. Tick the Show question text checkbox. Verify that a. the page reloads, and the question text appears, and b. the checkbox is now checked. 3. Now uncheck the checkbox. Verify that a. the page reloads, and the question text is now hidden, and b. the checkbox is now unchecked.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      18840

      Description

      This is a follow-up to MDL-27631.

      After you tick the Show question text checkbox, the page reloads, and the question text is shown.

      However, on that page, the checkbox is not correct, so you cannot un-check it to hide the question text.

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Note that the fix on the 2.0 branch includes back-porting the fix for MDL-27631 to that branch. Once this bug has been integrated, it might be nice to mark that bug as fixed in 2.0.x

          Show
          Tim Hunt added a comment - Note that the fix on the 2.0 branch includes back-porting the fix for MDL-27631 to that branch. Once this bug has been integrated, it might be nice to mark that bug as fixed in 2.0.x
          Hide
          Tim Hunt added a comment -

          By the way, I am aware how badly it sucks that the same optional param calls for qbshowtext happens in question_edit_setup and question_bank_view. Perhaps I will have time to fix this before Moodle 2.2.

          Note that the back-port to 2.0 is not a clean cherry-pick. It had to be fixed up by hand because of coding-style and other changes.

          Show
          Tim Hunt added a comment - By the way, I am aware how badly it sucks that the same optional param calls for qbshowtext happens in question_edit_setup and question_bank_view. Perhaps I will have time to fix this before Moodle 2.2. Note that the back-port to 2.0 is not a clean cherry-pick. It had to be fixed up by hand because of coding-style and other changes.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Tim, this line:

          +    if(($showquestiontext = optional_param('qbshowtext', -1, PARAM_BOOL)) != -1) {
          

          Could you:

          1) Split it into 2, one for getting the param and another for the condition... and
          2) add one space after "if"

          Really it's unreadable in its current incarnation. Surely no a matter of rejecting this, but I prefer to give you a chance to amend it.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Tim, this line: + if (($showquestiontext = optional_param('qbshowtext', -1, PARAM_BOOL)) != -1) { Could you: 1) Split it into 2, one for getting the param and another for the condition... and 2) add one space after "if" Really it's unreadable in its current incarnation. Surely no a matter of rejecting this, but I prefer to give you a chance to amend it. TIA and ciao
          Hide
          Tim Hunt added a comment -

          I am not going to change it.

          1. That idiom is used in many places in that file. I am not going to change one without changing them all, and code clean-up like that should not happen on stable branches, only on master.

          2. In the past you complained and told me not to fix coding style when making small changes in existing code, because it made the diff harder to read. Now you are saying that I should make that sort of style fix. Would you like to decide what you want, then document it?

          Show
          Tim Hunt added a comment - I am not going to change it. 1. That idiom is used in many places in that file. I am not going to change one without changing them all, and code clean-up like that should not happen on stable branches, only on master. 2. In the past you complained and told me not to fix coding style when making small changes in existing code, because it made the diff harder to read. Now you are saying that I should make that sort of style fix. Would you like to decide what you want, then document it?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Tim:

          1) Agree: I would recommend you to perform the split to 2 lines only into master no problem with that.
          2) Don't agree: I never have said that lines being part of the solution must not be coding-style fixed. Instead I always have said that we should not be doing coding-style fixing in unrelated lines (but create separate cleanup issue for that). And you know it, so don't try to mislead me, please!

          Said that, oki, I'm going to integrate this, I hope you will cleanup it some day. Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Tim: 1) Agree: I would recommend you to perform the split to 2 lines only into master no problem with that. 2) Don't agree: I never have said that lines being part of the solution must not be coding-style fixed. Instead I always have said that we should not be doing coding-style fixing in unrelated lines (but create separate cleanup issue for that). And you know it, so don't try to mislead me, please! Said that, oki, I'm going to integrate this, I hope you will cleanup it some day. Thanks and ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And note that you are contradicting yourself completely as far as you have already introduced 100% unrelated changes at least in the 20_STABLE branch, so guy... surely a bit of thinking before stating (false) things/principles would be greatly welcome.

          PS: All this said 100% joking, eh! 1000% in fact. I hope all problems are like this, lol.

          Show
          Eloy Lafuente (stronk7) added a comment - And note that you are contradicting yourself completely as far as you have already introduced 100% unrelated changes at least in the 20_STABLE branch, so guy... surely a bit of thinking before stating (false) things/principles would be greatly welcome. PS: All this said 100% joking, eh! 1000% in fact. I hope all problems are like this, lol.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Sam Hemelryk added a comment -

          Passed as bro!

          Show
          Sam Hemelryk added a comment - Passed as bro!
          Hide
          Tim Hunt added a comment -

          Welcome home to New Zealand Sam!

          Show
          Tim Hunt added a comment - Welcome home to New Zealand Sam!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          git & cvs repositories have been populated with this solution. Many thanks for your collaboration, yay!

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - git & cvs repositories have been populated with this solution. Many thanks for your collaboration, yay! Closing, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: