Uploaded image for project: '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
    • Status: Closed
    • Priority: 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:

      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
        

        Gliffy Diagrams

          Activity

          Hide
          mudrd8mz 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
          mudrd8mz 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 Mohamed Alsharaf added a comment -
          Show
          mohamed.alsharaf 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
          rajeshtaneja 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
          rajeshtaneja 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 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 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
          rajeshtaneja 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
          rajeshtaneja 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 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 Mohamed Alsharaf added a comment - @Rajesh I have updated the code in the branches in comment "03/Jul/13 8:03 PM". Thanks
          Hide
          rajeshtaneja Rajesh Taneja added a comment -

          Thanks Mohamed,

          I have added your branches.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks Mohamed, I have added your branches.
          Hide
          marina 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 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
          rajeshtaneja 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
          rajeshtaneja 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 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 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 Marina Glancy added a comment -

          PS current patch would be fine if it was checking

           if (empty($courseformatoptions['numsections']))

          Show
          marina Marina Glancy added a comment - PS current patch would be fine if it was checking if (empty($courseformatoptions['numsections']))
          Hide
          rajeshtaneja 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
          rajeshtaneja 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 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 Marina Glancy added a comment - Raj, not at all. My singleactivity course format for example does not have numsections option but has others.
          Hide
          abgreeve Adrian Greeve added a comment -

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

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

          Hello Mohammed,

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

          Show
          rajeshtaneja 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 Mohamed Alsharaf added a comment -

          @Rajesh I have updated the patch. Thanks

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

          Thanks Mohammed,

          Pushing for integration.

          Show
          rajeshtaneja Rajesh Taneja added a comment - Thanks Mohammed, Pushing for integration.
          Hide
          stronk7 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
          stronk7 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
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Integrated (24, 25 and master), thanks!

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

          Test passed, thanks!

          Show
          fred Frédéric Massart added a comment - Test passed, thanks!
          Hide
          damyon 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 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:
                Fix Release Date:
                9/Sep/13