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

Feature/prisma test setup #40

Merged
merged 80 commits into from
May 12, 2022
Merged

Feature/prisma test setup #40

merged 80 commits into from
May 12, 2022

Conversation

eadsoy
Copy link
Collaborator

@eadsoy eadsoy commented Mar 1, 2022

Looks like the test script is picking up the correct db but the migration is failing
Screen Shot 2022-03-01 at 15 35 56

eadsoy added 14 commits March 2, 2022 16:46
…a to BigInt

Seed database from test itself
Write tests for certain results
…imageFileName].ts into its own file

at /web-app/helpers/expiration-query.ts
Refactor [imageFileName].ts to import getExpiration from expiration-query.ts
Refactor Synth data generator to generate data from data sources:
- /web-app/synth/account_names.json
- /web-app/synth/receipt_ids.json
- /web-app/synth/timestamps.json
Synth data schemas:
- /web-app/synth/test_db/action_receipts.json
- /web-app/synth/test_db/receipts.json
Remove beforeAll() function in /web-app/__tests__/test-query.test.ts since Synth will take care of populaating the test db.
diff_from_last_activity_to_render_date was calculating the days between render date(now) and last activity date of account
but since this comparison is made outside of this function, diff_from_last_activity_to_render_date became redundant
- Refactor test to not inlcude diff_from_last_activity_to_render_date
- Remove getRawQuery from query.ts in /web-app/prisma/test-helpers/query.ts
- Change included_in_block_timestamp field type in test-schema.prisma from BigInt to Decimal as in the original schema
to reflect same properties.
- Remove Synth script in package.json test script and add seed script to populate Prisma test db using Prisma's seeding option.
- This change is made due to the bug in generating numeric data in Synth.
- Add ts-node as dev dependency to be able to use Prisma seed
- Change test file name to a more descriptive name.
@eadsoy eadsoy force-pushed the feature/prisma-test-setup branch from 760259c to 15522d3 Compare March 8, 2022 07:23
eadsoy and others added 7 commits March 10, 2022 17:09
Use bn.js to calculate Unix Timestamp in nanoseconds in /web-app/helpers/expiration-date.ts and /web-app/prisma/test-helpers/query.ts
Change issuedAtUnixNano type to string and refactor raw query to cast numeric type
add test cases for account with more activity
move test-helpers and __test__ folders under /test folder
…g on docker and delete console.table in test file
eadsoy and others added 21 commits April 19, 2022 14:34
refactor query to show start_of_long_period_of_inactivity column as start date of long period of inactivity, if present
change calculation of expiration date when long period of inactivity is present
remove has_long_period_of_inactivity column
change test cases to return correct expiration dates
…ynamicActivityData helper function to generate-account-activities.ts
…f long period of inactivity) and diff_to_previous_activity
…ccount in different intervals and time units such as day, hour, etc.
…tivity (since issue date doesn't show up on account activities)
…ery result and combine case expression in query
@sccheruku
Copy link
Collaborator

sccheruku commented May 8, 2022

Approved. But I am wondering if we tried to use Prisma ORM code instead of using the raw sql approach ?
One downside of using raw sql that it opens us to sql injection attacks. It's not a big deal in this case, because we are working with a read only database.

I'll share a gist to an example I was working on using Prisma ORM code, but I do not think we should block this PR. We have 6 months to decide if we want to refactor anything, right?

https://www.loom.com/share/d87f4807646c4b79b2f5eae3ab4e2879

Here is the gist: https://gist.github.com/sccheruku/94f0d1406c3ebace3de4045c095eef9f (on testnet, my query took about 300ms when I set the minimum activity period to 7 days instead of 180 days)

It's a first iteration of the code, so it does not tackle all the edge cases @eadsoy covered in the tests.

@ryancwalsh
Copy link
Contributor

@sccheruku Thanks for the review, the gist, and the video. I've used Prisma before but have definitely run into limitations, and I'm not 100% sure it can handle what we're trying to do.

If we can use Prisma, then I agree that we should seriously consider it. Abstracting away from raw SQL can provide some flexibility (allows us to swap out the database underneath, theoretically).

I might be misunderstanding your approach, but something doesn't feel ideal about iterating an indeterminately large number of times to check for each period (currently defined as 6 months, but that's an arbitrary number that might change int he future too).

We can talk about it live today.

@ryancwalsh ryancwalsh marked this pull request as ready for review May 12, 2022 14:13
@ryancwalsh ryancwalsh merged commit 91a8add into develop May 12, 2022
@ryancwalsh ryancwalsh deleted the feature/prisma-test-setup branch May 12, 2022 14:13
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.

None yet

3 participants