Moodle
  1. Moodle
  2. MDL-33683

Adding resource to topic section that has been moved adds it to old location

    Details

    • Testing Instructions:
      Hide
      • Open a course page with at least 4 sections
      • Turn editing on
      • Ensure that the Activity chooser is on
      • Open the Activity chooser for the general section (at the top)
      • Create an activity (I used a label as they're short forms) and return to the course page
        • Confirm that the activity was created in the general section
      • Open the Activity chooser for the section 1
      • Create an activity (I used a label as they're short forms) and return to the course page
        • Confirm that the activity was created in section 1
      • Open the Activity chooser for the section 4
      • Create an activity (I used a label as they're short forms) and return to the course page
        • Confirm that the activity was created in section 4
      • Drag section 4 to the top
      • Open its Activity chooser
      • Create an activity (I used a label as they're short forms) and return to the course page
        • Confirm that the activity was created in section 1
      • Drag section 2 to the section 3 position
      • Open its Activity chooser
      • Create an activity (I used a label as they're short forms) and return to the course page
        • Confirm that the activity was created in section 3
      Show
      Open a course page with at least 4 sections Turn editing on Ensure that the Activity chooser is on Open the Activity chooser for the general section (at the top) Create an activity (I used a label as they're short forms) and return to the course page Confirm that the activity was created in the general section Open the Activity chooser for the section 1 Create an activity (I used a label as they're short forms) and return to the course page Confirm that the activity was created in section 1 Open the Activity chooser for the section 4 Create an activity (I used a label as they're short forms) and return to the course page Confirm that the activity was created in section 4 Drag section 4 to the top Open its Activity chooser Create an activity (I used a label as they're short forms) and return to the course page Confirm that the activity was created in section 1 Drag section 2 to the section 3 position Open its Activity chooser Create an activity (I used a label as they're short forms) and return to the course page Confirm that the activity was created in section 3
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-33683-master-2
    • Rank:
      41681

      Description

      1. Add an additional section to a course (using topics, not tested in week layout) - Topic A
      2. Move Topic A to a different location on course page
      3. Add a resource to Topic A
      Expected result: Resource is added to Topic A section
      Actual result: Resource is added to last topic section in course (which was original location of Topic A)

      Using Rocket Theme.

        Issue Links

          Activity

          Hide
          Michael de Raadt added a comment -

          Hi, Emma.

          I was able to replicate that in 2.3 with the new Activity chooser, but not in 2.2.3 or in 2.3 with the Activity chooser turned off.

          For example, without the chooser I get...

          http://michael.moodle.local/moodle_testing/course/modedit.php?add=resource&type=&course=2&section=3&return=0

          ...where 3 is the correct section, but with the chooser, I get...

          http://michael.moodle.local/moodle_testing/course/modedit.php?add=resource&type=&course=2&section=5&return=0

          ...and 5 is not the correct section, it's the section before the move.

          It would be interesting to see what URL parameters you're getting.

          There has been a lot of work on sections lately, which might explain differences between our system. Perhaps you could try to replicate this on qa.moodle.net and see if you get the same result.

          Show
          Michael de Raadt added a comment - Hi, Emma. I was able to replicate that in 2.3 with the new Activity chooser, but not in 2.2.3 or in 2.3 with the Activity chooser turned off. For example, without the chooser I get... http://michael.moodle.local/moodle_testing/course/modedit.php?add=resource&type=&course=2&section=3&return=0 ...where 3 is the correct section, but with the chooser, I get... http://michael.moodle.local/moodle_testing/course/modedit.php?add=resource&type=&course=2&section=5&return=0 ...and 5 is not the correct section, it's the section before the move. It would be interesting to see what URL parameters you're getting. There has been a lot of work on sections lately, which might explain differences between our system. Perhaps you could try to replicate this on qa.moodle.net and see if you get the same result.
          Hide
          Michael de Raadt added a comment -

          I don't think this is affected by themes.

          Andrew: I'm assigning this to you as I think this may have more to do with the Activity chooser than the course sections. I will add Dan as a watcher though. If you think this is another problem, please reassign it to moodle.com.

          Show
          Michael de Raadt added a comment - I don't think this is affected by themes. Andrew: I'm assigning this to you as I think this may have more to do with the Activity chooser than the course sections. I will add Dan as a watcher though. If you think this is another problem, please reassign it to moodle.com.
          Hide
          Michael de Raadt added a comment -

          Hi, Dan.

          I've added you as a watcher on this issue.

          Show
          Michael de Raadt added a comment - Hi, Dan. I've added you as a watcher on this issue.
          Hide
          Michael de Raadt added a comment -

          I tried this with drag-and-drop and that added the resource to the correct location after a section move.

          Show
          Michael de Raadt added a comment - I tried this with drag-and-drop and that added the resource to the correct location after a section move.
          Hide
          Andrew Nicols added a comment -

          Rather than getting the drag/drop to swap the section IDs around, we're determining the sectionid when displaying the activity chooser.

          Show
          Andrew Nicols added a comment - Rather than getting the drag/drop to swap the section IDs around, we're determining the sectionid when displaying the activity chooser.
          Hide
          Emma Richardson added a comment -

          I am confused - I presume you are referring to the new activity chooser in 2.3. I have been having this issue in 2.2.3 - I have been adding a resource (file or url). It appears that the testing has been pushed to another level - if you need me to reverify anything, please let me know.

          Show
          Emma Richardson added a comment - I am confused - I presume you are referring to the new activity chooser in 2.3. I have been having this issue in 2.2.3 - I have been adding a resource (file or url). It appears that the testing has been pushed to another level - if you need me to reverify anything, please let me know.
          Hide
          Andrew Nicols added a comment -

          Hi Emma,

          Sorry - I misread the affected version as being 2.3 rather than 2.2.3 - my mistake.
          We've completely rewritten all of the Javascript for the drag and drop in 2.3 and changed how this part works. Unfortunately, we did still manage to introduce this as a bug which I've now fixed.

          Fixing this for Moodle 2.2 and Moodle 2.1 is a whole different kettle of fish. From my reading of the code, it should already do the correct thing but obviously something is going awry.

          I'll have a further look and see if we can produce a patch for 2.1 and 2.2. This is going to be a completely different fix to the fix on master.

          Thanks,

          Andrew

          Show
          Andrew Nicols added a comment - Hi Emma, Sorry - I misread the affected version as being 2.3 rather than 2.2.3 - my mistake. We've completely rewritten all of the Javascript for the drag and drop in 2.3 and changed how this part works. Unfortunately, we did still manage to introduce this as a bug which I've now fixed. Fixing this for Moodle 2.2 and Moodle 2.1 is a whole different kettle of fish. From my reading of the code, it should already do the correct thing but obviously something is going awry. I'll have a further look and see if we can produce a patch for 2.1 and 2.2. This is going to be a completely different fix to the fix on master. Thanks, Andrew
          Hide
          Andrew Nicols added a comment -

          Hi Emma,

          I'm unable to replicate this bug on Moodle 2.2.3 or MOODLE_22_STABLE – what browser are you using?
          I've tried using the Rocket Theme and with Google Chrome.

          Andrew

          Show
          Andrew Nicols added a comment - Hi Emma, I'm unable to replicate this bug on Moodle 2.2.3 or MOODLE_22_STABLE – what browser are you using? I've tried using the Rocket Theme and with Google Chrome. Andrew
          Hide
          Dan Poltawski added a comment -

          Seems sensible to me. Is this using the same technique to work out the section as file drag/drop to section is using? If so +1.

          If not, it'd be good to make sure both were using a consistent approach (and thus will both be broken in one swoop if something changes )

          Show
          Dan Poltawski added a comment - Seems sensible to me. Is this using the same technique to work out the section as file drag/drop to section is using? If so +1. If not, it'd be good to make sure both were using a consistent approach (and thus will both be broken in one swoop if something changes )
          Hide
          Andrew Nicols added a comment -

          Hi Dan,

          This works in a similar way to dndupload.js but it's not the same. We both make the same assumptions but use them in a slightly different manner. I take the section id 'section-1' and perform a string replace to remove 'section-' leaving just the number 1. This is the same method as used in the section toolboxes (show/hide sections etc) and in the section and resource drag/drop.

          In comparison, dndupload.js performs a split on the '-' character, leaving an array ['section', '1']. It then takes the second field '1' as the section number.

          I think that perhaps this should all be standardised in the future. We've introduced a M.course.coursebase which is already used by toolboxes, section/resource drag and drop, the module chooser, and dndupload.js. We should look at moving the existing get_section_id() function from toolboxes to coursebase and using it uniformly across all of these locations. There are other similar improvements we should also consider.

          Do you agree that since this is the same technique as used by several other locations it's best to keep it as is, rather than convert to the same method as dndupload.js?

          Andrew

          Show
          Andrew Nicols added a comment - Hi Dan, This works in a similar way to dndupload.js but it's not the same. We both make the same assumptions but use them in a slightly different manner. I take the section id 'section-1' and perform a string replace to remove 'section-' leaving just the number 1. This is the same method as used in the section toolboxes (show/hide sections etc) and in the section and resource drag/drop. In comparison, dndupload.js performs a split on the '-' character, leaving an array ['section', '1'] . It then takes the second field '1' as the section number. I think that perhaps this should all be standardised in the future. We've introduced a M.course.coursebase which is already used by toolboxes, section/resource drag and drop, the module chooser, and dndupload.js. We should look at moving the existing get_section_id() function from toolboxes to coursebase and using it uniformly across all of these locations. There are other similar improvements we should also consider. Do you agree that since this is the same technique as used by several other locations it's best to keep it as is, rather than convert to the same method as dndupload.js? Andrew
          Hide
          Dan Poltawski added a comment - - edited

          Its essentially the same in both (you both depend on that section format, even if you compute it different ways).

          So pushing for integration.

          Show
          Dan Poltawski added a comment - - edited Its essentially the same in both (you both depend on that section format, even if you compute it different ways). So pushing for integration.
          Hide
          Aparup Banerjee added a comment - - edited

          hm, i'm getting conflicts with the master branch on integration - i tried
          https://github.com/nebgor/moodle/compare/integration_master...int_MDL-33683
          but tried some of the test, the bit about creating after the dragging to top didn't work for me... something really messes up.

          theres also an issue with dragging sections on a long page.. i have to hop up rather than drag through in one shot as the page doesn't scroll to let me drop it on to.. but thats probably another issue.

          Show
          Aparup Banerjee added a comment - - edited hm, i'm getting conflicts with the master branch on integration - i tried https://github.com/nebgor/moodle/compare/integration_master...int_MDL-33683 but tried some of the test, the bit about creating after the dragging to top didn't work for me... something really messes up. theres also an issue with dragging sections on a long page.. i have to hop up rather than drag through in one shot as the page doesn't scroll to let me drop it on to.. but thats probably another issue.
          Hide
          Andrew Nicols added a comment -

          I've updated the pull branch - you're missing the change in CSS selector in your rebase.

          Show
          Andrew Nicols added a comment - I've updated the pull branch - you're missing the change in CSS selector in your rebase.
          Hide
          Aparup Banerjee added a comment -

          ok the fix for master has been integrated. (Andrew, my tests were probably on a borked course format,a new course and all seems fine now) - up for more testing.

          Hi Emma, i'm not sure if this is still an issue for 2.2.x. If it is, we can open up a new issue - this would issue would have to move along for now as we're near to release.

          Show
          Aparup Banerjee added a comment - ok the fix for master has been integrated. (Andrew, my tests were probably on a borked course format,a new course and all seems fine now) - up for more testing. Hi Emma, i'm not sure if this is still an issue for 2.2.x. If it is, we can open up a new issue - this would issue would have to move along for now as we're near to release.
          Hide
          Andrew Davis added a comment -

          Seems to work as described. Passing.

          Show
          Andrew Davis added a comment - Seems to work as described. Passing.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay!

          Many, many thanks for your hard work!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - And this has been incorporated to all the weekly builds and also, to Moodle 2.3 Release Candidate 1, yay! Many, many thanks for your hard work! Ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: