Moodle
  1. Moodle
  2. MDL-35793

Admin options for when a user starts a preview

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.2
    • Fix Version/s: 2.5
    • Component/s: Questions
    • Testing Instructions:
      Hide

      1. As admin, go to Admin -> Plugins -> Question types -> Question preview defaults and set some options.
      2. Create a new user 'teacher', and enrol that user as a Teacher in a course.
      3. Log in as 'teacher' and go to the course question bank.
      4. Preview a question (create on if necessary).
      5. Verify that the display options used are the ones the admin set.
      6. Change some options, and click 'Start again with these options'
      7. Log out.
      8. Log back in as 'teacher', go back to the course, and preview another question. The preferences used should be the ones you set in step 6.

      Show
      1. As admin, go to Admin -> Plugins -> Question types -> Question preview defaults and set some options. 2. Create a new user 'teacher', and enrol that user as a Teacher in a course. 3. Log in as 'teacher' and go to the course question bank. 4. Preview a question (create on if necessary). 5. Verify that the display options used are the ones the admin set. 6. Change some options, and click 'Start again with these options' 7. Log out. 8. Log back in as 'teacher', go back to the course, and preview another question. The preferences used should be the ones you set in step 6.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      44547

      Description

      At the moment, when a new user first previews a question, the display option are taken from some hard-coded values in question_preview_options::__construct().

      Instead, we would like admin options for these.

      This would require a new admin settings page

      • Site administration
        • Plugins
          • Question types
            • Manage question types
            • Default settings <-- this is the new one.

      For now there will be one group of settings here, with title "Default display options when previewing a question." There will be those 7 settings from the question preview window.

      These settings should be stored in the config_plugings table, with plugin = 'question'.

        Activity

        Hide
        Thanh Le added a comment - - edited

        I've coded a solution and applied it to Moodle MASTER branch.

        See:
        https://github.com/dtle/moodle/tree/MDL-35793

        Diff:
        1) https://github.com/dtle/moodle/commit/9c377a27837d025c83a54361e6f83907921cb57e (initial commit)
        2) https://github.com/dtle/moodle/commit/3bffa18285d538c47a4ddc82a3992bb9ef75b0ef (correction to default values)
        3) https://github.com/dtle/moodle/commit/3bffa18285d538c47a4ddc82a3992bb9ef75b0ef (minor new file header correction)

        Before continuing, might be worth you reviewing it. I dont see this as adding much value because as soon as the user previews a question and changes any of the preview option, all the preview options are saved as user preferences. The way the code is written, config settings are overrided by user defaults, then by url settings as in these line of code in /question/preview.php:

        $options = new question_preview_options($question);
        $options->load_user_defaults();
        $options->set_from_request();

        ... This means once user preferences are saved, they will always supercede the config settings in all question previews - so the config settings only apply in one case only... the first time a user (with no save user preferences) tries to preview a question.

        Show
        Thanh Le added a comment - - edited I've coded a solution and applied it to Moodle MASTER branch. See: https://github.com/dtle/moodle/tree/MDL-35793 Diff: 1) https://github.com/dtle/moodle/commit/9c377a27837d025c83a54361e6f83907921cb57e (initial commit) 2) https://github.com/dtle/moodle/commit/3bffa18285d538c47a4ddc82a3992bb9ef75b0ef (correction to default values) 3) https://github.com/dtle/moodle/commit/3bffa18285d538c47a4ddc82a3992bb9ef75b0ef (minor new file header correction) Before continuing, might be worth you reviewing it. I dont see this as adding much value because as soon as the user previews a question and changes any of the preview option, all the preview options are saved as user preferences. The way the code is written, config settings are overrided by user defaults, then by url settings as in these line of code in /question/preview.php: $options = new question_preview_options($question); $options->load_user_defaults(); $options->set_from_request(); ... This means once user preferences are saved, they will always supercede the config settings in all question previews - so the config settings only apply in one case only... the first time a user (with no save user preferences) tries to preview a question.
        Hide
        Tim Hunt added a comment -

        You are right that this does not seem very valuable, but actually it is, because it affects what people see when they first start using Moodle, and it is important to start people off using the right settings.

        I don't think it is right to create the new file admin/qdefaults.php. However, I am not sure where the best place to put it is. I will consult with some others.

        The main point is that you have to be very careful about performance in these settings files, because the tree of admin settings appear on every page to admins. Therefore, you need to avoid things like:

        • require_once($CFG->libdir . '/../question/engine/lib.php'); - where that then also requires a lot of other files.
        • any database queries.

        Techniques to avoid these problems include:

        1. hard-code values, rather than using the defined constant, if that constant is in a big library file.
        2. move constant definitions so they can be included cheaply
        3. For admin_setting_configselect, make a sub-class and override the load_choices method, so we only have to populate the choices if they are really needed.

        I am going to make some of these changes, and then ask you to review them.

        Show
        Tim Hunt added a comment - You are right that this does not seem very valuable, but actually it is, because it affects what people see when they first start using Moodle, and it is important to start people off using the right settings. I don't think it is right to create the new file admin/qdefaults.php. However, I am not sure where the best place to put it is. I will consult with some others. The main point is that you have to be very careful about performance in these settings files, because the tree of admin settings appear on every page to admins. Therefore, you need to avoid things like: require_once($CFG->libdir . '/../question/engine/lib.php'); - where that then also requires a lot of other files. any database queries. Techniques to avoid these problems include: hard-code values, rather than using the defined constant, if that constant is in a big library file. move constant definitions so they can be included cheaply For admin_setting_configselect, make a sub-class and override the load_choices method, so we only have to populate the choices if they are really needed. I am going to make some of these changes, and then ask you to review them.
        Hide
        Tim Hunt added a comment -

        Yay! admin_setting_question_behaviour already exists

        I think I am going to rename these settings to question_preview/, rather than question/. They are very specific, and only needed in one place, so better not to lump them in with the other question settings that are loaded in more places.

        Show
        Tim Hunt added a comment - Yay! admin_setting_question_behaviour already exists I think I am going to rename these settings to question_preview/, rather than question/. They are very specific, and only needed in one place, so better not to lump them in with the other question settings that are loaded in more places.
        Hide
        Tim Hunt added a comment -

        I think this is the way I would do it: https://github.com/timhunt/moodle/compare/MDL-35793. Comments?

        Show
        Tim Hunt added a comment - I think this is the way I would do it: https://github.com/timhunt/moodle/compare/MDL-35793 . Comments?
        Hide
        Sam Marshall added a comment -

        Tim: Looks good to me, some notes:

        1. I see you aren't using the actual constants but have referred to them in comments. I guess there is a policy to not do require_once calls in settings.php?

        2. This will add an extra db query to the load_user_defaults function, assuming it is only usually called once a page which I guess is likely, then it's probably fine.

        basically +1

        Show
        Sam Marshall added a comment - Tim: Looks good to me, some notes: 1. I see you aren't using the actual constants but have referred to them in comments. I guess there is a policy to not do require_once calls in settings.php? 2. This will add an extra db query to the load_user_defaults function, assuming it is only usually called once a page which I guess is likely, then it's probably fine. basically +1
        Hide
        Tim Hunt added a comment -

        Submitting for integration review.

        I would like this to go into MOODLE_24_STABLE because we then see the benefits more quickly at the OU, but that might not be a good enough reason.

        Show
        Tim Hunt added a comment - Submitting for integration review. I would like this to go into MOODLE_24_STABLE because we then see the benefits more quickly at the OU, but that might not be a good enough reason.
        Hide
        Dan Poltawski added a comment -

        I've added integration_held as this seems to be a new feature.

        Show
        Dan Poltawski added a comment - I've added integration_held as this seems to be a new feature.
        Hide
        Dan Poltawski added a comment -

        Integrated to master only (as this is a new feature).

        Thanks Tim

        Show
        Dan Poltawski added a comment - Integrated to master only (as this is a new feature). Thanks Tim
        Hide
        Rossiani Wijaya added a comment -

        This works as expected.

        Test passed.

        Show
        Rossiani Wijaya added a comment - This works as expected. Test passed.
        Hide
        Dan Poltawski added a comment -

        Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

        Show
        Dan Poltawski added a comment - Hurray! We did it! Thanks to all the reporters, testers, user and watchers for a bumper week of Moodling!

          People

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

            Dates

            • Created:
              Updated:
              Resolved: