-
Notifications
You must be signed in to change notification settings - Fork 221
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.
Looks good. I'm not sure why this upgrade to 5.2 didn't involve the new secrets.yml.enc
file, as we've seen with other projects being upgraded.
However, everything seems to run and be seeded fine once the MovieDB API key is setup (we should update the README to mention that probably).
Also, we should try to get this deployed through Travis. I haven't setup any projects with that, so you should probably check with Dan.
No Travis. We don't deploy this API, the students fork it and hack on it themselves. If they deploy they'll do it manually. |
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.
Reading the changes this looks good, though I have not run the app. I have two things:
- Looks like there are tests for checkin - can you confirm that these were failing before and now pass?
- Remove the travis file, since we don't deploy this one ourselves.
Then it should be good to merge.
Whoops I forgot we have a deployed demo of this! Ignore my earlier comments about Travis |
travis.yml
Outdated
provider: heroku | ||
api_key: | ||
secure: TODO | ||
app: videostoreconsumerapi |
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.
You can run travis setup heroku --force
and follow the prompts to set this up.
@droberts-ada I've added a test to prevent the check-in bug from reoccuring. It involved the fact that Checking out a movie left |
Looking at this, I updated the API to a newer version of Rails & Ruby, and added Dan's Cors test and moved it toward Postgresql.
Still not done, but putting this PR up for review to date
Finished:
Yet to do: