Moodle
  1. Moodle
  2. MDL-33546

Current week not updating after section drag-and-drop

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3, 2.4.1, 2.5
    • Fix Version/s: 2.3.8, 2.4.5, 2.5.1, 2.6
    • Component/s: Course
    • Labels:
    • Testing Instructions:
      Hide

      You may need to purge the caches because JS files were changed

      1. Open the course in weekly format, make sure that the course start is recent, so you have a highlighted the current section.
      2. Move the 'current' section somewhere up or down, on the dropping the replacement section of the current week should be highlighted.
      3. Move some section in replacement of the 'current', on the dropping the new section in the current week should be highlighted.
      4. Move section that was earlier than current to the position later than current and vice versa, make sure the current week is highlighted
      5. Change course start date to be far in the past or in the future (so there is no section highlighted as current)
      6. Make sure drag&drop moving of sections works fine

      Note that solution for 2.3 is different (because of different API) and unfortunately does not work well when there are hidden sections present.

      Show
      You may need to purge the caches because JS files were changed Open the course in weekly format, make sure that the course start is recent, so you have a highlighted the current section. Move the 'current' section somewhere up or down, on the dropping the replacement section of the current week should be highlighted. Move some section in replacement of the 'current', on the dropping the new section in the current week should be highlighted. Move section that was earlier than current to the position later than current and vice versa, make sure the current week is highlighted Change course start date to be far in the past or in the future (so there is no section highlighted as current) Make sure drag&drop moving of sections works fine Note that solution for 2.3 is different (because of different API) and unfortunately does not work well when there are hidden sections present.
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull 2.4 Branch:
      MDL-33546-MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-33546-master
    • Rank:
      41474

      Description

      When moving a section in a course using the Weekly format, if you move the section highlighted as current (according to the date) or if you move sections around the current week, the highlighting of the current week sticks with the original section rather than adjusting to match the section that contains the current date.

      Replication steps:

      1. Create a course and set the format to Weekly format (or use an existing course and switch it)
      2. Ensure the range of time covered by the course includes the current date
      3. The section relevant to the current date should be highlighted; drag this section to a new location
      4. Refresh the course page
      5. Move sections around the currently highlighted section causing the section dates to adjust

      Expected result: the highlighting of the current section changes to highlight the section relevant to the section containing the current date
      Actual result: the highlighting sticks to the previously highlighted section

      After refreshing the course page, the highlighting corrects itself.

        Issue Links

          Activity

          Hide
          Dan Poltawski added a comment -

          Andrew, I know this is Ruslan's area, but I was wondering if you could look at it. I suppose we need to just do a swap in weekly format.

          Show
          Dan Poltawski added a comment - Andrew, I know this is Ruslan's area, but I was wondering if you could look at it. I suppose we need to just do a swap in weekly format.
          Hide
          Andrew Nicols added a comment -

          I've had a look at this but was unable to work out the best fix. Passing back to Ruslan as he's available again.

          Show
          Andrew Nicols added a comment - I've had a look at this but was unable to work out the best fix. Passing back to Ruslan as he's available again.
          Hide
          Ruslan Kabalin added a comment -

          Passed through jshint, ready for revision.

          Show
          Ruslan Kabalin added a comment - Passed through jshint, ready for revision.
          Hide
          Ruslan Kabalin added a comment -

          Updated branches, added punctuation.

          Show
          Ruslan Kabalin added a comment - Updated branches, added punctuation.
          Hide
          Andrew Nicols added a comment -

          [Y] Syntax
          [Y] Output
          [Y] Whitespace
          [-] Language
          [-] Databases
          [Y] Testing
          [-] Security
          [-] Documentation
          [Y] Git
          [Y] Sanity check

          Looks good to me. Feel free to submit for integration when ready.

          Show
          Andrew Nicols added a comment - [Y] Syntax [Y] Output [Y] Whitespace [-] Language [-] Databases [Y] Testing [-] Security [-] Documentation [Y] Git [Y] Sanity check Looks good to me. Feel free to submit for integration when ready.
          Hide
          Dan Poltawski added a comment -

          Does this need backporting to 2.3 too?

          Show
          Dan Poltawski added a comment - Does this need backporting to 2.3 too?
          Hide
          Ruslan Kabalin added a comment -

          Dan: the work needs to be done to backport it to 2.3, as we did not have that neaty per-format JS hooks. So, unless it is absolutely necessary, I would not backport this to 2.3.

          Show
          Ruslan Kabalin added a comment - Dan: the work needs to be done to backport it to 2.3, as we did not have that neaty per-format JS hooks. So, unless it is absolutely necessary, I would not backport this to 2.3.
          Hide
          Dan Poltawski added a comment -

          Hi Ruslan,

          Unless I am missing something, these hooks are present. In MDL-32657 c77582fe42 you introduced course/format/weeks/format.js into 2.3.

          Show
          Dan Poltawski added a comment - Hi Ruslan, Unless I am missing something, these hooks are present. In MDL-32657 c77582fe42 you introduced course/format/weeks/format.js into 2.3.
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Ruslan Kabalin added a comment -

          Dan: you are right, sorry

          Show
          Ruslan Kabalin added a comment - Dan: you are right, sorry
          Hide
          Ruslan Kabalin added a comment - - edited

          Added MDL-33546-MOODLE_23_STABLE, diff needs to be reviewed. Thanks!

          Show
          Ruslan Kabalin added a comment - - edited Added MDL-33546 -MOODLE_23_STABLE, diff needs to be reviewed. Thanks!
          Hide
          Andrew Nicols added a comment -

          Looks fine - submit for IR when ready.

          Show
          Andrew Nicols added a comment - Looks fine - submit for IR when ready.
          Hide
          Ruslan Kabalin added a comment -

          Added 2.3 branch, ready for integration

          Show
          Ruslan Kabalin added a comment - Added 2.3 branch, ready for integration
          Hide
          Damyon Wiese added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          Thanks!

          Show
          Damyon Wiese added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. Thanks!
          Hide
          Dan Poltawski added a comment -

          Hi Ruslan,

          this appeares to be conflicting with MDL-34798, please could you rebase?

          Show
          Dan Poltawski added a comment - Hi Ruslan, this appeares to be conflicting with MDL-34798 , please could you rebase?
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Ruslan Kabalin added a comment -

          Conflicts resolved, sorry for the delay. Ready for integration (code has not been changed).

          Show
          Ruslan Kabalin added a comment - Conflicts resolved, sorry for the delay. Ready for integration (code has not been changed).
          Hide
          Marina Glancy added a comment -

          Ruslan, thanks for working on it. 2.4+ looks good. But in 2.3 we can not change the function visibility - it will break the themes that overwrite this function in this renderer.

          It seems the same can be done without changing of php files at all - after the move index of the current section stays the same. I just tried and this code works:

          --- a/course/format/weeks/format.js
          +++ b/course/format/weeks/format.js
          @@ -63,6 +63,7 @@ M.course.format.process_sections = function(Y, sectionlist, response, sectionfro
           
               if (response.action == 'move') {
                   // If moving up swap around 'sectionfrom' and 'sectionto' so the that loop operates.
          +        var movedirection = (sectionfrom > sectionto) ? -1 : 1;
                   if (sectionfrom > sectionto) {
                       var temp = sectionto;
                       sectionto = sectionfrom;
          @@ -82,6 +83,11 @@ M.course.format.process_sections = function(Y, sectionlist, response, sectionfro
                       newstr = str.substr(0, stridx +1) + i;
                       ele.setAttribute('alt', newstr);
                       ele.setAttribute('title', newstr); // For FireFox as 'alt' is not refreshed.
          +            if (sectionlist.item(i).hasClass('current')) {
          +                // Move the current section forward or backward
          +                sectionlist.item(i).removeClass('current');
          +                sectionlist.item(i + movedirection).addClass('current');
          +            }
                   }
               }
           }
          

          I do not fail this issue for now because correction should not be difficult. If you resubmit today or tomorrow I can still integrate it in this cycle.

          Show
          Marina Glancy added a comment - Ruslan, thanks for working on it. 2.4+ looks good. But in 2.3 we can not change the function visibility - it will break the themes that overwrite this function in this renderer. It seems the same can be done without changing of php files at all - after the move index of the current section stays the same. I just tried and this code works: --- a/course/format/weeks/format.js +++ b/course/format/weeks/format.js @@ -63,6 +63,7 @@ M.course.format.process_sections = function(Y, sectionlist, response, sectionfro if (response.action == 'move') { // If moving up swap around 'sectionfrom' and 'sectionto' so the that loop operates. + var movedirection = (sectionfrom > sectionto) ? -1 : 1; if (sectionfrom > sectionto) { var temp = sectionto; sectionto = sectionfrom; @@ -82,6 +83,11 @@ M.course.format.process_sections = function(Y, sectionlist, response, sectionfro newstr = str.substr(0, stridx +1) + i; ele.setAttribute('alt', newstr); ele.setAttribute('title', newstr); // For FireFox as 'alt' is not refreshed. + if (sectionlist.item(i).hasClass('current')) { + // Move the current section forward or backward + sectionlist.item(i).removeClass('current'); + sectionlist.item(i + movedirection).addClass('current'); + } } } } I do not fail this issue for now because correction should not be difficult. If you resubmit today or tomorrow I can still integrate it in this cycle.
          Hide
          Marina Glancy added a comment -

          something is not right in my suggestion - if the movesection==1 the current week will be moved over and over again each cycle. But similar approach should work

          Show
          Marina Glancy added a comment - something is not right in my suggestion - if the movesection==1 the current week will be moved over and over again each cycle. But similar approach should work
          Hide
          Ruslan Kabalin added a comment - - edited

          Marina, thanks for integration review. I have updated 2.3 branch following the idea in your suggestion. It indeed did not work in some cases. I utilised the swap function we have there instead.

          Show
          Ruslan Kabalin added a comment - - edited Marina, thanks for integration review. I have updated 2.3 branch following the idea in your suggestion. It indeed did not work in some cases. I utilised the swap function we have there instead.
          Hide
          Marina Glancy added a comment - - edited

          wow, awesome, thanks.

          I just did a quick test and noticed that 'hidden' sections don't have class 'current' and in this case this logic breaks.
          See https://github.com/moodle/moodle/blob/MOODLE_23_STABLE/course/format/renderer.php#L140

          Guess we just have to live with that (or create a separate issue).

          Show
          Marina Glancy added a comment - - edited wow, awesome, thanks. I just did a quick test and noticed that 'hidden' sections don't have class 'current' and in this case this logic breaks. See https://github.com/moodle/moodle/blob/MOODLE_23_STABLE/course/format/renderer.php#L140 Guess we just have to live with that (or create a separate issue).
          Hide
          Ruslan Kabalin added a comment -

          I see, I think we can live with that

          Show
          Ruslan Kabalin added a comment - I see, I think we can live with that
          Hide
          Marina Glancy added a comment -

          Thanks this has been integrated in 2.3, 2.4, 2.5, 2.6

          Show
          Marina Glancy added a comment - Thanks this has been integrated in 2.3, 2.4, 2.5, 2.6
          Hide
          Ankit Agarwal added a comment - - edited

          This is happening in 2.3
          I moved the section before the current, to few sections down than the current. The correct week was not highlighted.
          Also if I drop the section before current to current, the sections name is all getting messed up. It does go away after a refresh.
          Edit:- Browser used was firefox

          Show
          Ankit Agarwal added a comment - - edited This is happening in 2.3 I moved the section before the current, to few sections down than the current. The correct week was not highlighted. Also if I drop the section before current to current, the sections name is all getting messed up. It does go away after a refresh. Edit:- Browser used was firefox
          Hide
          Ruslan Kabalin added a comment -

          Section names should not be affected by this fix at all - a different bug then if you can replicate it again. Do you get any JS errors by the way?

          Show
          Ruslan Kabalin added a comment - Section names should not be affected by this fix at all - a different bug then if you can replicate it again. Do you get any JS errors by the way?
          Hide
          Ankit Agarwal added a comment - - edited

          No errors, the console was clean. Happens less often on master. Although in master I did end up with two sections highlighted at a point. Will update if I can figure out exact replication steps for this.
          Edit:- I am able to replicate the section name mess at will in master as well. I will report it as a different issue.

          Edit:- Am not able to replicate other issues at will, they did happen more than once. I will leave it to Marina and Ruslan , if they want to go forward with the patch or not.
          Thanks

          Show
          Ankit Agarwal added a comment - - edited No errors, the console was clean. Happens less often on master. Although in master I did end up with two sections highlighted at a point. Will update if I can figure out exact replication steps for this. Edit:- I am able to replicate the section name mess at will in master as well. I will report it as a different issue. Edit:- Am not able to replicate other issues at will, they did happen more than once. I will leave it to Marina and Ruslan , if they want to go forward with the patch or not. Thanks
          Hide
          Marina Glancy added a comment -

          I tested it myself moving sections for 10 minutes back and forth in 2.3 and 2.4 and noticed no errors at all. The current week was always highlighted.
          I would say that the double highlighting is a rare case and most likely result of multiple moves one after another without page refresh (so, there is one ajax response on top of another), so it is not likely to happen in real life. Code looks very clean.
          I will vote for passing it

          Show
          Marina Glancy added a comment - I tested it myself moving sections for 10 minutes back and forth in 2.3 and 2.4 and noticed no errors at all. The current week was always highlighted. I would say that the double highlighting is a rare case and most likely result of multiple moves one after another without page refresh (so, there is one ajax response on top of another), so it is not likely to happen in real life. Code looks very clean. I will vote for passing it
          Hide
          Ankit Agarwal added a comment -

          Passing as suggested.
          Thanks

          Show
          Ankit Agarwal added a comment - Passing as suggested. Thanks
          Hide
          Dan Poltawski added a comment -

          Thanks for your contributions!

          _main:
          @ BB#0:
                  push    {r7, lr}
                  mov     r7, sp
                  sub     sp, #4
                  movw    r0, :lower16:(L_.str-(LPC0_0+4))
                  movt    r0, :upper16:(L_.str-(LPC0_0+4))
          LPC0_0:
                  add     r0, pc
                  bl      _printf
                  movs    r1, #0
                  movt    r1, #0
                  str     r0, [sp]                @ 4-byte Spill
                  mov     r0, r1
                  add     sp, #4
                  pop     {r7, pc}
          
                  .section        __TEXT,__cstring,cstring_literals
          L_.str:                                 @ @.str
                  .asciz   "This code is now upstream!"
          
          Show
          Dan Poltawski added a comment - Thanks for your contributions! _main: @ BB#0: push {r7, lr} mov r7, sp sub sp, #4 movw r0, :lower16:(L_.str-(LPC0_0+4)) movt r0, :upper16:(L_.str-(LPC0_0+4)) LPC0_0: add r0, pc bl _printf movs r1, #0 movt r1, #0 str r0, [sp] @ 4- byte Spill mov r0, r1 add sp, #4 pop {r7, pc} .section __TEXT,__cstring,cstring_literals L_.str: @ @.str .asciz "This code is now upstream!"

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: