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

Rename created and updated properties on models #274

Merged
merged 3 commits into from
Jun 19, 2020

Conversation

iaincollins
Copy link
Member

@iaincollins iaincollins commented Jun 16, 2020

Based on changes suggested in #254

  • Renames created property to created_at (createdAt on MongoDB) on all models
  • Renames updated property to updated_at (updated At on MongoDB) on all models
  • Improves schema tests and related documentation

@vercel
Copy link

vercel bot commented Jun 16, 2020

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/iaincollins/next-auth-docs/l3fe37cqj
✅ Preview: https://next-auth-docs-git-feature-rename-schema-properties.iaincollins.vercel.app

@iaincollins
Copy link
Member Author

iaincollins commented Jun 16, 2020

Update: I have refactored and slightly improved the original process in this PR and have updated this comment accordingly.

What I did

I thought I would document what I did as this "process" (which is a strong word for it…) is not documented anywhere right now! I wouldn't mind refactoring the database tests as part of this PR.

Start test databases

To get started, I loaded testdocker/docker-compose.yml, which includes Docker instances of MySQL, Postgres and MongoDB - this obviously Docker, but doesn't require anything else.

Note: They all listen locally on their default ports, so will clash if you are already running any of these databases on your system. This is something I plan to address in the tests by running them from inside a Docker container at some point (not in this PR).

The Docker images have hard coded auth credentials (nextauth:password) and include a database called nextauth.

  1. Start databases with npm run db:start
  2. Start databases with npm run db:stop (also deletes databases)
  3. Check everything is working with npm run test (should not error)

I use the commands to run/clean up the databases during manual testing too, as it's a lot easier to test against different databases that way.

After starting databases, I run npm run watch in the background to build from source, and trigger new builds when we start refactoring the models.

Updated models

  1. Update schemas in test/schemas/* to expect created_at and updated_at
  2. Run npm run test - it should error and print out the schema it found; you can compare this against the one it was expecting in test/schemas/*

Modify schemas

  1. Change the schemas in src/adapters/typeorm/models - this was a very simple and small change.
  2. . Run npm run test to run the tests - I say tests, but right now they are just node scripts that exit with success or fail states.

The whole process is a bit janky but I feel helpful to have some level of automation to test and document schema changes. In a future PR I tend to update the tests and documentation to migrate the tests to an actual test framework and refactor the documentation so that it is programatically generated (and so is always up-to-date).

@iaincollins iaincollins force-pushed the feature/rename-schema-properties branch from ba770e8 to b1bb850 Compare June 18, 2020 16:19
@vercel vercel bot temporarily deployed to Preview June 18, 2020 16:19 Inactive
@iaincollins
Copy link
Member Author

iaincollins commented Jun 18, 2020

Somehow this branch is messed up, ugh.

Fixed! I rebased against an old branch (instead of main). Doh!

Changes should be easier to read now.

@iaincollins iaincollins force-pushed the feature/rename-schema-properties branch 3 times, most recently from 4d5bfb5 to ea78020 Compare June 18, 2020 17:07
@iaincollins iaincollins force-pushed the feature/rename-schema-properties branch from ea78020 to fab1214 Compare June 18, 2020 17:12
@vercel vercel bot temporarily deployed to Preview June 18, 2020 17:12 Inactive
CONTRIBUTING.md Outdated Show resolved Hide resolved
@vercel vercel bot temporarily deployed to Preview June 18, 2020 17:15 Inactive
@iaincollins
Copy link
Member Author

iaincollins commented Jun 18, 2020

@LoriKarikari Based on feedback I've removed the 'default' value when the field is not nullable so it's more clear if a field actually has a default value or not.

Note: I don't actually think hand rolling some DB tests like this is a great idea, I was just out of other better ways to do SOMETHING in the short term. I think the new test stuff that is coming will result in something much better (and we can uses some of this to help get there I think)

RE: @JeffersonBledsoe / #66

@iaincollins iaincollins merged commit 935c4f2 into main Jun 19, 2020
@iaincollins iaincollins deleted the feature/rename-schema-properties branch June 19, 2020 07:28
@JeffersonBledsoe JeffersonBledsoe mentioned this pull request Jun 22, 2020
20 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.

None yet

1 participant