Details

    • Type: New Feature
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 1.8, 1.8.1, 1.8.2, 1.9, 2.0.2, 2.1, 2.2, 2.6, 2.8.1
    • Fix Version/s: 2.9
    • Component/s: Course
    • Testing Instructions:
      Hide

      pretty good covered by behat tests
      play around with deleting sections in topics and weeks formats, especially orphaned ones

      set format options to display one section per page. Make sure when you delete section you are returned to the course page, when you cancel deleting a section you are returned to the page where you previously were (course or section)

      Show
      pretty good covered by behat tests play around with deleting sections in topics and weeks formats, especially orphaned ones set format options to display one section per page. Make sure when you delete section you are returned to the course page, when you cancel deleting a section you are returned to the page where you previously were (course or section)
    • Affected Branches:
      MOODLE_18_STABLE, MOODLE_19_STABLE, MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE, MOODLE_26_STABLE, MOODLE_28_STABLE
    • Fixed Branches:
      MOODLE_29_STABLE
    • Pull Master Branch:
      wip-MDL-10405-master

      Description

      This has been discussed before, notably in bug 2245. There is no way to delete a section (e.g. week or topic) from a Moodle course. There seems to be general agreement that this is A Good Thing, but I can't really see why. I understand the argument that you don't want the teacher deleting a populated section by accident, but you can't even delete an empty section.

      The usual workaround is to move the offending section to the bottom of the course page, and reduce the number of sections the course displays. However, bug 4719 indicates the drawbacks in this approach. If a teacher wants to add a new section by increasing this number, the "deleted" block reappears. This is irksome.

      If a teacher/editor has powers to delete a resource or activity, shouldn't they have the power to delete an entire section? You are asked for confirmation in cautionary terms every time you attempt to delete a resource, so why not have the same for a section, e.g. "Are you absolutely sure you want to completely delete the section 'foo'?"

        Gliffy Diagrams

        1. delete.section.part1.diff
          9 kB
          Daniel Neis
        2. delete.section.part2.diff
          3 kB
          Daniel Neis
        3. MDL-10405.diff
          4 kB
          Daniel Neis
        4. MDL-10405.full.diff
          9 kB
          Daniel Neis
        1. 01_current.png
          70 kB
        2. mockup.png
          73 kB
        3. option1.png
          71 kB
        4. option2.png
          72 kB

          Issue Links

            Activity

            Hide
            estephan500 eric stephan added a comment -

            OOooof, if there ever was a time that I wanted to stuff a ballot box, this is it.

            This has just generated a real, and ridiculous, problem for me.

            In a course, a fellow administrator wrongheadedly added a bunch of new topics to the course, like 10 of them, thinking that new material would be added.

            He was wrong. But these topics are interspersed throughout the course. To delete these 10 topics, I am going to (1) move each of them to the bottom one topic at a time, one step at a time – because there is no "move to this location box" feature for moving topics? ... and then do the silly limit-the-number-of-topics feature?

            I'm hoping there is some other way – but this could be hours of ridiculous clicking, to delete 10 topics?

            thx
            eric

            Show
            estephan500 eric stephan added a comment - OOooof, if there ever was a time that I wanted to stuff a ballot box, this is it. This has just generated a real, and ridiculous, problem for me. In a course, a fellow administrator wrongheadedly added a bunch of new topics to the course, like 10 of them, thinking that new material would be added. He was wrong. But these topics are interspersed throughout the course. To delete these 10 topics, I am going to (1) move each of them to the bottom one topic at a time, one step at a time – because there is no "move to this location box" feature for moving topics? ... and then do the silly limit-the-number-of-topics feature? I'm hoping there is some other way – but this could be hours of ridiculous clicking, to delete 10 topics? thx eric
            Hide
            daveyboond Steve Bond added a comment -

            The way it works at present is effectively duplicating functionality. There is already a way to hide sections. Reducing the number of sections is like a stronger version of the same thing (i.e. you're hiding them from all users, not just students), but it adds nothing in terms of usefulness.

            Another reason why it is annoying is that because (rightly) you cannot delete a file from the file store when it is being referenced by a resource. But that goes for the resources in non-visible (as opposed to hidden - you see how crazy this is!) sections as well. So if, for example, an editor inherits a course from a predecessor and tries to revamp it, they are find that they are unable to delete files that do not appear to be in use. Then they extend the number of topics to 25 and realise that they are going to spend the rest of the day clicking red crosses...

            Show
            daveyboond Steve Bond added a comment - The way it works at present is effectively duplicating functionality. There is already a way to hide sections. Reducing the number of sections is like a stronger version of the same thing (i.e. you're hiding them from all users, not just students), but it adds nothing in terms of usefulness. Another reason why it is annoying is that because (rightly) you cannot delete a file from the file store when it is being referenced by a resource. But that goes for the resources in non-visible (as opposed to hidden - you see how crazy this is!) sections as well. So if, for example, an editor inherits a course from a predecessor and tries to revamp it, they are find that they are unable to delete files that do not appear to be in use. Then they extend the number of topics to 25 and realise that they are going to spend the rest of the day clicking red crosses...
            Hide
            danielneis Daniel Neis added a comment -

            the related issue has a patch that works on 1.9.5

            Show
            danielneis Daniel Neis added a comment - the related issue has a patch that works on 1.9.5
            Hide
            danielneis Daniel Neis added a comment -

            The patches (delete.section.part1.diff and delete.section.part2.diff, which should be applied in this order) implement the code to remove a section of a course (both in weekly and topics format, ajax and not) with all it's modules. Hope you like the solution.

            Show
            danielneis Daniel Neis added a comment - The patches (delete.section.part1.diff and delete.section.part2.diff, which should be applied in this order) implement the code to remove a section of a course (both in weekly and topics format, ajax and not) with all it's modules. Hope you like the solution.
            Hide
            aborrow Anthony Borrow added a comment -

            As requested by Daniel - I am adding 1.9 as an affected version. Peace - Anthony

            Show
            aborrow Anthony Borrow added a comment - As requested by Daniel - I am adding 1.9 as an affected version. Peace - Anthony
            Hide
            swaroop Swaroop Kumar added a comment -

            Functions that are written in patches will delete the modules of section from section sequence and course_modules table but they doesn't delete the instance record from the module specific table. In other words it doesn't execute the $deleteinstancefunction for the module.

            Show
            swaroop Swaroop Kumar added a comment - Functions that are written in patches will delete the modules of section from section sequence and course_modules table but they doesn't delete the instance record from the module specific table. In other words it doesn't execute the $deleteinstancefunction for the module.
            Hide
            danielneis Daniel Neis added a comment -

            Hello, Swaroop Kumar

            thanks for your comments. I have made some modifications in the deletesection system since i've uploaded the patches to correct what you said but forgot to re-upload them.

            So now, MDL-10405.diff is a new patch that should be applied INSTEAD of the other two.

            Thank you again and good luck.

            Show
            danielneis Daniel Neis added a comment - Hello, Swaroop Kumar thanks for your comments. I have made some modifications in the deletesection system since i've uploaded the patches to correct what you said but forgot to re-upload them. So now, MDL-10405 .diff is a new patch that should be applied INSTEAD of the other two. Thank you again and good luck.
            Hide
            accurateit Gary Greer added a comment -

            I've applied the patch, but can't see any change to the UI. How is this exposed to the user?

            Show
            accurateit Gary Greer added a comment - I've applied the patch, but can't see any change to the UI. How is this exposed to the user?
            Hide
            swaroop Swaroop Kumar added a comment -

            Third patch doesn't include the delete section link in the course page. Applying the first patch and then the third one shud work i guess.

            Show
            swaroop Swaroop Kumar added a comment - Third patch doesn't include the delete section link in the course page. Applying the first patch and then the third one shud work i guess.
            Hide
            danielneis Daniel Neis added a comment -

            Yes, Swaroop

            you're right. I forgot that part on the last patch, and also the rest.php part.
            The new file MDL-10405.full.diff has the complete implementation.

            Sorry for the trouble.

            Show
            danielneis Daniel Neis added a comment - Yes, Swaroop you're right. I forgot that part on the last patch, and also the rest.php part. The new file MDL-10405 .full.diff has the complete implementation. Sorry for the trouble.
            Hide
            bazzymg Basil Gohar added a comment -

            I would like to see this feature added to Moodle 2.0, as well. There is still no way to do this without a patch, and I am not sure this patch will work for 2.0 at the moment.

            Show
            bazzymg Basil Gohar added a comment - I would like to see this feature added to Moodle 2.0, as well. There is still no way to do this without a patch, and I am not sure this patch will work for 2.0 at the moment.
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            i've rewritten the patch to work on Moodle 2.1.1+.
            You can see the code on github and also download a patch version.

            Hope you like,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, i've rewritten the patch to work on Moodle 2.1.1+. You can see the code on github and also download a patch version . Hope you like, Daniel
            Show
            danielneis Daniel Neis added a comment - http://moodle.org/mod/forum/discuss.php?d=186801
            Hide
            timhunt Tim Hunt added a comment -

            Nothing to do with me guv.

            Show
            timhunt Tim Hunt added a comment - Nothing to do with me guv.
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            i've rebased the patch to last MOODLE_22_STABLE. See the full patch at: https://github.com/danielneis/moodle/compare/MOODLE_22_STABLE...MDL-10405

            Show
            danielneis Daniel Neis added a comment - Hello, i've rebased the patch to last MOODLE_22_STABLE. See the full patch at: https://github.com/danielneis/moodle/compare/MOODLE_22_STABLE...MDL-10405
            Hide
            gaudreaj Jean-Philippe Gaudreau added a comment -

            Hi Daniel,

            I merged the diff you gave and it seems there's a bug when deleting the first section and trying to add a file resource in the last one. The file resource is put in the section "Orphaned activities".

            Just to let you know. Maybe it's my merge.. I did it pretty fast just to check what it looks like. Anyway great job it will sure be a great improvement!

            Show
            gaudreaj Jean-Philippe Gaudreau added a comment - Hi Daniel, I merged the diff you gave and it seems there's a bug when deleting the first section and trying to add a file resource in the last one. The file resource is put in the section "Orphaned activities". Just to let you know. Maybe it's my merge.. I did it pretty fast just to check what it looks like. Anyway great job it will sure be a great improvement!
            Hide
            gaudreaj Jean-Philippe Gaudreau added a comment -

            Seems like no one's assign for "Peer review in progress". Status must be changed.

            Show
            gaudreaj Jean-Philippe Gaudreau added a comment - Seems like no one's assign for "Peer review in progress". Status must be changed.
            Hide
            danielneis Daniel Neis added a comment -

            Hello, Jean-Philippe

            unfortunately, i could not reproduce your problem. I've deleted some sections and added a file resource to the last section and it seems ok. If the "first section" is the section 0, it should not be able to remove (there shouldn't be a link at session 0) because the code tests this before deleting.

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, Jean-Philippe unfortunately, i could not reproduce your problem. I've deleted some sections and added a file resource to the last section and it seems ok. If the "first section" is the section 0, it should not be able to remove (there shouldn't be a link at session 0) because the code tests this before deleting. Kind regards, Daniel
            Hide
            gaudreaj Jean-Philippe Gaudreau added a comment -

            Hi Daniel,

            Sorry, by first section I meant the first one removeable. Ok well like I said, I only made a quick tour so it might be my integration of your fix that is wrong. I'll take more time in the next days to check it the right way.

            Was there anyone assigned as peer reviewer before it was "moodle.com"?

            Show
            gaudreaj Jean-Philippe Gaudreau added a comment - Hi Daniel, Sorry, by first section I meant the first one removeable. Ok well like I said, I only made a quick tour so it might be my integration of your fix that is wrong. I'll take more time in the next days to check it the right way. Was there anyone assigned as peer reviewer before it was "moodle.com"?
            Hide
            danielneis Daniel Neis added a comment -

            Hi, Jean-Philippe

            there was no peer reviewer before moodle.com =T
            Also, this patch is on production at moodle.ufsc.br since mid-february and we have around 900 sections deleted on logs =D

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hi, Jean-Philippe there was no peer reviewer before moodle.com =T Also, this patch is on production at moodle.ufsc.br since mid-february and we have around 900 sections deleted on logs =D Kind regards, Daniel
            Hide
            gaudreaj Jean-Philippe Gaudreau added a comment -

            Hi Daniel,

            It's seems there's a bug afterall... Just to be clear, I merged the code you gave in https://github.com/danielneis/moodle/compare/MOODLE_22_STABLE...MDL-10405. Could it be that there's only a problem with the modification you did on github and not on your production server?

            Here's my testing instructions :
            1- Create a new course
            2- Delete the first section removeable
            3- Add a file resource in the last section
            4- Save and return to the course
            You should see the resource in a new section called "Orphaned activities"

            I tested on Firefox 11.0 with ajax ON.

            Another problem on my side : In the confirmation box of deleting a section, I get "undefined" instead of "Are you sure you want to delete section ...? All activities and resources within this section will be deleted. This operation cannot be undone.". Problem is there even after purging all caches.

            Hope it helps!

            Jean-Philippe

            Show
            gaudreaj Jean-Philippe Gaudreau added a comment - Hi Daniel, It's seems there's a bug afterall... Just to be clear, I merged the code you gave in https://github.com/danielneis/moodle/compare/MOODLE_22_STABLE...MDL-10405 . Could it be that there's only a problem with the modification you did on github and not on your production server? Here's my testing instructions : 1- Create a new course 2- Delete the first section removeable 3- Add a file resource in the last section 4- Save and return to the course You should see the resource in a new section called "Orphaned activities" I tested on Firefox 11.0 with ajax ON. Another problem on my side : In the confirmation box of deleting a section, I get "undefined" instead of "Are you sure you want to delete section ...? All activities and resources within this section will be deleted. This operation cannot be undone.". Problem is there even after purging all caches. Hope it helps! Jean-Philippe
            Hide
            08201923 PEDRO HENRIQUE DIAS added a comment -

            Hi Daniel,

            I'm Pedro, from PUCRS.

            I had exactly the same problems as Jean-Philippe.
            After introductions tests, we see the "Orphaned activities" on last section. I also see another problem like Jean, and I click on Delete button, the message of confirmation box is "undefined".

            I think the ideia is very useful, we had already used it in version 1.9, and now we want use in Moodle 2.x.

            Congratulations for the initiative and I hope we can solve this problem.

            Pedro Dias

            Show
            08201923 PEDRO HENRIQUE DIAS added a comment - Hi Daniel, I'm Pedro, from PUCRS. I had exactly the same problems as Jean-Philippe. After introductions tests, we see the "Orphaned activities" on last section. I also see another problem like Jean, and I click on Delete button, the message of confirmation box is "undefined". I think the ideia is very useful, we had already used it in version 1.9, and now we want use in Moodle 2.x. Congratulations for the initiative and I hope we can solve this problem. Pedro Dias
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            i've managed to reproduce the bug: it occurred only with ajax (and thats why i couldn't reproduce it first time). The incorrect behaviour was because i was not updating the "jumpto" URLs of the "add resource" and "add activity" menus.
            The branch is updated on github with the fix.

            During the tests i noted two other minor problems: in the weekly format, after a delete numbers are added to sections (easy to solve, i guess) and the dates are not updated (not so easy, i guess, beacuse in 1.9 the move section didn't updates the dates too).

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, i've managed to reproduce the bug: it occurred only with ajax (and thats why i couldn't reproduce it first time). The incorrect behaviour was because i was not updating the "jumpto" URLs of the "add resource" and "add activity" menus. The branch is updated on github with the fix. During the tests i noted two other minor problems: in the weekly format, after a delete numbers are added to sections (easy to solve, i guess) and the dates are not updated (not so easy, i guess, beacuse in 1.9 the move section didn't updates the dates too). Kind regards, Daniel
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            i've also solved the problem with the confirmation message

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, i've also solved the problem with the confirmation message Kind regards, Daniel
            Hide
            gaudreaj Jean-Philippe Gaudreau added a comment -

            Hi Daniel,

            First of all, thx for the great work! It seems the two bugs are fixed now! Like you said, there's still those two minor problems. I think that the one with the dates that are not updated after deletion should be fix even though it maybe hard to do...

            Just a suggestion : In my opinion it would be better to put the delete button before the visible one to match the order they are shown for the resources/activities.

            Anyway thx again! I think this task isn't far from being completed.

            Jean-Philippe

            Show
            gaudreaj Jean-Philippe Gaudreau added a comment - Hi Daniel, First of all, thx for the great work! It seems the two bugs are fixed now! Like you said, there's still those two minor problems. I think that the one with the dates that are not updated after deletion should be fix even though it maybe hard to do... Just a suggestion : In my opinion it would be better to put the delete button before the visible one to match the order they are shown for the resources/activities. Anyway thx again! I think this task isn't far from being completed. Jean-Philippe
            Hide
            danielneis Daniel Neis added a comment -

            Hi, Jean-Philippe

            sorry for the hiatus, but i was busy with my undergraduate course...

            i've fixed the problems with the section ids, the weekdates (based on swap_dates) and also reordered the icon as you said.

            Also, i've rebased to the last MOODLE_22_STABLE branch. The diff is on the same link: https://github.com/danielneis/moodle/compare/MOODLE_22_STABLE...MDL-10405

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hi, Jean-Philippe sorry for the hiatus, but i was busy with my undergraduate course... i've fixed the problems with the section ids, the weekdates (based on swap_dates) and also reordered the icon as you said. Also, i've rebased to the last MOODLE_22_STABLE branch. The diff is on the same link: https://github.com/danielneis/moodle/compare/MOODLE_22_STABLE...MDL-10405 Kind regards, Daniel
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            i've started the work in rebase to MOODLE_23_STABLE :

            https://github.com/danielneis/moodle/compare/MOODLE_23_STABLE...MDL-10405-v23

            It is not updating the IDs correctly, so we have the same ajax bugs of the first version: topicnames not updated and incorrect ids after first delete (adding activities, deleting more than one topic will not work correctly). Any help woul be appreciated.

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, i've started the work in rebase to MOODLE_23_STABLE : https://github.com/danielneis/moodle/compare/MOODLE_23_STABLE...MDL-10405-v23 It is not updating the IDs correctly, so we have the same ajax bugs of the first version: topicnames not updated and incorrect ids after first delete (adding activities, deleting more than one topic will not work correctly). Any help woul be appreciated. Kind regards, Daniel
            Hide
            maherne Michael Aherne added a comment -

            I'm not sure if it's important, but this patch could leave orphaned records in the course_sections_availability table.

            Show
            maherne Michael Aherne added a comment - I'm not sure if it's important, but this patch could leave orphaned records in the course_sections_availability table.
            Hide
            derekcx Derek Chirnside added a comment -

            Just in case someone stumbles on this, there is a fix written up here which will work for some needs: https://moodle.org/mod/forum/discuss.php?d=243077

            ie backup and restore, but don't restore the sections you want.

            -Derek

            Show
            derekcx Derek Chirnside added a comment - Just in case someone stumbles on this, there is a fix written up here which will work for some needs: https://moodle.org/mod/forum/discuss.php?d=243077 ie backup and restore, but don't restore the sections you want. -Derek
            Hide
            bobpuffer Bob Puffer added a comment -

            Or you could try my Quickset block which has been available for the past three years:
            https://moodle.org/plugins/view.php?plugin=block_quickset

            Show
            bobpuffer Bob Puffer added a comment - Or you could try my Quickset block which has been available for the past three years: https://moodle.org/plugins/view.php?plugin=block_quickset
            Hide
            derekcx Derek Chirnside added a comment -

            @Robert. Never noticed this. I think I was misled by the title. There are now too many blocks for me to keep track of. Docs for V2.6 tweaked: http://docs.moodle.org/26/en/Course_homepage#To_delete_a_course_section

            -Derek

            Show
            derekcx Derek Chirnside added a comment - @Robert. Never noticed this. I think I was misled by the title. There are now too many blocks for me to keep track of. Docs for V2.6 tweaked: http://docs.moodle.org/26/en/Course_homepage#To_delete_a_course_section -Derek
            Hide
            danielneis Daniel Neis added a comment -

            Hello,

            the workaround to delete sections that is describe in the forum (move to all activities and resources to a section in the end and reduce numsections) is not really good and the current behaviour of the quickset block is "if resources exist in that section they will be moved to the "Zero" section", so it is not really what we want in this issue.

            I have updated the patch to the latest 2.6 stable branch and also fixed the "orphaned records in the course_sections_availability table." as noted by Michael.

            I still don't have the titles updating correcting maybe due some misunderstanding of the "M.course.format.process_sections" function. I would appreciate very much if someone could help with it.

            The new version is available at https://github.com/danielneis/moodle/tree/MDL-10405-v26

            Hope you like.

            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, the workaround to delete sections that is describe in the forum (move to all activities and resources to a section in the end and reduce numsections) is not really good and the current behaviour of the quickset block is "if resources exist in that section they will be moved to the "Zero" section", so it is not really what we want in this issue. I have updated the patch to the latest 2.6 stable branch and also fixed the "orphaned records in the course_sections_availability table." as noted by Michael. I still don't have the titles updating correcting maybe due some misunderstanding of the "M.course.format.process_sections" function. I would appreciate very much if someone could help with it. The new version is available at https://github.com/danielneis/moodle/tree/MDL-10405-v26 Hope you like. Daniel
            Hide
            marina Marina Glancy added a comment -

            Hello Daniel. I quickly reviewed your code. This is a very nice work.

            Something that you don't keep in mind is that each course format handles sections as it wishes. In 2.4 doing the course format refactoring I took the field 'numsections' out of 'course' table and defined it as a course format option. Topics and Weeks formats support it but, for example, Social or Single Activity formats not. Contributed course formats that supports sections do not need to have 'numsections'.
            As the author of https://moodle.org/plugins/view.php?plugin=format_flexsections I can tell you that your code would not work with my course format. I can not tell for other contributed formats but I'm pretty sure some of them have already implemented deleting sections the way they want.

            Another thing is that the word "section" is internal. Each course format defines the language string that describes "sections" in it. For example, they are "weeks" in weeks format and "topics" in topics format. This means that we can not put language strings in the core, they should be in each course format.

            What I would strongly recommend you is to define all functions and strings inside a particular course format. Actually IMO only topics format needs it. Ideally your branch should have changes only in files that are inside course/format/topics/ folder. Instead of changes in course/rest.php and course/yui/toolboxes/toolboxes.js there should be (probably new) scripts inside the plugin.

            Also regarding your implementation of function course_delete_section(), it should use the API functions to delete modules. Also when you write SQL queries it should not be:

            $DB->execute("UPDATE {$CFG->prefix}course_sections
                                 SET section = section - 1
                               WHERE course = {$courseid}
                                 AND section > {$section->section}");

            but instead:

            $DB->execute("UPDATE {course_sections}
                                 SET section = section - 1
                               WHERE course = ?
                                 AND section > ?", array($courseid, $section->section));

            Show
            marina Marina Glancy added a comment - Hello Daniel. I quickly reviewed your code. This is a very nice work. Something that you don't keep in mind is that each course format handles sections as it wishes. In 2.4 doing the course format refactoring I took the field 'numsections' out of 'course' table and defined it as a course format option. Topics and Weeks formats support it but, for example, Social or Single Activity formats not. Contributed course formats that supports sections do not need to have 'numsections'. As the author of https://moodle.org/plugins/view.php?plugin=format_flexsections I can tell you that your code would not work with my course format. I can not tell for other contributed formats but I'm pretty sure some of them have already implemented deleting sections the way they want. Another thing is that the word "section" is internal. Each course format defines the language string that describes "sections" in it. For example, they are "weeks" in weeks format and "topics" in topics format. This means that we can not put language strings in the core, they should be in each course format. What I would strongly recommend you is to define all functions and strings inside a particular course format. Actually IMO only topics format needs it. Ideally your branch should have changes only in files that are inside course/format/topics/ folder. Instead of changes in course/rest.php and course/yui/toolboxes/toolboxes.js there should be (probably new) scripts inside the plugin. Also regarding your implementation of function course_delete_section(), it should use the API functions to delete modules. Also when you write SQL queries it should not be: $DB->execute("UPDATE {$CFG->prefix}course_sections SET section = section - 1 WHERE course = {$courseid} AND section > {$section->section}"); but instead: $DB->execute("UPDATE {course_sections} SET section = section - 1 WHERE course = ? AND section > ?", array($courseid, $section->section));
            Hide
            debra94942 Debra Thayer added a comment -

            I don't see any mention of plans for 2.4 or 2.5. Will either of these versions eventually have this feature?

            Show
            debra94942 Debra Thayer added a comment - I don't see any mention of plans for 2.4 or 2.5. Will either of these versions eventually have this feature?
            Hide
            marycooch Mary Cooch added a comment - - edited

            2.4 and 2.5 won't be treated any differently; they just were not added to the list, Debra.

            Show
            marycooch Mary Cooch added a comment - - edited 2.4 and 2.5 won't be treated any differently; they just were not added to the list, Debra.
            Hide
            derekcx Derek Chirnside added a comment -

            @Daniel. Any movement on this and Marina's comments?

            -Derek

            Show
            derekcx Derek Chirnside added a comment - @Daniel. Any movement on this and Marina's comments? -Derek
            Hide
            miro_ilias Miro Ilias added a comment -

            Greetings,

            deleting the entire section in the course is nice feature. I support it.

            Would be there also a "duplicate section" feature, where one would duplicate all section's activities and resources ?

            Thanks, Miro

            Show
            miro_ilias Miro Ilias added a comment - Greetings, deleting the entire section in the course is nice feature. I support it. Would be there also a "duplicate section" feature, where one would duplicate all section's activities and resources ? Thanks, Miro
            Hide
            danielneis Daniel Neis added a comment -

            Hello, everybody

            as the major difficult in this issue is the YUI, i will work on this again on when Moodle completes transition to JQuery (MDL-47036).

            Kind regards,
            Daniel

            Show
            danielneis Daniel Neis added a comment - Hello, everybody as the major difficult in this issue is the YUI, i will work on this again on when Moodle completes transition to JQuery ( MDL-47036 ). Kind regards, Daniel
            Hide
            fuhrmanator Christopher Fuhrman added a comment -

            Voted for this issue. It's soon to be technically 10 years old, according to this post from the forums in 2005. https://moodle.org/mod/forum/discuss.php?d=26383 – I just created a new course for this winter's semester, and it comes with 10 default topics (1-10) based on our admin's template. I don't use topics, so it's annoying. I realize part of the problem is the template, but the underlying issue is the lack of simple way to delete topics.

            Show
            fuhrmanator Christopher Fuhrman added a comment - Voted for this issue. It's soon to be technically 10 years old, according to this post from the forums in 2005. https://moodle.org/mod/forum/discuss.php?d=26383 – I just created a new course for this winter's semester, and it comes with 10 default topics (1-10) based on our admin's template. I don't use topics, so it's annoying. I realize part of the problem is the template, but the underlying issue is the lack of simple way to delete topics.
            Hide
            bobpuffer Bob Puffer added a comment -

            Administration block->Course administration->Edit settings->Number of sections.

            If you don't use topics you can change the course format

            Show
            bobpuffer Bob Puffer added a comment - Administration block->Course administration->Edit settings->Number of sections. If you don't use topics you can change the course format
            Hide
            poltawski Dan Poltawski added a comment -

            There is also a little + and - icon at the bottom of the page to reduce number incrementally and Site administration > Courses > Course default settings to change the default.

            Show
            poltawski Dan Poltawski added a comment - There is also a little + and - icon at the bottom of the page to reduce number incrementally and Site administration > Courses > Course default settings to change the default.
            Hide
            marina Marina Glancy added a comment -

            It's really a pity this issue is delayed. I just implemented deleting section in my custom course format https://github.com/marinaglancy/moodle-format_flexsections/commit/d5e7a5bf66f28f0ec237286dec35e0ee4d928c3c - took me only couple of hours (plus another hour for behat tests). The biggest part of this diff is confirmation dialogue. The actual deleting of the section is super easy:

            • move the section to the end
            • delete all activities from it
            • delete corresponding records from tables course_sections and course_format_options
            • in case of topics/weeks we will also need to reduce numsections (if deleted section number was less than number of sections)
            Show
            marina Marina Glancy added a comment - It's really a pity this issue is delayed. I just implemented deleting section in my custom course format https://github.com/marinaglancy/moodle-format_flexsections/commit/d5e7a5bf66f28f0ec237286dec35e0ee4d928c3c - took me only couple of hours (plus another hour for behat tests). The biggest part of this diff is confirmation dialogue. The actual deleting of the section is super easy: move the section to the end delete all activities from it delete corresponding records from tables course_sections and course_format_options in case of topics/weeks we will also need to reduce numsections (if deleted section number was less than number of sections)
            Hide
            marina Marina Glancy added a comment -

            sending for peer review.

            The second commit will allow to remove extra sections from topics and weeks format. They become invisible when numsections is changed to be smaller than the original/actual number of sections.
            User expects that they are deleted but they are not and appear on backup screen.
            Obviously, I do not remove the orphaned sections that have activities in them

            Show
            marina Marina Glancy added a comment - sending for peer review. The second commit will allow to remove extra sections from topics and weeks format. They become invisible when numsections is changed to be smaller than the original/actual number of sections. User expects that they are deleted but they are not and appear on backup screen. Obviously, I do not remove the orphaned sections that have activities in them
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-10405 using repository: git://github.com/marinaglancy/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-10405 using repository: git://github.com/marinaglancy/moodle.git master (19 errors / 2 warnings) [branch: wip-MDL-10405-master | CI Job ] phplint (0/0) , php (13/2) , js (0/0) , css (0/0) , phpdoc (6/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            cibot CiBoT added a comment -

            Code verified against automated checks.

            Checked MDL-10405 using repository: git://github.com/marinaglancy/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Code verified against automated checks. Checked MDL-10405 using repository: git://github.com/marinaglancy/moodle.git master (0 errors / 0 warnings) [branch: wip-MDL-10405-master | CI Job ] More information about this report
            Hide
            nmagill Neill Magill added a comment -

            I took a look at the delete code and noticed one issue that could happen on sites where there are multiple people editing one course.

            Because the delete functions rely on the section number if someone tries to delete a section when someone else has moved it a section that was not intended would be delete. Take this scenario:

            When the page is loaded by user A and user B separately
            section id, section number
            1, 0
            2, 1
            3, 2

            User A now reorders the sections to:
            section id, section number
            1, 0
            3, 1
            2, 2

            User B who has not reloaded deletes the section that they see in position 1
            The result would be:
            section id, section number
            1, 0
            2, 1

            rather than result intended:
            section id, section number
            1, 0
            3, 1

            I think to delete an id that does not change with reordering should be used.

            Show
            nmagill Neill Magill added a comment - I took a look at the delete code and noticed one issue that could happen on sites where there are multiple people editing one course. Because the delete functions rely on the section number if someone tries to delete a section when someone else has moved it a section that was not intended would be delete. Take this scenario: When the page is loaded by user A and user B separately section id, section number 1, 0 2, 1 3, 2 User A now reorders the sections to: section id, section number 1, 0 3, 1 2, 2 User B who has not reloaded deletes the section that they see in position 1 The result would be: section id, section number 1, 0 2, 1 rather than result intended: section id, section number 1, 0 3, 1 I think to delete an id that does not change with reordering should be used.
            Hide
            marina Marina Glancy added a comment -

            Hi Neill, section id is passed as an argument to the editsection.php

            Show
            marina Marina Glancy added a comment - Hi Neill, section id is passed as an argument to the editsection.php
            Hide
            nmagill Neill Magill added a comment -

            I admit I was looking at the functions in course/lib.php that allow an int that is the section number to be passed.

            Show
            nmagill Neill Magill added a comment - I admit I was looking at the functions in course/lib.php that allow an int that is the section number to be passed.
            Hide
            fred Frédéric Massart added a comment -

            Hi Marina,

            thanks for providing a patch for this long standing bug, most of those points are pretty minor compared to the whole patch.

            Changenumsections.php

            1. Can you explain why we should not call the course format in that case?

            Editsection

            1. Re-using the same page to edit a section and delete a section seems a bit strange to me, if the purpose is just to have a page to hold the bit of logic you added, cannot we add a new one?
            2. It seems to be that the cancel and confirm URL should be the same. According to the API the second parameter is ignored when the 3rd contains 'sr'.

            Renderer

            1. You have inconsistent spacing between parameters and values in the definition of section_controls().
            2. I agree that it's a good idea to display the delete action the course page, however I would have placed it where the other icons are (move up/down, hide/show, highlight). The cog icon is more about the summary itself, even though it leads to a page with more settings.
              • This applies to stealth sections too, except that the stealth ones should cannot be moved, etc...
            3. Why are you changing the order in which the summary and the controls are sent to the output?

            Topics/Weeks

            1. Shouldn't we check for if $this->can_delete_section() deleting the sections upon numsections update? (see update_course_format_options())

            Behat

            1. Missing Then in scenario: Deleting the middle section in topics format
            2. Maybe that would be nice to check that other sections are still present after a section is deleted, on top of checking the value of numsections.

            Course lib

            1. My personal preference would have been to set the default parameter of $deleteemptyonly to true.
            2. Why not using modinfo rather than parsing the sequence to get the list of CMs in the section?
            3. It seems that you could use $DB->delete_records() rather than $DB->execute().
            4. Shouldn't we use a transaction around the section delete process?

            Nice work, lots of test, and a functionality missing for a long time!

            Cheers,
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Marina, thanks for providing a patch for this long standing bug, most of those points are pretty minor compared to the whole patch. Changenumsections.php Can you explain why we should not call the course format in that case? Editsection Re-using the same page to edit a section and delete a section seems a bit strange to me, if the purpose is just to have a page to hold the bit of logic you added, cannot we add a new one? It seems to be that the cancel and confirm URL should be the same. According to the API the second parameter is ignored when the 3rd contains 'sr'. Renderer You have inconsistent spacing between parameters and values in the definition of section_controls(). I agree that it's a good idea to display the delete action the course page, however I would have placed it where the other icons are (move up/down, hide/show, highlight). The cog icon is more about the summary itself, even though it leads to a page with more settings. This applies to stealth sections too, except that the stealth ones should cannot be moved, etc... Why are you changing the order in which the summary and the controls are sent to the output? Topics/Weeks Shouldn't we check for if $this->can_delete_section() deleting the sections upon numsections update? (see update_course_format_options()) Behat Missing Then in scenario: Deleting the middle section in topics format Maybe that would be nice to check that other sections are still present after a section is deleted, on top of checking the value of numsections . Course lib My personal preference would have been to set the default parameter of $deleteemptyonly to true . Why not using modinfo rather than parsing the sequence to get the list of CMs in the section? It seems that you could use $DB->delete_records() rather than $DB->execute(). Shouldn't we use a transaction around the section delete process? Nice work, lots of test, and a functionality missing for a long time! Cheers, Fred
            Hide
            marina Marina Glancy added a comment -

            Hi Fred, thanks for the review.

            Changenumsections.php - this was actually left from my previous attempt to use event observers. update_course() will trigger event and current code won't. I still think it's better to use API
            Editsection:
            1. we have one single script for modules that does all actions, I did not really see necessary to have more than one for section
            2. not always. When each section is displayed on the separate page and you cancel - you return to the section page; when you delete - you return to the course page. This reminds me to add testing instructions about it

            Renderer
            1. lol, codechecker did not complain
            2. We have the move control on the left, edit in the middle and hide/highlight on the right. So a bit of a tough question where to put "delete" button.
            3. because it was really confusing. The "edit" control also changes the section title, try adding a long summary to the section and then figure out how to change a title.

            topics/weeks
            1. no, because we only remove empty sections there and only 'course/update' capability is needed and it is also required to change course format options (such as numsections). But calling can_delete_sections will call get_fast_modinfo and will rebuild course cache several times if several sections need to be deleted

            behat:
            1. ok
            2. unittests do that, it's impossible in the behat (empty orphaned sections would not be displayed anyway)

            course lib:
            1. ok
            2. again performance - when several sections are deleted, we don't want to rebuild cache all the time
            3. agree. this again was left from previous version of code
            4. I'm afraid transactions can be used inside deleting modules, in such case we would have nested transactions - not sure how it is possible. Also thanks to the sequence of commands in this function, even if the process is terminated in the middle, there will be no bad data in the database

            I'll make screenshots of my ui changes and ask other people to vote

            Show
            marina Marina Glancy added a comment - Hi Fred, thanks for the review. Changenumsections.php - this was actually left from my previous attempt to use event observers. update_course() will trigger event and current code won't. I still think it's better to use API Editsection: 1. we have one single script for modules that does all actions, I did not really see necessary to have more than one for section 2. not always. When each section is displayed on the separate page and you cancel - you return to the section page; when you delete - you return to the course page. This reminds me to add testing instructions about it Renderer 1. lol, codechecker did not complain 2. We have the move control on the left, edit in the middle and hide/highlight on the right. So a bit of a tough question where to put "delete" button. 3. because it was really confusing. The "edit" control also changes the section title, try adding a long summary to the section and then figure out how to change a title. topics/weeks 1. no, because we only remove empty sections there and only 'course/update' capability is needed and it is also required to change course format options (such as numsections). But calling can_delete_sections will call get_fast_modinfo and will rebuild course cache several times if several sections need to be deleted behat: 1. ok 2. unittests do that, it's impossible in the behat (empty orphaned sections would not be displayed anyway) course lib: 1. ok 2. again performance - when several sections are deleted, we don't want to rebuild cache all the time 3. agree. this again was left from previous version of code 4. I'm afraid transactions can be used inside deleting modules, in such case we would have nested transactions - not sure how it is possible. Also thanks to the sequence of commands in this function, even if the process is terminated in the middle, there will be no bad data in the database I'll make screenshots of my ui changes and ask other people to vote
            Hide
            marina Marina Glancy added a comment -

            Attached files - current view, option1 and option2 for the location of the "delete" link. Please vote!

            Show
            marina Marina Glancy added a comment - Attached files - current view, option1 and option2 for the location of the "delete" link. Please vote!
            Hide
            nmagill Neill Magill added a comment -

            I prefer option 2

            Show
            nmagill Neill Magill added a comment - I prefer option 2
            Hide
            syxton Matthew Davidson added a comment - - edited

            option 2 seems best to me. option 1 would make it seem that you are deleting just the summary.
            Would it be possible to see an option with the edit AND delete on the right, or do you think that would be too much on the right? Maybe it is time to have a drop menu there. Declutter the interface a little

            Show
            syxton Matthew Davidson added a comment - - edited option 2 seems best to me. option 1 would make it seem that you are deleting just the summary. Would it be possible to see an option with the edit AND delete on the right, or do you think that would be too much on the right? Maybe it is time to have a drop menu there. Declutter the interface a little
            Hide
            chrisf Chris Fryer added a comment -

            Option 2. That's where the other icons relating to the section are, and that's where I looked for it when looking at the other two images.

            Show
            chrisf Chris Fryer added a comment - Option 2. That's where the other icons relating to the section are, and that's where I looked for it when looking at the other two images.
            Hide
            syxton Matthew Davidson added a comment -

            Here is a quick mock-up of what a drop down menu there could look like

            Show
            syxton Matthew Davidson added a comment - Here is a quick mock-up of what a drop down menu there could look like
            Hide
            marina Marina Glancy added a comment -

            thanks for the votes, option 2 it is then. Actually when I created mockups yesterday I saw myself that option2 looks better, so I already changed the code. I'll just finish addressing other Fred's comments and push the branch again
            Matthew Davidson, mock-up looks good but let's leave it to the separate issue.

            Show
            marina Marina Glancy added a comment - thanks for the votes, option 2 it is then. Actually when I created mockups yesterday I saw myself that option2 looks better, so I already changed the code. I'll just finish addressing other Fred's comments and push the branch again Matthew Davidson , mock-up looks good but let's leave it to the separate issue.
            Hide
            marina Marina Glancy added a comment -

            Frédéric Massart, do you want to take another look?

            Show
            marina Marina Glancy added a comment - Frédéric Massart , do you want to take another look?
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-10405 using repository: git://github.com/marinaglancy/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-10405 using repository: git://github.com/marinaglancy/moodle.git master (1 errors / 0 warnings) [branch: wip-MDL-10405-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            syxton Matthew Davidson added a comment -

            Marina,
            I've opened up a new issue to roll the buttons into a menu MDL-48947
            Thanks.

            Show
            syxton Matthew Davidson added a comment - Marina, I've opened up a new issue to roll the buttons into a menu MDL-48947 Thanks.
            Hide
            fred Frédéric Massart added a comment -

            Hi Marina,

            thanks for the comments, I just want to clarify a few of my points in this last review.

            1. We had a chat about the call to update_course vs. calling the course format section, if I understood correctly that is to trigger an event. Which means that events are not triggered when you call directly the course format, which might be an issue. I remember that for most (if not all) actions we should rely on the course format, so to me this case should as well.
            2. The file that contains both an edit form and code to delete the section seems strange to me because it gives the file two purposes. As far as I can tell, the mod.php that you're referring could almost act as an ajax file as it does not do anything but handling the actions, where as editsection.php would handle both the action of deleting, and the action and UI for editing.

            Thanks!
            Fred

            Show
            fred Frédéric Massart added a comment - Hi Marina, thanks for the comments, I just want to clarify a few of my points in this last review. We had a chat about the call to update_course vs. calling the course format section, if I understood correctly that is to trigger an event. Which means that events are not triggered when you call directly the course format, which might be an issue. I remember that for most (if not all) actions we should rely on the course format, so to me this case should as well. The file that contains both an edit form and code to delete the section seems strange to me because it gives the file two purposes. As far as I can tell, the mod.php that you're referring could almost act as an ajax file as it does not do anything but handling the actions, where as editsection.php would handle both the action of deleting, and the action and UI for editing. Thanks! Fred
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-10405 using repository: git://github.com/marinaglancy/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-10405 using repository: git://github.com/marinaglancy/moodle.git master (1 errors / 0 warnings) [branch: wip-MDL-10405-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            marina Marina Glancy added a comment -

            course_get_format()->course_update_options() is an internal function, it is called from update_course() and should not be called directly. So it should not be called by itself.

            I liked your idea and swapped how global and format functions work, it looks better now.

            Submitting for integration (Fred gave me his +1 in chat)

            Show
            marina Marina Glancy added a comment - course_get_format()->course_update_options() is an internal function, it is called from update_course() and should not be called directly. So it should not be called by itself. I liked your idea and swapped how global and format functions work, it looks better now. Submitting for integration (Fred gave me his +1 in chat)
            Hide
            cibot CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            cibot CiBoT added a comment -

            Fails against automated checks.

            Checked MDL-10405 using repository: git://github.com/marinaglancy/moodle.git

            More information about this report

            Show
            cibot CiBoT added a comment - Fails against automated checks. Checked MDL-10405 using repository: git://github.com/marinaglancy/moodle.git master (1 errors / 0 warnings) [branch: wip-MDL-10405-master | CI Job ] phplint (0/0) , php (1/0) , js (0/0) , css (0/0) , phpdoc (0/0) , commit (0/0) , savepoint (0/0) , thirdparty (0/0) , More information about this report
            Hide
            dmonllao David Monllaó added a comment -

            Thanks Marina, looks all very good, having a lot of tests makes things a lot easier, just a couple of questions:

            Show
            dmonllao David Monllaó added a comment - Thanks Marina, looks all very good, having a lot of tests makes things a lot easier, just a couple of questions: What would you think of setting $PAGE title, heading and a navbar title when $deletesection && !$confirm? (course/editsection.php) otherwise the page seems quite empty, would be great to have something similar to what we see when deleting blocks instances, or at least showing a header, title... https://github.com/marinaglancy/moodle/compare/moodle:master...wip-MDL-10405-master#diff-cd380652e5a565f95767120443b90670R268 It seems that we will move MDL-39079 forward, the a tag contains $strdelete, but the image alt attribute is just get_string('delete') may be better to set $strdelete also to the alt attribute as it will be the one read by screen readers. Also BTW, why don't we change the img class to 'icon' like we do in other section controls, now you are setting class to 'iconsmall edit' like the edit icon does. I understand that it is necessary to duplicate all update_course_format_options to keep the formats independents https://github.com/marinaglancy/moodle/compare/moodle:master...wip-MDL-10405-master#diff-3c138d17b4b1dd855388d0542d90030fR349
            Hide
            marina Marina Glancy added a comment -

            Thanks for review David, I added some headers/navbar to the delete page. Hopefully soon somebody will implement JS confirmation and this page will not be used at all for displaying confirmation.
            I corrected 'alt' as well too

            Show
            marina Marina Glancy added a comment - Thanks for review David, I added some headers/navbar to the delete page. Hopefully soon somebody will implement JS confirmation and this page will not be used at all for displaying confirmation. I corrected 'alt' as well too
            Hide
            dmonllao David Monllaó added a comment -

            Thanks marina, integrated to master. I've just realised that you didn't changed the delete img class, feel free to add an extra commit if there is no specific reason to use a different class than the other left controls.

            Show
            dmonllao David Monllaó added a comment - Thanks marina, integrated to master. I've just realised that you didn't changed the delete img class, feel free to add an extra commit if there is no specific reason to use a different class than the other left controls.
            Hide
            marina Marina Glancy added a comment -

            oops, sorry for forgetting the CSS class for the icon. New commit added to the same branch if you want to pick it

            Show
            marina Marina Glancy added a comment - oops, sorry for forgetting the CSS class for the icon. New commit added to the same branch if you want to pick it
            Hide
            dmonllao David Monllaó added a comment -

            Thanks Marina, pushed

            Show
            dmonllao David Monllaó added a comment - Thanks Marina, pushed
            Hide
            markn Mark Nelson added a comment -

            Thanks for working on this Marina, I am passing this as it works as expected.

            Just two minor points -

            1. The string "Are you absolutely sure you want to delete "Topic 2"? All activities will be also deleted" varies from other deletion messages used in Moodle. For example, when deleting a course the message is "Are you absolutely sure you want to completely delete this course and all the data it contains?". So, rather than having a question, then a statement afterwards, my suggestion would be "Are you absolutely sure you want to completely delete the section 'Topic 2' and all the activities it contains?".
            2. The submit buttons are 'Yes' and 'No', where as when deleting a course it is 'Continue' and 'Cancel'.

            If you feel these are not important then simply ignore, otherwise they can be addressed in another issue.

            Perhaps Mary Cooch and Helen Foster could suggestion an appropriate deletion message and whether or not to use 'Continue' and 'Cancel', rather than 'Yes' and 'No'.

            Cheers!

            Show
            markn Mark Nelson added a comment - Thanks for working on this Marina, I am passing this as it works as expected. Just two minor points - The string "Are you absolutely sure you want to delete "Topic 2"? All activities will be also deleted" varies from other deletion messages used in Moodle. For example, when deleting a course the message is "Are you absolutely sure you want to completely delete this course and all the data it contains?". So, rather than having a question, then a statement afterwards, my suggestion would be "Are you absolutely sure you want to completely delete the section 'Topic 2' and all the activities it contains?". The submit buttons are 'Yes' and 'No', where as when deleting a course it is 'Continue' and 'Cancel'. If you feel these are not important then simply ignore, otherwise they can be addressed in another issue. Perhaps Mary Cooch and Helen Foster could suggestion an appropriate deletion message and whether or not to use 'Continue' and 'Cancel', rather than 'Yes' and 'No'. Cheers!
            Hide
            aquisse Thomas Hanley added a comment -

            Thanks Marina, Mark, David, Fred and CiBot for your work on this issue. I have seen countless lecturers at the university I work in confused by this aspect of Moodle.

            Re Mark's points yes I agree from a user experience / UI point of view consistent language is important. There is an article by Joshua Porter here which pretty convincingly argues that "the fastest way to improve your interface is to improve your copy-writing".
            http://bokardo.com/archives/writing-microcopy/

            ~thomas

            Show
            aquisse Thomas Hanley added a comment - Thanks Marina, Mark, David, Fred and CiBot for your work on this issue. I have seen countless lecturers at the university I work in confused by this aspect of Moodle. Re Mark's points yes I agree from a user experience / UI point of view consistent language is important. There is an article by Joshua Porter here which pretty convincingly argues that "the fastest way to improve your interface is to improve your copy-writing". http://bokardo.com/archives/writing-microcopy/ ~thomas
            Hide
            tsala Helen Foster added a comment - - edited

            Thanks Mark for asking my opinion on the language string confirmdeletesection. I agree with your suggestion for reasons of consistency:

            'Are you absolutely sure you want to completely delete the section '{$a}' and all the activities it contains?'

            As for your point 2. I checked where the question 'Are you absolutely sure you want to...' is used elsewhere in Moodle and found

            • When deleting a user - Continue and Cancel
            • When deleting quiz questions - Continue and Cancel
            • When deleting SCORM attempts - in a window Cancel and OK

            Thus for consistency I suggest we go for Continue and Cancel.

            Show
            tsala Helen Foster added a comment - - edited Thanks Mark for asking my opinion on the language string confirmdeletesection. I agree with your suggestion for reasons of consistency: 'Are you absolutely sure you want to completely delete the section '{$a}' and all the activities it contains?' As for your point 2. I checked where the question 'Are you absolutely sure you want to...' is used elsewhere in Moodle and found When deleting a user - Continue and Cancel When deleting quiz questions - Continue and Cancel When deleting SCORM attempts - in a window Cancel and OK Thus for consistency I suggest we go for Continue and Cancel.
            Hide
            markn Mark Nelson added a comment -

            Thanks Helen. I created MDL-49182 to deal with the minor string changes.

            Show
            markn Mark Nelson added a comment - Thanks Helen. I created MDL-49182 to deal with the minor string changes.
            Hide
            markn Mark Nelson added a comment -

            Hey guys, so while working on MDL-49182 I went to run the behat tests (with the tag @format) and received a few failures. I initially thought it was due to my changes, but it also happens on master.

            mark@mma:~/htdocs/im$ vendor/bin/behat --config /Users/mark/htdocs/mstorage/im/moodledata_behat/behat/behat.yml --tags @format
            Moodle 2.9dev (Build: 20150205), pgsql, c44da977115aeb64836c172dcf511a9fd217e7d9
            Server OS "Darwin", Browser: "chrome"
            Browser specific fixes have been applied. See http://docs.moodle.org/dev/Acceptance_testing#Browser_specific_fixes
            Started at 11-02-2015, 16:28
            ...................................................................... 70
            ...................................................................... 140
            ...................................................................... 210
            ...................................................................... 280
            ..........................................F-----..........F-------.... 350
            ......F--------.........................................
             
            (::) failed steps (::)
             
            01. "8 May - 14 May" text was not found in the "li#section-2" element
                In step `Given I should see "8 May - 14 May" in the "li#section-2" "css_element"'. # behat_general::assert_element_contains_text()
                From scenario `Edit section default name in weeks format'.                         # /Users/mark/htdocs/mstorage/im/moodle/course/format/weeks/tests/behat/edit_delete_sections.feature:34
                Of feature `Sections can be edited and deleted in weeks format'.                   # /Users/mark/htdocs/mstorage/im/moodle/course/format/weeks/tests/behat/edit_delete_sections.feature
             
            02. "29 May - 4 June" text was not found in the "li#section-5" element
                In step `Given I should see "29 May - 4 June" in the "li#section-5" "css_element"'. # behat_general::assert_element_contains_text()
                From scenario `Deleting the last section in weeks format'.                          # /Users/mark/htdocs/mstorage/im/moodle/course/format/weeks/tests/behat/edit_delete_sections.feature:44
                Of feature `Sections can be edited and deleted in weeks format'.                    # /Users/mark/htdocs/mstorage/im/moodle/course/format/weeks/tests/behat/edit_delete_sections.feature
             
            03. "29 May - 4 June" text was not found in the "li#section-5" element
                In step `Given I should see "29 May - 4 June" in the "li#section-5" "css_element"'. # behat_general::assert_element_contains_text()
                From scenario `Deleting the middle section in weeks format'.                        # /Users/mark/htdocs/mstorage/im/moodle/course/format/weeks/tests/behat/edit_delete_sections.feature:54
                Of feature `Sections can be edited and deleted in weeks format'.
            

            Show
            markn Mark Nelson added a comment - Hey guys, so while working on MDL-49182 I went to run the behat tests (with the tag @format) and received a few failures. I initially thought it was due to my changes, but it also happens on master. mark@mma:~/htdocs/im$ vendor/bin/behat --config /Users/mark/htdocs/mstorage/im/moodledata_behat/behat/behat.yml --tags @format Moodle 2.9dev (Build: 20150205), pgsql, c44da977115aeb64836c172dcf511a9fd217e7d9 Server OS "Darwin", Browser: "chrome" Browser specific fixes have been applied. See http://docs.moodle.org/dev/Acceptance_testing#Browser_specific_fixes Started at 11-02-2015, 16:28 ...................................................................... 70 ...................................................................... 140 ...................................................................... 210 ...................................................................... 280 ..........................................F-----..........F-------.... 350 ......F--------.........................................   (::) failed steps (::)   01. "8 May - 14 May" text was not found in the "li#section-2" element In step `Given I should see "8 May - 14 May" in the "li#section-2" "css_element"'. # behat_general::assert_element_contains_text() From scenario `Edit section default name in weeks format'. # /Users/mark/htdocs/mstorage/im/moodle/course/format/weeks/tests/behat/edit_delete_sections.feature:34 Of feature `Sections can be edited and deleted in weeks format'. # /Users/mark/htdocs/mstorage/im/moodle/course/format/weeks/tests/behat/edit_delete_sections.feature   02. "29 May - 4 June" text was not found in the "li#section-5" element In step `Given I should see "29 May - 4 June" in the "li#section-5" "css_element"'. # behat_general::assert_element_contains_text() From scenario `Deleting the last section in weeks format'. # /Users/mark/htdocs/mstorage/im/moodle/course/format/weeks/tests/behat/edit_delete_sections.feature:44 Of feature `Sections can be edited and deleted in weeks format'. # /Users/mark/htdocs/mstorage/im/moodle/course/format/weeks/tests/behat/edit_delete_sections.feature   03. "29 May - 4 June" text was not found in the "li#section-5" element In step `Given I should see "29 May - 4 June" in the "li#section-5" "css_element"'. # behat_general::assert_element_contains_text() From scenario `Deleting the middle section in weeks format'. # /Users/mark/htdocs/mstorage/im/moodle/course/format/weeks/tests/behat/edit_delete_sections.feature:54 Of feature `Sections can be edited and deleted in weeks format'.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Hello Mark,

            This is not reproducible on nightly, and on my local machine. Looking at fails it seems to be similar to MDL-48670 and will be dealt separately.

            So I assume it's a pass...

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello Mark, This is not reproducible on nightly, and on my local machine. Looking at fails it seems to be similar to MDL-48670 and will be dealt separately. So I assume it's a pass...
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Results on my local machine:

            rajesh@rajesh:/var/www/im$ vendor/bin/behat --config /home/rajesh/moodles/p/moodledata_behat/behat/behat.yml /var/www/im/course/format/weeks/tests/behat/edit_delete_sections.feature
            Moodle 2.9dev (Build: 20150205), pgsql, d3dac400199e3154591d35bfcfd825f0fc10f398
            Server OS "Linux", Browser: "firefox"
            Started at 12-02-2015, 09:43
            ...................................................................... 70
            ......................................
             
            6 scenarios (6 passed)
            108 steps (108 passed)
            0m49.635s
            

            Passing it...

            Show
            rajeshtaneja Rajesh Taneja added a comment - Results on my local machine: rajesh@rajesh:/var/www/im$ vendor/bin/behat --config /home/rajesh/moodles/p/moodledata_behat/behat/behat.yml /var/www/im/course/format/weeks/tests/behat/edit_delete_sections.feature Moodle 2.9dev (Build: 20150205), pgsql, d3dac400199e3154591d35bfcfd825f0fc10f398 Server OS "Linux", Browser: "firefox" Started at 12-02-2015, 09:43 ...................................................................... 70 ......................................   6 scenarios (6 passed) 108 steps (108 passed) 0m49.635s Passing it...
            Hide
            markn Mark Nelson added a comment -

            Thanks Raj for pointing that out. The test course/tests/behat/course_controls.feature mentioned in MDL-48670 fails for me as well. So I agree, this can be set to passed and it can be dealt in MDL-48670.

            Show
            markn Mark Nelson added a comment - Thanks Raj for pointing that out. The test course/tests/behat/course_controls.feature mentioned in MDL-48670 fails for me as well. So I agree, this can be set to passed and it can be dealt in MDL-48670 .
            Hide
            dmonllao David Monllaó added a comment -

            Thanks for your contribution - this change is now part of Moodle and will be available on our download site shortly.

            Expect problems and eat them for breakfast.

            • Alfred A. Montapert
            Show
            dmonllao David Monllaó added a comment - Thanks for your contribution - this change is now part of Moodle and will be available on our download site shortly. Expect problems and eat them for breakfast. Alfred A. Montapert
            Hide
            chrisf Chris Fryer added a comment -

            Thanks for your persistence everyone. 7 years, 7 months and 2 days must be some kind of record.

            Show
            chrisf Chris Fryer added a comment - Thanks for your persistence everyone. 7 years, 7 months and 2 days must be some kind of record.
            Hide
            marycooch Mary Cooch added a comment - - edited

            Removing the docs_required label as this improvement is now documented: https://docs.moodle.org/29/en/Course_homepage
            If you notice that the documentation can be improved, please feel free to log in to the wiki and edit the page.
            Help in keeping Moodle documentation accurate and up-to-date is much appreciated

            Show
            marycooch Mary Cooch added a comment - - edited Removing the docs_required label as this improvement is now documented: https://docs.moodle.org/29/en/Course_homepage If you notice that the documentation can be improved, please feel free to log in to the wiki and edit the page. Help in keeping Moodle documentation accurate and up-to-date is much appreciated

              People

              • Votes:
                81 Vote for this issue
                Watchers:
                53 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/May/15