-
Notifications
You must be signed in to change notification settings - Fork 0
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
F001 add author biographies #2
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great first pass, I left a few comments and suggestions for changes I'd like to see, mostly answering your questions and looking for some better error handling.
2d8d693
to
ab95a13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requested a couple more changes, most notably around using transactions to better ensure data consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Go ahead and merge this, and that finishes up the API side of this project.
🚢 🚢 |
This PR adds backend support for author biographies.
Work is grouped in two commits: One for migrating the DB to include the new fields, and one for adding the new
rake
command.DB
biography
andgithub_issue_id
columns added toAuthors
tablebiography
data is added toauthor_serializer.rb
, and was manually checked withcurl
to verify that the data is returnedData sync
bin/rake github:sync
has been added to update author data in the application's database as per story requirementsoctokit
to connect to github API, andliterate_randomizer
to generate random book titles)books
foreign key dependenciesSome of my thoughts have been posted as comments in-line.
Readme updates to come, and a few modification to the tests to support this are available, but require PR #1 to be merged first.