MDL-49609 mod_lti: Fixes for problems found in testing

* Load contentitem_return doc before processing * Also use new AMD modal instead of the YUI one. * Remove conversion of text columns in union queries - Since the value column in lti_types_config was changed to a text type, there is no need to use $DB->sql_compare_text() for the lti_types columns involved in the union queries in lti_get_type_config().
safatshahin · Sep 29, 2016 · 6c43d83 · 6c43d83
1 parent d231f3c
commit 6c43d83
Show file tree

Hide file tree

Showing 6 changed files with 53 additions and 43 deletions.
diff --git a/mod/lti/amd/build/contentitem.min.js b/mod/lti/amd/build/contentitem.min.js
diff --git a/mod/lti/amd/build/contentitem_return.min.js b/mod/lti/amd/build/contentitem_return.min.js
diff --git a/mod/lti/amd/src/contentitem.js b/mod/lti/amd/src/contentitem.js
@@ -26,8 +26,17 @@
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  * @since      3.2
  */
-define(['jquery', 'core/notification', 'core/str', 'core/templates', 'mod_lti/form-field', 'core/yui'],
-    function($, notification, str, templates, FormField) {
+define(
+    [
+        'jquery',
+        'core/notification',
+        'core/str',
+        'core/templates',
+        'mod_lti/form-field',
+        'core/modal_factory',
+        'core/modal_events'
+    ],
+    function($, notification, str, templates, FormField, ModalFactory, ModalEvents) {
         var dialogue;
         var contentItem = {
             /**
@@ -44,35 +53,35 @@ define(['jquery', 'core/notification', 'core/str', 'core/templates', 'mod_lti/fo
                         url: url,
                         postData: postData
                     };
-                    return templates.render('mod_lti/contentitem', context);
 
-                }).then(function(html, js) {
-                    // Set dialog's body content.
-                    dialogue = new M.core.dialogue({
-                        modal: true,
-                        headerContent: dialogueTitle,
-                        bodyContent: html,
-                        draggable: true,
-                        width: '800px',
-                        height: '600px'
-                    });
+                    var body = templates.render('mod_lti/contentitem', context);
+                    if (dialogue) {
+                        // Set dialogue body.
+                        dialogue.setBody(body);
+                        // Display the dialogue.
+                        dialogue.show();
+                    } else {
+                        ModalFactory.create({
+                            title: dialogueTitle,
+                            body: body,
+                            large: true
+                        }).done(function(modal) {
+                            dialogue = modal;
 
-                    // Show dialog.
-                    dialogue.show();
+                            // Display the dialogue.
+                            dialogue.show();
 
-                    // Destroy after hiding.
-                    dialogue.after('visibleChange', function(e) {
-                        // Going from visible to hidden.
-                        if (e.prevVal && !e.newVal) {
-                            this.destroy();
-                            // Fetch notifications.
-                            notification.fetchNotifications();
-                        }
-                    }, dialogue);
+                            // On hide handler.
+                            modal.getRoot().on(ModalEvents.hidden, function () {
+                                // Empty modal contents when it's hidden.
+                                modal.setBody('');
 
-                    templates.runTemplateJS(js);
-
-                }).fail(notification.exception);
+                                // Fetch notifications.
+                                notification.fetchNotifications();
+                            });
+                        });
+                    }
+                });
             }
         };
 

diff --git a/mod/lti/amd/src/contentitem_return.js b/mod/lti/amd/src/contentitem_return.js
@@ -23,18 +23,21 @@
  * @license    http://www.gnu.org/copyleft/gpl.html GNU GPL v3 or later
  * @since      3.2
  */
-define([], function() {
+define(['jquery'], function($) {
     return {
         /**
          * Init function.
          *
          * @param {string} returnData The returned data.
          */
         init: function(returnData) {
-            if (window != top) {
-                // Send return data to be processed by the parent window.
-                parent.processContentItemReturnData(returnData);
-            }
+            // Make sure the window has loaded before we perform processing.
+            $(window).ready(function() {
+                if (window != top) {
+                    // Send return data to be processed by the parent window.
+                    parent.processContentItemReturnData(returnData);
+                }
+            });
         }
     };
 });
diff --git a/mod/lti/locallib.php b/mod/lti/locallib.php
@@ -1344,15 +1344,15 @@ function lti_get_type_config($typeid) {
                 FROM {lti_types_config}
                WHERE typeid = :typeid1
            UNION ALL
-              SELECT 'toolurl' AS name, " . $DB->sql_compare_text('baseurl', 1333) . " AS value
+              SELECT 'toolurl' AS name, baseurl AS value
                 FROM {lti_types}
                WHERE id = :typeid2
            UNION ALL
-              SELECT 'icon' AS name, " . $DB->sql_compare_text('icon', 1333) . " AS value
+              SELECT 'icon' AS name, icon AS value
                 FROM {lti_types}
                WHERE id = :typeid3
            UNION ALL
-              SELECT 'secureicon' AS name, " . $DB->sql_compare_text('secureicon', 1333) . " AS value
+              SELECT 'secureicon' AS name, secureicon AS value
                 FROM {lti_types}
                WHERE id = :typeid4";
 

diff --git a/mod/lti/templates/contentitem.mustache b/mod/lti/templates/contentitem.mustache
@@ -76,13 +76,11 @@
 
             // Adjust iframe's width to the fit the container's width.
             var containerWidth = $('div.contentitem-container').width();
-            $('#contentitem-page-iframe').attr('width', containerWidth);
+            $('#contentitem-page-iframe').width(containerWidth + 'px');
 
-            var dialogueContentDiv = $('div.contentitem-container').parent();
-            var dialogueContainer = dialogueContentDiv.parent();
-            // Adjust iframe's height to container's height - 55px (dialogue title bar + top/bottom margins).
-            var containerHeight = dialogueContainer.height() - 55;
-            $('#contentitem-page-iframe').attr('height', containerHeight);
+            // Adjust iframe's height to 75% of the width.
+            var containerHeight = containerWidth * 0.75;
+            $('#contentitem-page-iframe').height(containerHeight + 'px');
         });
     });
 {{/js}}