Oembed fix #853

kak-tus · 2019-02-24T10:48:17Z

Oembed support seems to be broken.
May be in case of running alerta in docker+postgresql. May be entirelly.

As example: /api/oembed endpoint accessible as /api/oembed, but not as /api/oembed.json.
Also: qb.from_params forms incorrect query to postgresql.

I am not a python programmer and don't know flask framework, so I fixed this not ideally.

After this fixes I've got oembed work in grafana

Use usual json (not jsonp request) with Access-Control-Allow-Origin header. Correct path to oembed location.

Removed width/height from parameters. Allow to hide title.

Auto-renew. Ability to open page locally (we heed to set http/https prefixes). Fix loading: we need to set alerta defaults only after script loading.

qb.from_params forms incorrect arguments for query.

Grafana won't load script on "load". So it's better to embed js.

vykulakov · 2019-03-01T07:39:54Z

examples/oembed.html

@@ -1,7 +1,7 @@
 <!DOCTYPE html>
 <html lang="en">
 <head>
-  <link rel="stylesheet" href="//netdna.bootstrapcdn.com/bootstrap/3.0.0/css/bootstrap.min.css" />
+  <link rel="stylesheet" href="http://netdna.bootstrapcdn.com/bootstrap/3.0.0/css/bootstrap.min.css" />


Why have you changed // to http:// here and in other places of code? // looks like a more general approach that will work in a wider range of environments. For instance, I'm trying to access the oembed.html page by HTTPs in my experiments.

To get ability to user open this page locally and test with local instance of alerta.
Without this fix, user opens page with "file:///" protocol and external scripts tries to open with "file:///" protocol too.
If this page is not test page, but it is an example page - I will revert this fixes.

OK, your reasons are clear. But it may not suit the original idea. Maybe @satterly will leave some comments here and explain the original idea. About my comment - feel free to ignore it now.

Yes, please revert this change.

vykulakov · 2019-03-01T07:45:29Z

examples/oembed.html

-<script src="http://api.alerta.io/embed.js"></script>
+<script src="http://ajax.googleapis.com/ajax/libs/jquery/2.1.3/jquery.min.js"></script>
+<script src="http://netdna.bootstrapcdn.com/bootstrap/3.3.2/js/bootstrap.min.js"></script>
+<script src="http://localhost:8080/api/embed.js"></script>


Using the /api path prefix here is not correct because it will work only in your environment. Please, use more general URL like this: //localhost:8080/embed.js

vykulakov · 2019-03-01T07:47:00Z

examples/oembed.html


  // $.alerta.defaults.endpoint = 'http://localhost:8080';
  // $.alerta.defaults.key = 'demo-key';
  // or
  $.alerta.defaults = {
-      endpoint: 'http://api.alerta.io', // oEmbed endpoint becomes http://api.alerta.io/oembed.json
+      endpoint: 'http://localhost:8080/api', // oEmbed endpoint becomes http://localhost:8080/api/oembed


See the comment about /api above.

But leave a comment, how to use it with alerta, running in docker.

satterly · 2019-03-12T09:31:23Z

examples/oembed.html

      key: 'demo-key'
  };

-  $('.mobile-alerts').alerta('http://api.alerta.io/alerts/count?service=Mobile&status=open', {title:'Mobile Service'});
+  function renew() {


Can you call this function refresh() instead of renew()? Thanks.

satterly · 2019-03-12T09:32:22Z

alerta/views/oembed.py

-        width = int(request.args['maxwidth'])
-        height = int(request.args['maxheight'])
-        title = request.args.get('title', 'Alerts')
+        title = request.args['title']


Why did you remove the default title? It will throw an exception here now if someone does not provide title as a URL param.

satterly · 2019-03-12T09:34:47Z

Thanks for the PR. It looks good except for a few minor changes. I'll be happy to merge this once you've addressed the concerns I've raised above. 👍

vykulakov · 2019-03-01T07:55:59Z

examples/oembed-for-grafana.html

+      renew();
+    });
+  });
+</script>


Thank you very much for this example.

Maybe, you can add some comments with instructions on how to use this HTML in Grafana.

vykulakov · 2019-03-01T08:07:06Z

alerta/views/oembed.py

 from . import api

+Query = namedtuple('Query', ['where', 'vars', 'sort', 'group'])

 @api.route('/oembed', defaults={'format': 'json'}, methods=['OPTIONS', 'GET'])
 @api.route('/oembed.<format>', methods=['OPTIONS', 'GET'])


You can remove the /oembed.<format> endpoint as well because there is only one available format - json. We can return it back when we have more than one format.

vykulakov · 2019-03-01T08:09:32Z

alerta/views/oembed.py


 @api.route('/oembed', defaults={'format': 'json'}, methods=['OPTIONS', 'GET'])
 @api.route('/oembed.<format>', methods=['OPTIONS', 'GET'])
 @cross_origin()
 @permission(Scope.read_oembed)
 @jsonp
 def oembed(format):
-
-    if format != 'json':


If you remove this validation, you should remove the /oembed.<format> endpoint. Or if you leave the endpoint in place, you should leave this check in place as well.

vykulakov · 2019-03-01T08:10:53Z

alerta/views/oembed.py

-    if format != 'json':
-        return jsonify(status='error', message='unsupported format: %s' % format), 400
-
-    if 'url' not in request.args or 'maxwidth' not in request.args \


'url' not in request.args is a good validation. Please, don't remove it.

vykulakov · 2019-03-01T08:17:31Z

alerta/views/oembed.py

-    if 'url' not in request.args or 'maxwidth' not in request.args \
-            or 'maxheight' not in request.args:
-        return jsonify(status='error', message='missing default parameters: url, maxwidth, maxheight'), 400
-
    try:
        url = request.args['url']
        title = request.args['title']


The title param is not mandatory now so this code can raise the KeyError: 'title' error. Using .get('title') here solves this problem.

vykulakov · 2019-03-01T08:58:56Z

examples/oembed-for-grafana.html

+<a href="http://localhost:8080/">Alerta</a>
+
+<!-- Content from http://localhost:8080/api/embed.js -->
+<script type="text/javascript">


Loading script should work fine. I use this <script src="/embed.js"></script> and it works.

vykulakov · 2019-03-01T20:35:18Z

examples/oembed.html

@@ -1,7 +1,7 @@
 <!DOCTYPE html>
 <html lang="en">
 <head>
-  <link rel="stylesheet" href="//netdna.bootstrapcdn.com/bootstrap/3.0.0/css/bootstrap.min.css" />
+  <link rel="stylesheet" href="http://netdna.bootstrapcdn.com/bootstrap/3.0.0/css/bootstrap.min.css" />


OK, your reasons are clear. But it may not suit the original idea. Maybe @satterly will leave some comments here and explain the original idea. About my comment - feel free to ignore it now.

vykulakov · 2019-03-12T09:46:13Z

Looks like I've forgotten to submit my review earlier. My mistake, sorry.

* Fix oembed. Use usual json (not jsonp request) with Access-Control-Allow-Origin header. Correct path to oembed location. * Move styling to embedding page from embedded to simplify configure. Removed width/height from parameters. Allow to hide title. * More powerfull oembed. Auto-renew. Ability to open page locally (we heed to set http/https prefixes). Fix loading: we need to set alerta defaults only after script loading. * oembed example for grafana. * Remove unused width/height from js. * Ability to set empty background in case of no alerts. * Oembed work fix. qb.from_params forms incorrect arguments for query. * Grafana load fix. Grafana won't load script on "load". So it's better to embed js. * Revert api path to usual environment prefix. But leave a comment, how to use it with alerta, running in docker.

Andrey Kuzmin added 8 commits February 23, 2019 00:02

Fix oembed.

c52bfc5

Use usual json (not jsonp request) with Access-Control-Allow-Origin header. Correct path to oembed location.

Move styling to embedding page from embedded to simplify configure.

f289dc4

Removed width/height from parameters. Allow to hide title.

More powerfull oembed.

5b8b43b

Auto-renew. Ability to open page locally (we heed to set http/https prefixes). Fix loading: we need to set alerta defaults only after script loading.

oembed example for grafana.

89e4283

Remove unused width/height from js.

84df626

Ability to set empty background in case of no alerts.

e0ed614

Oembed work fix.

3556c96

qb.from_params forms incorrect arguments for query.

Grafana load fix.

2527c3d

Grafana won't load script on "load". So it's better to embed js.

vykulakov reviewed Mar 1, 2019

View reviewed changes

Revert api path to usual environment prefix.

fb413a6

But leave a comment, how to use it with alerta, running in docker.

satterly reviewed Mar 12, 2019

View reviewed changes

vykulakov reviewed Mar 12, 2019

View reviewed changes

satterly merged commit cc8f930 into alerta:master Mar 24, 2019

This was referenced Mar 25, 2019

The /oembed endpoint returns 500 response code #861

Closed

The /oembed.json endpoint returns 500 response code #860

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oembed fix #853

Oembed fix #853

kak-tus commented Feb 24, 2019 •

edited

vykulakov Mar 1, 2019 •

edited

kak-tus Mar 1, 2019

vykulakov Mar 1, 2019

satterly Mar 12, 2019

vykulakov Mar 1, 2019

kak-tus Mar 1, 2019

vykulakov Mar 1, 2019

kak-tus Mar 1, 2019

satterly Mar 12, 2019

satterly Mar 12, 2019

satterly commented Mar 12, 2019

vykulakov Mar 1, 2019

vykulakov Mar 1, 2019

vykulakov Mar 1, 2019

vykulakov Mar 1, 2019

vykulakov Mar 1, 2019

vykulakov Mar 1, 2019

vykulakov Mar 1, 2019

vykulakov commented Mar 12, 2019

Oembed fix #853

Oembed fix #853

Conversation

kak-tus commented Feb 24, 2019 • edited

vykulakov Mar 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satterly commented Mar 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vykulakov commented Mar 12, 2019

kak-tus commented Feb 24, 2019 •

edited

vykulakov Mar 1, 2019 •

edited