Uploaded image for project: '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 Master Branch:

      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.

        Gliffy Diagrams

          Issue Links

            Activity

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

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

            Show
            dobedobedoh Andrew Nicols added a comment - Time to learn something new in Moodle I'll take a look in the morning Mary. Thanks!
            Hide
            markn 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
            markn 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
            salvetore Michael de Raadt added a comment -

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

            Show
            salvetore Michael de Raadt added a comment - Thanks for volunteering to have a look at this, Andrew.
            Hide
            dobedobedoh 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
            dobedobedoh 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
            rajeshtaneja 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
            rajeshtaneja 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
            dobedobedoh Andrew Nicols added a comment -

            Thanks for that - Learning more about adminlib!

            I guess this is right now..?

            Show
            dobedobedoh Andrew Nicols added a comment - Thanks for that - Learning more about adminlib! I guess this is right now..?
            Hide
            rajeshtaneja 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
            rajeshtaneja 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
            lazydaisy 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
            lazydaisy 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
            stronk7 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
            stronk7 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
            samhemelryk Sam Hemelryk added a comment -

            Thanks Andrew, changes look good and have been integrated now

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

            Tested in 23 and master, it passes

            Show
            dmonllao David Monllaó added a comment - Tested in 23 and master, it passes
            Hide
            stronk7 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
            stronk7 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:
                  Fix Release Date:
                  11/Mar/13