From 4247a0bd5d949687dc25e02968e3d025a0cc0d42 Mon Sep 17 00:00:00 2001
From: Sunil Mohan Adapa <sunil@medhas.org>
Date: Wed, 11 Mar 2020 15:42:35 -0700
Subject: [PATCH] pagekite: Merge all the configuration retrieval actions

- Merge actions is-disabled, get-frontend, get-kite and get-services into
get-config. This improves the initial startup time for FreedomBox service and
also the page load time for pagekite app. This also significantly simplifies the
code.

- Only use the pagekite service enabled status determine if pagekite is enabled.
Don't use the configuration setting.

- For custom services, provide additional data such as display URL from
get-config action. This removes the need for additional processing
prepare_service_for_display() and template tag create_pagekite_service_url.

- Also reduce the number of times configuration is retrieved to 1 when loading
the app view page and during startup of FreedomBox service.

- Ensure that all keys of the configuration always present and use that to
simplify some code.

- Remove ContextMixin from view DeleteServiceView that does not need it. Use
AppView and drop ContextMixin.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: Veiko Aasa <veiko17@disroot.org>
---
 actions/pagekite                              |  91 +++++++++------
 .../templates/pagekite_configure.html         |  12 +-
 .../pagekite/templatetags/pagekite_extras.py  |  29 -----
 plinth/modules/pagekite/utils.py              | 106 ++----------------
 plinth/modules/pagekite/views.py              |  49 +++-----
 5 files changed, 86 insertions(+), 201 deletions(-)
 delete mode 100644 plinth/modules/pagekite/templatetags/pagekite_extras.py

diff --git a/actions/pagekite b/actions/pagekite
index 310fb94d4..b560062e2 100755
--- a/actions/pagekite
+++ b/actions/pagekite
@@ -5,11 +5,12 @@ Configuration helper for PageKite interface.
 """
 
 import argparse
-import augeas
 import json
 import os
 import sys
 
+import augeas
+
 from plinth import action_utils
 from plinth.modules.pagekite import utils
 
@@ -40,19 +41,16 @@ def parse_arguments():
     subparsers.add_parser('start-and-enable', help='Enable PageKite service')
     subparsers.add_parser('stop-and-disable', help='Disable PageKite service')
     subparsers.add_parser('restart', help='Restart PageKite service')
-    subparsers.add_parser(
-        'is-disabled', help=('Whether PageKite is disabled in the file '
-                             '/etc/pagekite.d/10_accounts.rc'))
+
+    # Configuration
+    subparsers.add_parser('get-config', help='Return current configuration')
 
     # Frontend
-    subparsers.add_parser('get-frontend', help='Get pagekite frontend')
     set_frontend = subparsers.add_parser('set-frontend',
                                          help='Set pagekite frontend')
     set_frontend.add_argument('url', help='frontend url')
 
     # Kite details (name + secret)
-    subparsers.add_parser('get-kite',
-                          help='Get configured kite name and secret')
     set_kite = subparsers.add_parser(
         'set-kite',
         help='Configure kite name and its secret. Secret is read from stdin.')
@@ -60,7 +58,6 @@ def parse_arguments():
                           help='Name of the kite (eg: mybox.pagekite.me)')
 
     # Add/remove pagekite services (service_on entries)
-    subparsers.add_parser('get-services', help='Get list of enabled services')
     add_service = subparsers.add_parser('add-service',
                                         help='Add a pagekite service')
     add_service.add_argument('--service', help='json service dictionary')
@@ -78,13 +75,6 @@ def subcommand_restart(_):
     print('restarted')
 
 
-def subcommand_is_disabled(_):
-    if aug.match(PATHS['abort_not_configured']):
-        print('true')
-    else:
-        print('false')
-
-
 def subcommand_start_and_enable(_):
     aug.remove(PATHS['abort_not_configured'])
     aug.save()
@@ -100,13 +90,58 @@ def subcommand_stop_and_disable(_):
     print('disabled')
 
 
-def subcommand_get_frontend(_):
-    """Get pagekite frontend url"""
+def subcommand_get_config(_):
+    """Print the current configuration as JSON dictionary."""
     if aug.match(PATHS['defaults']):
-        print("pagekite.net")
+        frontend = 'pagekite.net'
     else:
-        url = aug.get(PATHS['frontend'])
-        print(url or '')
+        frontend = aug.get(PATHS['frontend']) or ''
+
+    frontend = frontend.split(':')
+    server_domain = frontend[0]
+    server_port = frontend[1] if len(frontend) >= 2 else '80'
+
+    status = {
+        'is_enabled': action_utils.service_is_enabled('pagekite'),
+        'kite_name': aug.get(PATHS['kitename']),
+        'kite_secret': aug.get(PATHS['kitesecret']),
+        'server_domain': server_domain,
+        'server_port': server_port,
+        'predefined_services': {
+            proto: False
+            for proto in utils.PREDEFINED_SERVICES
+        },
+        'custom_services': [],
+    }
+
+    # 1. predefined_services: {'http': False, 'ssh': True, 'https': True}
+    # 2. custom_services: [{'protocol': 'http', 'secret' 'nono', ..}, [..]}
+    for match in aug.match(PATHS['service_on']):
+        service = dict([(param, aug.get(os.path.join(match, param)))
+                        for param in utils.SERVICE_PARAMS])
+        for name, predefined_service in utils.PREDEFINED_SERVICES.items():
+            if service == predefined_service['params']:
+                status['predefined_services'][name] = True
+                break
+        else:
+            status['custom_services'].append(service)
+            if '/' in service['protocol']:
+                service['protocol'], service['frontend_port'] = service[
+                    'protocol'].split('/')
+
+            service['subdomains'] = service['kitename'].startswith('*.')
+            kite_name = status['kite_name']
+            protocol = service['protocol']
+            if service['subdomains']:
+                kite_name = f'*.{kite_name}'
+
+            url = f'{protocol}://{kite_name}'
+            if 'frontend_port' in service and service['frontend_port']:
+                url = "%s:%s" % (url, service['frontend_port'])
+
+            service['url'] = url
+
+    print(json.dumps(status))
 
 
 def subcommand_set_frontend(arguments):
@@ -131,14 +166,6 @@ def enable_pagekitenet_frontend():
     print("enabled")
 
 
-def subcommand_get_services(arguments):
-    """ lists all available (enabled) services """
-    for match in aug.match(PATHS['service_on']):
-        service = dict([(param, aug.get(os.path.join(match, param)))
-                        for param in utils.SERVICE_PARAMS])
-        print(json.dumps(service))
-
-
 def subcommand_remove_service(arguments):
     """Searches and removes the service(s) that match all given parameters"""
     service = utils.load_service(arguments.service)
@@ -213,14 +240,6 @@ def get_new_service_path(protocol):
     return os.path.join(root, str(new_index))
 
 
-def subcommand_get_kite(_):
-    """Print details of the currently configured kite"""
-    kitename = aug.get(PATHS['kitename'])
-    kitesecret = aug.get(PATHS['kitesecret'])
-    print(kitename or '')
-    print(kitesecret or '')
-
-
 def subcommand_set_kite(arguments):
     """Set details of the kite"""
     aug.set(PATHS['kitename'], arguments.kite_name)
diff --git a/plinth/modules/pagekite/templates/pagekite_configure.html b/plinth/modules/pagekite/templates/pagekite_configure.html
index 4ab42f477..706348a1c 100644
--- a/plinth/modules/pagekite/templates/pagekite_configure.html
+++ b/plinth/modules/pagekite/templates/pagekite_configure.html
@@ -6,7 +6,6 @@
 {% load bootstrap %}
 {% load i18n %}
 {% load static %}
-{% load pagekite_extras %}
 
 {% block page_head %}
   <style type="text/css">
@@ -43,14 +42,13 @@
 
       <div class="list-group">
         {% for service in custom_services %}
-          {% create_pagekite_service_url service kite_name as service_url %}
           <div class="list-group-item clearfix">
             <span class="service">
-              <span title="Connects {{ service_url }} to {{ service.backend_host }}:{{ service.backend_port }}">
-                {% if service_url|slice:":4" == "http" %}
-                  <a href="{{ service_url }}">{{ service_url }}</a>
+              <span title="Connects {{ service.url }} to {{ service.backend_host }}:{{ service.backend_port }}">
+                {% if service.url|slice:":4" == "http" %}
+                  <a href="{{ service.url }}">{{ service.url }}</a>
                 {% else %}
-                  {{ service_url }}
+                  {{ service.url }}
                 {% endif %}
                 <br>
                 {% blocktrans trimmed with backend_host=service.backend_host backend_port=service.backend_port %}
@@ -62,7 +60,7 @@
                   action="{% url 'pagekite:delete-custom-service' %}">
               <div style='display:none'>
                 {% csrf_token %}
-                {{ service.form.as_p }}
+                {{ service.delete_form.as_p }}
               </div>
               <button type="submit" class="btn btn-default"
                       title="{% trans "Delete this service" %}">
diff --git a/plinth/modules/pagekite/templatetags/pagekite_extras.py b/plinth/modules/pagekite/templatetags/pagekite_extras.py
deleted file mode 100644
index a76f47469..000000000
--- a/plinth/modules/pagekite/templatetags/pagekite_extras.py
+++ /dev/null
@@ -1,29 +0,0 @@
-# SPDX-License-Identifier: AGPL-3.0-or-later
-
-from django import template
-from plinth.modules.pagekite import utils
-
-register = template.Library()
-
-
-@register.simple_tag
-def create_pagekite_service_url(service, kite_name):
-    """Create a URL out of a pagekite service
-
-    Parameters: - service: the service params dictionary
-                - kite_name: kite name from the pagekite configuration, not
-                             from the service params
-    """
-    # add extra information if it's missing
-    if 'subdomains' not in service:
-        service = utils.prepare_service_for_display(service)
-
-    urlparams = {'protocol': service['protocol']}
-    if service['subdomains']:
-        urlparams['kite_name'] = "*.%s" % kite_name
-    else:
-        urlparams['kite_name'] = kite_name
-    url = "{protocol}://{kite_name}".format(**urlparams)
-    if 'frontend_port' in service and service['frontend_port']:
-        url = "%s:%s" % (url, service['frontend_port'])
-    return url
diff --git a/plinth/modules/pagekite/utils.py b/plinth/modules/pagekite/utils.py
index 1a7bc77a8..3d24d8b28 100644
--- a/plinth/modules/pagekite/utils.py
+++ b/plinth/modules/pagekite/utils.py
@@ -6,7 +6,7 @@ import os
 
 from django.utils.translation import ugettext_lazy as _
 
-from plinth import action_utils, actions
+from plinth import actions
 from plinth.signals import domain_added, domain_removed
 
 LOGGER = logging.getLogger(__name__)
@@ -76,84 +76,9 @@ PREDEFINED_SERVICES = {
 }
 
 
-def get_kite_details():
-    output = run(['get-kite'])
-    kite_details = output.split()
-    return {'kite_name': kite_details[0], 'kite_secret': kite_details[1]}
-
-
-def get_pagekite_config():
-    """
-    Return the current PageKite configuration by executing various actions.
-    """
-    status = {}
-
-    # PageKite service enabled/disabled
-    # To enable PageKite two things are necessary:
-    # 1) pagekite not being disabled in /etc/pagekite.d/10_account.rc
-    # 2) the pagekite service running
-    is_disabled = (run(['is-disabled']).strip() == 'true')
-    service_running = action_utils.service_is_running('pagekite')
-    status['is_enabled'] = service_running and not is_disabled
-
-    # PageKite kite details
-    status.update(get_kite_details())
-
-    # PageKite frontend server
-    server = run(['get-frontend']).strip()
-
-    # Frontend entries are only considered valid if there's a ':' in
-    # them otherwise, pagekite refuses to work, and we only set values
-    # with ':'.
-    if ':' in server:
-        server_domain, server_port = server.split(':')
-        status['server_domain'] = server_domain
-        status['server_port'] = int(server_port)
-    else:
-        status['server_domain'] = server
-        # No valid entry exists, default to port 80.  Hack: Return
-        # string instead of int to force setting port on save
-        status['server_port'] = '80'
-
-    return status
-
-
-def get_pagekite_services():
-    """Get enabled services. Returns two values:
-
-    1. predefined services: {'http': False, 'ssh': True, 'https': True}
-    2. custom services: [{'protocol': 'http', 'secret' 'nono', ..}, [..]}
-    """
-    custom = []
-    predefined = {}
-    # set all predefined services to 'disabled' by default
-    [predefined.update({proto: False}) for proto in PREDEFINED_SERVICES.keys()]
-    # now, search for the enabled ones
-    for serviceline in run(['get-services']).split('\n'):
-        if not serviceline:  # skip empty lines
-            continue
-
-        service = json.loads(serviceline)
-        for name, predefined_service in PREDEFINED_SERVICES.items():
-            if service == predefined_service['params']:
-                predefined[name] = True
-                break
-        else:
-            custom.append(service)
-    return predefined, custom
-
-
-def prepare_service_for_display(service):
-    """ Add extra information that is used when displaying a service
-
-    - protocol is split into 'protocol' and 'frontend_port'
-    - detect whether 'subdomains' are supported (as boolean)
-    """
-    protocol = service['protocol']
-    if '/' in protocol:
-        service['protocol'], service['frontend_port'] = protocol.split('/')
-    service['subdomains'] = service['kitename'].startswith('*.')
-    return service
+def get_config():
+    """Return the current PageKite configuration."""
+    return json.loads(run(['get-config']))
 
 
 def run(arguments, superuser=True, input=None):
@@ -247,27 +172,18 @@ def update_names_module(initial_registration=False, enabled=None,
     """
     domain_removed.send_robust(sender='pagekite',
                                domain_type='domain-type-pagekite')
-
+    config = get_config()
     if enabled is None:
-        try:
-            enabled = get_pagekite_config()['is_enabled']
-        except IndexError:
-            enabled = False
+        enabled = config.get('is_enabled', False)
 
+    enabled_services = None
+    kite_name = None
     if enabled:
-        # Get enabled services and kite name
-        services = get_pagekite_services()[0]
+        kite_name = config['kite_name']
+        services = config['predefined_services']
         enabled_services = [
-            service for service in services if services[service]
+            service for service, value in services.items() if value
         ]
-        if kite_name is None:
-            try:
-                kite_name = get_kite_details()['kite_name']
-            except IndexError:
-                pass
-    else:
-        enabled_services = None
-        kite_name = None
 
     if initial_registration or (enabled and kite_name):
         domain_added.send_robust(sender='pagekite',
diff --git a/plinth/modules/pagekite/views.py b/plinth/modules/pagekite/views.py
index b155df784..209cf8a11 100644
--- a/plinth/modules/pagekite/views.py
+++ b/plinth/modules/pagekite/views.py
@@ -6,31 +6,14 @@ from django.utils.translation import ugettext as _
 from django.views.generic import View
 from django.views.generic.edit import FormView
 
-from plinth.modules import pagekite
+from plinth.views import AppView
 
 from . import utils
 from .forms import (AddCustomServiceForm, ConfigurationForm,
                     DeleteCustomServiceForm)
 
 
-class ContextMixin(object):
-    """Mixin to add 'subsubmenu' and 'title' to the context.
-
-    Also adds the requirement of all necessary packages to be installed
-    """
-    def get_context_data(self, **kwargs):
-        """Use self.title and the module-level subsubmenu"""
-        context = super(ContextMixin, self).get_context_data(**kwargs)
-        context['app_info'] = pagekite.app.info
-        context['title'] = pagekite.app.info.name
-        context['is_enabled'] = pagekite.app.is_enabled()
-        return context
-
-    def dispatch(self, *args, **kwargs):
-        return super(ContextMixin, self).dispatch(*args, **kwargs)
-
-
-class DeleteServiceView(ContextMixin, View):
+class DeleteServiceView(View):
     def post(self, request):
         form = DeleteCustomServiceForm(request.POST)
         if form.is_valid():
@@ -56,27 +39,25 @@ class AddCustomServiceView(FormView):
         return super().form_valid(form)
 
 
-class ConfigurationView(ContextMixin, FormView):
+class ConfigurationView(AppView):
+    app_id = 'pagekite'
     template_name = 'pagekite_configure.html'
     form_class = ConfigurationForm
     prefix = 'pagekite'
     success_url = reverse_lazy('pagekite:index')
 
-    def get_context_data(self, *args, **kwargs):
-        context = super(ConfigurationView,
-                        self).get_context_data(*args, **kwargs)
-        unused, custom_services = utils.get_pagekite_services()
-        for service in custom_services:
-            service['form'] = AddCustomServiceForm(initial=service)
-        context['custom_services'] = [
-            utils.prepare_service_for_display(service)
-            for service in custom_services
-        ]
-        context.update(utils.get_kite_details())
-        return context
+    def __init__(self, *args, **kwargs):
+        """Load and store the current configuration."""
+        super().__init__(*args, **kwargs)
+        self.config = utils.get_config()
+        self.initial = self.config
 
-    def get_initial(self):
-        return utils.get_pagekite_config()
+    def get_context_data(self, *args, **kwargs):
+        context = super().get_context_data(*args, **kwargs)
+        for service in self.config['custom_services']:
+            service['delete_form'] = DeleteCustomServiceForm(initial=service)
+        context.update(self.config)
+        return context
 
     def form_valid(self, form):
         form.save(self.request)