Moodle
  1. Moodle
  2. MDL-30302

Default lookahead of 21 days is ignored in calendar preferences

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor 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:
    • Rank:
      32648

      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.

        Activity

        Hide
        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
        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
        Andrew Nicols added a comment -

        This commit cherry-picks cleanly to 2.0 and 2.1 branches

        Show
        Andrew Nicols added a comment - This commit cherry-picks cleanly to 2.0 and 2.1 branches
        Hide
        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
        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
        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
        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
        Michael de Raadt added a comment -

        Hi, Rosie.

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

        Show
        Michael de Raadt added a comment - Hi, Rosie. As you've been working on the calendar lately, could you peer review this issue?
        Hide
        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
        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
        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
        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
        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
        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
        Rajesh Taneja added a comment -

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

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

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

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

        Thanks everyone!
        This has been integrated now

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

        Tested during integration

        Show
        Sam Hemelryk added a comment - Tested during integration
        Hide
        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
        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: