Moodle
  1. Moodle
  2. MDL-41644

Backup uses a nonexistent CSS class "notifywarning" twice

    Details

      Description

      backup/import.php
      147: echo $OUTPUT->notification(get_string('warning'), 'notifywarning');

      backup/util/ui/renderer.php
      395: $output .= $this->output->notification($warning, 'notifywarning notifyproblem');

      The above are the only two uses of notifywarning in the whole of Moodle (including in the CSS, so they don't actually do anything visually).

      Presumably both should simply be "notifyproblem".

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Michael de Raadt added a comment -

            Thanks, David.

            Show
            Michael de Raadt added a comment - Thanks, David.
            Show
            Amanpreet Singh added a comment - Diff at https://github.com/apsdehal/moodle/tree/MDL-41644
            Hide
            Mark Nelson added a comment -

            Hey Amanpreet, that patch looks great. Thanks. However, that branch contains a commit for MDL-41646 as well. Are you able to remove that commit and rebase the branch onto master and then backport this to 2.6 and 2.5? I would also suggest changing the commit message to "MDL-41644 Backup: Update CSS class .notifywarning with .notifyproblem". We usually only include the component before the ':'. Thanks.

            Show
            Mark Nelson added a comment - Hey Amanpreet, that patch looks great. Thanks. However, that branch contains a commit for MDL-41646 as well. Are you able to remove that commit and rebase the branch onto master and then backport this to 2.6 and 2.5? I would also suggest changing the commit message to " MDL-41644 Backup: Update CSS class .notifywarning with .notifyproblem". We usually only include the component before the ':'. Thanks.
            Hide
            Mark Nelson added a comment -

            Whoops, also forgot to mention that you will need to include testing instructions. So, basically just write down a list of steps to that will take the user to screens where this change should be present and ask them to inspect the element and ensure the CSS class has been changed from 'notifywarning' to 'notifyproblem'. Thanks!

            Show
            Mark Nelson added a comment - Whoops, also forgot to mention that you will need to include testing instructions. So, basically just write down a list of steps to that will take the user to screens where this change should be present and ask them to inspect the element and ensure the CSS class has been changed from 'notifywarning' to 'notifyproblem'. Thanks!
            Hide
            CiBoT added a comment -

            Results for MDL-41644

            • Remote repository: git://github.com/apsdehal/moodle.git
            Show
            CiBoT added a comment - Results for MDL-41644 Remote repository: git://github.com/apsdehal/moodle.git Remote branch MDL-41644 _25 to be integrated into upstream MOODLE_25_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/949 Error: The MDL-41644 _25 branch at git://github.com/apsdehal/moodle.git is very old (>60 days ago). Please rebase against current MOODLE_25_STABLE. Remote branch MDL-41644 _26 to be integrated into upstream MOODLE_26_STABLE Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/950 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/950/artifact/work/smurf.html Remote branch MDL-41644 to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/951 Details: http://integration.moodle.org/job/Precheck%20remote%20branch/951/artifact/work/smurf.html
            Hide
            Mark Nelson added a comment -

            Hi Amanpreet, thanks a lot. Looks good. I have updated the testing instructions to make it easier for the tester. Also, can you please rebase the 2.5 branch onto the latest Moodle 2.5 branch? (Notice if you view the diff it is quite a large difference). Once this has been done I will push for integration. Cheers!

            Show
            Mark Nelson added a comment - Hi Amanpreet, thanks a lot. Looks good. I have updated the testing instructions to make it easier for the tester. Also, can you please rebase the 2.5 branch onto the latest Moodle 2.5 branch? (Notice if you view the diff it is quite a large difference). Once this has been done I will push for integration. Cheers!
            Hide
            Amanpreet Singh added a comment -

            Hi Mark, rebase has been done. Thanks for reviewing

            Show
            Amanpreet Singh added a comment - Hi Mark, rebase has been done. Thanks for reviewing
            Hide
            Mark Nelson added a comment -

            Thanks Aman, pushing for integration.

            Show
            Mark Nelson added a comment - Thanks Aman, pushing for integration.
            Hide
            CiBoT added a comment -

            Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

            Show
            CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
            Hide
            Sam Hemelryk added a comment -

            Hi Aparup Banerjee, any chance you can please grep contrib themes and see if any of them include styles for ".notifywarning"?

            If none are found this is fine to land as is, otherwise we will need to come up with a combo solution for the stable branches so as to not cause regressions.

            Show
            Sam Hemelryk added a comment - Hi Aparup Banerjee , any chance you can please grep contrib themes and see if any of them include styles for ".notifywarning"? If none are found this is fine to land as is, otherwise we will need to come up with a combo solution for the stable branches so as to not cause regressions.
            Hide
            Sam Hemelryk added a comment -

            I had a quick check of a handful of contrib themes and could not find anyone who had styled those classes, I'm happy with the thought that the chance of regression here is minimal and as such this has been integrated now. Thanks!

            Show
            Sam Hemelryk added a comment - I had a quick check of a handful of contrib themes and could not find anyone who had styled those classes, I'm happy with the thought that the chance of regression here is minimal and as such this has been integrated now. Thanks!
            Hide
            Sam Hemelryk added a comment -

            Tested and passed during integration review.

            Show
            Sam Hemelryk added a comment - Tested and passed during integration review.
            Hide
            Aparup Banerjee added a comment -

            confirmed - none found.

            Show
            Aparup Banerjee added a comment - confirmed - none found.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            Fetch your remotes, prune them,
            clean your integrated branches and say "Ahem".

            Rebase your ongoing stuff, keep conflicts away
            don't forget to test the code and we'll love you again.

            Thanks, closing!

            Show
            Eloy Lafuente (stronk7) added a comment - Fetch your remotes, prune them, clean your integrated branches and say "Ahem". Rebase your ongoing stuff, keep conflicts away don't forget to test the code and we'll love you again. Thanks, closing!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: