Options improvements (#159)

* improvements to company options

* tweak CompanyOptionsModel

* allow tc domain

* correct thumb var names

* fix image saving

* add support for tiff images

* fix contractor sorting

Author: samuelcolvin

tcsocket/Dockerfile

@@ -1,6 +1,6 @@
 FROM python:3.6-alpine
 
-RUN apk --update --no-cache add gcc g++ musl-dev zlib-dev libuv libffi-dev make postgresql-dev jpeg-dev \
+RUN apk --update --no-cache add gcc g++ musl-dev zlib-dev libuv libffi-dev make postgresql-dev jpeg-dev tiff-dev \
  && rm -rf /var/cache/apk/*
 
 ADD ./requirements.txt /home/root/requirements.txt

tcsocket/app/middleware.py

@@ -118,7 +118,7 @@ async def pg_conn_middleware(request, handler):
 
 
 def domain_allowed(allow_domains, current_domain):
-    return any(
+    return current_domain.endswith('tutorcruncher.com') or any(
         allow_domain == current_domain or (allow_domain.startswith('*') and current_domain.endswith(allow_domain[1:]))
         for allow_domain in allow_domains
     )

tcsocket/app/validation.py

@@ -31,10 +31,17 @@ class RouterMode(str, Enum):
     history = 'history'
 
 
+@unique
+class SortOn(str, Enum):
+    name = 'name'
+    review_rating = 'review_rating'
+    last_updated = 'last_updated'
+
+
 class CompanyCreateModal(BaseModel):
     name: constr(min_length=3, max_length=63)
+    domains: Optional[List[constr(max_length=255)]] = []
     name_display: NameOptions = NameOptions.first_name_initial
-    url: NoneStr = None
     public_key: constr(min_length=18, max_length=20) = None
     private_key: constr(min_length=20, max_length=50) = None
 
@@ -53,13 +60,35 @@ class CompanyUpdateModel(BaseModel):
     private_key: constr(min_length=20, max_length=50) = None
 
     domains: Optional[List[constr(max_length=255)]] = 'UNCHANGED'
-    name_display: NameOptions = None
 
+    name_display: NameOptions = None
     show_stars: bool = None
     display_mode: DisplayMode = None
     router_mode: RouterMode = None
     show_hours_reviewed: bool = None
     show_labels: bool = None
+    show_location_search: bool = None
+    show_subject_filter: bool = None
+    sort_on: SortOn = None
+    pagination: int = None
+
+
+class CompanyOptionsModel(BaseModel):
+    """
+    Used for options views, this is the definitive set of defaults for company options
+    """
+    name: str
+    name_display: NameOptions
+
+    show_stars: bool = True
+    display_mode: DisplayMode = DisplayMode.grid
+    router_mode: RouterMode = RouterMode.hash
+    show_hours_reviewed: bool = True
+    show_labels: bool = True
+    show_location_search: bool = True
+    show_subject_filter: bool = True
+    sort_on: SortOn = SortOn.name
+    pagination: int = 100
 
 
 class ContractorModel(BaseModel):

tcsocket/app/views.py

@@ -17,14 +17,13 @@
 from sqlalchemy.dialects.postgresql import insert as pg_insert
 from sqlalchemy.sql import and_, distinct, or_
 from sqlalchemy.sql.functions import count as count_func
-from yarl import URL
 
 from .geo import geocode, get_ip
 from .models import (Action, NameOptions, sa_companies, sa_con_skills, sa_contractors, sa_labels, sa_qual_levels,
                      sa_subjects)
 from .processing import contractor_set as _contractor_set
 from .utils import HTTPBadRequestJson, json_response
-from .validation import CompanyCreateModal, CompanyUpdateModel, ContractorModel, DisplayMode, RouterMode
+from .validation import CompanyCreateModal, CompanyOptionsModel, CompanyUpdateModel, ContractorModel
 
 EXTRA_ATTR_TYPES = 'checkbox', 'text_short', 'text_extended', 'integer', 'stars', 'dropdown', 'datetime', 'date'
 MISSING = object()
@@ -58,8 +57,7 @@ async def company_create(request):
     """
     company: CompanyCreateModal = request['model']
     existing_company = bool(company.private_key)
-    data = company.dict(exclude={'url'})
-    data['domains'] = company.url and [URL(company.url).host]  # TODO here for backwards compatibility, to be removed
+    data = company.dict()
 
     conn = await request['conn_manager'].get_connection()
     v = await conn.execute((
@@ -91,6 +89,12 @@ async def company_create(request):
         )
 
 
+OPTIONS_FIELDS = {
+    'show_stars', 'display_mode', 'router_mode', 'show_hours_reviewed', 'show_labels', 'show_location_search',
+    'show_subject_filter', 'sort_on', 'pagination'
+}
+
+
 async def company_update(request):
     """
     Modify a company.
@@ -101,7 +105,7 @@ async def company_update(request):
     if company.domains != 'UNCHANGED':
         data['domains'] = company.domains
 
-    options = company.dict(include={'show_stars', 'display_mode', 'router_mode', 'show_hours_reviewed', 'show_labels'})
+    options = company.dict(include=OPTIONS_FIELDS)
     options = {k: v for k, v in options.items() if v is not None}
     if options:
         data['options'] = options
@@ -148,17 +152,12 @@ async def company_options(request):
     """
     Get a companies options
     """
-    options = request['company'].options or {}
-    return json_response(
-        request,
+    opts = CompanyOptionsModel(
         name=request['company'].name,
-        name_display=request['company'].name_display or NameOptions.first_name_initial,
-        show_stars=options.get('show_stars') or False,
-        display_mode=options.get('display_mode') or DisplayMode.grid,
-        router_mode=options.get('router_mode') or RouterMode.hash,
-        show_hours_reviewed=options.get('show_hours_reviewed') or False,
-        show_labels=options.get('show_labels') or False,
+        name_display=request['company'].name_display,
+        **(request['company'].options or {})
     )
+    return json_response(request, **opts.dict())
 
 
 async def contractor_set(request):
@@ -200,12 +199,10 @@ async def clear_enquiry(request):
     )
 
 
-DISTANCE_SORT = '__distance__'
 SORT_OPTIONS = {
-    'update': sa_contractors.c.last_updated,
+    'last_updated': sa_contractors.c.last_updated,
+    'review_rating': sa_contractors.c.review_rating,
     'name': sa_contractors.c.first_name,
-    'distance': DISTANCE_SORT,
-    # TODO some configurable sort index
 }
 
 
@@ -247,7 +244,7 @@ def _get_arg(request, field, *, decoder: Callable[[str], Any]=int, default: Any=
 
 async def contractor_list(request):  # noqa: C901 (ignore complexity)
     sort_val = request.GET.get('sort')
-    sort_col = SORT_OPTIONS.get(sort_val, SORT_OPTIONS['update'])
+    sort_col = SORT_OPTIONS.get(sort_val, SORT_OPTIONS['last_updated'])
     page = _get_arg(request, 'page', default=1)
     pagination = min(_get_arg(request, 'pagination', default=100), 100)
     offset = (page - 1) * pagination
@@ -302,17 +299,12 @@ async def contractor_list(request):  # noqa: C901 (ignore complexity)
         distance_func = func.earth_distance(request_loc, con_loc)
         where += distance_func < max_distance,
         fields += distance_func.label('distance'),
-        if not sort_val:
-            sort_col = DISTANCE_SORT
-        if sort_col == DISTANCE_SORT:
-            sort_col = distance_func
-    elif sort_col == DISTANCE_SORT:
-        raise HTTPBadRequestJson(
-            status='invalid_argument',
-            details=f'distance sorting not available if latitude and longitude are not provided',
-        )
+        sort_col = distance_func
+
+    sort_on = sort_col.asc()
+    if sort_col in {sa_contractors.c.last_updated, sa_contractors.c.review_rating}:
+        sort_on = sort_col.desc()
 
-    sort_on = sort_col.desc() if sort_col == sa_contractors.c.last_updated else sort_col.asc()
     q_iter = (
         select(fields)
         .where(and_(*where))

tcsocket/app/worker.py

@@ -71,13 +71,12 @@ async def get_image(self, company, contractor_id, url):
                     f.write(chunk)
             f.seek(0)
             with Image.open(f) as img:
-                if img.mode == 'RGBA':
-                    img = img.convert('RGB')
-                img_thumb = ImageOps.fit(img, SIZE_LARGE, Image.LANCZOS)
-                img_thumb.save(path_str + '.jpg', 'JPEG')
+                img = img.convert('RGB')
+                img_large = ImageOps.fit(img, SIZE_LARGE, Image.LANCZOS)
+                img_large.save(path_str + '.jpg', 'JPEG')
 
-                img_large = ImageOps.fit(img, SIZE_SMALL, Image.LANCZOS)
-                img_large.save(path_str + '.thumb.jpg', 'JPEG')
+                img_thumb = ImageOps.fit(img, SIZE_SMALL, Image.LANCZOS)
+                img_thumb.save(path_str + '.thumb.jpg', 'JPEG')
         return 200
 
     def request_headers(self, company):

tests/conftest.py

@@ -10,7 +10,7 @@
 import pytest
 from aiohttp.web import Application, Response, json_response
 from aiopg.sa import create_engine as aio_create_engine
-from PIL import Image
+from PIL import Image, ImageDraw
 from sqlalchemy import create_engine as sa_create_engine
 from sqlalchemy import select
 from sqlalchemy.sql.functions import count as count_func
@@ -25,11 +25,21 @@
 
 
 async def test_image_view(request):
-    image = Image.new('RGB', (2000, 1200), (50, 100, 150))
+    image_format = request.GET.get('format')
     stream = BytesIO()
-    image.save(stream, format='JPEG', optimize=True)
-    request.app['request_log'].append('test_image')
-    return Response(body=stream.getvalue(), content_type='image/jpeg')
+    request.app['request_log'].append(('test_image', image_format))
+
+    if image_format == 'RGBA':
+        create_as, save_as = 'RGBA', 'PNG'
+    elif image_format == 'P':
+        create_as, save_as = 'RGBA', 'GIF'
+    else:
+        create_as, save_as = 'RGB', 'JPEG'
+
+    image = Image.new(create_as, (2000, 1200), (50, 100, 150))
+    ImageDraw.Draw(image).line((0, 0) + image.size, fill=128)
+    image.save(stream, format=save_as, optimize=True)
+    return Response(body=stream.getvalue(), content_type=f'image/{save_as.lower()}')
 
 
 async def contractor_list_view(request):

tests/test_public_filtering_views.py

@@ -171,7 +171,6 @@ async def test_qual_level_list(cli, db_conn, company):
 
 @pytest.mark.parametrize('params, con_distances', [
     ({'location': 'SW1W 0EN'}, [('1-bcon-t', 3129), ('2-acon-t', 10054)]),
-    ({'location': 'SW1W 0EN', 'sort': 'name'}, [('2-acon-t', 10054), ('1-bcon-t', 3129)]),
     ({'location': 'SW1W 0EN', 'max_distance': 4000}, [('1-bcon-t', 3129)]),
     ({'location': 'SW1W 0ENx', 'max_distance': 4000}, [('2-acon-t', None), ('1-bcon-t', None)]),
 ])
@@ -193,13 +192,6 @@ async def test_distance_filter(cli, db_conn, company, params, con_distances):
     obj = await r.json()
     assert list(map(itemgetter('link', 'distance'), obj['results'])) == con_distances
 
-    r = await cli.get(url, params={'sort': 'distance'})
-    assert r.status == 400, await r.text()
-    assert (await r.json()) == {
-        'details': 'distance sorting not available if latitude and longitude are not provided',
-        'status': 'invalid_argument'
-    }
-
 
 async def test_geocode_cache(cli, other_server, company):
     url = str(cli.server.app.router['contractor-list'].url_for(company=company.public_key))

tests/test_set_company.py

@@ -39,7 +39,7 @@ async def test_create(cli, db_conn):
 async def test_create_with_url_public_key(cli, db_conn):
     payload = json.dumps({
         'name': 'foobar',
-        'url': 'https://www.example.com',
+        'domains': ['www.example.com'],
         'public_key': 'X' * 20,
         '_request_time': int(time()),
     })
@@ -212,6 +212,24 @@ async def test_list_invalid_time(cli, company, payload_func, name):
     assert r.status == 403, await r.text()
 
 
+async def test_default_options(cli, db_conn, company):
+    r = await cli.get(f'/{company.public_key}/options')
+    assert r.status == 200, await r.text()
+    assert {
+        'display_mode': 'grid',
+        'name': 'foobar',
+        'name_display': 'first_name_initial',
+        'pagination': 100,
+        'router_mode': 'hash',
+        'show_hours_reviewed': True,
+        'show_labels': True,
+        'show_location_search': True,
+        'show_stars': True,
+        'show_subject_filter': True,
+        'sort_on': 'name',
+    } == await r.json()
+
+
 async def test_update_company(cli, db_conn, company, other_server):
     curr = await db_conn.execute(sa_companies.select())
     result = await curr.first()
@@ -224,14 +242,21 @@ async def test_update_company(cli, db_conn, company, other_server):
         signing_key_='this is the master key',
         domains=['changed.com'],
         display_mode='enquiry-modal',
-        show_hours_reviewed=True,
+        show_location_search=False,
+        pagination=20,
+        sort_on='review_rating',
     )
     assert r.status == 200, await r.text()
     response_data = await r.json()
     assert response_data == {
         'details': {
             'domains': ['changed.com'],
-            'options': {'display_mode': 'enquiry-modal', 'show_hours_reviewed': True},
+            'options': {
+                'display_mode': 'enquiry-modal',
+                'show_location_search': False,
+                'pagination': 20,
+                'sort_on': 'review_rating',
+            }
         },
         'company_domains': ['changed.com'],
         'status': 'success',
@@ -241,18 +266,23 @@ async def test_update_company(cli, db_conn, company, other_server):
     curr = await db_conn.execute(sa_companies.select())
     result = await curr.first()
     assert result.domains == ['changed.com']
-    assert result.options == {'display_mode': 'enquiry-modal', 'show_hours_reviewed': True}
+    assert result.options == {'display_mode': 'enquiry-modal', 'pagination': 20, 'show_location_search': False,
+                              'sort_on': 'review_rating'}
 
     r = await cli.get(f'/{company.public_key}/options')
     assert r.status == 200, await r.text()
     assert {
         'display_mode': 'enquiry-modal',
         'name': 'foobar',
         'name_display': 'first_name_initial',
+        'pagination': 20,
         'router_mode': 'hash',
         'show_hours_reviewed': True,
-        'show_labels': False,
-        'show_stars': False,
+        'show_labels': True,
+        'show_location_search': False,
+        'show_stars': True,
+        'show_subject_filter': True,
+        'sort_on': 'review_rating',
     } == await r.json()
 
 

tests/test_set_contractor.py

@@ -4,6 +4,7 @@
 from pathlib import Path
 from time import time
 
+import pytest
 from PIL import Image
 
 from tcsocket.app.models import sa_con_skills, sa_contractors, sa_labels, sa_qual_levels, sa_subjects
@@ -278,15 +279,18 @@ async def test_extra_attributes_null(cli, db_conn, company):
     assert result.primary_description is None
 
 
-async def test_photo(cli, db_conn, company, image_download_url, tmpdir):
+@pytest.mark.parametrize('image_format', ['JPEG', 'RGBA', 'P'])
+async def test_photo(cli, db_conn, company, image_download_url, tmpdir, other_server, image_format):
     r = await signed_post(
         cli,
         f'/{company.public_key}/webhook/contractor',
         id=123,
         first_name='Fred',
-        photo=image_download_url
+        photo=f'{image_download_url}?format={image_format}'
     )
     assert r.status == 201, await r.text()
+    assert other_server.app['request_log'] == [('test_image', image_format)]
+
     assert [cs.first_name async for cs in await db_conn.execute(sa_contractors.select())] == ['Fred']
     path = Path(tmpdir / 'media' / company.public_key / '123.jpg')
     assert path.exists()