Moodle
  1. Moodle
  2. MDL-37548

Invalid value entered in colour picker displays warning with last value saved shown, rather than the erroneous value.

    Details

    • Testing Instructions:
      Hide
      • Open the 'Formal white' theme's settings page
      • Enter a deliberately invalid value for one of the colour pickers (e.g. #1234567)
      • Attempt to save the form
        • Confirm that the form reports an invalid value
        • Confirm that the colour picker in question is highlighted
        • Confirm that the value shown for that colour picker is the value you that you entered
      Show
      Open the 'Formal white' theme's settings page Enter a deliberately invalid value for one of the colour pickers (e.g. #1234567) Attempt to save the form Confirm that the form reports an invalid value Confirm that the colour picker in question is highlighted Confirm that the value shown for that colour picker is the value you that you entered
    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE
    • Pull 2.4 Branch:
      MDL-37548-m24
    • Pull Master Branch:
    • Rank:
      47196

      Description

      1. Login as an admin.
      2. Navigate to Site administration ► Appearance ► Themes ► Formal white.
      3. In any of the colour pickers on the page enter #0404E61 as the value.
      4. Click save.
      5. You will be given a warning message around the field with the value displayed being the last colour that was saved successfully, rather than the value that caused the warning to display. This is goes against standard Moodle behaviour.

        Issue Links

          Activity

          Hide
          Mary Evans added a comment - - edited

          This is not a theme issue, at least its not something that can be fixed in a theme. As I am not sure where one would fix this, so I'm taking a wild guess that it's a JavaScript thing, if it's to do with the ColorPicker.

          Show
          Mary Evans added a comment - - edited This is not a theme issue, at least its not something that can be fixed in a theme. As I am not sure where one would fix this, so I'm taking a wild guess that it's a JavaScript thing, if it's to do with the ColorPicker.
          Hide
          Andrew Nicols added a comment -

          Time to learn something new in Moodle I'll take a look in the morning Mary. Thanks!

          Show
          Andrew Nicols added a comment - Time to learn something new in Moodle I'll take a look in the morning Mary. Thanks!
          Hide
          Mark Nelson added a comment -

          Sorry Mary, I realise it is not a theme issue but could not think of a suitable component from the list (not that I gave it much thought - sorry). I just assumed it would be assigned to moodle.com and changed later.

          Show
          Mark Nelson added a comment - Sorry Mary, I realise it is not a theme issue but could not think of a suitable component from the list (not that I gave it much thought - sorry). I just assumed it would be assigned to moodle.com and changed later.
          Hide
          Michael de Raadt added a comment -

          Thanks for volunteering to have a look at this, Andrew.

          Show
          Michael de Raadt added a comment - Thanks for volunteering to have a look at this, Andrew.
          Hide
          Andrew Nicols added a comment -

          This turned out to be an adminlib issues rather than a JS issue.

          I'll produce backport branches following peer review.

          Show
          Andrew Nicols added a comment - This turned out to be an adminlib issues rather than a JS issue. I'll produce backport branches following peer review.
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew,

          Patch looks good.
          Just wondering can't we use $data to display this value?

          FYI: $data is default/entered value in form, so setting $value=$data should solve this problem.

          Show
          Rajesh Taneja added a comment - Thanks Andrew, Patch looks good. Just wondering can't we use $data to display this value? FYI: $data is default/entered value in form, so setting $value=$data should solve this problem.
          Hide
          Andrew Nicols added a comment -

          Thanks for that - Learning more about adminlib!

          I guess this is right now..?

          Show
          Andrew Nicols added a comment - Thanks for that - Learning more about adminlib! I guess this is right now..?
          Hide
          Rajesh Taneja added a comment -

          Thanks Andrew,

          Looks good to me.

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

          Please create 23 and 24 branches, before pushing for integration.

          Show
          Rajesh Taneja added a comment - Thanks Andrew, Looks good to me. [y] Syntax [y] Output [y] Whitespace [-] Language [-] Databases [y] Testing [-] Security [-] Documentation [y] Git [y] Sanity check Please create 23 and 24 branches, before pushing for integration.
          Hide
          Mary Evans added a comment -

          That looks neat Andrew.

          That's also a neat way to write the Diff line I write mine as ../moodle/compare/master...MDL-xxxxx

          writing it your way as ../moodle/commit/MDL-xxxxx is much simpler...I'll try that next time.

          Show
          Mary Evans added a comment - That looks neat Andrew. That's also a neat way to write the Diff line I write mine as ../moodle/compare/master...MDL-xxxxx writing it your way as ../moodle/commit/MDL-xxxxx is much simpler...I'll try that next time.
          Hide
          Eloy Lafuente (stronk7) 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.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) 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. TIA and ciao
          Hide
          Sam Hemelryk added a comment -

          Thanks Andrew, changes look good and have been integrated now

          Show
          Sam Hemelryk added a comment - Thanks Andrew, changes look good and have been integrated now
          Hide
          David Monllaó added a comment -

          Tested in 23 and master, it passes

          Show
          David Monllaó added a comment - Tested in 23 and master, it passes
          Hide
          Eloy Lafuente (stronk7) added a comment -

          A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed.

          (and won't be revisiting it unless some regression is found)

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - A brilliant future is awaiting us out there, better with your code. Let's look towards the future together, this is now closed. (and won't be revisiting it unless some regression is found) Thanks and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: