From e717e4ebc26ad8e07ad501b8e9e6ee29cb6250e5 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Thu, 14 Jul 2022 17:20:26 +0200 Subject: [wip] implement multi track download --- fietsboek/routes.py | 2 ++ fietsboek/templates/browse.jinja2 | 1 + fietsboek/views/browse.py | 51 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 54 insertions(+) diff --git a/fietsboek/routes.py b/fietsboek/routes.py index dad8026..9b17173 100644 --- a/fietsboek/routes.py +++ b/fietsboek/routes.py @@ -7,6 +7,8 @@ def includeme(config): config.add_route('logout', '/logout') config.add_route('browse', '/track/') + config.add_route('track-archive', '/track/archive') + config.add_route('password-reset', '/password-reset') config.add_route('use-token', '/token/{uuid}') config.add_route('create-account', '/create-account') diff --git a/fietsboek/templates/browse.jinja2 b/fietsboek/templates/browse.jinja2 index 10b16e3..1f61bc6 100644 --- a/fietsboek/templates/browse.jinja2 +++ b/fietsboek/templates/browse.jinja2 @@ -5,6 +5,7 @@ {% for track in tracks %}
+ {{ track.title | default(track.date, true) }} {% if track.text_tags() %} {% for tag in track.tags %}{{ tag.tag }} {% endfor %} diff --git a/fietsboek/views/browse.py b/fietsboek/views/browse.py index 3739061..24cf082 100644 --- a/fietsboek/views/browse.py +++ b/fietsboek/views/browse.py @@ -1,11 +1,28 @@ """Views for browsing all tracks.""" +from zipfile import ZipFile + from pyramid.view import view_config +from pyramid.httpexceptions import HTTPForbidden +from pyramid.response import Response from sqlalchemy import select from .. import models, util +class Stream: + def __init__(self): + self.buffer = [] + + def write(self, b): + self.buffer.append(b) + + def readall(self): + buf = self.buffer + self.buffer = [] + return b"".join(buf) + + def visible_tracks(dbsession, user): """Returns all visible tracks for the given user. @@ -43,3 +60,37 @@ def browse(request): 'tracks': tracks, 'mps_to_kph': util.mps_to_kph, } + + +@view_config(route_name="track-archive", request_method="GET") +def archive(request): + """Packs multiple tracks into a single archive. + + :param request: The Pyramid request. + :type request: pyramid.request.Request + :return: The HTTP response. + :rtype: pyramid.response.Response + """ + from pprint import pformat + + track_ids = set(map(int, request.params.getall("track_id[]"))) + tracks = request.dbsession.execute( + select(models.Track).filter(models.Track.id.in_(track_ids))).scalars().fetchall() + + for track in tracks: + if not track.is_visible_to(request.identity): + return HTTPForbidden() + + def generate(): + stream = Stream() + with ZipFile(stream, "w") as zipfile: + for track in tracks: + zipfile.writestr(f"track_{track.id}.gpx", track.gpx_data) + yield sream.readall() + yield stream.readall() + + return Response( + app_iter=generate(), + content_type="application/zip", + content_disposition="attachment; filename=tracks.zip", + ) -- cgit v1.2.3 From b9bbeafafb1f0750e4adaf12462b86094a5cad25 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Sat, 16 Jul 2022 23:54:06 +0200 Subject: actually implement the multi download button --- fietsboek/static/fietsboek.js | 20 ++++++++++++++++++++ fietsboek/templates/browse.jinja2 | 6 ++++-- fietsboek/views/browse.py | 33 +++++++++++++++++++++++---------- 3 files changed, 47 insertions(+), 12 deletions(-) diff --git a/fietsboek/static/fietsboek.js b/fietsboek/static/fietsboek.js index dd5b9bf..d27e21e 100644 --- a/fietsboek/static/fietsboek.js +++ b/fietsboek/static/fietsboek.js @@ -130,4 +130,24 @@ document.addEventListener('DOMContentLoaded', function(event) { }, false) }) + /* Enable the "Download archive" button */ + var button = $("#archiveDownloadButton"); + if (button) { + button.addEventListener('click', () => { + let checked = document.querySelectorAll(".archive-checkbox:checked"); + let url = new URL("/track/archive", window.location); + checked.forEach((c) => { + url.searchParams.append("track_id[]", c.value); + }); + window.location.assign(url); + }); + } + + /* Enable checkbox listeners */ + document.querySelectorAll(".archive-checkbox").forEach((c) => { + c.addEventListener("change", () => { + let checked = document.querySelectorAll(".archive-checkbox:checked"); + $("#archiveDownloadButton").disabled = (checked.length == 0); + }); + }); }); diff --git a/fietsboek/templates/browse.jinja2 b/fietsboek/templates/browse.jinja2 index 1f61bc6..2732984 100644 --- a/fietsboek/templates/browse.jinja2 +++ b/fietsboek/templates/browse.jinja2 @@ -2,10 +2,11 @@ {% block content %}

{{ _("page.browse.title") }}

+ {% if tracks %} {% for track in tracks %}
- + {{ track.title | default(track.date, true) }} {% if track.text_tags() %} {% for tag in track.tags %}{{ tag.tag }} {% endfor %} @@ -56,7 +57,8 @@
{% endfor %} - {% if not tracks %} + + {% else %}

{{ _("page.browse.no_tracks") }}

{% endif %}
diff --git a/fietsboek/views/browse.py b/fietsboek/views/browse.py index 24cf082..490c0cf 100644 --- a/fietsboek/views/browse.py +++ b/fietsboek/views/browse.py @@ -1,5 +1,6 @@ """Views for browsing all tracks.""" -from zipfile import ZipFile +from io import RawIOBase +from zipfile import ZipFile, ZIP_DEFLATED from pyramid.view import view_config from pyramid.httpexceptions import HTTPForbidden @@ -10,12 +11,20 @@ from sqlalchemy import select from .. import models, util -class Stream: +class Stream(RawIOBase): + """A :class:`Stream` represents an in-memory buffered FIFO. + + This is useful for the zipfile module, as it needs a file-like object, but + we do not want to create an actual temporary file. + """ + def __init__(self): + super().__init__() self.buffer = [] def write(self, b): self.buffer.append(b) + return len(b) def readall(self): buf = self.buffer @@ -71,10 +80,11 @@ def archive(request): :return: The HTTP response. :rtype: pyramid.response.Response """ - from pprint import pformat + # We need to create a separate session, otherwise we will get detached instances + session = request.registry['dbsession_factory']() track_ids = set(map(int, request.params.getall("track_id[]"))) - tracks = request.dbsession.execute( + tracks = session.execute( select(models.Track).filter(models.Track.id.in_(track_ids))).scalars().fetchall() for track in tracks: @@ -82,12 +92,15 @@ def archive(request): return HTTPForbidden() def generate(): - stream = Stream() - with ZipFile(stream, "w") as zipfile: - for track in tracks: - zipfile.writestr(f"track_{track.id}.gpx", track.gpx_data) - yield sream.readall() - yield stream.readall() + try: + stream = Stream() + with ZipFile(stream, "w", ZIP_DEFLATED) as zipfile: + for track in tracks: + zipfile.writestr(f"track_{track.id}.gpx", track.gpx_data) + yield stream.readall() + yield stream.readall() + finally: + session.close() return Response( app_iter=generate(), -- cgit v1.2.3 From 7e86285887866ed10bc9868a0f9aa3b5e5094aba Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Sun, 17 Jul 2022 20:54:28 +0200 Subject: track: don't fail when id is not set This is just debug logging information, but accessing self.id for an object that is not in the database raises an error, because self.id is None and %d cannot format None. In this case, we simply use -1 as id instead. --- fietsboek/models/track.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fietsboek/models/track.py b/fietsboek/models/track.py index 58e9745..1b1c38a 100644 --- a/fietsboek/models/track.py +++ b/fietsboek/models/track.py @@ -275,7 +275,7 @@ class Track(Base): def date(self, value): if value.tzinfo is None: LOGGER.debug('Non-aware datetime passed (track_id=%d, value=%s), assuming offset=0', - self.id, value) + self.id or -1, value) self.date_tz = 0 else: self.date_tz = value.tzinfo.utcoffset(value).total_seconds() // 60 @@ -576,7 +576,7 @@ class TrackCache(Base): def start_time(self, value): if value.tzinfo is None: LOGGER.debug('Non-aware datetime passed (cache_id=%d, value=%s), assuming offset=0', - self.id, value) + self.id or -1, value) self.start_time_tz = 0 else: self.start_time_tz = value.tzinfo.utcoffset(value).total_seconds() // 60 @@ -599,7 +599,7 @@ class TrackCache(Base): def end_time(self, value): if value.tzinfo is None: LOGGER.debug('Non-aware datetime passed (cache_id=%d, value=%s), assuming offset=0', - self.id, value) + self.id or -1, value) self.end_time_tz = 0 else: self.end_time_tz = value.tzinfo.utcoffset(value).total_seconds() // 60 -- cgit v1.2.3 From d8ba9371656f9bd1492b9fbe13234f216ebbd240 Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Sun, 17 Jul 2022 20:56:03 +0200 Subject: make sure all tracks are found in the archive --- fietsboek/views/browse.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/fietsboek/views/browse.py b/fietsboek/views/browse.py index 490c0cf..17dd45a 100644 --- a/fietsboek/views/browse.py +++ b/fietsboek/views/browse.py @@ -3,7 +3,7 @@ from io import RawIOBase from zipfile import ZipFile, ZIP_DEFLATED from pyramid.view import view_config -from pyramid.httpexceptions import HTTPForbidden +from pyramid.httpexceptions import HTTPForbidden, HTTPNotFound from pyramid.response import Response from sqlalchemy import select @@ -87,6 +87,9 @@ def archive(request): tracks = session.execute( select(models.Track).filter(models.Track.id.in_(track_ids))).scalars().fetchall() + if len(tracks) != len(track_ids): + return HTTPNotFound() + for track in tracks: if not track.is_visible_to(request.identity): return HTTPForbidden() -- cgit v1.2.3 From 69c9167911a8449fef20951abe924a1c4528545c Mon Sep 17 00:00:00 2001 From: Daniel Schadt Date: Sun, 17 Jul 2022 20:56:38 +0200 Subject: add tests for archive download --- pytest.ini | 3 +- tests/conftest.py | 22 ++++++++++ tests/integration/test_browse.py | 91 ++++++++++++++++++++++++++++++++++++++++ tests/integration/test_upload.py | 27 +----------- tests/testutils.py | 20 +++++++++ tests/unit/test_util.py | 8 +--- tests/unit/views/test_browse.py | 14 +++++++ 7 files changed, 152 insertions(+), 33 deletions(-) create mode 100644 tests/integration/test_browse.py create mode 100644 tests/testutils.py create mode 100644 tests/unit/views/test_browse.py diff --git a/pytest.ini b/pytest.ini index 1fffd6f..f0d41e6 100644 --- a/pytest.ini +++ b/pytest.ini @@ -1,5 +1,6 @@ [pytest] -addopts = --strict-markers +addopts = --strict-markers --import-mode=importlib +pythonpath = tests testpaths = fietsboek diff --git a/tests/conftest.py b/tests/conftest.py index b8e3090..79f0245 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -141,3 +141,25 @@ def route_path(app_request): def get_route_path(*args, **kwargs): return app_request.route_path(*args, **kwargs) return get_route_path + + +@pytest.fixture() +def logged_in(dbsession, testapp, route_path): + """ + A fixture that represents a logged in state. + + This automatically creates a user and returns the created user. + + Returns the user that was logged in. + """ + user = models.User(email='foo@bar.com', is_verified=True) + user.set_password("foobar") + dbsession.add(user) + + login = testapp.get(route_path('login')) + form = login.form + form['email'] = 'foo@bar.com' + form['password'] = 'foobar' + response = form.submit() + assert response.status_code == 302 + return user diff --git a/tests/integration/test_browse.py b/tests/integration/test_browse.py new file mode 100644 index 0000000..cfd1d71 --- /dev/null +++ b/tests/integration/test_browse.py @@ -0,0 +1,91 @@ +import io +import zipfile +from contextlib import contextmanager +from datetime import datetime + +from sqlalchemy import select, func +from webtest import Upload + +from testutils import load_gpx_asset +from fietsboek import models +from fietsboek.models.track import Visibility + + +@contextmanager +def added_tracks(tm, dbsession, owner): + """Adds some tracks to the database session. + + This function should be used as a context manager and it ensures that the + added tracks are deleted again after the test, to make a clean slate for + the next test. + """ + # The normal transaction is "doomed", so we need to abort it, start a fresh + # one, and then explicitely commit it, otherwise we will not persist the + # objects to the database. + tm.abort() + + tracks = [] + with tm: + track = models.Track( + owner=owner, + title="Foobar", + visibility=Visibility.PUBLIC, + description="A foo'd track", + badges=[], + link_secret="foobar", + tagged_people=[], + ) + track.date = datetime(2022, 3, 14, 9, 26, 54) + track.gpx_data = load_gpx_asset("MyTourbook_1.gpx.gz") + dbsession.add(track) + tracks.append(track) + + track = models.Track( + owner=owner, + title="Barfoo", + visibility=Visibility.PUBLIC, + description="A bar'd track", + badges=[], + link_secret="barfoo", + tagged_people=[], + ) + track.date = datetime(2022, 10, 29, 13, 37, 11) + track.gpx_data = load_gpx_asset("Teasi_1.gpx.gz") + dbsession.add(track) + tracks.append(track) + + tm.begin() + tm.doom() + + try: + yield tracks + finally: + tm.abort() + with tm: + for track in tracks: + dbsession.delete(track) + tm.begin() + tm.doom() + + +def test_browse(testapp, dbsession, route_path, logged_in, tm): + # Ensure there are some tracks in the database + with added_tracks(tm, dbsession, logged_in): + # Now go to the browse page + browse = testapp.get(route_path('browse')) + + assert "Foobar" in browse.text + assert "Barfoo" in browse.text + + +def test_archive(testapp, dbsession, route_path, logged_in, tm): + with added_tracks(tm, dbsession, logged_in): + archive = testapp.get( + route_path('track-archive', _query=[("track_id[]", "1"), ("track_id[]", "2")]) + ) + result = io.BytesIO(archive.body) + + with zipfile.ZipFile(result, 'r') as zipped: + assert len(zipped.namelist()) == 2 + assert "track_1.gpx" in zipped.namelist() + assert "track_2.gpx" in zipped.namelist() diff --git a/tests/integration/test_upload.py b/tests/integration/test_upload.py index 651aedf..1903cbe 100644 --- a/tests/integration/test_upload.py +++ b/tests/integration/test_upload.py @@ -1,34 +1,9 @@ -import gzip -from pathlib import Path - -import pytest from sqlalchemy import select, func from webtest import Upload +from testutils import load_gpx_asset from fietsboek import models - -@pytest.fixture() -def logged_in(dbsession, testapp, route_path): - user = models.User(email='foo@bar.com', is_verified=True) - user.set_password("foobar") - dbsession.add(user) - - login = testapp.get(route_path('login')) - form = login.form - form['email'] = 'foo@bar.com' - form['password'] = 'foobar' - response = form.submit() - assert response.status_code == 302 - - -def load_gpx_asset(filename): - asset_dir = Path(__file__).parent.parent / 'assets' - test_file = asset_dir / filename - with gzip.open(test_file, 'rb') as fobj: - return fobj.read() - - def test_upload_forbidden(testapp, route_path): upload_form = testapp.get(route_path('upload'), status="4*") diff --git a/tests/testutils.py b/tests/testutils.py new file mode 100644 index 0000000..3ddbdbe --- /dev/null +++ b/tests/testutils.py @@ -0,0 +1,20 @@ +"""Various utility functions for testing.""" +import gzip +from pathlib import Path + + +def load_gpx_asset(filename): + """Load a GPX test asset. + + This looks in the tests/assets/ folder, reads and unzips the file and + returns its contents. + + :param filename: Name of the asset to load. + :type filename: str + :return: The content of the asset as bytes. + :rtype: bytes + """ + asset_dir = Path(__file__).parent / 'assets' + test_file = asset_dir / filename + with gzip.open(test_file, 'rb') as fobj: + return fobj.read() diff --git a/tests/unit/test_util.py b/tests/unit/test_util.py index 8f45611..13f4bfe 100644 --- a/tests/unit/test_util.py +++ b/tests/unit/test_util.py @@ -1,11 +1,10 @@ -import gzip from datetime import timedelta -from pathlib import Path import pytest import gpxpy from markupsafe import Markup +from testutils import load_gpx_asset from fietsboek import util @@ -60,10 +59,7 @@ def test_round_timedelta_to_multiple(delta, multiple, expected): ("MyTourbook_1.gpx.gz", timedelta(hours=2)), ]) def test_guess_gpx_timezone(gpx_file, offset): - asset_dir = Path(__file__).parent.parent / 'assets' - test_file = asset_dir / gpx_file - with gzip.open(test_file, 'rb') as fobj: - parsed_gpx = gpxpy.parse(fobj) + parsed_gpx = gpxpy.parse(load_gpx_asset(gpx_file)) timezone = util.guess_gpx_timezone(parsed_gpx) # Here we hope (and assume) that utcoffset is ignored. This is true for # datetime.timezone objects, but may not be for other datetime.tzinfo diff --git a/tests/unit/views/test_browse.py b/tests/unit/views/test_browse.py new file mode 100644 index 0000000..93eb0ae --- /dev/null +++ b/tests/unit/views/test_browse.py @@ -0,0 +1,14 @@ +from fietsboek.views.browse import Stream + + +class TestStream: + def test_write(self): + stream = Stream() + n = stream.write(b"foobar") + assert n == 6 + + def test_write_read(self): + stream = Stream() + stream.write(b"foo") + stream.write(b"bar") + assert stream.readall() == b"foobar" -- cgit v1.2.3