Moodle
  1. Moodle
  2. MDL-34810

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

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      43305

      Description

      That is, the three options

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

        Activity

        Hide
        Tim Hunt added a comment -

        Also qperpage

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

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

        Show
        Tim Hunt added a comment - Once again, I will rebase and cherry-pick once the weekly is out.
        Hide
        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
        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
        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
        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
        Tim Hunt added a comment -

        Submitting for integration.

        Show
        Tim Hunt added a comment - Submitting for integration.
        Hide
        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
        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 added a comment -

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

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        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
        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
        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
        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
        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
        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
        Eloy Lafuente (stronk7) added a comment -

        YEAR!*

        CAF*, TOT!*

        • Your effort amazingly resulted. (unbelievable :-P)
        • Closing as fixed.
        • Tons of thanks.
        Show
        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: