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

Default lookahead of 21 days is ignored in calendar preferences

    Details

    • Type: Bug
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.0.5, 2.1.2, 2.2
    • Fix Version/s: 2.0.7, 2.1.4
    • Component/s: Calendar
    • Labels:

      Description

      The default lookahead for the calendar is 21 days.
      However the calendar preferences page only has options for 1-20 days which means that the <select> element defaults to the first element - 1 days.

      As a result, users visiting the page may inadvertently update their calendar preferences to set their lookahead days to 1 day.

        Gliffy Diagrams

          Attachments

            Activity

            Hide
            dobedobedoh Andrew Nicols added a comment -

            In addition to this, /admin/settings.php?section=calendar allows administrators to specify the number of days as a string and doesn't limit them to 21 days.

            As a result, an administrator could increase the lookahead (and maxdays) to greater than the maximum and thereby cause this issue for all users.

            Show
            dobedobedoh Andrew Nicols added a comment - In addition to this, /admin/settings.php?section=calendar allows administrators to specify the number of days as a string and doesn't limit them to 21 days. As a result, an administrator could increase the lookahead (and maxdays) to greater than the maximum and thereby cause this issue for all users.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            This commit cherry-picks cleanly to 2.0 and 2.1 branches

            Show
            dobedobedoh Andrew Nicols added a comment - This commit cherry-picks cleanly to 2.0 and 2.1 branches
            Hide
            dobedobedoh Andrew Nicols added a comment -

            There are several options for this fix:

            • convert the lookahead and maxdays to text boxes on the calendar/preferences_form.php definition;
            • increase the lookahead option in the calendar/preferences_form.php definition; and convert these options to dropdowns in /admin/settings.php?section=calendar; or
            • only increase the lookahead option in the calendar/preferences_form.php definition and ignore the text box issue

            I've gone for the middle option. I looked at converting the definition to use text boxes but wasn't happy with the result (mainly style-related).

            I also looked at modifying the drop down to give options like:

            • 1 day
            • 2 days
            • 3 days
            • 1 week
            • 2 weeks
            • 3 weeks
            • 1 month

            But getting the plurals to work and translate nicely feels like a deal-breaker to me.

            Show
            dobedobedoh Andrew Nicols added a comment - There are several options for this fix: convert the lookahead and maxdays to text boxes on the calendar/preferences_form.php definition; increase the lookahead option in the calendar/preferences_form.php definition; and convert these options to dropdowns in /admin/settings.php?section=calendar; or only increase the lookahead option in the calendar/preferences_form.php definition and ignore the text box issue I've gone for the middle option. I looked at converting the definition to use text boxes but wasn't happy with the result (mainly style-related). I also looked at modifying the drop down to give options like: 1 day 2 days 3 days 1 week 2 weeks 3 weeks 1 month But getting the plurals to work and translate nicely feels like a deal-breaker to me.
            Hide
            salvetore Michael de Raadt added a comment -

            Thanks for suggesting that and providing a solution. I'll see if I can get a peer reviewer for you.

            Show
            salvetore Michael de Raadt added a comment - Thanks for suggesting that and providing a solution. I'll see if I can get a peer reviewer for you.
            Hide
            salvetore Michael de Raadt added a comment -

            Hi, Rosie.

            As you've been working on the calendar lately, could you peer review this issue?

            Show
            salvetore Michael de Raadt added a comment - Hi, Rosie. As you've been working on the calendar lately, could you peer review this issue?
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks for providing the patch, Andrew
            I agree that there should be some synchronization between admin and user calendar settings. But, I don't agree with the patch provided by you.
            Adding Sam, to provide some feedback on this.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks for providing the patch, Andrew I agree that there should be some synchronization between admin and user calendar settings. But, I don't agree with the patch provided by you. Adding Sam, to provide some feedback on this.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Hello Andrew,

            Just had word with Sam and Martin. Your patch seems Great, it's just that we can increase the range to 99 (i.e 1 to 99) for "Upcoming events look-ahead".
            Can you please update your patch, so that I can push it for integration review

            Thanks for reporting the issue and providing solution for it.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Hello Andrew, Just had word with Sam and Martin. Your patch seems Great, it's just that we can increase the range to 99 (i.e 1 to 99) for "Upcoming events look-ahead". Can you please update your patch, so that I can push it for integration review Thanks for reporting the issue and providing solution for it.
            Hide
            dobedobedoh Andrew Nicols added a comment -

            Patch updated as per your request - Changed from 21 days to 99.

            This should still cherry-pick cleanly to stable branches

            Show
            dobedobedoh Andrew Nicols added a comment - Patch updated as per your request - Changed from 21 days to 99. This should still cherry-pick cleanly to stable branches
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            Thanks Andrew for providing spot-on patch
            Pushing this for integration review.

            Show
            rajeshtaneja Rajesh Taneja added a comment - Thanks Andrew for providing spot-on patch Pushing this for integration review.
            Hide
            rajeshtaneja Rajesh Taneja added a comment -

            @Integrator's:
            Please cherry-pick this on 2.x stable branches.

            Show
            rajeshtaneja Rajesh Taneja added a comment - @Integrator's: Please cherry-pick this on 2.x stable branches.
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Thanks everyone!
            This has been integrated now

            Show
            samhemelryk Sam Hemelryk added a comment - Thanks everyone! This has been integrated now
            Hide
            samhemelryk Sam Hemelryk added a comment -

            Tested during integration

            Show
            samhemelryk Sam Hemelryk added a comment - Tested during integration
            Hide
            stronk7 Eloy Lafuente (stronk7) added a comment -

            The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days.

            Thanks for the hard work! Closing, ciao

            Show
            stronk7 Eloy Lafuente (stronk7) added a comment - The master fixes corresponding to this issue have been sent upstream. Fixes for other branches (19, 20, 21 stable) will be sent in the very-next days. Thanks for the hard work! Closing, ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  9/Jan/12