Show right messages as soon as possible (#632)

* Flashed messaged should be flushed in every page

* Show error messages in AJAX style

* Introduce the decorator "api"

* Move toggleCheckbox() to the right place and trigger it in jQuery style

caravel/assets/javascripts/common.js

@@ -0,0 +1,11 @@
+var $ = require('jquery');
+var utils = require('./modules/utils');
+
+$(document).ready(function () {
+    $(':checkbox[data-checkbox-api-prefix]').change(function () {
+        var $this = $(this);
+        var prefix = $this.data('checkbox-api-prefix');
+        var id = $this.attr('id');
+        utils.toggleCheckbox(prefix, "#" + id);
+    });
+});

caravel/assets/javascripts/dashboard.jsx

@@ -365,9 +365,12 @@ var Dashboard = function (dashboardData) {
             });
           },
           error: function (error) {
+            var respJSON = error.responseJSON;
+            var errorMsg = (respJSON && respJSON.message) ? respJSON.message :
+                error.responseText;
             showModal({
               title: "Error",
-              body: "Sorry, there was an error saving this dashboard:<br />" + error
+              body: "Sorry, there was an error saving this dashboard:<br />" + errorMsg
             });
             console.warn("Save dashboard error", error);
           }

caravel/assets/javascripts/modules/utils.js

@@ -74,7 +74,30 @@ function showModal(options) {
   $(options.modalSelector).modal("show");
 }
 
+var showApiMessage = function (resp) {
+  var template = '<div class="alert"> ' +
+      '<button type="button" class="close" ' +
+      'data-dismiss="alert">×</button> </div>';
+
+  var severity = resp.severity || 'info';
+  $(template)
+      .addClass('alert-' + severity)
+      .append(resp.message)
+      .appendTo($('#alert-container'));
+};
+
+var toggleCheckbox = function (apiUrlPrefix, selector) {
+  var apiUrl = apiUrlPrefix + $(selector)[0].checked;
+  $.get(apiUrl).fail(function (xhr, textStatus, errorThrown) {
+    var resp = xhr.responseJSON;
+    if (resp && resp.message) {
+      showApiMessage(resp);
+    }
+  });
+};
+
 module.exports = {
   wrapSvgText: wrapSvgText,
-  showModal: showModal
+  showModal: showModal,
+  toggleCheckbox: toggleCheckbox
 };

caravel/assets/webpack.config.js

@@ -10,7 +10,8 @@ var config = {
     explore: APP_DIR + '/javascripts/explore.js',
     welcome: APP_DIR + '/javascripts/welcome.js',
     sql: APP_DIR + '/javascripts/sql.js',
-    standalone: APP_DIR + '/javascripts/standalone.js'
+    standalone: APP_DIR + '/javascripts/standalone.js',
+    common: APP_DIR + '/javascripts/common.js'
   },
   output: {
     path: BUILD_DIR,

caravel/templates/appbuilder/baselayout.html

@@ -16,7 +16,7 @@
     <div class="container">
       <div class="row">
           {% block messages %}
-            {% include 'appbuilder/flash.html' %}
+            {% include 'caravel/flash_wrapper.html' %}
           {% endblock %}
           {% block content %}
           {% endblock %}

caravel/templates/appbuilder/general/widgets/list.html

@@ -64,7 +64,7 @@
                           {{'checked' if item[value] }}
                           name="{{ '{}__{}'.format(pk, value) }}"
                           id="{{ '{}__{}'.format(pk, value) }}"
-                          onchange="$.get('/caravel/checkbox/{{ modelview_name }}/{{ pk }}/{{ value }}/' + $('#{{ '{}__{}'.format(pk, value) }}')[0].checked ) + '/';">
+                          data-checkbox-api-prefix="/caravel/checkbox/{{ modelview_name }}/{{ pk }}/{{ value }}/">
                     {% else %}
                       {{ item[value]|safe }}
                     {% endif %}

caravel/templates/caravel/base.html

@@ -10,3 +10,8 @@
     {{super()}}
     <script src="/static/assets/javascripts/dist/css-theme.entry.js"></script>
   {% endblock %}
+
+  {% block tail_js %}
+      {{super()}}
+      <script src="/static/assets/javascripts/dist/common.entry.js"></script>
+  {% endblock %}

caravel/templates/caravel/basic.html

@@ -27,6 +27,7 @@
     {% endblock %}
 
     {% block body %}
+      {% include 'caravel/flash_wrapper.html' %}
       <div id="app">
         Oops! React.js is not working properly.
       </div>

caravel/templates/caravel/dashboard.html

@@ -8,6 +8,7 @@
 {% block body %}
 
 <div class="dashboard container-fluid" data-dashboard="{{ dashboard.json_data }}" data-css="{{ dashboard.css }}">
+  {% include 'caravel/flash_wrapper.html' %}
 
 <!-- Modal -->
 <div class="modal fade" id="css_modal" tabindex="-1" role="dialog">

caravel/templates/caravel/flash_wrapper.html

@@ -0,0 +1,3 @@
+<div id="alert-container">
+    {% include 'appbuilder/flash.html' %}
+</div>

caravel/templates/caravel/models/database/macros.html

@@ -32,7 +32,12 @@
             $('#' + id).click(function(){window.location = '/tablemodelview/add';})
           });
       }).fail(function(error) {
-          alert("ERROR: " + error.responseText);
+          var respJSON = error.responseJSON;
+          var errorMsg = error.responseText;
+          if (respJSON && respJSON.message) {
+              errorMsg = respJSON.message;
+          }
+          alert("ERROR: " + errorMsg);
       });
       return false;
     });

caravel/templates/caravel/sql.html

@@ -3,6 +3,7 @@
 
 {% block body %}
 <div class="container-fluid">
+  {% include 'caravel/flash_wrapper.html' %}
   <div class="sqlcontent" style="display: none;">
     <h3>db: [{{ db }}]</h3>
     <div class="row interactions">

caravel/templates/caravel/welcome.html

@@ -9,6 +9,7 @@
 
 {% block body %}
 <div class="container welcome">
+  {% include 'caravel/flash_wrapper.html' %}
   <div class="header">
     <h3>{{ _("Welcome!") }}</h3>
   </div>

caravel/views.py

@@ -12,6 +12,7 @@
 import traceback
 from datetime import datetime
 
+import functools
 import pandas as pd
 import sqlalchemy as sqla
 
@@ -20,7 +21,7 @@
 from flask_appbuilder import ModelView, CompactCRUDMixin, BaseView, expose
 from flask_appbuilder.actions import action
 from flask_appbuilder.models.sqla.interface import SQLAInterface
-from flask_appbuilder.security.decorators import has_access
+from flask_appbuilder.security.decorators import has_access, has_access_api
 from flask_babelpkg import gettext as __
 from flask_babelpkg import lazy_gettext as _
 from flask_appbuilder.models.sqla.filters import BaseFilter
@@ -37,6 +38,38 @@
 log_this = models.Log.log_this
 
 
+def get_error_msg():
+    if config.get("SHOW_STACKTRACE"):
+        error_msg = traceback.format_exc()
+    else:
+        error_msg = "FATAL ERROR \n"
+        error_msg += (
+            "Stacktrace is hidden. Change the SHOW_STACKTRACE "
+            "configuration setting to enable it")
+    return error_msg
+
+
+def api(f):
+    """
+    A decorator to label an endpoint as an API. Catches uncaught exceptions and
+    return the response in the JSON format
+    """
+    def wraps(self, *args, **kwargs):
+        try:
+            return f(self, *args, **kwargs)
+        except Exception as e:
+            logging.exception(e)
+            resp = Response(
+                json.dumps({
+                    'message': get_error_msg()
+                }),
+                status=500,
+                mimetype="application/json")
+            return resp
+
+    return functools.update_wrapper(wraps, f)
+
+
 def check_ownership(obj, raise_if_false=True):
     """Meant to be used in `pre_update` hooks on models to enforce ownership
 
@@ -861,7 +894,8 @@ def overwrite_slice(self, slc):
             msg = "Slice [{}] has been overwritten".format(slc.slice_name)
             flash(msg, "info")
 
-    @has_access
+    @api
+    @has_access_api
     @expose("/checkbox/<model_view>/<id_>/<attr>/<value>", methods=['GET'])
     def checkbox(self, model_view, id_, attr, value):
         """endpoint for checking/unchecking any boolean in a sqla model"""
@@ -875,7 +909,8 @@ def checkbox(self, model_view, id_, attr, value):
             db.session.commit()
         return Response("OK", mimetype="application/json")
 
-    @has_access
+    @api
+    @has_access_api
     @expose("/activity_per_day")
     def activity_per_day(self):
         """endpoint to power the calendar heatmap on the welcome page"""
@@ -891,7 +926,8 @@ def activity_per_day(self):
         payload = {str(time.mktime(dt.timetuple())): ccount for dt, ccount in qry if dt}
         return Response(json.dumps(payload), mimetype="application/json")
 
-    @has_access
+    @api
+    @has_access_api
     @expose("/save_dash/<dashboard_id>/", methods=['GET', 'POST'])
     def save_dash(self, dashboard_id):
         """Save a dashboard's metadata"""
@@ -915,7 +951,8 @@ def save_dash(self, dashboard_id):
         session.close()
         return "SUCCESS"
 
-    @has_access
+    @api
+    @has_access_api
     @expose("/testconn", methods=["POST", "GET"])
     def testconn(self):
         """Tests a sqla connection"""
@@ -1121,13 +1158,7 @@ def refresh_datasources(self):
 
     @app.errorhandler(500)
     def show_traceback(self):
-        if config.get("SHOW_STACKTRACE"):
-            error_msg = traceback.format_exc()
-        else:
-            error_msg = "FATAL ERROR\n"
-            error_msg = (
-                "Stacktrace is hidden. Change the SHOW_STACKTRACE "
-                "configuration setting to enable it")
+        error_msg = get_error_msg()
         return render_template(
             'caravel/traceback.html',
             error_msg=error_msg,

tests/core_tests.py

@@ -258,7 +258,7 @@ def test_only_owners_can_save(self):
 
         self.logout()
         self.assertRaises(
-            utils.CaravelSecurityException, self.test_save_dash, 'alpha')
+            AssertionError, self.test_save_dash, 'alpha')
 
         alpha = appbuilder.sm.find_user('alpha')