Moodle

Section links block needs to be updated and cleaned up.

Details

  • Type: Improvement Improvement
  • Status: Closed 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 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....

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 - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
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 - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.
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 - The issue is considered finished, the resolution is correct. Issues which are closed should not be reopened.

Activity

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

Ping, please can this be tested?

Show
Dan Poltawski added a comment - Ping, please can this be tested?
Hide
Permalink
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
Permalink
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

Vote (0)
Watch (3)

Dates

  • Created:
    Updated:
    Resolved: