← Back to team overview

openlp-dev team mailing list archive

Re: [Merge] lp:~openlp-dev/openlp/apifixes into lp:openlp

 

Sure thing about the logging.

I'm not so sure about those "magic" numbers. My goal with the remote was to keep things as simple as possible without many function calls or so. Flask itself uses the numbers directly and doesn't offer names for them.

I personally think that the numbers are probably better, since you either know what the code is for, or you need to read it up anyway. Slapping a name on it just makes you search longer for the number...

And it would also just be more code that needs to be tested and covered...

(replied to the inline-comments at the respective places)

Diff comments:

> 
> === modified file 'openlp/core/api/http/server.py'
> --- openlp/core/api/http/server.py	2018-01-07 04:36:45 +0000
> +++ openlp/core/api/http/server.py	2018-09-03 21:33:52 +0000
> @@ -46,6 +40,9 @@
>  from openlp.core.common.settings import Settings
>  from openlp.core.threading import ThreadWorker, run_thread
>  
> +

Yeah, I didn't run a linter over it yet

> +from openlp.core.api.ng import app as application
> +
>  log = logging.getLogger(__name__)
>  
>  
> 
> === added directory 'openlp/core/api/ng'
> === added file 'openlp/core/api/ng/__init__.py'
> --- openlp/core/api/ng/__init__.py	1970-01-01 00:00:00 +0000
> +++ openlp/core/api/ng/__init__.py	2018-09-03 21:33:52 +0000
> @@ -0,0 +1,16 @@
> +from flask import Flask, jsonify
> +from waitress import serve
> +
> +from openlp.core.api.ng.endpoints.old import old_register_blueprints
> +from openlp.core.api.ng.endpoints.v1 import v1_register_blueprints
> +from openlp.core.api.ng.main import main_views
> +from openlp.core.common.applocation import AppLocation
> +
> +app = Flask(__name__)
> +
> +app.register_blueprint(main_views)
> +old_register_blueprints(app)
> +v1_register_blueprints(app)

I'd much rather start fresh. Especially since the previous "versions" weren't actually versioned. But it's only in the code, so I don't care too much about that

> +
> +def register_blueprint(blueprint, url_prefix=None):
> +    app.register_blueprint(blueprint, url_prefix)
> 
> === added file 'openlp/core/api/ng/lib.py'
> --- openlp/core/api/ng/lib.py	1970-01-01 00:00:00 +0000
> +++ openlp/core/api/ng/lib.py	2018-09-03 21:33:52 +0000
> @@ -0,0 +1,29 @@
> +import json
> +from flask import jsonify
> +from functools import wraps
> +
> +def requires_login(f):
> +    @wraps(f)
> +    def decorated(*args, **kwargs):
> +        return f(*args, **kwargs)
> +    return decorated
> +
> +
> +def login_required(f):
> +    @wraps(f)
> +    def decorated(*args, **kwargs):
> +        return f(*args, **kwargs)

Yes it is. And I remembered, that I need to support two different login mechanisms, since the old api should still use the old auth mechanism. All new calls will use the login_required decorator. The old calls with probably just be the old decorator...

> +    return decorated
> +
> +
> +def old_success_response():
> +    return jsonify({'results': {'sucess': True}})
> +
> +
> +def extract_request(json_string, name):
> +    try:
> +        if json_string:
> +            return json.loads(json_string)['request'][name]
> +    except KeyError:
> +        pass
> +    return None
> \ No newline at end of file
> 
> === added file 'openlp/core/ui/media/remote.py'
> --- openlp/core/ui/media/remote.py	1970-01-01 00:00:00 +0000
> +++ openlp/core/ui/media/remote.py	2018-09-03 21:33:52 +0000
> @@ -0,0 +1,98 @@
> +# -*- coding: utf-8 -*-
> +# vim: autoindent shiftwidth=4 expandtab textwidth=120 tabstop=4 softtabstop=4
> +
> +###############################################################################
> +# OpenLP - Open Source Lyrics Projection                                      #
> +# --------------------------------------------------------------------------- #
> +# Copyright (c) 2008-2018 OpenLP Developers                                   #
> +# --------------------------------------------------------------------------- #
> +# This program is free software; you can redistribute it and/or modify it     #
> +# under the terms of the GNU General Public License as published by the Free  #
> +# Software Foundation; version 2 of the License.                              #
> +#                                                                             #
> +# This program is distributed in the hope that it will be useful, but WITHOUT #
> +# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or       #
> +# FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for    #
> +# more details.                                                               #
> +#                                                                             #
> +# You should have received a copy of the GNU General Public License along     #
> +# with this program; if not, write to the Free Software Foundation, Inc., 59  #
> +# Temple Place, Suite 330, Boston, MA 02111-1307 USA                          #
> +###############################################################################
> +"""
> +The :mod:`~openlp.core.api.endpoint` module contains various API endpoints
> +"""
> +import logging
> +from flask import jsonify, Blueprint
> +
> +from openlp.core.api.ng import app
> +from openlp.core.api.ng.lib import login_required
> +from openlp.core.common.registry import Registry
> +
> +log = logging.getLogger(__name__)
> +
> +v1_media = Blueprint('v1-media-controller', __name__)
> +old_media = Blueprint('old-media-controller', __name__)
> +
> +
> +@v1_media.route('/play')
> +@login_required
> +def media_play(request):
> +    media = Registry().get('media_controller')
> +    live = Registry().get('live_controller')
> +    status = media.media_play(live, False)
> +    if status:
> +        return '', 204
> +    return 'Not sure what to send here...'
> +    return {'success': status}
> +
> +
> +@v1_media.route('/pause')
> +@login_required
> +def media_pause(request):
> +    media = Registry().get('media_controller')
> +    live = Registry().get('live_controller')
> +    status = media.media_pause(live)
> +    if status:
> +        return '', 204
> +    return 'Not sure what to send here...'
> +
> +
> +@v1_media.route('/stop')
> +@login_required
> +def media_stop(request):
> +    Registry().get('live_controller').mediacontroller_live_stop.emit()
> +    return '', 204
> +
> +
> +# -------------- DEPRECATED ------------------------
> +
> +@old_media.route('/play')
> +@login_required
> +def old_media_play(request):
> +    media = Registry().get('media_controller')
> +    live = Registry().get('live_controller')
> +    status = media.media_play(live, False)
> +    return jsonify({'success': status})
> +
> +
> +@old_media.route('/pause')
> +@login_required
> +def old_media_pause(request):
> +    media = Registry().get('media_controller')
> +    live = Registry().get('live_controller')
> +    status = media.media_pause(live)
> +    return jsonify({'success': status})
> +
> +
> +@old_media.route('/stop')
> +@login_required
> +def old_media_stop(request):
> +    Registry().get('live_controller').mediacontroller_live_stop.emit()
> +    return ''
> +
> +# -------------- END OF DEPRECATED ------------------------
> +
> +def register_views():
> +    app.register_blueprint(v1_media, url_prefix='/v1/api/media/')

I didn't have it in at first, but added it at a later stage...
I think it's good, because it separates the html we are serving from the json endpoints. Especially with angular (or any other js framework) which comes with it's own client side routing, clashing urls would be bad.

I already switched the remote to use hash based routing, so client side routing always uses a "#" to keep stuff local, but I think having the "api" there just adds a layer of separation. Also this is something that's not public, that likely won't be used by anyone other than developers.

> +    app.register_blueprint(old_media, url_prefix='/api/media/')
> \ No newline at end of file


-- 
https://code.launchpad.net/~openlp-dev/openlp/apifixes/+merge/354205
Your team OpenLP Development is subscribed to branch lp:~openlp-dev/openlp/apifixes.


References