Moodle
  1. Moodle
  2. MDL-37615

Section links block needs to be updated and cleaned up.

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1
    • Fix Version/s: 2.5
    • Component/s: Blocks
    • Labels:
    • Testing Instructions:
      Hide
      1. Log in as an admin
      2. Create two courses:
        • Course A: using weeks format with 10 sections.
        • Course B: using topics format with 50 topics.
      3. In each course add a sections links block to the front page.
        • Check course A shows 1..10
        • Check course B shows 5,10,15...50
      4. In course B click the edit icon for the section links blocks.
      5. Chang alternative increase by from 5 => 3. Browse back to the course.
      6. Check course B shows 3,6,9...
      7. Browse to Settings > Plugins > Blocks > Section links
      8. Change the settings as follows:
        • Number of sections => 2
        • Increase by => 2
        • Alternative number of sections => 5
        • Alternative increase by => 2
      9. Browse to course A and check that the section block shows 2,4,6,...
      10. Browse to course B and check that the section block still hows 3,6,9...
      Show
      Log in as an admin Create two courses: Course A: using weeks format with 10 sections. Course B: using topics format with 50 topics. In each course add a sections links block to the front page. Check course A shows 1..10 Check course B shows 5,10,15...50 In course B click the edit icon for the section links blocks. Chang alternative increase by from 5 => 3. Browse back to the course. Check course B shows 3,6,9... Browse to Settings > Plugins > Blocks > Section links Change the settings as follows: Number of sections => 2 Increase by => 2 Alternative number of sections => 5 Alternative increase by => 2 Browse to course A and check that the section block shows 2,4,6,... Browse to course B and check that the section block still hows 3,6,9...
    • Affected Branches:
      MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-37615-m25
    • Rank:
      47305

      Description

      There are several things wrong with the section links block:

      1. Block instance configuration doesn't work. Still has config_instance.html file, should be using edit_form.php.
      2. Doesn't conform with our current coding style.
      3. Could be made more effecient by using modinfo cache.
      4. Could be using a renderer.

      Just to get started....

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          This is currently based upon my work on MDL-37535. There are two commits on the branch, one for that issue and one for this issue.

          Show
          Sam Hemelryk added a comment - This is currently based upon my work on MDL-37535 . There are two commits on the branch, one for that issue and one for this issue.
          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
          Sam Hemelryk added a comment -

          Ok I've tidied this up quite a bit.
          It addresses the points noted and gets the instance configuration working once more.

          Testing instruction will be added once this has been peer-reviewed to ensure they are accurate to any changes.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Ok I've tidied this up quite a bit. It addresses the points noted and gets the instance configuration working once more. Testing instruction will be added once this has been peer-reviewed to ensure they are accurate to any changes. Cheers Sam
          Hide
          Rajesh Taneja added a comment -

          Thanks for improving this code Sam,

          Patch looks spot-on, although you might consider:

          1. Fixing brackets in block_section_links.php -line 74 () (brackets) missing for stdClass
          2. I think you missed usage of $inc in foreach https://github.com/samhemelryk/moodle/commit/c518ea1c4b9c146d1a186af8e263af53a5c928df#L0R115

          Should we consider dimming section number, while editing in AJAX mode and hiding a section? Probably a different improvement.

          Show
          Rajesh Taneja added a comment - Thanks for improving this code Sam, Patch looks spot-on, although you might consider: Fixing brackets in block_section_links.php -line 74 () (brackets) missing for stdClass I think you missed usage of $inc in foreach https://github.com/samhemelryk/moodle/commit/c518ea1c4b9c146d1a186af8e263af53a5c928df#L0R115 Should we consider dimming section number, while editing in AJAX mode and hiding a section? Probably a different improvement.
          Hide
          Sam Hemelryk added a comment -

          Thanks for looking at that Raj.

          I've refactored that loop to factor $inc back in again.
          Re: stdClass brackets, I perfer to leave the brackets off. Presently devs can do as they please, we don't appear to have a strict rule to that.

          As for dimming section numbers that should be a new feature request if we do want to implement it.
          I want this issue to be purely about updating the code rather than introducing anything new.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for looking at that Raj. I've refactored that loop to factor $inc back in again. Re: stdClass brackets, I perfer to leave the brackets off. Presently devs can do as they please, we don't appear to have a strict rule to that. As for dimming section numbers that should be a new feature request if we do want to implement it. I want this issue to be purely about updating the code rather than introducing anything new. Many thanks Sam
          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
          Dan Poltawski added a comment -

          DEAR TESTER. Please could you look through the subtasks on MDL-32593 and see if they have been fixed by this issue? Close them if so.

          thanks!

          Show
          Dan Poltawski added a comment - DEAR TESTER. Please could you look through the subtasks on MDL-32593 and see if they have been fixed by this issue? Close them if so. thanks!
          Hide
          Dan Poltawski added a comment -

          Ping, please can this be tested?

          Show
          Dan Poltawski added a comment - Ping, please can this be tested?
          Hide
          Dan Poltawski added a comment -

          It looks good and passes. Just noting that Martin shared my opinion about getting of this block in MDL-32593

          Show
          Dan Poltawski added a comment - It looks good and passes. Just noting that Martin shared my opinion about getting of this block in MDL-32593
          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:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: