Moodle
  1. Moodle
  2. MDL-33623

Section highlighting does not stick with topic section with AJAX off

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide
      1. Run phpunit tests
      1. Log in as admin
      2. Navigate to Site admin > Appearance > AJAX and JavaScript
      3. Uncheck "Enable AJAX" and save settings
      4. Navigate to a course or create a new one
      5. Edit the course settings and set the course format to Topics
      6. Add some content to sections so you can track their movement
      7. Highlight a section somewhere in the middle
      8. Click the up icon to shift the highlighted section up
      9. VERIFY: highlighted section is moved up
      10. Click the down icon to shift the highlighted section down
      11. VERIFY: highlighted section is moved down
      12. Navigate to Site admin > Appearance > AJAX and JavaScript
      13. Check "Enable AJAX" and save settings
      14. Navigate to the same course
      15. Drag the highlighted course section up
      16. VERIFY: turn editing off and verify that the highlighted section has moved
      17. Drag the highlighted course section down
      18. VERIFY: turn editing off and verify that the highlighted section has moved
      19. Drag the a non-highlighted section from below the highlighted section to a place above it
      20. VERIFY: turn editing off and verify that the highlighted section has now changed, it is the same section.
      21. Drag the a non-highlighted section from above the highlighted section to a place below it
      22. VERIFY: turn editing off and verify that the highlighted section has now changed, it is the same section.
      Show
      Run phpunit tests Log in as admin Navigate to Site admin > Appearance > AJAX and JavaScript Uncheck "Enable AJAX" and save settings Navigate to a course or create a new one Edit the course settings and set the course format to Topics Add some content to sections so you can track their movement Highlight a section somewhere in the middle Click the up icon to shift the highlighted section up VERIFY: highlighted section is moved up Click the down icon to shift the highlighted section down VERIFY: highlighted section is moved down Navigate to Site admin > Appearance > AJAX and JavaScript Check "Enable AJAX" and save settings Navigate to the same course Drag the highlighted course section up VERIFY: turn editing off and verify that the highlighted section has moved Drag the highlighted course section down VERIFY: turn editing off and verify that the highlighted section has moved Drag the a non-highlighted section from below the highlighted section to a place above it VERIFY: turn editing off and verify that the highlighted section has now changed, it is the same section. Drag the a non-highlighted section from above the highlighted section to a place below it VERIFY: turn editing off and verify that the highlighted section has now changed, it is the same section.
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE
    • Pull Master Branch:
    • Rank:
      41568

      Description

      I discovered a peculiar error when shifting sections while testing. When I highlighted a section and shifted that section up, the highlighting shifted to the section that took its place (formerly above, now below). When I shifted a highlighted section down, the highlighting stayed with the topic section. This seems to happen only with AJAX turned off.

      This reminded me of a case that Raj mentioned recently where unit tests were only testing section moving in one direction, but not the other. It might be worth checking the unit tests for this as well.

      Replication steps:

      1. Log in as admin
      2. Navigate to Site admin > Appearance > AJAX and JavaScript
      3. Uncheck "Enable AJAX" and save settings
      4. Navigate to a course or create a new one
      5. Edit the course settings and set the course format to Topics
      6. Add some content to sections so you can track their movement
      7. Highlight a section somewhere in the middle
      8. Click the up icon to shift the highlighted section up
      9. Click the down icon to shift the highlighted section down
      10. Navigate to Site admin > Appearance > AJAX and JavaScript
      11. Check "Enable AJAX" and save settings
      12. Navigate to the same course
      13. Drag the highlighted course section up
      14. Drag the highlighted course section down

      Expected result: The highlighting stays with the same topic section

      Actual result: When moving the highlighted section upwards, with AJAX off, highlighting shifts to another section

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Hmm. the move section function depends on a decent course record being passed to it.

          Show
          Dan Poltawski added a comment - Hmm. the move section function depends on a decent course record being passed to it.
          Hide
          Dan Poltawski added a comment -

          Hmm, well this is very weird because it seemed to be a line of code doing exactly the wrong thing. I've pondered for a while if i'm missing something, but I don't think I am. I expect it was made incorrect by the changes in MDL-33212.

          I have added unit tests to prove my theories in moving sections and ensuring the marker is kept correctly.

          Show
          Dan Poltawski added a comment - Hmm, well this is very weird because it seemed to be a line of code doing exactly the wrong thing. I've pondered for a while if i'm missing something, but I don't think I am. I expect it was made incorrect by the changes in MDL-33212 . I have added unit tests to prove my theories in moving sections and ensuring the marker is kept correctly.
          Hide
          Dan Poltawski added a comment -

          Adding Ruslan here as the code came from him in MDL-31216

          Show
          Dan Poltawski added a comment - Adding Ruslan here as the code came from him in MDL-31216
          Hide
          Rajesh Taneja added a comment -

          Code looks Good Dan. Few things you might want to consider before pushing it for integration:

          1. We don't need $moveup status, so probably can remove
                $moveup = false;
                if ($section > $destination) {
                    $moveup = true;
                }
            

            and it's use.

          2. Add test instructions
          Show
          Rajesh Taneja added a comment - Code looks Good Dan. Few things you might want to consider before pushing it for integration: We don't need $moveup status, so probably can remove $moveup = false ; if ($section > $destination) { $moveup = true ; } and it's use. Add test instructions
          Hide
          Dan Poltawski added a comment -

          Hi Raj, moveup is used just a few lines below it?

          Show
          Dan Poltawski added a comment - Hi Raj, moveup is used just a few lines below it?
          Hide
          Dan Poltawski added a comment -

          Raj explained to me that in fact there is no need for moveup in the following lines because of the other checks going on.

          Show
          Dan Poltawski added a comment - Raj explained to me that in fact there is no need for moveup in the following lines because of the other checks going on.
          Hide
          Dan Poltawski added a comment -

          Thanks Raj for the comments, i've updated testing instructions and submitted

          Show
          Dan Poltawski added a comment - Thanks Raj for the comments, i've updated testing instructions and submitted
          Hide
          Dan Poltawski added a comment -

          (Updated code and testing instructions)

          Show
          Dan Poltawski added a comment - (Updated code and testing instructions)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Integrated, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Integrated, thanks!
          Hide
          Dan Poltawski added a comment -

          Pinging Michael!

          Show
          Dan Poltawski added a comment - Pinging Michael!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          getting onto this for rc1 release...

          Show
          Eloy Lafuente (stronk7) added a comment - getting onto this for rc1 release...
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Passing.

          I've run the unit tests against mysql and postgresql and also followed the steps (all good). Thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Passing. I've run the unit tests against mysql and postgresql and also followed the steps (all good). Thanks!
          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:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: