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
2016-06-21 11:42:54 -05:00 · 2016-06-21 11:42:54 -05:00 · 13095eb550
parent 40e1787948
commit 13095eb550
15 changed files with 105 additions and 19 deletions
--- a/caravel/assets/javascripts/common.js
+++ b/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);
+    });
+});
--- a/caravel/assets/javascripts/dashboard.jsx
+++ b/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);
          }
--- a/caravel/assets/javascripts/modules/utils.js
+++ b/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
 };
--- a/caravel/assets/webpack.config.js
+++ b/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,
--- a/caravel/templates/appbuilder/baselayout.html
+++ b/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 %}
--- a/caravel/templates/appbuilder/general/widgets/list.html
+++ b/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 %}
--- a/caravel/templates/caravel/base.html
+++ b/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 %}
--- a/caravel/templates/caravel/basic.html
+++ b/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>
--- a/caravel/templates/caravel/dashboard.html
+++ b/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">
--- a/caravel/templates/caravel/flash_wrapper.html
+++ b/caravel/templates/caravel/flash_wrapper.html
@ -0,0 +1,3 @@
+<div id="alert-container">
+    {% include 'appbuilder/flash.html' %}
+</div>
--- a/caravel/templates/caravel/models/database/macros.html
+++ b/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;
    });
--- a/caravel/templates/caravel/sql.html
+++ b/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">
--- a/caravel/templates/caravel/welcome.html
+++ b/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>
--- a/caravel/views.py
+++ b/caravel/views.py
@ -12,6 +12,7 @@ import time
 import traceback
 from datetime import datetime

+import functools
 import pandas as pd
 import sqlalchemy as sqla

@ -20,7 +21,7 @@ from flask import (
 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 @@ config = app.config
 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 @@ class Caravel(BaseView):
            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 @@ class Caravel(BaseView):
            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 @@ class Caravel(BaseView):
        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 @@ class Caravel(BaseView):
        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 @@ class Caravel(BaseView):

    @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,
--- a/tests/core_tests.py
+++ b/tests/core_tests.py
@ -258,7 +258,7 @@ class CoreTests(CaravelTestCase):

        self.logout()
        self.assertRaises(
-            utils.CaravelSecurityException, self.test_save_dash, 'alpha')
+            AssertionError, self.test_save_dash, 'alpha')

        alpha = appbuilder.sm.find_user('alpha')