drusepth / Indent

Indent is a set of tools for writers, game designers, and roleplayers to create magnificent universes – and everything within them.
http://indentapp.com
1 stars 1 forks source link

Upgrade to Rails 4 with SQLite databases #360

Closed Cantido closed 9 years ago

Cantido commented 10 years ago

Long story short, I created a brand new Rails 4 app, and migrated code over. It's probably very messy and I do plan on putting more commits into this branch before you pull it. However all of your code is retained and you're dealing with the same models, so I don't think you should run in to any problems. The only actual changes I made were to the models and database configurations.

I will be working on migrating the tests, and writing tests, since this has tests that pass now! Of course they are very simple tests, I wanted to get this pull request out so you can see my progress.

The hard part is going to be migrating your Mongo prod data into an SQL database. Your data isn't very complicated, so it shouldn't be horrible. Though I have no experience migrating databases.

Since my branch is not likely to merge cleanly by the time I'm done:

BRANCH BUILD: Build Status

Cantido commented 10 years ago

The failing Travis build is due to the failure of some tests. Particularly, the tests that would modify a Character are failing.

@drusepth I'm not too familiar with the higher levels of your code, but I'm assuming you put a little security around it. Do you know how I could make the tests pass? I think I just need to create a session for it.

Cantido commented 10 years ago

I have discovered the existence of before_filter and I'm gonna work with that. Learning! :neckbeard:

Cantido commented 10 years ago

There we go! Rails 4 makes you do a lot more work to specify the parameters that you're allowed to modify. Most of my work was figuring out where that went.

Cantido commented 10 years ago

While I am setting up Rails 4 strong parameters, I have a question. Is there a case where you would ever want to allow the user to move characters/equipment/languages etc to another user? Could they be moved to another universe? Currently I am allowing the controllers to modify all the fields available, but I would like to restrict that before you merge this.

drusepth commented 10 years ago

Sorry I haven't had much time to contribute alongside you, but wanted to pop in to answer your question.

One eventual direction I'd be interested in exploring Indent is an officialization of the "adoption" phenomenon that happens annually on NaNoWriMo forums, among other writing forums. Traditionally, the premise is that someone comes up with a character/equipment/language/whatever and posts it for anyone to use; I think it'd be pretty cool to allow people to eventually post their characters/locations/etc into some sort of adoption agency (related: #29 and #30), where others can "adopt" it into one of their universes. Could do some cool stuff with the system, like letting idea creators see how their characters eventually got used, maybe let users offer up resources (like character artwork) for sale to other users, etc.

Not something I'm streamrolling towards, but something to keep in mind as an eventual possibility, I think.

P.S. All the work you're doing is glorious! Also, before_filter is awesome. Similarly, you might be interested in checking out after_filter, skip_filter, and append_around_filter, and the other variants. :+1:

Cantido commented 10 years ago

Sure, I understand adoptables, totally. It would be a one-line change to open it up, so we can make the decision both now and later. Do your forms provide a part to change users or universes currently? I could lock it down for now and when you introduce adoptables, we could update the controllers for it.

Cantido commented 10 years ago

Man replying to threads via email makes the text awkward.

I haven't looked into how one migrates a Mongo database to an SQL one, but I've seen a few threads on the topic as I was researching other things. The only issue I imagine is images for Locations. I notice you use Paperclip to store the attachment, so I included Paperclip in the Gemfile and stated the attachment in the migration.

What SQL database would like to use? What's available in your product environment?

drusepth commented 10 years ago

Currently production is running on Heroku, but I plan on moving it over to EC2 as soon as possible. Therefore, any database would work.

What do you think about postgres over sqlite? Postgres offers a column data type called hstore (http://www.postgresql.org/docs/9.0/static/hstore.html), which basically offers micro non-relational databases in a column. It might be useful eventually in things like allowing users to define their own fields for characters et al, but I'm sure there's ways to accomplish most things with any database. If you've got a pro railscasts account, you can watch a pretty good video on hstore at http://railscasts.com/episodes/345-hstore?view=comments.

As for migrating data between databases, the mongo db is actually pretty structured. It shouldn't be hard to write some migrations to go through each content type and migrate stuff over to sqlite columns. Location images might be a difficult case indeed, but there's only a couple of locations on production, so worst case it's just a manual re-adding, I think. I'm also not a very big database guy, so I may be completely wrong, but I'm pretty sure I'm not too wrong.

Do your forms provide a part to change users or universes currently?

Universes can be modified like any other content type currently, and users are modified through the account settings dropdown in the top right.

drusepth commented 10 years ago

Just went through all the pull request changes (so far?), and just wanted to pop in and thank you again for all the awesome refactoring, tests, and groundwork you've done. It looks great.

Cantido commented 10 years ago

My pleasure! This is giving me the satisfaction that my day job isn't, lol.

I don't think the secret should be checked in to source control:

Make sure the secrets in this file are kept private

You may want to regenerate it now.

I don't know a lot about different SQL technologies, Postgre sounds fine. It would be neat to get some exposure to another DB.

Since we can lock down parameters at the controller level, we can customize the API a little bit, so it's something to consider if there's only certain circumstances we would allow anyone to transfer a character between users or universes.

drusepth commented 10 years ago

@Cantido Is this pull request in a state that me/you should start merging it into master? I just merged #332 which was pretty big, but only had conflicts in Gemfile.lock, and I'd love to see the fruits of this upgrade and testing see production sometime soon. :)

Before deploying it, we'll have to look at some kind of migration from mongo to sqlite3 (or postgres if we go with that), as all the production data is in mongo (via mongohq) right now, and that of course needs to persist into the new database.

Cantido commented 10 years ago

I don't think it's quite ready, but I can prioritize the functional portion and continue writing tests later. I've been trying to get some nice tests written alongside migrating prod code. You'll have to do plenty of functional testing on your staging domain.

I was going to write some functional tests using Capybara. I think it's a really nice DSL and I'm learning about it on Code School :+1:. But that can come after actual production code.

drusepth commented 10 years ago

Sounds good. I hear good things about Capybara, so if you think that'd be the best way to move forward with testing, go for it! :camel:

Cantido commented 10 years ago

OKAY I've gotten production code written, and I was able to run through content creation and editing with a quick rails s and nothing horrible happened. I don't think the merge will be very messy, I didn't touch any of the content files. This was a pure Ruby job.

Cantido commented 10 years ago

So you can see in 080c7b2 right there that I'm writing functional tests now. I think I'd like to invert my focus on testing and start from the outside in. I think those kinds of tests will be more valuable. Also Capybara is really neat!

You can merge this request at any time, I'll just keep writing tests. I'm not sure how to handle branching and everything. I could just write tests straight to my master branch, or I could make a testing branch. What do you think would be best?

drusepth commented 10 years ago

That Capybara test looks rad.

I'll look into converting the prod database into sqlite3/postgres, and after that's done start the merge in. To prevent loss of data, I'll bring down the site for maintenance while doing the final conversion/upgrade on prod.

I think, in general, it'd be better to work new features in through their own feature branches. However, I think in this case, as soon as this pull request is merged, working in master would let you get in tests of current functionality that we know works, and avoid delays in writing tests that might need changed if functionality changes before they get merged.

Cantido commented 10 years ago

So I'm working through functional tests and I'm having trouble logging in as a user. I checked in the app I have running locally, and I couldn't do it via browsing, so I created an account on the production website and I actually couldn't log in there after creating an account either. Is this an issue with the production app or am I screwing up my login?

To be honest I would be delighted because then automated testing would have uncovered its first critical bug.

drusepth commented 10 years ago

Interesting. I was able to create an account just now both locally and on production.

Are you creating the account from http://indentapp.com/register ? What username are you trying to create (maybe it's a problem with validations)? What happens if you try to create an anon account through /anon-login?

Cantido commented 10 years ago

I was able to create an account, but once I logged off, I was not able to log back in with the same info.

drusepth commented 10 years ago

I see. I see that too, and it's happening on production as well. I'll look into it and see if I can figure out what's happening; feel free to do the same!

Cantido commented 10 years ago

I'll do my best. I haven't gotten into the views yet so I don't know my way around well yet.

drusepth commented 10 years ago

The bug is fixed in https://github.com/drusepth/Indent/commit/1140037f3d708888dc46f48f970a592d3da1670a, but is worse than I thought. Accounts that have been created through /register (rather than converted through an anonymous account) since the extra line was added since upgrading Rails had their passwords double-hashed and could not (and can not) be logged in to. The problem has been fixed for accounts from now on, but I'll do some investigation through data on production to ensure that everyone's existing accounts are still available and no content is lost.

Great catch, thank you!

Cantido commented 10 years ago

My pleasure! and the test that detected it is in this pull request. I'll make sure the fix is included as well.

drusepth commented 10 years ago

:+1:

Cantido commented 10 years ago

I don't want to make this merge any more messy than it probably is. I'll try merging master into this branch, but I think otherwise I'll leave it and wait for this to be pulled in.

Cantido commented 10 years ago

Ugh I think I messed up the merge. I spun up the site after merging and formatting was all goofed up. I don't know how to do this =\

drusepth commented 10 years ago

@Cantido No worries. I don't have much time to work on things tonight, but I'll try to get a merge of master into this branch tomorrow if you'd like, which will probably make the merge of this branch into master a lot easier. Can also go ahead and merge into master, and just hold off on any prod releases until the data migration is done as well, if things are especially bad.

Tests are looking great, I'm really excited for them. :)

Cantido commented 10 years ago

I am excited to write more! Though it would be a lot easier to push non-feature-related tests if I had collaborator access :wink: otherwise I'd have the world's longest pull request as I keep developing in one pull request

drusepth commented 10 years ago

:o Do you not have collaborator access already? I'll have that fixed in a jiffy.

Cantido commented 10 years ago

I just realized that since I rebuilt the app with rails new there might be files in completely different places. I did a git merge and it only asked me about five or six files to resolve conflicts on, so I'm thinking a bunch of files are in the wrong place. I can keep working on it but of course you know the front-end way better than I do.

drusepth commented 10 years ago

Working on setting up the migration from Mongo to Postgres (so we can use hstore functionality for things like #8) unless you have a strong preference for sqlite over postgres. Created #380 to track progress on the migration, since it's pretty involved.

Cantido commented 10 years ago

I don't have any preference for DB, sqlite just makes a very handy local database for testing.

drusepth commented 10 years ago

In that case, I'll plan on sqlite. If we ever decide to move to postgres, I'm sure a migration from sql->sql would be far easier than nosql->sql. :)

Cantido commented 10 years ago

Do you know what kind of traffic your database is getting? I've been reading up on sqlite and there is am upper limit to the number of requests per second sqlite can efficiently handle. I'll go dig up that number.

drusepth commented 10 years ago

I'm not sure how to profile db traffic, but the site has around 50 weekly users, according to google analytics. Probably about half that actually using the site (as opposed to just stumbling on it). On Aug 11, 2014 2:18 PM, "Robert Richter" notifications@github.com wrote:

Do you know what kind of traffic your database is getting? I've been reading up on sqlite and there is am upper limit to the number of requests per second sqlite can efficiently handle. I'll go dig up that number.

— Reply to this email directly or view it on GitHub https://github.com/drusepth/Indent/pull/360#issuecomment-51827073.

Cantido commented 10 years ago

It looks like sqlite can handle "a few hundred transactions per minute" before you start to see performance issues. We are good for now but having an upgrade planned is a good idea.

drusepth commented 10 years ago

Just a quick update on this: the script in the migrate-to-sql branch is successfully converting data from one DB format to the other, but still has the minor issue of only associating one piece of content per account to that account. :lollipop: Once that's fixed, it should be good to go for smoke testing and prepping for release.

Because it is a huge migration of data, I'd also like to hold off on releasing the migration until #382 is implemented, just so we can know immediately (or at all!) if someone is hitting an error somewhere. Not a blocker on accepting the pull request, but just a blocker for the next release which would include it.

Cantido commented 10 years ago

I understand. I know it's a big migration and I feel bad for getting you started :P

Once this is merged, I'll start writing some tests again and I'm going to focus on the sort of high-level functional tests that could facilitate changes like these.

Cantido commented 9 years ago

Hows this going? Are we just waiting on #382? I've been not doing work because I'd just make a merge more complicated. I'm getting antsy to keep contributing tho

drusepth commented 9 years ago

Sorry, have been swamped outside of the repo for a little while and never finished fixing the data migration for current data. I will try and nail it down this week and move this forward; I've been itching to do some massive refactoring/cleaning now that I know way more Rails than I did when I started.

Cantido commented 9 years ago

It's all good. I understand. Once this is out of the way I can continue writing the Capybara tests which should cover a majority of functionality, and that would make the refactoring safer. I'm having some trouble having Coveralls track the code covered by the Capybara tests, it's only seeing the Test:Unit tests.

drusepth commented 9 years ago

The data migration script is done: it dumps the current live data from mongoHQ and populates an sqlite db that can be swapped in with a rails restart. Code is here: https://gist.github.com/drusepth/6f72a4e1d2e248b6415d

Looks like there needs to be a little work done in ApplicationController and a few other places to migrate away from expecting to use Mongo and actually using sqlite (like having a config/database.yml file instead of config/mongoid.yml, etc), but not much.

Hopefully going to get this train back on the rails now that the script is done.

Cantido commented 9 years ago

Great to hear! I still feel bad for causing you to do so much extra work to make this migration. I hope running on SQL makes things as easy as I imagine it will!

drusepth commented 9 years ago

I'm going to go ahead and close this (and the other) pull request now that the code is actually in sqlrails4, and open a PR from it into master to track the few remaining issues. :+1: