Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: models #148

Merged
merged 10 commits into from
Sep 15, 2022
Merged

Refactor: models #148

merged 10 commits into from
Sep 15, 2022

Conversation

angela-tran
Copy link
Member

@angela-tran angela-tran commented Sep 7, 2022

Closes #122

This will need to be rebased onto #144. done

This PR splits the eligibility type out from User into its own model called Eligibility. There is a many-to-many relationship between these two.

Table Before After
user image image
eligibility - image
user_eligibility - image

How to view the tables like that ^^

In a terminal in the devcontainer:

ln -s /tmp/test.db db.sqlite
code db.sqlite

(Makes a symlink to the SQLite database file with an extension that VS Code recognizes. Then open it with VS Code.)

This PR also makes a few changes (improvements?) to the expected format of CSV data import files:

data/server.csv
Before

A1234567;Garcia;['type1']
B2345678;Hernandez;['type2']
C3456789;Smith;[]
D4567890;Jones;['type1', 'type2']
a93ef111829829ea5038398ba6583dc2e1a2f2c309e24aee535f725dbe1f1250;2c3aaefea8267c66822f6edc0e42d9b7384695f9c0407eabda141770aab8901e;['type1']
095feaba889222ee1bc97aaf3bb89c90c21238fb556fb48b5cc54760461ea18ce814d016e70b1929d05190edbec1032e07921228fcb61ad3417aaff0abd9d082;123c86e1f2ac255ba31f1ad742defe23d194269669d2aac0d2572e20e9378e395976f84db305caeba1f91e7996463031d4c49365a7a9f4c7dc404873ad330974;['type1']

After

A1234567;Garcia;type1
B2345678;Hernandez;type2
C3456789;Smith;
D4567890;Jones;type1
D4567890;Jones;type2
a93ef111829829ea5038398ba6583dc2e1a2f2c309e24aee535f725dbe1f1250;2c3aaefea8267c66822f6edc0e42d9b7384695f9c0407eabda141770aab8901e;type1
095feaba889222ee1bc97aaf3bb89c90c21238fb556fb48b5cc54760461ea18ce814d016e70b1929d05190edbec1032e07921228fcb61ad3417aaff0abd9d082;123c86e1f2ac255ba31f1ad742defe23d194269669d2aac0d2572e20e9378e395976f84db305caeba1f91e7996463031d4c49365a7a9f4c7dc404873ad330974;type1

or

A1234567;Garcia;type1
B2345678;Hernandez;type2
C3456789;Smith;
D4567890;Jones;"type1,type2"
a93ef111829829ea5038398ba6583dc2e1a2f2c309e24aee535f725dbe1f1250;2c3aaefea8267c66822f6edc0e42d9b7384695f9c0407eabda141770aab8901e;type1
095feaba889222ee1bc97aaf3bb89c90c21238fb556fb48b5cc54760461ea18ce814d016e70b1929d05190edbec1032e07921228fcb61ad3417aaff0abd9d082;123c86e1f2ac255ba31f1ad742defe23d194269669d2aac0d2572e20e9378e395976f84db305caeba1f91e7996463031d4c49365a7a9f4c7dc404873ad330974;type1
  • Lists of eligibility types are expected to be separated by ,. (We used to expect them as Python-style lists that would be evaluted by ast.literal_eval.)
    • If the CSV delimiter is also ,, then the list should be surrounded by the CSV quote character (specified by CSV_QUOTCHAR setting).
  • Removes ast module import since we don't need it anymore
  • Adds support for CSVs to contain multiple rows for a given sub, name pair (i.e. a User). I think this better matches what you might see from a SQL query result.

Wrote unit tests for the init-db and drop-db commands - the init-db test is hard-coded to the sample data.

@angela-tran angela-tran added this to the Courtesy Cards milestone Sep 7, 2022
@angela-tran angela-tran self-assigned this Sep 7, 2022
@angela-tran
Copy link
Member Author

Although this will have minor merge conflicts with #144, I think this can be reviewed now and spot-checked after the rebase.

@angela-tran angela-tran marked this pull request as ready for review September 8, 2022 21:12
@angela-tran angela-tran requested a review from a team as a code owner September 8, 2022 21:12
@@ -45,34 +45,43 @@ 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])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how this is cleaner than the previous str() version.

Copy link
Member

@thekaveman thekaveman Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it's a little funky to pass user and then use user as a key for data - not super obvious what is going on here? data is a dict, might be clearer to iterate over data.items() to deal with the key/value tuple.

Also this JSON file is... the oldest remnant of the old school approach from our toy. Not that important to maintain / make clearer right now, probably just drop it entirely in the future?

ALSO I've updated this section for the remote data anyway! So no worries.

@thekaveman
Copy link
Member

We decided today that we're going to assume the CSV file always has the eligibility types in it, and not worry about filling in default types etc.

@angela-tran
Copy link
Member Author

Done rebasing on main to resolve conflicts with #144 changes. This is ready for review @thekaveman @machikoyasuda

Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is an interesting problem, and a possible solution...

If you run the tests using the VSCode interface, they should all pass. But then immediately run them again without touching anything else - many of the tests in test_verify.py will fail.

This is because tests in db/test_setup.py delete the database and it isn't being recreated elsewhere; the verify tests expect it to already exist. This is probably part of what we've been talking/thinking about in #131.

An immediate resolution could be to add a call to init-db at the very end of the test_drop_db_command test? Simple and kind of a hack, but it would at least make all the tests idempotent (a la #103).

A possible longer term solution could be to abstract that runner.invoke(args="init-db") as a fixture (and the drop-db) that can be injected/used across tests?

tests/db/test_setup.py Show resolved Hide resolved
eligibility_server/settings.py Show resolved Hide resolved
@@ -45,34 +45,43 @@ 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])
Copy link
Member

@thekaveman thekaveman Sep 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it's a little funky to pass user and then use user as a key for data - not super obvious what is going on here? data is a dict, might be clearer to iterate over data.items() to deal with the key/value tuple.

Also this JSON file is... the oldest remnant of the old school approach from our toy. Not that important to maintain / make clearer right now, probably just drop it entirely in the future?

ALSO I've updated this section for the remote data anyway! So no worries.

@angela-tran
Copy link
Member Author

If you run the tests using the VSCode interface, they should all pass. But then immediately run them again without touching anything else - many of the tests in test_verify.py will fail.

Hmm that is interesting. Good catch 🙏

@angela-tran angela-tran merged commit b2d38f4 into main Sep 15, 2022
@angela-tran angela-tran deleted the refactor/models branch September 15, 2022 23:24
@thekaveman
Copy link
Member

🎉

@angela-tran angela-tran mentioned this pull request Nov 14, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor database class
3 participants