Refactor: organize application as a package #140

angela-tran · 2022-08-31T00:44:53Z

Part of #131

Closes #139

This PR makes changes to the organization of our code so that

database-related code is grouped together
our application can be imported as a package.

This should allow us to move towards using the app factory pattern, which in turn should move us towards using a separate database when testing.

Note that the setup.py and teardown.py scripts for modifying the database were converted into click commands to facilitate moving them into a db sub-package and since that's the approach shown in Flask documentation.

this is to prepare for moving setup and teardown into the same module make sure tests have an app context so that the db object knows which app to work with

run it using `flask init-db`

run it using `flask drop-db`

machikoyasuda

Small documentation change.

I ran and tested:

Ran cp .env.sample .env
Ran docker compose build --no-cache server
Ran and passed all tests
Opened http://localhost:5000/healthcheck, http://localhost:5000/publickey
Tested flask drop-db

docs/getting-started.md

machikoyasuda · 2022-08-31T21:24:24Z

I believe this PR also closes #139

thekaveman · 2022-09-01T00:22:42Z

👀 Started reading through this tonight, will finish a little later or tomorrow!

thekaveman

This looks like a good direction overall.

I'm not totally thrilled with the Database class (nothing you did in this PR) - it feels weird, we called it that in the past because it literally was the hardcoded database. Now it's just that check_user function... which is really doing the eligibility check? Oh well. Not too worried about this for now.

Left a few comments inline, a few areas we need some changes and a few potential cleanups/reorgs.

setup.py

eligibility_server/db/setup.py

eligibility_server/db/teardown.py

eligibility_server/verify.py

eligibility_server/db/models.py

eligibility_server/db/setup.py

eligibility_server/verify.py

angela-tran · 2022-09-01T20:33:47Z

Thanks @machikoyasuda and @thekaveman for your thorough reviews! I agree with the things you pointed out and will work through these items.

temporarily move Database class to separate module to avoid circular import

angela-tran · 2022-09-02T22:09:03Z

@thekaveman I made some changes based on your feedback. Also, I agree with this:

I'm not totally thrilled with the Database class (nothing you did in this PR) - it feels weird, we called it that in the past because it literally was the hardcoded database. Now it's just that check_user function... which is really doing the eligibility check? Oh well. Not too worried about this for now.

Should I try to move the check_user logic into the verify module as a part of this PR?

thekaveman

All of my comments below are mostly stylistic and don't require any changes.

But one thing that should be addressed: We want the Docker app container to install this package!

Need a line in the Dockerfile like:

RUN pip install -e .

Obviously the source and setup.py need to the in the image layer for this to work, so may need to adjust how/where pip install happens, or just do it a second time if that's preferable.

eligibility_server/app.py

eligibility_server/db/database.py

eligibility_server/db/models.py

eligibility_server/verify.py

eligibility_server/db/__init__.py

eligibility_server/app.py

eligibility_server/verify.py

tests/test_database.py

thekaveman · 2022-09-02T22:18:45Z

@angela-tran

@thekaveman I made some changes based on your feedback. Also, I agree with this:

I'm not totally thrilled with the Database class (nothing you did in this PR) - it feels weird, we called it that in the past because it literally was the hardcoded database. Now it's just that check_user function... which is really doing the eligibility check? Oh well. Not too worried about this for now.

Should I try to move the check_user logic into the verify module as a part of this PR?

Didn't see this comment before leaving my review - but yeah that could be a better place for it! Maybe turn verify into a sub-package with api.py and somethingelse.py?

setup.py

tests/test_database.py

remove unnecessary Database class, and port test_database to use Verify.

thekaveman

Looks great!

All tests passed locally
The package is installed (in a python REPL session I could import eligibility_server)
/healthcheck, /publickey, /verify return expected results

angela-tran added 7 commits August 30, 2022 23:46

refactor: move db and User into database module

da446ac

this is to prepare for moving setup and teardown into the same module make sure tests have an app context so that the db object knows which app to work with

refactor: move Database into db package

258f958

refactor: move db and User into a models module inside db package

e246508

refactor: convert setup script into click CLI command

24fb1ec

run it using `flask init-db`

refactor: convert teardown script into click CLI command

d475f6e

run it using `flask drop-db`

refactor: verify module uses current_app instead of actual app object

7a857d9

feat: make package importable through setup.py

a23143b

angela-tran added this to the Courtesy Cards milestone Aug 31, 2022

angela-tran self-assigned this Aug 31, 2022

angela-tran requested a review from a team as a code owner August 31, 2022 00:44

ci: add variable for use in run-tests workflow

2d65a63

angela-tran force-pushed the refactor/package branch from f8f5f55 to 2d65a63 Compare August 31, 2022 00:56

machikoyasuda requested changes Aug 31, 2022

View reviewed changes

docs/getting-started.md Show resolved Hide resolved

thekaveman requested changes Sep 1, 2022

View reviewed changes

thekaveman mentioned this pull request Sep 1, 2022

Hide settings keys in an importable config object/module #142

Closed

angela-tran added 5 commits September 1, 2022 22:37

docs: correct some details about init script

75dea8e

chore: remove unnecessary parameters leftover from refactoring

7de32c1

refactor: move init_app to module initialization

5458a42

temporarily move Database class to separate module to avoid circular import

refactor: move database commands into single file

1385b77

chore(metadata): flesh out package metadata in setup script

ae836f0

angela-tran requested review from thekaveman and machikoyasuda September 2, 2022 22:09

thekaveman requested changes Sep 2, 2022

View reviewed changes

thekaveman reviewed Sep 2, 2022

View reviewed changes

setup.py Outdated Show resolved Hide resolved

thekaveman reviewed Sep 2, 2022

View reviewed changes

setup.py Show resolved Hide resolved

thekaveman mentioned this pull request Sep 3, 2022

Feat: settings/configuration helper #144

Merged

angela-tran commented Sep 7, 2022

View reviewed changes

tests/test_database.py Show resolved Hide resolved

angela-tran added 5 commits September 7, 2022 03:28

refactor: move check_user logic into Verify

a898926

remove unnecessary Database class, and port test_database to use Verify.

refactor(tests): move ported tests into more appropriate file

8481506

chore: switch to using absolute imports

2a0669a

feat(container): install package into app container

4d3bdb1

chore(metadata): remove unneeded specification of minor Python versions

39aaa7b

angela-tran force-pushed the refactor/package branch from c4841dc to 39aaa7b Compare September 7, 2022 03:31

chore: fix some docstrings for database commands

fd1c915

angela-tran requested a review from thekaveman September 7, 2022 18:31

thekaveman approved these changes Sep 7, 2022

View reviewed changes

machikoyasuda approved these changes Sep 7, 2022

View reviewed changes

angela-tran merged commit 06037dd into main Sep 7, 2022

angela-tran deleted the refactor/package branch September 7, 2022 21:56

angela-tran mentioned this pull request Sep 19, 2022

Make it easier for tests to use a separate app and database #131

Closed

angela-tran mentioned this pull request Nov 14, 2022

Make Release: 2022.11.1 #191

Closed

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: organize application as a package #140

Refactor: organize application as a package #140

angela-tran commented Aug 31, 2022 •

edited by thekaveman

Loading

machikoyasuda left a comment •

edited

Loading

machikoyasuda commented Aug 31, 2022

thekaveman commented Sep 1, 2022

thekaveman left a comment •

edited

Loading

angela-tran commented Sep 1, 2022

angela-tran commented Sep 2, 2022

thekaveman left a comment

thekaveman commented Sep 2, 2022 •

edited

Loading

thekaveman left a comment

Refactor: organize application as a package #140

Refactor: organize application as a package #140

Conversation

angela-tran commented Aug 31, 2022 • edited by thekaveman Loading

machikoyasuda left a comment • edited Loading

Choose a reason for hiding this comment

machikoyasuda commented Aug 31, 2022

thekaveman commented Sep 1, 2022

thekaveman left a comment • edited Loading

Choose a reason for hiding this comment

angela-tran commented Sep 1, 2022

angela-tran commented Sep 2, 2022

thekaveman left a comment

Choose a reason for hiding this comment

thekaveman commented Sep 2, 2022 • edited Loading

thekaveman left a comment

Choose a reason for hiding this comment

angela-tran commented Aug 31, 2022 •

edited by thekaveman

Loading

machikoyasuda left a comment •

edited

Loading

thekaveman left a comment •

edited

Loading

thekaveman commented Sep 2, 2022 •

edited

Loading