PR Overview and Plan0:00
Today, I have a somewhat large PR that we'll work through together, Barebones Administration Interface with Channel Creation from John Sugar. So we can see here there's 9 different commits and 20 files that have been changed. So this is actually a fairly significant PR that we need to work through together. Seems like there's a couple of things we might want to tweak and clean up, but mostly we're just going through the process, trying it out, making sure it works, running the tests, and then going over every single file to make sure that everything fits properly. Okay, so with that in mind, and of course there's lots of commentary here, but anyways, why don't we pull it in through the command line. So we're going to check out a new branch and then pull in John's fork.
Pull PR and Inspect Routes0:37
why don't we pull it in through the command line. So we're going to check out a new branch and then pull in John's fork. So now we have that. So you can see he's added an admin namespace here with a DashboardController and then a specific ChannelsController. So if we take a look at that DashboardController, yeah, here's what we have at this point. But now I want to take a look at this in the browser, of course. So if we go to the routes file, it looks like this has been added. All right, so we have a prefix of admin, so that means if I hit console.test, that'll take us to a DashboardController.
Test Admin Access in Browser1:07
All right, so we have a prefix of admin, so that means if I hit console.test, that'll take us to a DashboardController. All right, let's give this all a shot. I will pull up our forum where I'm signed in as myself, and we're going to go to /admin, and immediately we get an HTTP exception. You don't have permission to perform this action. So let's see what the deal is, and I assume it's permission related. Yeah, so all of these routes are bound to the admin middleware, and we can see here if you're signed in and you are in fact an admin, then we can continue on with the request. Otherwise, we're going to abort.
if you're signed in and you are in fact an admin, then we can continue on with the request. Otherwise, we're going to abort. So let's see if he's changed the logic for what determines an administrator. So we're going to look at the user's email address, and we'll figure out is that within the array that we get from this configuration file that's new. Okay, so let's see, config, this file's new. At one point I added this myself, but then I deleted it, but we will definitely need this for a number of things. So it's useful that he added this. Anyways, so add the email addresses of users who should be administrators here.
Does it match up with the style that I prefer? All of these things are important. Otherwise, if you don't create some consistency there, it's going to be very clear from looking at the code base that a hundred different completely opposing viewpoints and mindsets have contributed to the project, and it ends up being a mess. So as the maintainer, it's really important that you kind of keep the code close. You make sure that any PRs you do merge conform to a basic idea of how you think of code. All right? And of course, on the contributor's end, it's important for them to adopt the style guide of the project.
Fix Migration and Refresh DB4:38
Anyways, so you are on the administration dashboard, and then you go to the channels page, and here's all the channels there. So that means I could create a new channel for php, everything PHP, add, column not found, unknown column description. So maybe he's updated a migration to accept a description. Let's see. Yeah, he actually did. So he went back to create channels table and he updated that. Okay, that's fair. So why don't we php artisan migrate:refresh.
Okay, that's fair. So why don't we migrate, refresh. So I'm basically rolling everything back and rerunning them, or by the way, you can also say in Laravel 5.5, I think it was added, there's also one called fresh, and the only difference is we're not going to roll every migration back, or in other words, we're not going to hit the down method for every single one. We're just going to start it with a clean slate. We're going to drop everything and then begin. So you can see there, just get rid of all of them and then start from scratch, which honestly, I would say more often than not is what you want in these cases.
Give it a refresh, channels, and that is represented there, which means you should now be able to choose a channel without having to go to the database like we were doing before when we didn't yet have an admin section. All right. So this is useful. So let's go back to our review. Now I believe there are tests here, which is vital. If somebody submits a PR with tons of changes and there are no tests, it's like, well, how do I know that everything works without spending hours running through every possible iteration of the code to see if your change broke anything?
Review and Refactor Tests6:47
do I know that everything works without spending hours running through every possible iteration of the code to see if your change broke anything? So these tests really do become vital. So let's see. Where is it? All right. So he has an admin section. Administrator tests. All right. Let's go through these together.
okay. And then things like this. We would use a create function instead. So create a User and then assume that that User is an administrator. Yeah, we'll need to tweak that a little bit. And then sign in. So we're signing in a User, but then we're setting them as a logged in User again. So a few things we would want to tweak here. So I'll tell you what, here's what I'm going to do. I think this is good enough to merge with the understanding that I'm going to make a
So I'll tell you what, here's what I'm going to do. I think this is good enough to merge with the understanding that I'm going to make a couple little tweaks. So if we come back to the command line instructions, we can merge it in by checking out our master branch, merging in John's repo, and then we could push it back to GitHub if we want, but I'm not quite ready for that. So let's just merge it in. Oh, whoops. We have to fix this. Let's just say, git checkout that file.
We have to fix this. Let's just say, git checkout that file. Okay. So once again, let's check out the master branch and merge it in. Okay. So now we're back on master with those changes, and at this point, I can just do style guide type stuff. All right. So for example, here, if we run that, it's going to return green. In this case, we're signing in twice, so I should be able to get rid of that entirely.
So for example, here, if we run that, it's going to return green. In this case, we're signing in twice, so I should be able to get rid of that entirely unless there's something I'm not seeing. Yeah, we still get green there. So you can put that on its own line. Next up here, he may not know this, but we have a helper called create that we can use, and that'll still return green. Next, we could use signIn, but actually in this case, acting as an administrator reads nicely to me. So why don't we get rid of that?
nicely to me. So why don't we get rid of that? And then at this point, I would put them on their own line. Acting as an Administrator, get to the admin page, and then assert that we have a 200 status. So this is pretty common knowledge, but it is nice to use the constants for a little more clarity. So if we're going to set it up that to create an Administrator requires a number of steps, we might want to add a little helper function for this. Like a method like signInAdministrator would do this for you so that you don't have to keep running this config call every single test.
Like a method like signIn Administrator would do this for you so that you don't have to keep running this config call every single test. Anyways, a non-Administrator cannot access, so these can be very useful to ensure that nobody gets access to your administration page without permission. So in this case, we're just saying, well, given you have some random User and they try to access it, nope, we're not going to let them. And we run that, we get green. Now one thing, we could swap this out for the named route. So admin-board.index, that's fine. Run that, still green, and then do the same thing here.
So admin-board.index, that's fine. Run that, still green, and then do the same thing here. Not a big deal either way, but if we change the routing, that way we don't have to update all the tests. Okay, so I'll do a quick auto-format there, and that looks pretty good to me. Although, I don't like a lowercase namespace, so I'm going to update that. So now we've added that, and then this one we'll set as well. And we can get working on this. Alright, so let's do an auto-format, looks like we had an unused import there we got rid of.
Alright, so let's do an auto-format, looks like we had an unused import there we got rid of. An Administrator can access the channel administration section. So once again, sign in an Administrator. So all these tests are doing is just making sure you get a 200 response. So if you are at the correct type, you can access that page. And this can actually be pretty useful. So let's update to start the named route, admin-channels-store. Still green. Yeah, whenever you're making changes like this, just get in the habit of running your
Still green. Yeah, whenever you're making changes like this, just get in the habit of running your tests over and over. So in this case, we have another duplicate sign-in. Run it, that didn't break anything. That gives you some pretty useful rapid feedback there. Next, that one seems fine to me. An Administrator can create a Channel. Alright, so in this case, he's got a helper create-channel. Yeah, so you see this repetition everywhere.
Alright, so in this case, he's got a helper createChannel. Yeah, so you see this repetition everywhere. We keep doing kind of the same three lines of code. This would be kind of a prime candidate for a helper function. Nonetheless, we're just going to take everything step by step. So sign in an Administrator, make a Channel with the given overrides, and then submit a POST request to admin-channels. Once again, that's what we need. Actually, you know what? I think I made a mistake.
Actually, you know what? I think I made a mistake. Right here would actually be admin.channels.index, because we're making a GET request. It ultimately ends up being the same URL, and that's why it's still passed. But anyways, where are we? Here we go. So make me a channel, and then submit a POST request to this given route, and then pass through the attributes for the channel. Okay. So we're back up.
Okay. So we're back up. Let's keep going. So create a Channel. In this case, we're creating a Channel, getting the redirect location, and, okay, we're just asserting some things on the page. The Channel requires a name, so we have some validation. This can be useful. I like these. So create a Channel, and if there is no name, then we expect name to be within the errors.
I like these. So create a Channel, and if there is no name, then we expect name to be within the errors variable. So for example, if we go to ChannelsController right here, and maybe we've removed that entirely, then our test is going to blow up, and it's going to let us know, nope, you need to make sure you add the necessary validation rules, and, of course, the same will be true for a description. Okay. This seems fine. Let's do a refactor.
This seems fine. Let's do a refactor. I'm sorry, a reformat. This still needs to be extracted, so let's see. Maybe we can have a method signInAdministrator. So we would accept an optional User, but if you don't give us one, we'll just quickly whip up a User, and let's go back. We need to update the config as part of that, and, you know what, why don't we rename this to admin, like so, and let's put this up. Okay.
to admin, like so, and let's put this up. Okay. I think that looks okay to me, which means, well, let's try this out. Let's run everything here. All of those are passing, but if I were to comment that out, everything's going to fail, of course. But I can now say signInAdministrator, and if we run it, now we're back to green, which means we should be able to clean things up in a couple of places, like this section right here. We keep seeing that repeated over and over.
What else? Okay. That seems fair. Let's close all this out. Let's go back to administrator test. Same thing here. So signInAdmin, run it. Back to green. Okay. Let's go to GitHub and see what else, because I know this is a little boring to watch.
Oh, I know why he's doing this. So he's just setting it up. So if you are an administrator, you're not bound by this rule, which is perfectly fair. User is admin, I assume that's to make it available to the JavaScript at some point. What else? ModelFactory. So sections like this, let's see. And this was just commented out because we're doing it a different way. That's fine. What else?
That's fine. What else? So we add a select description to the channel. That's fine. Update the migration. Update the README. Okay. Authorizations is no longer duplicating. Okay. Fair enough.
Merge PR and Update README15:34
not bound by time. And then we'll push all of this up to GitHub and we'll now have an administration section for the forum. All right. So if I fast forward, I've made some little tweaks, mostly formatting, and I have merged the pull request. And also as part of that, I updated the README.md. So you can see this is what we have at this point. git clone counsel, install your dependencies, run that new php artisan command that we reviewed, compile your front end dependencies.
Clone counsel, install your dependencies, run that new artisan command that we reviewed, compile your front end dependencies. And then here's our new step. So now you don't have to manually create a channels table. You can do that directly from the admin panel, which is pretty cool. So that takes care of that. In the next episode, let's take care of something else.
