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

          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