Skip to main content
30

Contributing To Open Source: Fixing A Bug In Devise

Episode 92 · October 27, 2015

See how I discovered and fixed a bug in the Devise open source project

Debugging


Transcripts

What's up guys? This episode we're going to take a look at fixing a slight bug or usability thing in the devise gem that I discovered last night, I created a new rails app, rails new user_manager, let's just call it something really basic and built the example that we've got here. Devise has the ability to scope your views, so let's install devise and take a look at how this works. If we just add devise in here and we run bundle to install it, then we can run rails server rails g devise:install, and then rails g devise User so we actually have devise models. Now, the views for devise, like the log in and registration pages are all stored inside the gem, and when you run the rails g devise views it will make a copy of those inside your rails app, or that it can override them, and then it will use your views instead. That way you can customize them and make them look pretty, but they also provide the ability for you to have multiple folders of those views, so let's say we generate another devise user, called admin user, and we want it to have different login pages for both of those, and they should look different as well.

That's where scope to views come in in devise. Devise allows you to edit their config in devise.rb, and you can look for scoped views, basically just turn this to true, and rather than looking for the devise sessions, it will look for the user's sessions new template instead. Simply setting that and restarting your rails server will change the way that that works, and then you can create those views in these folders properly by saying rails g devise users, and that will create the user's folder and put those in, so you get app/views/users and devise will just know what to do. The problem here is that if you're going to make these scoped views, for example we go up here and we did rails g devise user, this is not plural, and this is actually plural. What would be nice is if we could put in the non-plural one and it would automatically pluralize it, and that's what I ran into as a bug, where I did something like this where I said rails g devise AdminUser or something like this and it actually straight up uses this word as the folder name for the views, whereas the generation for the model automatically converts it to the underscored name and all of that, we should actually have the same functionality in the generate views, and allow you just to say: Let's type in user or admin user, and we'll automatically convert it to the same thing, because what happens is when you do this and you get app/views/AdminUser, devise doesn't actually use that, it's still going to look for app/views/admin_users/ whatever, so what you would actually need to do is rails g devise:views admin_users, so it's just a little bit of a gotcha, and it's something that has been around for a couple years, it appears.

If we go to GitHub, we go to the devise repository, I just created a new issue for it last night and asked basically: Hey, I saw that other people had this problem, I ran into it again last night, and I was just curious, would it make sense to fix this bug? And Jose, the author of this bug was like: Sounds awesome. So that's what we're going to do in this episode and that was the background of what happened, so basically, these other people complained about the same thing, and it was kind of a hard thing to debug, because it was one of those pluralization errors that you run into that is just kind of troublesome, so that is the goal for this episode, we're going to clone devise, we're going to edit the generator and we're going to fix that in order to match the same functionality so that you'll be able to type rails g devise:views admin_user and it will, in both cases convert that to the plural admin_users for the folder name as you would expect, and we're going to fix that bug and then submit it up to him to take a look at, and hopefully include that in the next release of devise. So let's get started.

All right, so first thing's first, you're going to want to hit the fork button on devise, and then you're going to fork their code, you'll have a copy for yourself, you can clone this to your machine, make updates to it, and then send a pull request to devise to fix that issue. I'm going to copy this url down and we're going to say git clone, that url and we'll have a copy of devise that we can play with. The key thing that we're looking for is the generator, so first we want to see how the default generator works, looks like our lib generators folders what we're going to look at, and my guess it that the devise folder is going to have the generators for the devise commands. As you'll notice, let's go back over to this one, the rails app that we just built, we ran rails g devise:install, and so that would invoke the devise install generator file, and then there's a devise generator which will be the default generator, which we used to say things like rails g devise AdminUser this is the one that does the magic in converting AdminUser in camel case into underscores and pluralizing, so let's take a look at that one. That's this devise generator and at a quick glance, there's nothing here, hardly, and we do notice that there's devise for plural name, which if we open up our routes in that app, you'll see that this actually generating what we wanted, it took AdminUser and make it underscored and pluralized, so this plural name comes from somewhere, it's not defined as a method or anything, so that has to come from either the inherited class or the include, and my guess is going to be in the name based rails generator inherited class that we used, so let's pull that up and take a look and see what we've got. Here's the plural name method, and the source code for this is extremely simple, it takes a singular name and it pluralizes it, and that's it, there's nothing else to it. Now, the thing that we're missing here, is that the singular name is not defined anywhere, you can see the singular table name and that's it, but the singular name, not sure where it comes from, so let's see if we can find it.

There's singular_name, so somewhere this attribute singular_name is being set as an attribute reader, maybe it's when this named base gets instantiated, or something like that. Somewhere in this code, we could dive into it even more if we wanted to, to figure out how that's done, but it looks like it's just calling the regular rails ActiveSupport pluralized method on the string, so let's just take a look at our views generator now, because this is the one that we need to fix, so the views generator is a good chunk of code here, and this is the piece that we need to figure out how we're going to define weather it goes into the devise folder or the scoped views, so when we do rails g devise:views AdminUser we want to take the scope here and use that as the folder name after we've converted it. It looks like this argument scope is what defines this, so this should allow us to go through and take a look at the code and see what we've got. As you can see here, this target path is actually super useful, so it says:

def target_path
  @target_path ||= "app/views/#{scope || :devise}"
end

So if you did pass in a scope, then that means it's going to use admin user, otherwise it's going to use devise, this actually seems to be perfect. That scope argument is exactly what we're looking for, now we need to modify this because that is the thing that we're trying to change instead of using the scope as it was directly passed in, we want to underscore and pluralize it, and looking at this here, this is just a module, and it's not inheriting from name base or anything, so we won't have access to the pluralize thing, and we have some classes for shared views generators, but it's just inheriting from base, not named base, and it's not doing too much, so none of these are really inheriting from something that would give us that plural name method from the other devise generators, so we may have to build our own plural scope, and also you can see these are all including this view path templates, which is that module we were looking at here, so if we look at how far this goes down, it includes these view directories and target paths. I think what we can simply do here is we can say: Let's make a plural scope method, and our plural scope, we will cache to the instance variable, and we will say: This is the scope underscored and pluralized, so then we can change our scope here to plural_scope

def target_path
  @target_path ||= "app/views/#{plural_scope || :devise}"
end

Let's see where else it is mentioned in this file. Basically we don't want to change that one because it's just checking t see if the argument exists, so that one is fine, we can leave that. Any other file references we should change though. Let's look through this file some more, here's one. This marker be generator, we should also change, and this plural scope will be available because we're including the view templates module, in each of these. That looks like it, there's no other references here although this is the actual views generator, so it's defining the argument of scope here, and it's also not including the module, but it's invoking the shared views generator, so then this is what you would call when you run rails devise views, and that delegates basically to the shared views generator, and then at some point these other ones will be invoked I'm guessing, so without having to understand how all of this is connected, we can make that change and we're probably going to be ok. This is all we changed, we've changed this line, we added a new method, and we also changed the plural scope down here. Let's save that, and then update our rails application with the new gem reference, so let's do this, go into the gem file, change this from ge 'devise' to gem 'devise', path: "/Users/chris/code/devise", close this and run bundle to reference devise from the folder on our computer instead of the ruby gem. Now if we were to run rails g devise:views AdminUser, this should use our new code, it shouldn't break and it should generate the other folder name instead, so let's run this, cross your fingers, and it worked. This, as you can see took AdminUser as a class camel case name, and then converted it to the underscore and pluralize version of it and generated all of those accordingly, so that's pretty cool, and that was the only changes we needed to do to make this fix and save a handful of people a little bit of time running into that bug, because last night I thought I was going crazy when it wasn't working and it was just because I had not pluralized it, and wasn't even paying attention. Let's run rake in the devise folder so that we can run their tests and we can just make sure that we didn't break anything, so first, to do that we need to run bundle to install all of the gems for running their tests, and then we can run it again to make sure that they run correctly. Now, our tests will be running for devise, and we've immediately got a few errors. That means that we may have broken something or we need to adjust the test accordingly to make sure that they handle that, new method as well. Let's take a look and see what broke.

undefined method underscore for nil:NilClass, basically we're calling the underscore method on Nil, which means that this has happened inside the piece of code we just wrote, so there's some cases where we've called plural_scope and scope is nil, and we're trying to underscore nil and it's not a string, so of course that doesn't work. That throws in some wrenches where we have to make sure that we make sure that we check to see if there's a scope more often inside of our plural scope method, so let's see what we can do with this. The solution to this without adding a bunch of if statements in there anything complicated is to use a trick that I think I've shown in the past with the present method. Let's take a look at how to do this in the terminal real quick, and you'll see what I mean when I talk about this, so imagine your scope is AdminUser, and we need to check to see if the exists, so if it doesn't exist, we also need this plural method to return nil, so we can do scope.underscore.pluralize and that is all fine, but if we set scope = nil then running that again gives us that error. How do we go clear this up??

We can take this method and we can say scope.presence && scope.underscore.pluralize, the beauty of this is that when we run this, scope.presence will fail early and it will return nil and will never even try to run the scope.underscore.pluralize method, and if we go and set scope to a string, and it does exist, then it is present, then we get the string that we want. This is a cool hack for you to basically say: Let's check to see if the scope is present, and if it is, then we're going to return the pluralize version, and then, if it's not present, then we're going to return nil. This is a sweet little trick that you can do to basically remove if statements in your code and just kind of run this in line in a way that's pretty readable. Here we're going to do that, and that should fix our tests, so let's clear this and run rake again and see what we get this time around. Cross your fingers and that will fix the bug.

They are in random order so we can't expect to see the errors immediately, so that seems to maybe have done the trick. Awesome, it did. That presence check is really cool. Definitely look at the source code for the presence method in rails, I'll include a link to that in the show notes just so you can take a look at how that works, but it is going to be doing a check to see if it exists behind the scenes, but you don't have to see that in your code, which is part of the beauty of all the methods and helpers that rails gives you, so that your code can be a little more declarative instead of having to have conditionals like that. That is the final solution, and let's do a git diff to see what we've changed. So that added plural_scope here, and we've added the new plural_scope method, we have plural scope one more time, and that's it. We haven't made any more changes to it, all the tests are still passing, and that is all we've got. Here I'm going to make a git commit and what we're going to do is go back to our original devise issue. That is going to be the issue that we're going to fix, and we can make our commits and say git commit -m "Automatically underscore and pluralize scoped views generator. Fixes #3790" So once they merge this commit into the master branch on theirs, then we'll be able to see that ticket automatically get closed. I'm going to commit that, actually, I have to do git add . then you can commit it. Then you can push it up to GitHub, and we'll create a pull request so that we can close this ticket.

Ok, now on GitHub, we can click the green button there to create a pull request, and we're going to take our commits from my branch of master. I should have made a feature branch or a bugfix branch rather than just committing this on master, but we can talk about that in the future, and we're going to submit this to devise master. All we have to really do is click "Create pull request".

We'll explain this a little bit better so that other people can see this and how it works. We will say "example of previous functionality".

This is just for anyone else reading this in the future, I think they'll understand if we didn't do all of this, but it's just nice for everybody to take a look at and see those changes and understand what was actually going wrong.

That seems good enough, it should be pretty self-explanatory then, and we'll just create the pull request, and hopefully that will all get taken care of. That should be that, this is going to be hopefully fixed in the next version of devise and we can have another open source commit on our resume basically. If you find any of these things like this in open source projects that you're using that seem unintuitive or something, open an issue and ask if that's intended or if it's broken, and if you can make a pull request for it. More than likely you'll get a response really fast from the author, saying: "Cool, I'd love for you to fix that, go ahead and do that, send me a pull request, I'll review it, and then merge it in". They're more than happy to help you with that stuff, but it's often good to actually ask to see if they want that to be fixed. Sometimes it's intentional and designed that way, and when you get people making pull requests and you have to turn them down because you don't want to make it work the way they want it to work, that can get kind of frustrating, so be polite, ask for advice and permission, it's not your project. If you can help them out, then awesome, do the right thing and find out if they actually want that to be working that way, and that's it. I hope you enjoyed this episode, and it gives you some insight into some things that you might notice using some other gems and be able to patch and fix on your own. I will talk to you in the next episode. Peace

Discussion


Fallback

Awesome post! I was just looking around today to see how to go about contributing open source projects!


Fallback

This is awesome you contributed to so popular open source project, but it is a good and recommended practice to provide a basic test of the functionality you have fixed. I mean a test that was failing and your fix made it pass. In that and only that way you can ensure that future changes to this project (Devise) will not break your fix. :)

Fallback

Absolutely correct on that. Normally I would write a test for it, but this is not core functionality to Devise and tests for little tiny things like this don't really add much other than slowing down your test suite. There are still tests that make sure the generators still run correctly so it still works. If the functionality ever got reverted back to the original on accident, it wouldn't be a big deal. You could certainly add a test, but I'd argue how much value you actually get out of it in this case.

Fallback

I understand your point for this particular case :) So on second thought, I agree with you!

Fallback

I meant to mention this in the video so thanks for bringing it up! :)


Fallback

Beautiful Chris! The `presence` method trick was really nice too. Thanks for this amazing contribution.

Fallback

While watching, I was wondering what the point of calling `presence` was, when you can just use the variable name alone for that trick. It turns out `presence` also returns nil for an empty string, which makes sense in this case.

http://stackoverflow.com/qu...


Fallback

very usefull. thanks. just did my first little pull request.


Login or create an account to join the conversation.