Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-33546

Current week not updating after section drag-and-drop

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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 Master Branch:
      MDL-33546-master

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            poltawski 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
            poltawski 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
            dobedobedoh 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
            dobedobedoh 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
            kabalin Ruslan Kabalin added a comment -

            Passed through jshint, ready for revision.

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

            Updated branches, added punctuation.

            Show
            kabalin Ruslan Kabalin added a comment - Updated branches, added punctuation.
            Hide
            dobedobedoh 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
            dobedobedoh 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
            poltawski Dan Poltawski added a comment -

            Does this need backporting to 2.3 too?

            Show
            poltawski Dan Poltawski added a comment - Does this need backporting to 2.3 too?
            Hide
            kabalin 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
            kabalin 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
            poltawski 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
            poltawski 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 CiBoT added a comment -

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

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

            Dan: you are right, sorry

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

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

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

            Looks fine - submit for IR when ready.

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

            Added 2.3 branch, ready for integration

            Show
            kabalin Ruslan Kabalin added a comment - Added 2.3 branch, ready for integration
            Hide
            damyon 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 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
            poltawski Dan Poltawski added a comment -

            Hi Ruslan,

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

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

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

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

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

            Show
            kabalin Ruslan Kabalin added a comment - Conflicts resolved, sorry for the delay. Ready for integration (code has not been changed).
            Hide
            marina 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 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 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 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
            kabalin 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
            kabalin 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 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 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
            kabalin Ruslan Kabalin added a comment -

            I see, I think we can live with that

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

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

            Show
            marina Marina Glancy added a comment - Thanks this has been integrated in 2.3, 2.4, 2.5, 2.6
            Hide
            ankit_frenz 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_frenz 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
            kabalin 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
            kabalin 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_frenz 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_frenz 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 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 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_frenz Ankit Agarwal added a comment -

            Passing as suggested.
            Thanks

            Show
            ankit_frenz Ankit Agarwal added a comment - Passing as suggested. Thanks
            Hide
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  8/Jul/13