Moodle
  1. Moodle
  2. MDL-38747

Block section_links throw notices when course format is changed from weekly/topic to social or scrom

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.4.3, 2.5.1, 2.6
    • Fix Version/s: 2.4.6, 2.5.2
    • Component/s: Blocks
    • Labels:
    • Rank:
      48805

      Description

      1. Log in as admin
      2. Go to a course
      3. Add section links block
      4. Edit course settings and change format to social
      5. Save and you will see following notice
        Stack trace:, referer: http://rajesh.moodle.local/m24/course/edit.php
        1. {main}() /var/www/m24/course/view.php:0, referer: http://rajesh.moodle.local/m24/course/edit.php
        2. core_renderer->header() /var/www/m24/course/view.php:240, referer: http://rajesh.moodle.local/m24/course/edit.php
        3. core_renderer->render_page_layout() /var/www/m24/lib/outputrenderers.php:734, referer: http://rajesh.moodle.local/m24/course/edit.php
        4. include() /var/www/m24/lib/outputrenderers.php:804, referer: http://rajesh.moodle.local/m24/course/edit.php
        5. block_manager->region_has_content() /var/www/m24/theme/base/layout/general.php:6, referer: http://rajesh.moodle.local/m24/course/edit.php
        6. block_manager->ensure_content_created() /var/www/m24/lib/blocklib.php:353, referer: http://rajesh.moodle.local/m24/course/edit.php
        7. block_manager->create_block_contents() /var/www/m24/lib/blocklib.php:1005, referer: http://rajesh.moodle.local/m24/course/edit.php
        8. block_base->get_content_for_output() /var/www/m24/lib/blocklib.php:953, referer: http://rajesh.moodle.local/m24/course/edit.php
        9. block_base->formatted_contents() /var/www/m24/blocks/moodleblock.class.php:232, referer: http://rajesh.moodle.local/m24/course/edit.php
        10. block_section_links->get_content() /var/www/m24/blocks/moodleblock.class.php:284, referer: http://rajesh.moodle.local/m24/course/edit.php
        

        Activity

        Hide
        David Mudrak added a comment -

        This issue was assigned to me, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

        Show
        David Mudrak added a comment - This issue was assigned to me, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment
        Hide
        Mohamed Alsharaf added a comment -
        Show
        Mohamed Alsharaf added a comment - I have a fix for this issue, but I don't have permission to edit the tracker. 2.5 Branch: wip- MDL-38747 -MOODLE_25_STABLE https://github.com/satrun77/moodle/compare/MOODLE_25_STABLE...wip-MDL-38747-MOODLE_25_STABLE 2.4 Branch: wip- MDL-38747 -MOODLE_24_STABLE https://github.com/satrun77/moodle/compare/MOODLE_24_STABLE...wip-MDL-38747-MOODLE_24_STABLE master Branch: wip- MDL-38747 -master https://github.com/satrun77/moodle/compare/master...wip-MDL-38747-master
        Hide
        Rajesh Taneja added a comment -

        Thanks for the patch Mohamed,

        Your patch will solve the problem, but I think rather then manipulating social format, section link block should be modified to check if formatoptions are set, if not then it should not try to add section links.

        I have added Marina as watcher to get her feedback on this.

        Show
        Rajesh Taneja added a comment - Thanks for the patch Mohamed, Your patch will solve the problem, but I think rather then manipulating social format, section link block should be modified to check if formatoptions are set, if not then it should not try to add section links. I have added Marina as watcher to get her feedback on this.
        Hide
        Mohamed Alsharaf added a comment - - edited

        @Rajesh I had both idea implemented, then I decided to go with the option in the patch.

        The way I thought about it is that the course format is the entity to decide how a course should look and behave. Where the section links does one thing displays links to sections exists, incase of the social format it is zero section. This is just my humble opinion. I'm happy to go with the other option

        Show
        Mohamed Alsharaf added a comment - - edited @Rajesh I had both idea implemented, then I decided to go with the option in the patch. The way I thought about it is that the course format is the entity to decide how a course should look and behave. Where the section links does one thing displays links to sections exists, incase of the social format it is zero section. This is just my humble opinion. I'm happy to go with the other option
        Hide
        Rajesh Taneja added a comment -

        Thanks Mohammed,

        I agree with your though. But looking at design of base_format, get_format_options() return empty array, if format doesn't have any options.
        Saying so, such case should be handled by calling method (in this case section block). As course_format_options() or section_format_options() are not abstract, it's upto course format to implement it. With your patch it will fix the problem for social course format, but it might be an issue for custom course format.

        If you have another patch, then feel free to post it here and I will be happy to replace the branches.

        Show
        Rajesh Taneja added a comment - Thanks Mohammed, I agree with your though. But looking at design of base_format, get_format_options() return empty array, if format doesn't have any options. Saying so, such case should be handled by calling method (in this case section block). As course_format_options() or section_format_options() are not abstract, it's upto course format to implement it. With your patch it will fix the problem for social course format, but it might be an issue for custom course format. If you have another patch, then feel free to post it here and I will be happy to replace the branches.
        Hide
        Mohamed Alsharaf added a comment -

        @Rajesh I have updated the code in the branches in comment "03/Jul/13 8:03 PM". Thanks

        Show
        Mohamed Alsharaf added a comment - @Rajesh I have updated the code in the branches in comment "03/Jul/13 8:03 PM". Thanks
        Hide
        Rajesh Taneja added a comment -

        Thanks Mohamed,

        I have added your branches.

        Show
        Rajesh Taneja added a comment - Thanks Mohamed, I have added your branches.
        Hide
        Marina Glancy added a comment -

        This is an interesting issue. Block section_links is only allowed in weeks and topics course formats. It implements the function applicable_formats() to check the course format when block is being added. But the problem is that when the format is changed the block is not deleted.
        So I would just return from the function if the format is anything else, somewhere around this line:
        https://github.com/moodle/moodle/blob/master/blocks/section_links/block_section_links.php#L93

        Show
        Marina Glancy added a comment - This is an interesting issue. Block section_links is only allowed in weeks and topics course formats. It implements the function applicable_formats() to check the course format when block is being added. But the problem is that when the format is changed the block is not deleted. So I would just return from the function if the format is anything else, somewhere around this line: https://github.com/moodle/moodle/blob/master/blocks/section_links/block_section_links.php#L93
        Hide
        Rajesh Taneja added a comment -

        Thanks Marina,

        I think current patch is fine as well.
        IMHO, if there is any custom format which provide sections information, then it should be supported.

        IMHO, either way it should be fine.

        Show
        Rajesh Taneja added a comment - Thanks Marina, I think current patch is fine as well. IMHO, if there is any custom format which provide sections information, then it should be supported. IMHO, either way it should be fine.
        Hide
        Marina Glancy added a comment -

        no, current patch is not fine. The error will appear when course format is changed from weeks/topics to some other course format that has courseformatoptions but they do not include numsections

        Show
        Marina Glancy added a comment - no, current patch is not fine. The error will appear when course format is changed from weeks/topics to some other course format that has courseformatoptions but they do not include numsections
        Hide
        Marina Glancy added a comment -

        PS current patch would be fine if it was checking

         if (empty($courseformatoptions['numsections']))
        
        Show
        Marina Glancy added a comment - PS current patch would be fine if it was checking if (empty($courseformatoptions['numsections']))
        Hide
        Rajesh Taneja added a comment -

        Well, if course format is setting courseformatoptions then it should is supposed to set numsections.
        Anyway, it won't harm to do specific check.

        +1 to do specific check for numsections.

        @Mohammed: Can you please update your patch, so I can pus it forward for integration.

        Show
        Rajesh Taneja added a comment - Well, if course format is setting courseformatoptions then it should is supposed to set numsections. Anyway, it won't harm to do specific check. +1 to do specific check for numsections. @Mohammed: Can you please update your patch, so I can pus it forward for integration.
        Hide
        Marina Glancy added a comment -

        Raj, not at all. My singleactivity course format for example does not have numsections option but has others.

        Show
        Marina Glancy added a comment - Raj, not at all. My singleactivity course format for example does not have numsections option but has others.
        Hide
        Adrian Greeve added a comment -

        Oops, It looks like this is under control. I'll remove myself as the peer reviewer.

        Show
        Adrian Greeve added a comment - Oops, It looks like this is under control. I'll remove myself as the peer reviewer.
        Hide
        Rajesh Taneja added a comment -

        Hello Mohammed,

        Can you please update patch, with Marina's input so I can push it for integration.

        Show
        Rajesh Taneja added a comment - Hello Mohammed, Can you please update patch, with Marina's input so I can push it for integration.
        Hide
        Mohamed Alsharaf added a comment -

        @Rajesh I have updated the patch. Thanks

        Show
        Mohamed Alsharaf added a comment - @Rajesh I have updated the patch. Thanks
        Hide
        Rajesh Taneja added a comment -

        Thanks Mohammed,

        Pushing for integration.

        Show
        Rajesh Taneja added a comment - Thanks Mohammed, Pushing for integration.
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Uhm... I'm going to integrate this but... somehow related... it's a bit annoying that while courseformat->get_format_options() does not return "numsections" anymore... instead... courseformat->get_sections() continues returning them. So code relying on that, instead of options will be inaccurate.

        Anyway, pushing this...

        Show
        Eloy Lafuente (stronk7) added a comment - Uhm... I'm going to integrate this but... somehow related... it's a bit annoying that while courseformat->get_format_options() does not return "numsections" anymore... instead... courseformat->get_sections() continues returning them. So code relying on that, instead of options will be inaccurate. Anyway, pushing this...
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Integrated (24, 25 and master), thanks!

        Show
        Eloy Lafuente (stronk7) added a comment - Integrated (24, 25 and master), thanks!
        Hide
        Frédéric Massart added a comment -

        Test passed, thanks!

        Show
        Frédéric Massart added a comment - Test passed, thanks!
        Hide
        Damyon Wiese added a comment -

        Moodle has many old functions,
        And although they cause no malfunction,
        There comes a day,
        When they get deprecated away,
        And get and put on the list for expulsion.

        Thanks for all the reports/testing/fixes this week. This issue has been sent upstream.

        Show
        Damyon Wiese added a comment - Moodle has many old functions, And although they cause no malfunction, There comes a day, When they get deprecated away, And get and put on the list for expulsion. Thanks for all the reports/testing/fixes this week. This issue has been sent upstream.

          People

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

            Dates

            • Created:
              Updated:
              Resolved: