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

The options in the question bank should be persisted as user preferences

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.2.4, 2.3.1, 2.4
    • Fix Version/s: 2.3.2
    • Component/s: Questions
    • Labels:
    • Testing Instructions:
      Hide

      1. Go to the question bank in a course with some questions.

      2. Try changing the three check-box options at the top, and the show all on one page option at the bottom. Make sure these options work.

      3. Once you have the options set to something other than the default, go back to the course page, then return to the question bank (or go to the question bank in another course) Make sure it has remembered your options.

      Show
      1. Go to the question bank in a course with some questions. 2. Try changing the three check-box options at the top, and the show all on one page option at the bottom. Make sure these options work. 3. Once you have the options set to something other than the default, go back to the course page, then return to the question bank (or go to the question bank in another course) Make sure it has remembered your options.
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:

      Description

      That is, the three options

      • Also show questions from sub-categories
      • Also show old questions
      • Show question text in the question list

        Gliffy Diagrams

          Activity

          Hide
          timhunt Tim Hunt added a comment -

          Also qperpage

          Show
          timhunt Tim Hunt added a comment - Also qperpage
          Hide
          timhunt Tim Hunt added a comment -

          Once again, I will rebase and cherry-pick once the weekly is out.

          Show
          timhunt Tim Hunt added a comment - Once again, I will rebase and cherry-pick once the weekly is out.
          Hide
          salvetore Michael de Raadt added a comment -

          That looks like a useful change.

          Some comments:

          • perpage is used in used in other areas (well, assignment at least) with the format module_perpage. It might be nice to have question_bank_perpage instead of question_bank_qperpage. Similarly for question_bank_qbshowtext.
          • I like the fact that you considered the URL with newly set parameters.
          • You could, perhaps, consolidate the two returns in question_get_display_preference() to create a single exit point.
          • Testing instructions are still needed.
          Show
          salvetore Michael de Raadt added a comment - That looks like a useful change. Some comments: perpage is used in used in other areas (well, assignment at least) with the format module_perpage. It might be nice to have question_bank_perpage instead of question_bank_qperpage. Similarly for question_bank_qbshowtext. I like the fact that you considered the URL with newly set parameters. You could, perhaps, consolidate the two returns in question_get_display_preference() to create a single exit point. Testing instructions are still needed.
          Hide
          timhunt Tim Hunt added a comment -

          I disagree with people who say a function should have a single exit point. In my experience it tends to lead to more nested conditionals, and more local variables, and hence less readable code.

          I am also disinclined to change the parameter names now, and hence I will leave the user pref names. The names are clear enough, and user pref internal names are an implementation details. Only developers ever see them.

          Show
          timhunt Tim Hunt added a comment - I disagree with people who say a function should have a single exit point. In my experience it tends to lead to more nested conditionals, and more local variables, and hence less readable code. I am also disinclined to change the parameter names now, and hence I will leave the user pref names. The names are clear enough, and user pref internal names are an implementation details. Only developers ever see them.
          Hide
          timhunt Tim Hunt added a comment -

          Submitting for integration.

          Show
          timhunt Tim Hunt added a comment - Submitting for integration.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          1) Please consider moving this to type bug to get chances of getting it in stables.
          2) "if(("
          3) Don't think it's important but.. the question_edit_setup() is called from various places... so those request/prefs.. are also get/set (potentially) there (on export, import, editing category...). Is that important enough to need any difference? Just guessing...

          Ciao

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - 1) Please consider moving this to type bug to get chances of getting it in stables. 2) "if((" 3) Don't think it's important but.. the question_edit_setup() is called from various places... so those request/prefs.. are also get/set (potentially) there (on export, import, editing category...). Is that important enough to need any difference? Just guessing... Ciao
          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
          timhunt Tim Hunt added a comment -

          1) I am not going to play silly semantic games like this. This fix is clearly an improvement, and it clearly will not invalidate anyone's familiarity with the UI, or training documentation. It just makes things work better. To me it makes sense that it goes into all stable branches.

          2) Oops! Rather than just fix the white-space, I re-jigged the code in a way that seems a bit more readable.

          3) It is intentional that the same set-up function is used on all qbank pages. It means that as you move around the various question bank pages, your state is preserved, even if not all URL params are used on all pages.

          Show
          timhunt Tim Hunt added a comment - 1) I am not going to play silly semantic games like this. This fix is clearly an improvement, and it clearly will not invalidate anyone's familiarity with the UI, or training documentation. It just makes things work better. To me it makes sense that it goes into all stable branches. 2) Oops! Rather than just fix the white-space, I re-jigged the code in a way that seems a bit more readable. 3) It is intentional that the same set-up function is used on all qbank pages. It means that as you move around the various question bank pages, your state is preserved, even if not all URL params are used on all pages.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated (23 & master). No way to land this to old 22_STABLE as far as it's clearly one improvement (or whatever you want to name it). Thanks!

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Integrated (23 & master). No way to land this to old 22_STABLE as far as it's clearly one improvement (or whatever you want to name it). Thanks!
          Hide
          dmonllao David Monllaó added a comment -

          Works as expected, tested in master, the 3 checkboxes and the 'show all' remains in the saved state, the same showing the question bank in a quiz. Tested forcing mdl_question->hidden and reducing the DEFAULT_QUESTIONS_PER_PAGE value.

          Show
          dmonllao David Monllaó added a comment - Works as expected, tested in master, the 3 checkboxes and the 'show all' remains in the saved state, the same showing the question bank in a quiz. Tested forcing mdl_question->hidden and reducing the DEFAULT_QUESTIONS_PER_PAGE value.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          YEAR!*

          CAF*, TOT!*

          • Your effort amazingly resulted. (unbelievable :-P)
          • Closing as fixed.
          • Tons of thanks.
          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                10/Sep/12