Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-41608

Add mustache-powered renderable for notifications

    XMLWordPrintable

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.5.1, 2.9
    • Fix Version/s: 2.9
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide

      Test on all supported browsers. There's no markup or CSS change occurring here, so this really shouldn't need cross-browser testing.

      Initial setup:

      1. Create a course; no need to enrol any users in it.

      On Base and Bootstrapbase:

      1. Log in to your Moodle install.
      2. Put this file: https://gist.github.com/jethac/fdffa8374e67e63a0ef5 in your Moodle directory and access it from your browser.
        • Verify that each of the notifications looks as it should (basic use, additional classes)
        • Verify that the notifications being created with additional classes retain their additional classes.
      3. Trigger a plugin upgrade (e.g. bump the version number in a theme's version.php upwards by 1, then check Site administration -> Notifications).
        • Verify that the notifications displayed to you during the plugin upgrade process appear as they should.
      Show
      Test on all supported browsers. There's no markup or CSS change occurring here, so this really shouldn't need cross-browser testing. Initial setup: Create a course; no need to enrol any users in it. On Base and Bootstrapbase: Log in to your Moodle install. Put this file: https://gist.github.com/jethac/fdffa8374e67e63a0ef5 in your Moodle directory and access it from your browser. Verify that each of the notifications looks as it should (basic use, additional classes) Verify that the notifications being created with additional classes retain their additional classes. Trigger a plugin upgrade (e.g. bump the version number in a theme's version.php upwards by 1, then check Site administration -> Notifications). Verify that the notifications displayed to you during the plugin upgrade process appear as they should.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_29_STABLE
    • Fixed Branches:
      MOODLE_29_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-41608-master
    • Story Points:
      5

      Description

      Grrrr took me 5 minutes to track this down, theme_bootstrapbase_core_renderer::notification doesn't apply any custom classes. It looks for just 4 classes and disregards the rest.
      Also it expects that $classes is a string which is not the case, it is valid to pass and array to classes as well.

          /*
           * This renders a notification message.
           * Uses bootstrap compatible html.
           */
          public function notification($message, $classes = 'notifyproblem') {
              $message = clean_text($message);
              $type = '';
       
              if ($classes == 'notifyproblem') {
                  $type = 'alert alert-error';
              }
              if ($classes == 'notifysuccess') {
                  $type = 'alert alert-success';
              }
              if ($classes == 'notifymessage') {
                  $type = 'alert alert-info';
              }
              if ($classes == 'redirectmessage') {
                  $type = 'alert alert-block alert-info';
              }
              return "<div class=\"$type\">$message</div>";
          }
      

      Edit 09/Aug/2014
      Summarizing this issue:

      • Sam initially discovered the issue on 05/Sep/13, wherein theme_bootstrapbase_core_renderer::notification doesn't apply any custom classes.
      • Discussion took place and David sketched out a new notification API where rather than calling notification with one or more classes, notifications should be emitted by using one of alert_info, alert_success, alert_warning, alert_danger depending on severity level.
      • Jason reviewed David's changes.
      • Jetha discovered Sam's initial issue in 2014 when trying to add support for detailed notifications (e.g. CSV import stack traces), and a discussion at HQ took place - the conclusion of which is that while deprecating notification and its use in core is the right thing to do, the surface area to amend is too large to do in one issue.

      This issue contained three things:

      • Fixing the behaviour Sam initially identified, wherein theme_bootstrapbase_core_renderer::notification doesn't apply any custom classes
        • This is achieved by removing theme_bootstrapbase_core_renderer::notification entirely such that core_renderer::notification can handle things with the help of a new array in theme_bootstrapbase_core_renderer containing mappings between Moodle and Bootstrap CSS classes.
      • Integrating David's new notification API, but not yet deprecating notification (which will be done in a separate issue, probably after the Gradebook 2014 push).
      • Adding support for an optional details parameter on both old and new notification APIs for more detailed messages.

      Edit 18/Mar/2015
      This issue is now simply about:

      • adding support for adding additional classes to a notification beyond simply notifyxxx
      • creating renderables
      • rendering notifications with Mustache, see MDL-49152

        Attachments

          Issue Links

            Activity

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved:
                  Fix Release Date:
                  11/May/15