Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide
      1. Go to a course, turn editing on
      2. Click the increase sections button and confirm that a new section is added
      3. Click to reduce sections button and confirm section is removed
      4. Attempt to reduce the sections to less than 0
      Show
      Go to a course, turn editing on Click the increase sections button and confirm that a new section is added Click to reduce sections button and confirm section is removed Attempt to reduce the sections to less than 0
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      39768

      Description

      So you don't need to go into course settings to reduce the number of sections.

      In practice this will need to actually rearrange sections and reduce the number of sections in the coruse

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Ruslan/Andrew we're going to need to make your JS remove the delete button if a course module is moved with this (I might have a crack at it)

          Show
          Dan Poltawski added a comment - Ruslan/Andrew we're going to need to make your JS remove the delete button if a course module is moved with this (I might have a crack at it)
          Hide
          Sam Hemelryk added a comment -

          Hi Dan,

          I really like the idea of the this new feature. It certainly makes very good sense and I'm sure people will appreciate it.

          Looking over the code most things seem to be in good order.
          There are a couple of things that I noted:

          1. At present the code isn't cleaning up files used within the section description when the section is being deleted. This needs to be fixed.
          2. I think confirmation is needed before deleting a section. While there may be no activities or resources their may be a meaningful name and/or description to the section and it would not be recoverable if the user accidentally hit the delete icon when trying to hide a section.
          3. course_delete_empty_section
            • Rather than using get_records to check if there are any modules it would be better to count_records and perhaps even better again in this circumstance to use record_exists && IGNORE_MULTIPLE.
            • I really think that this method should be responsible for reducing course->numsections.

          There was one other thing that occurred to me, perhaps it would be wise to give course formats a callback that gets fired when a section is deleted in case they need to do any other clean up or any other related operations.
          However I don't think it is presently needed and given course formats are still due to change perhaps best we keep API changes minimal.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Dan, I really like the idea of the this new feature. It certainly makes very good sense and I'm sure people will appreciate it. Looking over the code most things seem to be in good order. There are a couple of things that I noted: At present the code isn't cleaning up files used within the section description when the section is being deleted. This needs to be fixed. I think confirmation is needed before deleting a section. While there may be no activities or resources their may be a meaningful name and/or description to the section and it would not be recoverable if the user accidentally hit the delete icon when trying to hide a section. course_delete_empty_section Rather than using get_records to check if there are any modules it would be better to count_records and perhaps even better again in this circumstance to use record_exists && IGNORE_MULTIPLE. I really think that this method should be responsible for reducing course->numsections. There was one other thing that occurred to me, perhaps it would be wise to give course formats a callback that gets fired when a section is deleted in case they need to do any other clean up or any other related operations. However I don't think it is presently needed and given course formats are still due to change perhaps best we keep API changes minimal. Cheers Sam
          Hide
          Dan Poltawski added a comment -

          Thanks Sam, good points. Initially with this i'd not considered deleting the sections at all - rather rejigging the order. In fact I still think that deleting sections is a bit heavyweight, because someone only needs to increase the number of sections and a new section is then created. Hmm.

          1. Indeed, I hadn't considered this, maybe this actually counts as a being non-empty..
          2. This takes away some of the speed of it so makes me think fo 1.
          3.
          a) Good point, I had considered IGNORE_MULTIPLE but shyed away from the comment
          b) Hmm. On course_delete_empty_section being responsible for reducing the number of sections, the reason I didn't do that is because this could potentially be used to to delete sections > numsections. (Yes that is possible with the script I created, but you'd need to do it by altering the URL). Maybe I should do this only if the section less than number of sections.

          Show
          Dan Poltawski added a comment - Thanks Sam, good points. Initially with this i'd not considered deleting the sections at all - rather rejigging the order. In fact I still think that deleting sections is a bit heavyweight, because someone only needs to increase the number of sections and a new section is then created. Hmm. 1. Indeed, I hadn't considered this, maybe this actually counts as a being non-empty.. 2. This takes away some of the speed of it so makes me think fo 1. 3. a) Good point, I had considered IGNORE_MULTIPLE but shyed away from the comment b) Hmm. On course_delete_empty_section being responsible for reducing the number of sections, the reason I didn't do that is because this could potentially be used to to delete sections > numsections. (Yes that is possible with the script I created, but you'd need to do it by altering the URL). Maybe I should do this only if the section less than number of sections.
          Hide
          Dan Poltawski added a comment -

          OK, Martin and I just discussed it and think this is probably overkill and instead just got for a + - at the bottom right of of the course to quickly increase/decrease the number of sections.

          Show
          Dan Poltawski added a comment - OK, Martin and I just discussed it and think this is probably overkill and instead just got for a + - at the bottom right of of the course to quickly increase/decrease the number of sections.
          Hide
          Dan Poltawski added a comment -

          OK, submitting this simplified version for integration. Its not fairly straight forward, only issue is really the icons are too small.

          • addsection.php is renamed to changenumsections.php to reflect new purpose
          • The add button is changed to a +- toggle rahter than big 'add section'
          Show
          Dan Poltawski added a comment - OK, submitting this simplified version for integration. Its not fairly straight forward, only issue is really the icons are too small. addsection.php is renamed to changenumsections.php to reflect new purpose The add button is changed to a +- toggle rahter than big 'add section'
          Hide
          Dan Poltawski added a comment -

          Attaching a screenshot to demonstrate we really ned bigger icons for the +/-

          Show
          Dan Poltawski added a comment - Attaching a screenshot to demonstrate we really ned bigger icons for the +/-
          Hide
          Sam Hemelryk added a comment -

          Hi Dan,

          The code looks really good and I think this is a better solution than the previous.
          Two things I've noted neither of which actually block this issue from integration but perhaps you would like to address:

          1. The use of output->spacer... I STRONGLY dislike spacer. I admit it has its uses every now and again, however I think in this case 100% it should be done either with a   or better yet in CSS. Spacers lead to an unneeded image being loaded and are an annoyance to themers wanting to tweak things.
          2. The echo $this->output->action_link( lines have become rather complex and lenghty, perhaps it is in the best interests of readability if separate components were initialised and passed to action_link, or even to instantiate an action_link instance.

          Both food for thought and nothing more, I'll leave this in integration for your feedback.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Dan, The code looks really good and I think this is a better solution than the previous. Two things I've noted neither of which actually block this issue from integration but perhaps you would like to address: The use of output->spacer... I STRONGLY dislike spacer. I admit it has its uses every now and again, however I think in this case 100% it should be done either with a   or better yet in CSS. Spacers lead to an unneeded image being loaded and are an annoyance to themers wanting to tweak things. The echo $this->output->action_link( lines have become rather complex and lenghty, perhaps it is in the best interests of readability if separate components were initialised and passed to action_link, or even to instantiate an action_link instance. Both food for thought and nothing more, I'll leave this in integration for your feedback. Cheers Sam
          Hide
          Dan Poltawski added a comment -

          Ha, I thought that spacer did do an nbsp!

          Show
          Dan Poltawski added a comment - Ha, I thought that spacer did do an nbsp!
          Hide
          Dan Poltawski added a comment -

          Ok, thanks Sam - i've updated my branch breaking those lines up and introducing the spacing in CSS.

          In truth as well as being lazy I think I used a spacer because i'm always a bit unsure about rules for writing good CSS selectors/naming of elements. If you have any advice on that be good to hear!

          (btw switched to using a link rather than action link, no idea why I did that in first place)

          Show
          Dan Poltawski added a comment - Ok, thanks Sam - i've updated my branch breaking those lines up and introducing the spacing in CSS. In truth as well as being lazy I think I used a spacer because i'm always a bit unsure about rules for writing good CSS selectors/naming of elements. If you have any advice on that be good to hear! (btw switched to using a link rather than action link, no idea why I did that in first place)
          Hide
          Ruslan Kabalin added a comment -

          What will happen on the attempt to remove non-empty section?

          Show
          Ruslan Kabalin added a comment - What will happen on the attempt to remove non-empty section?
          Hide
          Dan Poltawski added a comment -

          It will be hidden, just like now if you change the number of sections in the course settings.

          (note I abandoned the changes discussed above about actually deleting a section)

          Show
          Dan Poltawski added a comment - It will be hidden, just like now if you change the number of sections in the course settings. (note I abandoned the changes discussed above about actually deleting a section)
          Hide
          Sam Hemelryk added a comment -

          Thanks for tidying those up Dan, I've integrated this now.

          In regards to CSS a couple of things I try to work by:

          1. Always give the root node an ID (has to be unique XHTML) and for all of your CSS for that component start the rule with that ID.
          2. Give things descriptive classes, I try to give all important nodes classes so that even if I'm not styling them someone else can.
          3. When writing rules don't overuse classes to denote structure (bad for rendering performance), because you gave the first element a good ID and things have descriptive classes most of the time you only need the id and a single class in your CSS rule.
          4. Avoid using  , and br for structure (fine of course in paragraphs of text). They are both things that can normally be achieved through CSS in 99% of cases, and if handled in CSS then themers can much more easily alter things.

          Personally I find even if I get those right the rest falls into place pretty easily, and even if I do mess up the CSS its normally easier to fix things up as at least I started in the right place.
          We don't have very many rules for CSS, really just that base theme must stay simple and contain only the absolutely essential styling.
          Other than that it just has to look right.
          The benefits to following a method, and/or best practices are the same as any other language. Things are easier to maintain and work with in the future.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks for tidying those up Dan, I've integrated this now. In regards to CSS a couple of things I try to work by: Always give the root node an ID (has to be unique XHTML) and for all of your CSS for that component start the rule with that ID. Give things descriptive classes, I try to give all important nodes classes so that even if I'm not styling them someone else can. When writing rules don't overuse classes to denote structure (bad for rendering performance), because you gave the first element a good ID and things have descriptive classes most of the time you only need the id and a single class in your CSS rule. Avoid using  , and br for structure (fine of course in paragraphs of text). They are both things that can normally be achieved through CSS in 99% of cases, and if handled in CSS then themers can much more easily alter things. Personally I find even if I get those right the rest falls into place pretty easily, and even if I do mess up the CSS its normally easier to fix things up as at least I started in the right place. We don't have very many rules for CSS, really just that base theme must stay simple and contain only the absolutely essential styling. Other than that it just has to look right. The benefits to following a method, and/or best practices are the same as any other language. Things are easier to maintain and work with in the future. Cheers Sam
          Hide
          Michael de Raadt added a comment -

          Test result: Success

          Tested in master. Works nicely with and without AJAX turned on in multiple courses.

          I've added docs related labels to this issue as this is an interface change.

          This will resolve an issue that has been lingering for a while. I will look that up.

          Show
          Michael de Raadt added a comment - Test result: Success Tested in master. Works nicely with and without AJAX turned on in multiple courses. I've added docs related labels to this issue as this is an interface change. This will resolve an issue that has been lingering for a while. I will look that up.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome?

          Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U

          Closing, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - This is now part of Moodle and a few millions people around the globe will be using it soon. Isn't that awesome? Many, many thanks and don't forget http://youtu.be/4N7dPaP5Z8U Closing, ciao
          Hide
          Mary Cooch added a comment -
          Show
          Mary Cooch added a comment - Now documented here http://docs.moodle.org/23/en/Course_homepage

            People

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

              Dates

              • Created:
                Updated:
                Resolved: