Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-37615

Section links block needs to be updated and cleaned up.

          Details

          • Type: Improvement
          • Status: Closed
          • Priority: 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 from Repository:
          • Pull Master Branch:
            wip-MDL-37615-m25
          • Pull Master Diff URL:

            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....

              Gliffy Diagrams

                Attachments

                  Issue Links

                  has a non-specific relationship to

                  Sub-task - Issues are sometimes broken into multiple sub-tasks. MDL-30062 core_course_update_courses()

                  • Minor - Minor loss of function where workaround is possible
                  • Closed
                  has been marked as being related by

                  Sub-task - Issues are sometimes broken into multiple sub-tasks. MDL-32593 Remove the section links block..

                  • Minor - Minor loss of function where workaround is possible
                  • Closed
                  is blocked by

                  Improvement - An enhancement to an existing Moodle feature. MDL-37535 Fix block_section_links use of config_plugins

                  • Minor - Minor loss of function where workaround is possible
                  • Closed

                    Activity

                    Ascending order - Click to sort in descending order
                    Hide
                    Permalink
                    samhemelryk 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
                    samhemelryk 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
                    Permalink
                    samhemelryk Sam Hemelryk added a comment -

                    Putting this up for peer-review now.

                    Show
                    samhemelryk Sam Hemelryk added a comment - Putting this up for peer-review now.
                    Hide
                    Permalink
                    samhemelryk 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
                    samhemelryk 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
                    Permalink
                    rajeshtaneja 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
                    rajeshtaneja 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
                    Permalink
                    samhemelryk 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
                    samhemelryk 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
                    Permalink
                    poltawski Dan Poltawski added a comment -

                    Integrated, thanks Sam

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

                    Show
                    poltawski Dan Poltawski added a comment - Integrated, thanks Sam (i'd have just rm'd the block :-P )
                    Hide
                    Permalink
                    poltawski 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
                    poltawski 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
                    Permalink
                    poltawski Dan Poltawski added a comment -

                    Ping, please can this be tested?

                    Show
                    poltawski Dan Poltawski added a comment - Ping, please can this be tested?
                    Hide
                    Permalink
                    poltawski 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
                    poltawski 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
                    Permalink
                    stronk7 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
                    stronk7 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:
                          Fix Release Date:
                          14/May/13