Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4
    • Fix Version/s: 2.5
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      There are just minor changes to the produced HTML (mostly in CSS), so basically the test is to ensure that everything is displayed as it was before.

      1. Create a course in topics or weeks format with enabled groups and activity completion
      2. Add groups and grouppings to the course
      3. Create several sections and several activities, make sure you have among activities:
        • Activity available only in one of the groups
        • Activity conditionally available (i.e. time based or completion based, etc.)
        • Activity conditionally available that should be displayed to students grayed-out
        • Activity with manual completion
        • Activity with automatic completion
        • Label and/or other activity without URLs
        • Glossary or other activity not supporting group mode
        • Activity with subtypes (i.e. mod_assignment)
      4. View the course as a teacher (both in view and edit mode) and as a student, make sure it looks the same as in master branch
      5. Make sure teacher can move modules in both JS enabled and disabled modes
      6. Make sure teacher can drag and drop file into course (in edit mode)
      7. Make sure student can change/view completion status
      1. Create course in social format
      2. Add some activities, make sure they are displayed correctly in social activities block
      1. In front page settings check the "Include a topic section" checkbox (numsections)
      2. Add site main menu block on the front page
      3. Add activities to the site main menu block and to the front page
      4. Make sure they are displayed correctly to both teacher (view/edit mode) and student

      Please re-test MDL-36808, MDL-35884, MDL-37015, MDL-30899, MDL-36736 because their changes were manually picked into renderer during rebase

      Show
      There are just minor changes to the produced HTML (mostly in CSS), so basically the test is to ensure that everything is displayed as it was before. Create a course in topics or weeks format with enabled groups and activity completion Add groups and grouppings to the course Create several sections and several activities, make sure you have among activities: Activity available only in one of the groups Activity conditionally available (i.e. time based or completion based, etc.) Activity conditionally available that should be displayed to students grayed-out Activity with manual completion Activity with automatic completion Label and/or other activity without URLs Glossary or other activity not supporting group mode Activity with subtypes (i.e. mod_assignment) View the course as a teacher (both in view and edit mode) and as a student, make sure it looks the same as in master branch Make sure teacher can move modules in both JS enabled and disabled modes Make sure teacher can drag and drop file into course (in edit mode) Make sure student can change/view completion status Create course in social format Add some activities, make sure they are displayed correctly in social activities block In front page settings check the "Include a topic section" checkbox (numsections) Add site main menu block on the front page Add activities to the site main menu block and to the front page Make sure they are displayed correctly to both teacher (view/edit mode) and student Please re-test MDL-36808 , MDL-35884 , MDL-37015 , MDL-30899 , MDL-36736 because their changes were manually picked into renderer during rebase
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-37085-master
    • Rank:
      46642

      Description

      Move HTML rendering from functions print_section() and print_section_add_menus() to renderer

      This includes:

      • deprecate get_print_section_cm_text(), substitute with cm_info::get_formatted_content() and cm_info::get_formatted_name()
      • deprecate make_editing_buttons(), substitute with course_get_cm_edit_actions() and core_course_renderer::course_renderer_cm_edit_actions()
      • deprecate print_section_add_menus(), substitute with core_course_renderer::course_section_add_cm_control()
      • deprecate print_section(), substitute with several functions in renderer, the main is core_course_renderer::course_section_cm_list(), which calls
        • core_course_renderer::course_section_cm(), which calls
          • core_course_renderer::course_section_cm_name()
          • core_course_renderer::course_section_cm_text()
          • core_course_renderer::course_section_cm_availability()
          • core_course_renderer::course_section_cm_completion()
          • core_course_renderer::course_section_cm_edit_actions()

      Also changes include:

      • modchooser is only loaded in core_course_renderer, so themes can overwrite not to use it and not to include javascript for it if it is not used
      • get_module_metadata() now returns instance of moodle_url in ->link attribute (instead of string),
      • added fields cm_info::$coursegroupmodeforce and cm_info::$coursegroupmode, removed hacks to overwrite cm_info::$groupmode in case of course forcing before passing them to course_get_cm_edit_actions() (former make_editing_buttons() )

        Issue Links

          Activity

          Hide
          Marina Glancy added a comment -

          Hi Andrew, added you as watcher here.

          Can you please review the commit "MDL-37085 Load modchooser only when needed"
          At the moment modchooser is only called from function print_section_add_menus() which I'm moving to the renderers. So I want to make sure that modchooser is not loaded anywhere if the theme overwrites function core_course_renderer::cm_add_menu().

          PS Though I can't imagine that somebody would want to exclude such a nice feature as modchooser.

          Show
          Marina Glancy added a comment - Hi Andrew, added you as watcher here. Can you please review the commit " MDL-37085 Load modchooser only when needed" At the moment modchooser is only called from function print_section_add_menus() which I'm moving to the renderers. So I want to make sure that modchooser is not loaded anywhere if the theme overwrites function core_course_renderer::cm_add_menu(). PS Though I can't imagine that somebody would want to exclude such a nice feature as modchooser.
          Hide
          Marina Glancy added a comment -

          Andrew, I added another commit "Add modchoosertoggle in renderer to allow themes to override" but I understand that it's not exactly code for renderers, maybe you have another suggestion.

          Show
          Marina Glancy added a comment - Andrew, I added another commit "Add modchoosertoggle in renderer to allow themes to override" but I understand that it's not exactly code for renderers, maybe you have another suggestion.
          Hide
          Andrew Nicols added a comment -

          Sorry for not looking earlier Marina. I'd managed to filter half of my tracker e-mails out of my inbox by mistake.

          The changes for "MDL-37085 Load modchooser only when needed" look perfect

          Regarding "Add modchoosertoggle in renderer to allow themes to override", this does seem like a good idea and allows greater flexibility with theme design. However, I think that implementation needs a bit more thought first as I no longer get the option to toggle the activity chooser (on /course/view.php). It looks like it's too late in the page state somehow.

          Additionally, this probably should not be shown if course_ajax_enabled returns false, which can happen in other circumstances than the theme not supporting it. (e.g. user_is_editing, course format not supporting it, or AJAX being globally disabled).

          Looks good otherwise (at least the modchooser bits).

          Show
          Andrew Nicols added a comment - Sorry for not looking earlier Marina. I'd managed to filter half of my tracker e-mails out of my inbox by mistake. The changes for " MDL-37085 Load modchooser only when needed" look perfect Regarding "Add modchoosertoggle in renderer to allow themes to override", this does seem like a good idea and allows greater flexibility with theme design. However, I think that implementation needs a bit more thought first as I no longer get the option to toggle the activity chooser (on /course/view.php). It looks like it's too late in the page state somehow. Additionally, this probably should not be shown if course_ajax_enabled returns false, which can happen in other circumstances than the theme not supporting it. (e.g. user_is_editing, course format not supporting it, or AJAX being globally disabled). Looks good otherwise (at least the modchooser bits).
          Hide
          Marina Glancy added a comment -

          Hi Andrew, it's strange, the menu item loads for me. Did you purge the cache?
          I'm finished with the rest of this issue, would like to push it for integration as soon as we decide with you on modchooser

          Show
          Marina Glancy added a comment - Hi Andrew, it's strange, the menu item loads for me. Did you purge the cache? I'm finished with the rest of this issue, would like to push it for integration as soon as we decide with you on modchooser
          Hide
          Marina Glancy added a comment -

          rebased, changes conflicted with MDL-36808 and MDL-35884

          Show
          Marina Glancy added a comment - rebased, changes conflicted with MDL-36808 and MDL-35884
          Hide
          Marina Glancy added a comment -

          TO INTEGRATORS: master only, the branch is based on MDL-37009

          Show
          Marina Glancy added a comment - TO INTEGRATORS: master only, the branch is based on MDL-37009
          Hide
          Andrew Nicols added a comment -

          I've updated from your latest branch, and purged caches.

          First off, I hit a bug in cbd5cd0a in lib/modinfolib.php:

           $options['context'] = context_module::instance($this->course);
          

          Should be $this->id instead of $this->course.

          I'm still not getting the modchooser toggle. It looks like the renderer function is called really late on. Debugging things a little, it looks like it's not called until the body printing has started. here's the stack trace of where it's called:

          line 71 of /course/renderer.php: moodle_exception thrown
          line 52 of /course/renderer.php: call to core_course_renderer->add_modchoosertoggle()
          line 240 of /lib/outputfactories.php: call to core_course_renderer->__construct()
          line 1282 of /lib/outputlib.php: call to standard_renderer_factory->get_renderer()
          line 754 of /lib/pagelib.php: call to theme_config->get_renderer()
          line 52 of /course/format/renderer.php: call to moodle_page->get_renderer()
          line 240 of /lib/outputfactories.php: call to format_section_renderer_base->__construct()
          line 1282 of /lib/outputlib.php: call to standard_renderer_factory->get_renderer()
          line 754 of /lib/pagelib.php: call to theme_config->get_renderer()
          line 44 of /course/format/weeks/format.php: call to moodle_page->get_renderer()
          line 266 of /course/view.php: call to require()
          

          Looking at course/view.php, the body output starts far earlier than when the course format is included on line 266 (and with it the renderer).

          Looking at course/view.php:
          The page header is printed on line 230, and further content straight after.
          The course format is included on line 266, and the course format file then includes the renderer which later tries to add the modchooser toggle. By this time, it's too late to start modifying the page navigation.

          Show
          Andrew Nicols added a comment - I've updated from your latest branch, and purged caches. First off, I hit a bug in cbd5cd0a in lib/modinfolib.php: $options['context'] = context_module::instance($ this ->course); Should be $this->id instead of $this->course. I'm still not getting the modchooser toggle. It looks like the renderer function is called really late on. Debugging things a little, it looks like it's not called until the body printing has started. here's the stack trace of where it's called: line 71 of /course/renderer.php: moodle_exception thrown line 52 of /course/renderer.php: call to core_course_renderer->add_modchoosertoggle() line 240 of /lib/outputfactories.php: call to core_course_renderer->__construct() line 1282 of /lib/outputlib.php: call to standard_renderer_factory->get_renderer() line 754 of /lib/pagelib.php: call to theme_config->get_renderer() line 52 of /course/format/renderer.php: call to moodle_page->get_renderer() line 240 of /lib/outputfactories.php: call to format_section_renderer_base->__construct() line 1282 of /lib/outputlib.php: call to standard_renderer_factory->get_renderer() line 754 of /lib/pagelib.php: call to theme_config->get_renderer() line 44 of /course/format/weeks/format.php: call to moodle_page->get_renderer() line 266 of /course/view.php: call to require() Looking at course/view.php, the body output starts far earlier than when the course format is included on line 266 (and with it the renderer). Looking at course/view.php: The page header is printed on line 230, and further content straight after. The course format is included on line 266, and the course format file then includes the renderer which later tries to add the modchooser toggle. By this time, it's too late to start modifying the page navigation.
          Hide
          Marina Glancy added a comment -

          Hi Andrew, thanks for feedback.
          The course context in module name was there in the original function. I guest it was a bug, so i changed it to module context now.
          I removed the commit for modchoosertoggle from this issue and created two other issues: MDL-37206 and MDL-37207

          Show
          Marina Glancy added a comment - Hi Andrew, thanks for feedback. The course context in module name was there in the original function. I guest it was a bug, so i changed it to module context now. I removed the commit for modchoosertoggle from this issue and created two other issues: MDL-37206 and MDL-37207
          Hide
          Marina Glancy added a comment -

          (master only)

          Show
          Marina Glancy added a comment - (master only)
          Hide
          Marina Glancy added a comment -

          rebased

          Show
          Marina Glancy added a comment - rebased
          Hide
          Sam Hemelryk added a comment -

          Thanks Marina - this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Marina - this has been integrated now
          Hide
          Frédéric Massart added a comment - - edited

          Raised the issue MDL-37544 not thinking it would be caused by this one. Please link/close after proper testing.

          Show
          Frédéric Massart added a comment - - edited Raised the issue MDL-37544 not thinking it would be caused by this one. Please link/close after proper testing.
          Hide
          Marina Glancy added a comment -
          Show
          Marina Glancy added a comment - thanks Fred, I added commit fixing it https://github.com/marinaglancy/moodle/commit/db82b130509bef8d1750b9b8f9261acdf8ef5bfb
          Hide
          Dan Poltawski added a comment -

          Pulled that commit into intgeration.

          Show
          Dan Poltawski added a comment - Pulled that commit into intgeration.
          Hide
          Rajesh Taneja added a comment -

          Thanks Marina and Fred,

          Works fine for me. Everything looks same as in master stable.

          FYI:
          Restricted activities in main menu block are not show as greyed out (is hidden for student irrespective of restrict access option set to show as grey), but that's the behaviour in STABLE_MASTER, so passing this.
          Not sure if this is expected or a bug.

          Show
          Rajesh Taneja added a comment - Thanks Marina and Fred, Works fine for me. Everything looks same as in master stable. FYI: Restricted activities in main menu block are not show as greyed out (is hidden for student irrespective of restrict access option set to show as grey), but that's the behaviour in STABLE_MASTER, so passing this. Not sure if this is expected or a bug.
          Hide
          Andrew Nicols added a comment -

          This causes a regression with module subtypes. The link generated for a module subtype is parsed by moodle_url and encoded.

          I think that this is the right thing to do, and that we should fix the module subtypes, either in course/lib.php::get_module_metadata(), or in the MODNAME_get_types() function.

          Moving discussion to the regression issue in MDL-37085. I think this should probably hit master, and be addressed in a subsequent patch.

          Show
          Andrew Nicols added a comment - This causes a regression with module subtypes. The link generated for a module subtype is parsed by moodle_url and encoded. I think that this is the right thing to do, and that we should fix the module subtypes, either in course/lib.php::get_module_metadata(), or in the MODNAME_get_types() function. Moving discussion to the regression issue in MDL-37085 . I think this should probably hit master, and be addressed in a subsequent patch.
          Hide
          Andrew Nicols added a comment - - edited

          I've added a patch to MDL-37085 - I'll leave it to the I-Team to decide whether to revert this or not

          Show
          Andrew Nicols added a comment - - edited I've added a patch to MDL-37085 - I'll leave it to the I-Team to decide whether to revert this or not
          Hide
          Marina Glancy added a comment -

          I suppose Andrew meant MDL-37538

          Show
          Marina Glancy added a comment - I suppose Andrew meant MDL-37538
          Hide
          Andrew Nicols added a comment -

          whimper yes...

          Show
          Andrew Nicols added a comment - whimper yes...
          Hide
          Marina Glancy added a comment - - edited

          Dear integrators, I cherry-picked Andrew's commit in this branch. Also I added another commit for backward compatibility of his changes. Pretty please integrate them!

          Both commits together: https://github.com/marinaglancy/moodle/compare/db82b13...fb30b93

          Show
          Marina Glancy added a comment - - edited Dear integrators, I cherry-picked Andrew's commit in this branch. Also I added another commit for backward compatibility of his changes. Pretty please integrate them! Both commits together: https://github.com/marinaglancy/moodle/compare/db82b13...fb30b93
          Hide
          Marina Glancy added a comment -

          but actually thinking more about this - only one line change is really required:

          --- a/course/lib.php
          +++ b/course/lib.php
          @@ -1372,7 +1377,7 @@ function get_module_metadata($course, $modnames, $sectionreturn = null) {
                               if (get_string_manager()->string_exists('help' . $subtype->name, $modname)) {
                                   $subtype->help = get_string('help' . $subtype->name, $modname);
                               }
          -                    $subtype->link = new moodle_url($urlbase, array('add' => $subtype->type));
          +                    $subtype->link = new moodle_url($urlbase, array('add' => $modname, 'type' => $subtype->name));
                               $group->types[] = $subtype;
                           }
                           $modlist[$course->id][$modname] = $group;
          

          We don't really want to change API or support anything about subtypes, they are deprecated anyway... Can I just replace those two commits with this small one and give credit to Andrew somehow?

          Show
          Marina Glancy added a comment - but actually thinking more about this - only one line change is really required: --- a/course/lib.php +++ b/course/lib.php @@ -1372,7 +1377,7 @@ function get_module_metadata($course, $modnames, $sectionreturn = null ) { if (get_string_manager()->string_exists('help' . $subtype->name, $modname)) { $subtype->help = get_string('help' . $subtype->name, $modname); } - $subtype->link = new moodle_url($urlbase, array('add' => $subtype->type)); + $subtype->link = new moodle_url($urlbase, array('add' => $modname, 'type' => $subtype->name)); $group->types[] = $subtype; } $modlist[$course->id][$modname] = $group; We don't really want to change API or support anything about subtypes, they are deprecated anyway... Can I just replace those two commits with this small one and give credit to Andrew somehow?
          Hide
          Marina Glancy added a comment -

          Ok I stopped talking to myself and replaced API improvement with one-line bug fix. MDL-37538 can be a separate issue for improvement of $type->subtype

          Show
          Marina Glancy added a comment - Ok I stopped talking to myself and replaced API improvement with one-line bug fix. MDL-37538 can be a separate issue for improvement of $type->subtype
          Hide
          Rajesh Taneja added a comment -

          Sorry Guys,

          I think I missed this somehow. Let me know when it's ready for re-test.

          Show
          Rajesh Taneja added a comment - Sorry Guys, I think I missed this somehow. Let me know when it's ready for re-test.
          Hide
          Marina Glancy added a comment -

          found another bug that failed test in MDL-37453. One more commit added to this branch

          Show
          Marina Glancy added a comment - found another bug that failed test in MDL-37453 . One more commit added to this branch
          Hide
          Dan Poltawski added a comment -

          This has all been integrated now, right?

          Show
          Dan Poltawski added a comment - This has all been integrated now, right?
          Hide
          Dan Poltawski added a comment -

          Apparently it wasn't. I pulled in 8df0765ad14267505fe07fb2f3da481c8cabf9cc. Please can you try to be clearer about what needs to be done on the issues, it makes it easier for us to work out what needs to be done when you are not online.

          Show
          Dan Poltawski added a comment - Apparently it wasn't. I pulled in 8df0765ad14267505fe07fb2f3da481c8cabf9cc. Please can you try to be clearer about what needs to be done on the issues, it makes it easier for us to work out what needs to be done when you are not online.
          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:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: