Samba: UI: Show toggle buttons and share names

Closes #1989

I checked that all Samba tests pass.

Signed-off-by: Veiko Aasa <veiko17@disroot.org>
[sunil: Minor indentation and styling fixes in template]
[sunil: Prevent multiple HTML attributes with same name in case of vfat]
Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
Reviewed-by: Sunil Mohan Adapa <sunil@medhas.org>

plinth/modules/samba/__init__.py

@@ -5,7 +5,6 @@ FreedomBox app to configure samba.
 
 import grp
 import json
-import os
 import pwd
 import socket
 
@@ -151,15 +150,6 @@ def get_shares():
     return json.loads(output)
 
 
-def disk_name(mount_point):
-    """Get a disk name."""
-    share_name = os.path.basename(mount_point)
-    if not share_name:
-        share_name = 'disk'
-
-    return share_name
-
-
 def backup_pre(packet):
     """Save registry share configuration."""
     actions.superuser_run('samba', ['dump-shares'])

plinth/modules/samba/templates/samba.html

@@ -12,11 +12,19 @@
   <style type="text/css">
     .progress {
         margin-bottom: 0;
+        width: 180px;
+    }
+    .samba-disk-name {
+        font-size: 1.3em;
+        float: left;
+    }
+    .samba-disk-shares {
+        padding-top: 10px;
+        margin-bottom: 20px;
     }
   </style>
 {% endblock %}
 
-
 {% block configuration %}
   {{ block.super }}
 
@@ -28,84 +36,112 @@
       not the whole disk.
     {% endblocktrans %}
     </p>
-      <table class="table table-bordered table-condensed table-striped">
-      <thead>
-        <tr>
-          <th>{% trans "Disk Name" %}</th>
-          <th>{% trans "Shares" %}</th>
-          <th>{% trans "Used" %}</th>
-        </tr>
-      </thead>
-      <tbody>
-        {% for disk in disks %}
-        <tr>
-          <td>{{ disk.name|default_if_none:"" }}</td>
-          <td>
-            <form class="form shareform" method="post"
-                  action="{% url 'samba:share' disk.mount_point|urlencode:'' %}">
-            {% csrf_token %}
-            <input type="hidden" name="filesystem_type" value="{{ disk.filesystem_type }}">
 
-            {% for share_type in share_types %}
+    {% for disk in disks %}
-              <button type="submit"
+      <div class="samba-disk-shares">
-                {% if share_type.0 in shared_mounts|lookup:disk.mount_point %}
+        <div class="samba-disk-header">
-                  class="btn btn-success" name="{{ share_type.0 }}_share" value="disable"
+          <div class="samba-disk-name">
-                {% else %}
+            <span class="fa fa-hdd-o"></span>
-                  class="btn btn-default" name="{{ share_type.0 }}_share" value="enable"
+            {{ disk.name }}
-                {% endif %}
+          </div>
-                {% if disk.filesystem_type == 'vfat' %}
+          <div class="pull-right">
-                  title='{% trans "VFAT partitions are not supported" %}' disabled
-                {% endif %}>{{ share_type.1 }}
-              </button>
-            {% endfor %}
-            </form>
-          </td>
-          <td >
             <div class="progress">
-              {% if disk.percent_used < 75 %}
+              <div class="progress-bar progress-bar-striped
-                <div class="progress-bar progress-bar-striped progress-bar-success"
+                {% if disk.percent_used < 75 %}
-              {% elif disk.percent_used < 90 %}
+                  progress-bar-success
-                <div class="progress-bar progress-bar-striped progress-bar-warning"
+                {% elif disk.percent_used < 90 %}
-              {% else %}
+                  progress-bar-warning
-                <div class="progress-bar progress-bar-striped progress-bar-danger"
+                {% else %}
-              {% endif %}
+                  progress-bar-danger
-                     role="progressbar" aria-valuenow="{{ disk.percent_used }}"
+                {% endif %}"
-                     aria-valuemin="0" aria-valuemax="100"
+                role="progressbar" aria-valuenow="{{ disk.percent_used }}"
-                     style="width: {{ disk.percent_used }}%;">
+                aria-valuemin="0" aria-valuemax="100"
-                  {{ disk.percent_used }}%
+                style="width: {{ disk.percent_used }}%;">
-                </div>
+                {{ disk.percent_used }}%
+              </div>
             </div>
             <div>{{ disk.used_str }} / {{ disk.size_str }}</div>
-          </td>
+          </div>
-        </tr>
+        </div>
-        {% endfor %}
-      </tbody>
-      </table>
 
-      <p>
+        <form class="form shareform" method="post"
+              action="{% url 'samba:share' disk.mount_point|urlencode:'' %}">
+          {% csrf_token %}
+          <input type="hidden" name="filesystem_type"
+                 value="{{ disk.filesystem_type }}">
+          <table class="table table-condensed"
+                 aria-describedby="{{ disk.name }}">
+            <thead>
+              <tr>
+                <th scope="col">{% trans 'Type' %}</th>
+                <th scope="col">{% trans 'Name' %}</th>
+                <th scope="col">{% trans 'Status' %}</th>
+              </tr>
+            </thead>
+            <tbody>
+              {% for share_type in share_types %}
+                <tr id="samba-share-{{ disk.share_name_prefix }}-{{ share_type.id }}"
+                    class="share">
+                  <td class="share-type">
+                    {{ share_type.type }}
+                  </td>
+                  <td class="share-name">
+                    {{ disk.share_name_prefix }}{{ share_type.share_name_suffix }}
+                  </td>
+                  <td class="share-status">
+                    <button type="submit"
+                      {% if disk.filesystem_type == 'vfat' %}
+                        class="btn toggle-button"
+                        title='{% trans "VFAT partitions are not supported" %}'
+                        disabled
+                        aria-readonly="true"
+                      {% elif share_type.id in shared_mounts|lookup:disk.mount_point %}
+                        class="btn toggle-button toggle-button--toggled"
+                        name="{{ share_type.id }}_share"
+                        value="disable"
+                        aria-pressed="true"
+                      {% else %}
+                        class="btn toggle-button"
+                        name="{{ share_type.id }}_share"
+                        value="enable"
+                        aria-pressed="false"
+                      {% endif %}>
+                    </button>
+                  </td>
+                </tr>
+              {% endfor %}
+            </tbody>
+          </table>
+        </form>
+      </div>
+    {% endfor %}
+
+    <p>
       {% url 'storage:index' as storage_url %}
       {% url 'users:index' as users_url %}
       {% blocktrans trimmed %}
         You can find additional information about disks on the
         <a href="{{ storage_url }}">storage</a> module page and configure
-        access to the shares on the <a href="{{ users_url }}">users</a> module page.
+        access to the shares on the <a href="{{ users_url }}">users</a> module
-      {% endblocktrans %}</p>
+        page.
+      {% endblocktrans %}
+    </p>
 
-      <p>{% trans "Users who can currently access group and home shares" %}:
+    <p>{% trans "Users who can currently access group and home shares" %}:
-        {{ users.access_ok|join:", " }}</p>
+      {{ users.access_ok|join:", " }}</p>
 
-      {% if users.password_re_enter_needed %}
+    {% if users.password_re_enter_needed %}
-        <p>{% trans "Users needing to re-enter their password on the password change page to access group and home shares" %}:
+      <p>{% trans "Users needing to re-enter their password on the password change page to access group and home shares" %}:
-          <strong>{{ users.password_re_enter_needed|join:", " }}</strong>.</p>
+        <strong>{{ users.password_re_enter_needed|join:", " }}</strong>.</p>
-      {% endif %}
+    {% endif %}
 
     {% if unavailable_shares %}
       <h3>{% trans "Unavailable Shares" %}</h3>
       <p>
-      {% blocktrans trimmed %}
+        {% blocktrans trimmed %}
-        Shares that are configured but the disk is not available. If the disk
+          Shares that are configured but the disk is not available. If the disk
-        is plugged back in, sharing will be automatically enabled.
+          is plugged back in, sharing will be automatically enabled.
-      {% endblocktrans %}
+        {% endblocktrans %}
       </p>
       <table class="table table-bordered table-condensed table-striped">
         <thead>

plinth/modules/samba/tests/test_functional.py

@@ -48,17 +48,16 @@ def samba_share_should_not_be_available(share_type):
 def _set_share(browser, share_type, status='enabled'):
     """Enable or disable samba share."""
     disk_name = 'disk'
-    share_type_name = '{0}_share'.format(share_type)
+    share_row_id = 'samba-share-{0}-{1}'.format(disk_name, share_type)
+
     functional.nav_to_module(browser, 'samba')
-    for elem in browser.find_by_tag('td'):
+    share = browser.find_by_id(share_row_id)
-        if elem.text == disk_name:
+    share_btn = share.find_by_css('.share-status').find_by_tag('button').first
-            share_form = elem.find_by_xpath('(..//*)[2]/form').first
+
-            share_btn = share_form.find_by_name(share_type_name).first
+    if status == 'enabled' and share_btn['value'] == 'enable':
-            if status == 'enabled' and share_btn['value'] == 'enable':
+        share_btn.click()
-                share_btn.click()
+    elif status == 'disabled' and share_btn['value'] == 'disable':
-            elif status == 'disabled' and share_btn['value'] == 'disable':
+        share_btn.click()
-                share_btn.click()
-            break
 
 
 def _write_to_share(share_type, as_guest=False):

plinth/modules/samba/views.py

@@ -4,6 +4,7 @@ Views for samba module.
 """
 
 import logging
+import os
 import urllib.parse
 from collections import defaultdict
 
@@ -20,12 +21,19 @@ logger = logging.getLogger(__name__)
 
 
 def get_share_mounts():
-    """Return list of mount points."""
+    """Return list of shareable mounts."""
-    ignore_points = ('/boot', '/boot/efi', '/boot/firmware', '/.snapshots')
+    ignore_mounts = ('/boot', '/boot/efi', '/boot/firmware', '/.snapshots')
-    return [
+    mounts = []
-        mount for mount in storage.get_mounts()
+
-        if mount['mount_point'] not in ignore_points
+    for mount in storage.get_mounts():
-    ]
+        mount_point = mount['mount_point']
+        if mount_point not in ignore_mounts:
+            basename = os.path.basename(mount_point)
+            mount['name'] = basename or _('FreedomBox OS disk')
+            mount['share_name_prefix'] = basename or 'disk'
+            mounts.append(mount)
+
+    return sorted(mounts, key=lambda k: k['mount_point'])
 
 
 class SambaAppView(views.AppView):
@@ -37,20 +45,27 @@ class SambaAppView(views.AppView):
         """Return template context data."""
         context = super().get_context_data(*args, **kwargs)
         disks = get_share_mounts()
-        shares = samba.get_shares()
-
-        for disk in disks:
-            disk['name'] = samba.disk_name(disk['mount_point'])
         context['disks'] = disks
 
+        shares = samba.get_shares()
         shared_mounts = defaultdict(list)
         for share in shares:
             shared_mounts[share['mount_point']].append(share['share_type'])
         context['shared_mounts'] = shared_mounts
 
-        context['share_types'] = [('open', _('Open Share')),
+        context['share_types'] = [{
-                                  ('group', _('Group Share')),
+            'id': 'open',
-                                  ('home', _('Home Share'))]
+            'type': _('Open Share'),
+            'share_name_suffix': ''
+        }, {
+            'id': 'group',
+            'type': _('Group Share'),
+            'share_name_suffix': '_group'
+        }, {
+            'id': 'home',
+            'type': _('Home Share'),
+            'share_name_suffix': '_home'
+        }]
 
         unavailable_shares = []
         for share in shares: