From d5e3b8af42d4668f9ce2ae8ca03379b2d3f2d692 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Wed, 7 Sep 2022 22:10:34 +0000 Subject: [PATCH 01/10] refactor(models): split out eligibility types from User model create associative table to support a many-to-many relationship. update init-db and drop-db. --- eligibility_server/db/models.py | 19 +++++++++++++++++-- eligibility_server/db/setup.py | 25 +++++++++++++++++++------ eligibility_server/verify.py | 3 +-- 3 files changed, 37 insertions(+), 10 deletions(-) diff --git a/eligibility_server/db/models.py b/eligibility_server/db/models.py index 8de8eaa4..2101e40d 100644 --- a/eligibility_server/db/models.py +++ b/eligibility_server/db/models.py @@ -1,11 +1,26 @@ from eligibility_server.db import db +user_eligibility = db.Table( + "user_eligibility", + db.Column("user_id", db.Integer, db.ForeignKey("user.id"), primary_key=True), + db.Column("eligibility_id", db.Integer, db.ForeignKey("eligibility.id"), primary_key=True), +) + + +class Eligibility(db.Model): + id = db.Column(db.Integer, primary_key=True) + name = db.Column(db.String, unique=True, nullable=False) + + def __repr__(self): + return f"" + + class User(db.Model): id = db.Column(db.Integer, primary_key=True) sub = db.Column(db.String, unique=True, nullable=False) name = db.Column(db.String, unique=True, nullable=False) - types = db.Column(db.String, unique=False, nullable=False) + types = db.relationship("Eligibility", secondary=user_eligibility, lazy="subquery", backref=db.backref("users", lazy=True)) def __repr__(self): - return "" % self.sub + return f"" diff --git a/eligibility_server/db/setup.py b/eligibility_server/db/setup.py index 6f333363..112b0c52 100644 --- a/eligibility_server/db/setup.py +++ b/eligibility_server/db/setup.py @@ -1,3 +1,4 @@ +import ast # needed while sample CSV contains Python-style list import csv import json @@ -6,7 +7,7 @@ from flask_sqlalchemy import inspect from eligibility_server.db import db -from eligibility_server.db.models import User +from eligibility_server.db.models import User, Eligibility from eligibility_server.settings import Configuration @@ -50,7 +51,7 @@ def import_users(): with open(file_path) as file: data = json.load(file)["users"] for user in data: - save_users(user, data[user][0], str(data[user][1])) + save_users(user, data[user][0], data[user][1]) elif file_format == "csv": with open(file_path, newline=config.csv_newline, encoding="utf-8") as file: data = csv.reader( @@ -60,14 +61,17 @@ def import_users(): quotechar=config.csv_quotechar, ) for user in data: - save_users(user[0], user[1], user[2]) + # todo: update sample CSV to use expected list format and change this parsing + types = ast.literal_eval(user[2]) + save_users(user[0], user[1], types) else: click.echo(f"Warning: File format is not supported: {file_format}") click.echo(f"Users added: {User.query.count()}") + click.echo(f"Eligibility types added: {Eligibility.query.count()}") -def save_users(sub: str, name: str, types: str): +def save_users(sub: str, name: str, types): """ Add users to the database User table @@ -76,8 +80,13 @@ def save_users(sub: str, name: str, types: str): @param types - Types of eligibilities, in a stringified list """ - item = User(sub=sub, name=name, types=types) - db.session.add(item) + user = User(sub=sub, name=name) + eligibility_types = [Eligibility.query.filter_by(name=type).first() or Eligibility(name=type) for type in types] + user.types.extend(eligibility_types) + + db.session.add(user) + db.session.add_all(eligibility_types) + db.session.commit() @@ -90,6 +99,10 @@ def drop_db_command(): try: click.echo(f"Users to be deleted: {User.query.count()}") User.query.delete() + + click.echo(f"Eligibility types to be deleted: {Eligibility.query.count()}") + Eligibility.query.delete() + db.session.commit() except Exception as e: click.echo("Failed to query for Users", e) diff --git a/eligibility_server/verify.py b/eligibility_server/verify.py index b08ac065..c5081725 100644 --- a/eligibility_server/verify.py +++ b/eligibility_server/verify.py @@ -2,7 +2,6 @@ Eligibility Verification route """ -import ast import datetime import json import logging @@ -140,7 +139,7 @@ def _check_user(self, sub, name, types): existing_user = User.query.filter_by(sub=sub, name=name).first() if existing_user: - existing_user_types = ast.literal_eval(existing_user.types) + existing_user_types = [type.name for type in existing_user.types] else: existing_user_types = [] From a9f066d10351997dd07356de7cb71f526cdbbf62 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Wed, 7 Sep 2022 22:17:56 +0000 Subject: [PATCH 02/10] refactor(init-db): remove usage of ast module lists in CSV import files are expected to be comma-separated. if the CSV delimiter is a comma, the list should be quoted. --- data/server.csv | 12 ++++++------ eligibility_server/db/setup.py | 5 ++--- eligibility_server/settings.py | 2 +- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/data/server.csv b/data/server.csv index b087786c..ef8beece 100644 --- a/data/server.csv +++ b/data/server.csv @@ -1,6 +1,6 @@ -A1234567;Garcia;['type1'] -B2345678;Hernandez;['type2'] -C3456789;Smith;[] -D4567890;Jones;['type1', 'type2'] -a93ef111829829ea5038398ba6583dc2e1a2f2c309e24aee535f725dbe1f1250;2c3aaefea8267c66822f6edc0e42d9b7384695f9c0407eabda141770aab8901e;['type1'] -095feaba889222ee1bc97aaf3bb89c90c21238fb556fb48b5cc54760461ea18ce814d016e70b1929d05190edbec1032e07921228fcb61ad3417aaff0abd9d082;123c86e1f2ac255ba31f1ad742defe23d194269669d2aac0d2572e20e9378e395976f84db305caeba1f91e7996463031d4c49365a7a9f4c7dc404873ad330974;['type1'] +A1234567;Garcia;type1 +B2345678;Hernandez;type2 +C3456789;Smith; +D4567890;Jones;"type1,type2" +a93ef111829829ea5038398ba6583dc2e1a2f2c309e24aee535f725dbe1f1250;2c3aaefea8267c66822f6edc0e42d9b7384695f9c0407eabda141770aab8901e;type1 +095feaba889222ee1bc97aaf3bb89c90c21238fb556fb48b5cc54760461ea18ce814d016e70b1929d05190edbec1032e07921228fcb61ad3417aaff0abd9d082;123c86e1f2ac255ba31f1ad742defe23d194269669d2aac0d2572e20e9378e395976f84db305caeba1f91e7996463031d4c49365a7a9f4c7dc404873ad330974;type1 diff --git a/eligibility_server/db/setup.py b/eligibility_server/db/setup.py index 112b0c52..4feab7e0 100644 --- a/eligibility_server/db/setup.py +++ b/eligibility_server/db/setup.py @@ -1,4 +1,3 @@ -import ast # needed while sample CSV contains Python-style list import csv import json @@ -61,8 +60,8 @@ def import_users(): quotechar=config.csv_quotechar, ) for user in data: - # todo: update sample CSV to use expected list format and change this parsing - types = ast.literal_eval(user[2]) + # lists are expected to be a comma-separated value and quoted if the CSV delimiter is a comma + types = [type.replace(config.csv_quotechar, "") for type in user[2].split(",") if type] save_users(user[0], user[1], types) else: click.echo(f"Warning: File format is not supported: {file_format}") diff --git a/eligibility_server/settings.py b/eligibility_server/settings.py index 9bc30f59..dfcbe1bf 100644 --- a/eligibility_server/settings.py +++ b/eligibility_server/settings.py @@ -41,7 +41,7 @@ CSV_DELIMITER = ";" CSV_NEWLINE = "" CSV_QUOTING = 3 -CSV_QUOTECHAR = "" +CSV_QUOTECHAR = "\"" class Configuration: From 0223b78bece9c902c2d2805fc4426ab3b2b1712a Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Wed, 7 Sep 2022 22:20:31 +0000 Subject: [PATCH 03/10] feat(init-db): allow import file to have multiple rows for a given user update sample CSV file to demonstrate --- data/server.csv | 3 ++- eligibility_server/db/setup.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/data/server.csv b/data/server.csv index ef8beece..5b3dc780 100644 --- a/data/server.csv +++ b/data/server.csv @@ -1,6 +1,7 @@ A1234567;Garcia;type1 B2345678;Hernandez;type2 C3456789;Smith; -D4567890;Jones;"type1,type2" +D4567890;Jones;type1 +D4567890;Jones;type2 a93ef111829829ea5038398ba6583dc2e1a2f2c309e24aee535f725dbe1f1250;2c3aaefea8267c66822f6edc0e42d9b7384695f9c0407eabda141770aab8901e;type1 095feaba889222ee1bc97aaf3bb89c90c21238fb556fb48b5cc54760461ea18ce814d016e70b1929d05190edbec1032e07921228fcb61ad3417aaff0abd9d082;123c86e1f2ac255ba31f1ad742defe23d194269669d2aac0d2572e20e9378e395976f84db305caeba1f91e7996463031d4c49365a7a9f4c7dc404873ad330974;type1 diff --git a/eligibility_server/db/setup.py b/eligibility_server/db/setup.py index 4feab7e0..cc96e804 100644 --- a/eligibility_server/db/setup.py +++ b/eligibility_server/db/setup.py @@ -79,7 +79,7 @@ def save_users(sub: str, name: str, types): @param types - Types of eligibilities, in a stringified list """ - user = User(sub=sub, name=name) + user = User.query.filter_by(sub=sub, name=name).first() or User(sub=sub, name=name) eligibility_types = [Eligibility.query.filter_by(name=type).first() or Eligibility(name=type) for type in types] user.types.extend(eligibility_types) From 14f29fe3ae1b56229dbbd5da744e2c72140214ac Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Thu, 8 Sep 2022 20:18:17 +0000 Subject: [PATCH 04/10] test(init-db): add test hard-coded to sample CSV file --- tests/conftest.py | 5 +++++ tests/db/__init__.py | 0 tests/db/test_setup.py | 26 ++++++++++++++++++++++++++ 3 files changed, 31 insertions(+) create mode 100644 tests/db/__init__.py create mode 100644 tests/db/test_setup.py diff --git a/tests/conftest.py b/tests/conftest.py index 5eae20b2..249c60c4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -13,3 +13,8 @@ def flask(): @pytest.fixture def client(): return app.test_client() + + +@pytest.fixture() +def runner(): + return app.test_cli_runner() diff --git a/tests/db/__init__.py b/tests/db/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/db/test_setup.py b/tests/db/test_setup.py new file mode 100644 index 00000000..52637edc --- /dev/null +++ b/tests/db/test_setup.py @@ -0,0 +1,26 @@ +import pytest + +from eligibility_server.db.models import Eligibility, User + + +@pytest.mark.usefixtures("flask") +def test_init_db_command(runner): + """Assumes that IMPORT_FILE_PATH is data/server.csv.""" + runner.invoke(args="drop-db") + + result = runner.invoke(args="init-db") + + assert result.exit_code == 0 + assert User.query.count() == 6 + assert Eligibility.query.count() == 2 + + user_with_one_eligibility = User.query.filter_by(sub="A1234567", name="Garcia").first() + type1_eligibility = Eligibility.query.filter_by(name="type1").first() + assert user_with_one_eligibility.types == [type1_eligibility] + + user_with_no_eligibility = User.query.filter_by(sub="C3456789", name="Smith").first() + assert user_with_no_eligibility.types == [] + + user_with_multiple_eligibilities = User.query.filter_by(sub="D4567890", name="Jones").first() + type2_eligibility = Eligibility.query.filter_by(name="type2").first() + assert user_with_multiple_eligibilities.types == [type1_eligibility, type2_eligibility] From 887af3bfc6061dae3fef12129f26610558e30fb5 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Thu, 8 Sep 2022 20:21:36 +0000 Subject: [PATCH 05/10] test(drop-db): add test --- tests/db/test_setup.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/db/test_setup.py b/tests/db/test_setup.py index 52637edc..7129212e 100644 --- a/tests/db/test_setup.py +++ b/tests/db/test_setup.py @@ -1,5 +1,7 @@ import pytest +from flask_sqlalchemy import inspect +from eligibility_server.db import db from eligibility_server.db.models import Eligibility, User @@ -11,6 +13,7 @@ def test_init_db_command(runner): result = runner.invoke(args="init-db") assert result.exit_code == 0 + assert User.query.count() == 6 assert Eligibility.query.count() == 2 @@ -24,3 +27,13 @@ def test_init_db_command(runner): user_with_multiple_eligibilities = User.query.filter_by(sub="D4567890", name="Jones").first() type2_eligibility = Eligibility.query.filter_by(name="type2").first() assert user_with_multiple_eligibilities.types == [type1_eligibility, type2_eligibility] + + +@pytest.mark.usefixtures("flask") +def test_drop_db_command(runner): + result = runner.invoke(args="drop-db") + + assert result.exit_code == 0 + + inspector = inspect(db.engine) + assert inspector.get_table_names() == [] From 84eaacf65fb391a7c6c6a26f049f1aa48006a749 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Thu, 8 Sep 2022 20:47:29 +0000 Subject: [PATCH 06/10] fix(drop-db): make sure drop-db still works on older databases --- eligibility_server/db/setup.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/eligibility_server/db/setup.py b/eligibility_server/db/setup.py index cc96e804..23e291a6 100644 --- a/eligibility_server/db/setup.py +++ b/eligibility_server/db/setup.py @@ -102,9 +102,10 @@ def drop_db_command(): click.echo(f"Eligibility types to be deleted: {Eligibility.query.count()}") Eligibility.query.delete() - db.session.commit() except Exception as e: - click.echo("Failed to query for Users", e) + click.echo(f"Failed to query models. Exception: {e}", err=True) + + db.session.commit() db.drop_all() click.echo("Database dropped.") From 5cdb41582ef21d1ef9738d0eff7416bb9ddb07b5 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Thu, 8 Sep 2022 21:50:22 +0000 Subject: [PATCH 07/10] chore: update comment about the data type for 'types' parameter --- eligibility_server/db/setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eligibility_server/db/setup.py b/eligibility_server/db/setup.py index 23e291a6..c0c077f8 100644 --- a/eligibility_server/db/setup.py +++ b/eligibility_server/db/setup.py @@ -76,7 +76,7 @@ def save_users(sub: str, name: str, types): @param sub - User's sub @param name - User's name - @param types - Types of eligibilities, in a stringified list + @param types - Types of eligibilities, in the form of a list of strings """ user = User.query.filter_by(sub=sub, name=name).first() or User(sub=sub, name=name) From 7ce18bf7aca22efc8d197f6e747773254634c6af Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 15 Sep 2022 19:48:55 +0000 Subject: [PATCH 08/10] chore(pre-commit): autofix run --- eligibility_server/settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/eligibility_server/settings.py b/eligibility_server/settings.py index dfcbe1bf..df13b3e1 100644 --- a/eligibility_server/settings.py +++ b/eligibility_server/settings.py @@ -41,7 +41,7 @@ CSV_DELIMITER = ";" CSV_NEWLINE = "" CSV_QUOTING = 3 -CSV_QUOTECHAR = "\"" +CSV_QUOTECHAR = '"' class Configuration: From 9d714b80a4124abc9d5270a071512f6b44eb586d Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Thu, 15 Sep 2022 22:05:01 +0000 Subject: [PATCH 09/10] fix(tests): short-term solution to ensure database tests are idempotent --- tests/db/test_setup.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/db/test_setup.py b/tests/db/test_setup.py index 7129212e..d80489d4 100644 --- a/tests/db/test_setup.py +++ b/tests/db/test_setup.py @@ -37,3 +37,5 @@ def test_drop_db_command(runner): inspector = inspect(db.engine) assert inspector.get_table_names() == [] + + runner.invoke(args="init-db") From 7e64a9456a3bfe1cd37eca8d7f9d34263f711379 Mon Sep 17 00:00:00 2001 From: Angela Tran Date: Thu, 15 Sep 2022 18:16:28 -0500 Subject: [PATCH 10/10] test: document reason for resetting the database Co-authored-by: Kegan Maher --- tests/db/test_setup.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/db/test_setup.py b/tests/db/test_setup.py index d80489d4..89528e7a 100644 --- a/tests/db/test_setup.py +++ b/tests/db/test_setup.py @@ -38,4 +38,5 @@ def test_drop_db_command(runner): inspector = inspect(db.engine) assert inspector.get_table_names() == [] + # temp fix to ensure database tests are idempotent - later tests need the database to exist runner.invoke(args="init-db")