DATA-012. delete_user supprimait les lignes Contract et laissait les PDF. Des contrats nominatifs signes restaient donc sur le serveur apres la suppression du compte, sans plus aucune reference en base : invisibles pour l application, ingerables par elle, et toujours des donnees personnelles. Les chemins sont lus **avant** que les lignes partent — apres, plus rien ne dit ou sont les fichiers — et les fichiers sont retires **apres** le commit. L ordre compte dans ce sens et pas dans l autre : un echec entre les deux doit laisser un fichier sans ligne, ce qui est recuperable et correspond exactement a l etat precedent, plutot qu une ligne sans fichier, qui est un telechargement en 500 pour toujours. Un fichier deja absent est journalise en info et ignore ; un fichier impossible a retirer est journalise en erreur avec ce que ca implique — il devient orphelin, donc plus rien dans l application ne proposera jamais de le supprimer. Rien ici ne peut faire echouer la suppression du compte : le compte est la partie que quelqu un a demandee. Le nombre de fichiers retires part dans le journal d authentification, a cote de account.deleted. 554 tests.
226 lines
8.2 KiB
Python
226 lines
8.2 KiB
Python
"""Where uploaded documents live (OPS-011).
|
|
|
|
Contract paths were absolute and built from `os.getcwd()`, so the storage
|
|
root followed whatever directory the process was started from. That is a
|
|
defect on its own — a server restarted from elsewhere writes new contracts
|
|
into a new tree and cannot read the old ones, while the database goes on
|
|
saying they are there — and it is what made a release-directory deployment
|
|
impossible: every stored path would point inside a release about to be
|
|
replaced.
|
|
|
|
The compatibility case is the one that matters most here. Rows written
|
|
before this change hold absolute paths, and they have to keep working
|
|
without a data migration, because the migration tooling does not exist yet
|
|
(DB-002, blocked).
|
|
"""
|
|
|
|
import os
|
|
|
|
import pytest
|
|
|
|
from app.extensions import db
|
|
from app.storage import CONTRACTS_DIR, document_path, documents_root
|
|
|
|
|
|
class TestDocumentsRoot:
|
|
def test_the_environment_wins(self, tmp_path, monkeypatch):
|
|
monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path / 'elsewhere'))
|
|
|
|
assert documents_root() == str(tmp_path / 'elsewhere')
|
|
|
|
def test_a_relative_override_is_made_absolute(self, monkeypatch):
|
|
monkeypatch.setenv('DOCUMENTS_ROOT', 'docs-here')
|
|
|
|
assert os.path.isabs(documents_root())
|
|
|
|
def test_the_default_does_not_follow_the_working_directory(self, tmp_path, monkeypatch):
|
|
"""The whole point. `os.getcwd()` made this move; the package
|
|
location does not."""
|
|
monkeypatch.delenv('DOCUMENTS_ROOT', raising=False)
|
|
before = documents_root()
|
|
|
|
monkeypatch.chdir(tmp_path)
|
|
after = documents_root()
|
|
|
|
assert before == after
|
|
|
|
def test_the_default_sits_beside_the_package(self, monkeypatch):
|
|
monkeypatch.delenv('DOCUMENTS_ROOT', raising=False)
|
|
|
|
# From the module file, not from `app.__file__`: `app` has no
|
|
# __init__.py, so it is a namespace package and __file__ is None.
|
|
from app import storage
|
|
|
|
package_dir = os.path.dirname(os.path.abspath(storage.__file__))
|
|
expected = os.path.join(os.path.dirname(package_dir), 'documents')
|
|
assert documents_root() == expected
|
|
|
|
|
|
class TestDocumentPath:
|
|
def test_a_relative_path_is_resolved_against_the_root(self, tmp_path, monkeypatch):
|
|
monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path))
|
|
|
|
resolved = document_path(os.path.join(CONTRACTS_DIR, 'abc.pdf'))
|
|
|
|
assert resolved == str(tmp_path / CONTRACTS_DIR / 'abc.pdf')
|
|
|
|
def test_an_absolute_path_is_left_alone(self, tmp_path, monkeypatch):
|
|
"""Rows written before this module existed. They must keep resolving
|
|
to where the file actually is, or every contract uploaded so far
|
|
becomes a 500 on download the day this ships."""
|
|
monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path / 'new-root'))
|
|
legacy = os.path.abspath(os.path.join('C:' + os.sep, 'old', 'place', 'abc.pdf'))
|
|
|
|
assert document_path(legacy) == legacy
|
|
|
|
def test_moving_the_root_moves_new_documents_and_not_old_ones(self, tmp_path, monkeypatch):
|
|
relative = os.path.join(CONTRACTS_DIR, 'abc.pdf')
|
|
legacy = os.path.abspath(os.path.join(str(tmp_path), 'legacy', 'abc.pdf'))
|
|
|
|
monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path / 'one'))
|
|
first_new, first_old = document_path(relative), document_path(legacy)
|
|
|
|
monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path / 'two'))
|
|
second_new, second_old = document_path(relative), document_path(legacy)
|
|
|
|
assert first_new != second_new, 'a release switch has to move new documents'
|
|
assert first_old == second_old, 'and must not move the ones already filed'
|
|
|
|
|
|
class TestThroughTheUploadRoute:
|
|
@pytest.fixture
|
|
def uploaded(self, app, client, as_role, make_user):
|
|
"""A contract filed by an admin for a player."""
|
|
as_role('admin')
|
|
player_id = make_user('player')
|
|
|
|
response = client.post(
|
|
'/users/contracts/upload',
|
|
data={
|
|
'player_id': str(player_id),
|
|
'notes': 'Season contract',
|
|
'contract_file': (_pdf(), 'contract.pdf'),
|
|
},
|
|
content_type='multipart/form-data',
|
|
follow_redirects=True,
|
|
)
|
|
assert response.status_code == 200
|
|
return player_id
|
|
|
|
def test_the_stored_path_is_relative(self, app, uploaded):
|
|
from app.models import Contract
|
|
|
|
with app.app_context():
|
|
contract = Contract.query.one()
|
|
assert not os.path.isabs(contract.file_path), (
|
|
'an absolute path pins the file to the directory the process '
|
|
'was started from — the one thing a release switch changes'
|
|
)
|
|
assert contract.file_path.startswith(CONTRACTS_DIR)
|
|
|
|
def test_the_file_lands_under_the_configured_root(self, app, uploaded):
|
|
from app.models import Contract
|
|
|
|
with app.app_context():
|
|
contract = Contract.query.one()
|
|
resolved = document_path(contract.file_path)
|
|
|
|
assert os.path.exists(resolved)
|
|
assert resolved.startswith(documents_root())
|
|
|
|
def test_it_can_be_downloaded_back(self, app, client, uploaded):
|
|
from app.models import Contract
|
|
|
|
with app.app_context():
|
|
contract_id = Contract.query.one().id
|
|
|
|
response = client.get(f'/users/contracts/{contract_id}/download')
|
|
|
|
assert response.status_code == 200
|
|
assert response.data.startswith(b'%PDF-')
|
|
|
|
|
|
def _pdf():
|
|
"""The smallest thing pdf_upload_error accepts."""
|
|
import io
|
|
|
|
return io.BytesIO(b'%PDF-1.4\n%%EOF\n')
|
|
|
|
|
|
class TestDeletingAnAccountTakesItsDocuments:
|
|
"""DATA-012. `delete_user` removed the Contract rows and left the PDFs.
|
|
|
|
Signed, named contracts stayed on the server after the account was
|
|
deleted, with nothing in the database pointing at them: invisible to the
|
|
application, unmanageable through it, and still personal data.
|
|
"""
|
|
|
|
@pytest.fixture
|
|
def player_with_contract(self, app, client, as_role, make_user):
|
|
as_role('admin')
|
|
player_id = make_user('player')
|
|
|
|
client.post(
|
|
'/users/contracts/upload',
|
|
data={
|
|
'player_id': str(player_id),
|
|
'contract_file': (_pdf(), 'contract.pdf'),
|
|
},
|
|
content_type='multipart/form-data',
|
|
follow_redirects=True,
|
|
)
|
|
|
|
from app.models import Contract
|
|
|
|
with app.app_context():
|
|
contract = Contract.query.one()
|
|
return player_id, document_path(contract.file_path)
|
|
|
|
def test_the_file_goes_with_the_account(self, app, client, player_with_contract):
|
|
player_id, path = player_with_contract
|
|
assert os.path.exists(path), 'the fixture never wrote the file'
|
|
|
|
client.post(f'/users/{player_id}/delete', follow_redirects=True)
|
|
|
|
from app.models import Contract
|
|
|
|
with app.app_context():
|
|
assert Contract.query.count() == 0
|
|
assert not os.path.exists(path), 'the row went and the PDF stayed'
|
|
|
|
def test_a_file_already_gone_does_not_stop_the_deletion(
|
|
self, app, client, player_with_contract
|
|
):
|
|
"""Removing a file must never be able to abort the removal of an
|
|
account: the account is the part somebody asked for."""
|
|
player_id, path = player_with_contract
|
|
os.remove(path)
|
|
|
|
response = client.post(f'/users/{player_id}/delete', follow_redirects=True)
|
|
|
|
assert response.status_code == 200
|
|
from app.models import User
|
|
|
|
with app.app_context():
|
|
assert db.session.get(User, player_id) is None
|
|
|
|
|
|
class TestDiscardDocuments:
|
|
def test_it_reports_how_many_it_removed(self, tmp_path, monkeypatch):
|
|
from app.storage import discard_documents
|
|
|
|
monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path))
|
|
(tmp_path / 'a.pdf').write_bytes(b'%PDF-')
|
|
(tmp_path / 'b.pdf').write_bytes(b'%PDF-')
|
|
|
|
assert discard_documents(['a.pdf', 'b.pdf', 'never-existed.pdf']) == 2
|
|
|
|
def test_a_none_path_is_skipped(self, tmp_path, monkeypatch):
|
|
"""signed_file_path is NULL until the player signs, and both paths of
|
|
every contract are passed in together."""
|
|
from app.storage import discard_documents
|
|
|
|
monkeypatch.setenv('DOCUMENTS_ROOT', str(tmp_path))
|
|
|
|
assert discard_documents([None, '']) == 0
|