Moodle
  1. Moodle
  2. MDL-37535

Fix block_section_links use of config_plugins

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3.4, 2.4.1
    • Fix Version/s: 2.5
    • Component/s: Blocks
    • Labels:
    • Testing Instructions:
      Hide

      Before upgrade...

      1. Log in as an admin.
      2. Browse to Settings > Plugins > Blocks > Section links setting.
      3. Change the default values and remember what you changed them to.
      4. Browse to a course.
      5. Add an instance of the section links block.

      After upgrade...

      1. Log in as an admin
      2. Browse to Settings > Plugins > Blocks > Section links setting.
      3. Make sure the values you selected are still there.
      4. Browse to the course.
      5. Make sure the section links block still works.
      Show
      Before upgrade... Log in as an admin. Browse to Settings > Plugins > Blocks > Section links setting. Change the default values and remember what you changed them to. Browse to a course. Add an instance of the section links block. After upgrade... Log in as an admin Browse to Settings > Plugins > Blocks > Section links setting. Make sure the values you selected are still there. Browse to the course. Make sure the section links block still works.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-37535-m25
    • Rank:
      47182

      Description

      While reviewing MDL-34344 it was detected that the block_section_links block:

      1) Is storing its plugin settings into blocks/section_links, while it should be using proper component name. So we must move those settings (upgrade) and switch to component.

      2) Surely because of the exceptional (incorrect) plugin name used... the block has implemented the before_delete() method. Once moved to proper component, it should be not needed any more. Standard plugin uninstallation takes rid of related config automatically.

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          Putting this up for peer-review now.

          Show
          Sam Hemelryk added a comment - Putting this up for peer-review now.
          Hide
          Rossiani Wijaya added a comment -

          Hi Sam,

          Just minor change needed for the upgrade.php. I think the value for the 3rd param in upgrade_block_savepoint() should be 'section_links' instead of 'block_section_links'.

          Other than that, the code looks great.

          [y] Syntax
          [-] Output
          [y] Whitespace
          [-] Language
          [y] Databases
          [-] Testing
          [-] Security
          [-] Documentation
          [y] Git
          [y] Sanity check

          Show
          Rossiani Wijaya added a comment - Hi Sam, Just minor change needed for the upgrade.php. I think the value for the 3rd param in upgrade_block_savepoint() should be 'section_links' instead of 'block_section_links'. Other than that, the code looks great. [y] Syntax [-] Output [y] Whitespace [-] Language [y] Databases [-] Testing [-] Security [-] Documentation [y] Git [y] Sanity check
          Hide
          Sam Hemelryk added a comment -

          Thanks Rosie, I've fixed up that upgrade call and am putting this up for integration review now.

          Show
          Sam Hemelryk added a comment - Thanks Rosie, I've fixed up that upgrade call and am putting this up for integration review now.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Dan Poltawski added a comment -

          This is blocking MDL-37615 so taking it.

          Show
          Dan Poltawski added a comment - This is blocking MDL-37615 so taking it.
          Hide
          Dan Poltawski added a comment -

          Integrated, thanks Sam

          (i'd have just rm'd the block :-P )

          Show
          Dan Poltawski added a comment - Integrated, thanks Sam (i'd have just rm'd the block :-P )
          Hide
          David Monllaó added a comment -

          Tested with latest stable and integration, it passes

          Show
          David Monllaó added a comment - Tested with latest stable and integration, it passes
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: