Moodle
  1. Moodle
  2. MDL-32744

Topics format CSS selector is hardcoded in course dragdrop.js

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: AJAX and JavaScript, Course
    • Labels:
      None
    • Testing Instructions:
      Hide

      Make sure that dragdrop works correctly for existing formats. If the new contributed format is added, drag-drop should not work unless it has format.js file with at least M.course.format.get_config function exist and contains configuration.

      Show
      Make sure that dragdrop works correctly for existing formats. If the new contributed format is added, drag-drop should not work unless it has format.js file with at least M.course.format.get_config function exist and contains configuration.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-32744-master-3
    • Rank:
      39736

      Description

      This will cause that for other format types drag-drop may work incorrectly

        Issue Links

          Activity

          Hide
          Ruslan Kabalin added a comment -

          Added patch, further re-factoring of MDL-32657

          Show
          Ruslan Kabalin added a comment - Added patch, further re-factoring of MDL-32657
          Hide
          Ruslan Kabalin added a comment -

          I am afraid, this is ready to revision.

          Show
          Ruslan Kabalin added a comment - I am afraid, this is ready to revision.
          Hide
          Ruslan Kabalin added a comment - - edited

          For example how this work with moodle-format_topcoll external format, see: https://github.com/kabalin/moodle-format_topcoll/commit/985dd91714c526bf1ea93dd6aa562898dcb67333 (should be used along with the patch in this issue)

          Show
          Ruslan Kabalin added a comment - - edited For example how this work with moodle-format_topcoll external format, see: https://github.com/kabalin/moodle-format_topcoll/commit/985dd91714c526bf1ea93dd6aa562898dcb67333 (should be used along with the patch in this issue)
          Hide
          Gareth J Barnard added a comment -

          Dear Ruslan,

          Just got your message, thank you - going to try the patch now.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Ruslan, Just got your message, thank you - going to try the patch now. Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Ruslan,

          Thank you so much for you hard work . I have integrated the patch into 'https://github.com/gjb2048/moodle-format_topcoll/tree/MDL-32744' as the closing 'tbody' was in the wrong place, the strings needed a little refactoring, slight coding issue with 'document.getElementById("sectioncontent-"+theToggle);' to 'document.getElementById("sectionbody-"+theToggle);' in 'module.js', the $displaysection variable needed fixing with the removal of 'course_get_display' etc. - but I'm not completely convinced that the code works properly yet for this functionality.

          When moving the sections the text within the 'tr.cps td' does not move consistently - any ideas why? My thought is an additional swap in 'format.js'.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Ruslan, Thank you so much for you hard work . I have integrated the patch into 'https://github.com/gjb2048/moodle-format_topcoll/tree/MDL-32744' as the closing 'tbody' was in the wrong place, the strings needed a little refactoring, slight coding issue with 'document.getElementById("sectioncontent-"+theToggle);' to 'document.getElementById("sectionbody-"+theToggle);' in 'module.js', the $displaysection variable needed fixing with the removal of 'course_get_display' etc. - but I'm not completely convinced that the code works properly yet for this functionality. When moving the sections the text within the 'tr.cps td' does not move consistently - any ideas why? My thought is an additional swap in 'format.js'. Cheers, Gareth
          Hide
          Ruslan Kabalin added a comment -

          Regarding fixes, I have just demonstrated you the general idea, I did not investigate how your format works, so there must be plenty of inconsistencies I was not aware about (the obvious one is language strings for different subformats). I leave it to you to make it work properly

          Show
          Ruslan Kabalin added a comment - Regarding fixes, I have just demonstrated you the general idea, I did not investigate how your format works, so there must be plenty of inconsistencies I was not aware about (the obvious one is language strings for different subformats). I leave it to you to make it work properly
          Hide
          Gareth J Barnard added a comment -

          To be honest Ruslan, you were fairly spot on the first time. I just need to understand the mechanisms implemented in 'format.js'

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - To be honest Ruslan, you were fairly spot on the first time. I just need to understand the mechanisms implemented in 'format.js' Cheers, Gareth
          Hide
          Gareth J Barnard added a comment -

          Dear Ruslan,

          I see on GitHub you have forked my Master branch. I also have a MDL-32744 branch for this very issue which is worth using instead so that changes can be swapped between the two for this.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Dear Ruslan, I see on GitHub you have forked my Master branch. I also have a MDL-32744 branch for this very issue which is worth using instead so that changes can be swapped between the two for this. Cheers, Gareth
          Hide
          Dan Poltawski added a comment -

          Hi,

          I really wanted to send this straight for integration for you today. But I couldn't because:

          1. It conflcits with master: spinners, left/right stuff and I didn't know if I was removing the right things
          2. It doesn't have testing instructions.

          So sorry, but I couldn't do that.

          Show
          Dan Poltawski added a comment - Hi, I really wanted to send this straight for integration for you today. But I couldn't because: It conflcits with master: spinners, left/right stuff and I didn't know if I was removing the right things It doesn't have testing instructions. So sorry, but I couldn't do that.
          Hide
          Ruslan Kabalin added a comment -

          Thanks a lot Dan, my fault. I have re-based the patch and added testing instruction.

          Show
          Ruslan Kabalin added a comment - Thanks a lot Dan, my fault. I have re-based the patch and added testing instruction.
          Hide
          Gareth J Barnard added a comment -

          Gents,

          This code is proving to be such a nightmare that Buffy the Vampire Slayer could not deal with, so I've created CONTRIB-3652 to help me get to grips with all of the changes that need to be implemented for Moodle 2.3 - any comments appreciated.

          But pragmatically, I do consider restricting tags to 'ul' and 'li' to be bad because of creativity with course formats (and indeed possibly the concept of course layouts, my comments on MDL-33027) which is why this code is so important.

          Cheers,

          Gareth

          Show
          Gareth J Barnard added a comment - Gents, This code is proving to be such a nightmare that Buffy the Vampire Slayer could not deal with, so I've created CONTRIB-3652 to help me get to grips with all of the changes that need to be implemented for Moodle 2.3 - any comments appreciated. But pragmatically, I do consider restricting tags to 'ul' and 'li' to be bad because of creativity with course formats (and indeed possibly the concept of course layouts, my comments on MDL-33027 ) which is why this code is so important. Cheers, Gareth
          Hide
          Dan Poltawski added a comment -
          git pull git://git.luns.net.uk/moodle.git MDL-32744-master-2
          remote: Counting objects: 29, done.
          remote: Compressing objects: 100% (12/12), done.
          remote: Total 15 (delta 9), reused 0 (delta 0)
          Unpacking objects: 100% (15/15), done.
          From git://git.luns.net.uk/moodle
           * branch            MDL-32744-master-2 -> FETCH_HEAD
          Auto-merging course/yui/toolboxes/toolboxes.js
          Auto-merging course/format/weeks/format.js
          Auto-merging course/format/topics/format.js
          CONFLICT (content): Merge conflict in course/format/topics/format.js
          Automatic merge failed; fix conflicts and then commit the result.
          
          Show
          Dan Poltawski added a comment - git pull git: //git.luns.net.uk/moodle.git MDL-32744-master-2 remote: Counting objects: 29, done. remote: Compressing objects: 100% (12/12), done. remote: Total 15 (delta 9), reused 0 (delta 0) Unpacking objects: 100% (15/15), done. From git: //git.luns.net.uk/moodle * branch MDL-32744-master-2 -> FETCH_HEAD Auto-merging course/yui/toolboxes/toolboxes.js Auto-merging course/format/weeks/format.js Auto-merging course/format/topics/format.js CONFLICT (content): Merge conflict in course/format/topics/format.js Automatic merge failed; fix conflicts and then commit the result.
          Hide
          Ruslan Kabalin added a comment -

          Grrrr

          Show
          Ruslan Kabalin added a comment - Grrrr
          Hide
          Ruslan Kabalin added a comment -

          merging issue is fixed

          Show
          Ruslan Kabalin added a comment - merging issue is fixed
          Hide
          Dan Poltawski added a comment -

          I have integrated and tested this.

          This really came too late and had hiccups on the way but I integrated it for the benefit of our contrib course format authors despite these hurdles.

          Show
          Dan Poltawski added a comment - I have integrated and tested this. This really came too late and had hiccups on the way but I integrated it for the benefit of our contrib course format authors despite these hurdles.
          Hide
          Ruslan Kabalin added a comment -

          Thank you Dan.

          Show
          Ruslan Kabalin added a comment - Thank you Dan.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

          Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: