WIP - Transfer user controller to json

[troym9731]

Closes #85

Thought I'd throw this up to track some progress.

Also, getting a weird error when I try to run:

mix test test/controllers/user_controller_test.exs:8

...

** (ArgumentError) Postgrex expected an integer in -2147483648..2147483647, got <<14, 127, 114, 184, 132, 121, 75, 108, 137, 99, 238, 177, 93, 36, 193, 201>>. Please make sure the value you are passing matches the definition in your table or in your query or convert the value accordingly.

Based on the error, I would assume I'm not passing an integer somewhere, but I can't tell where. I can investigate again later, but if you could run it once locally to let me know if you get the same error, that would be helpful.

[troym9731]

Also, I'd like to draw attention to the fact that I removed created_at. I prefer created_at over inserted_at, but that's the default for Ecto. Now we can rename it if we'd like, but then we'd have to remember to do so for all tables. I think it is simpler just to live with inserted_at.

[therealklanni]

Will review more indepth after work.

[therealklanni]

Actually, this is here because it keeps a record of when the github user was created. I figured it would be useful in case we ever want to detect if someone just created an account 10 minutes earlier to get access.

[troym9731]

Oh, guess I should've asked first. How about github_created_at for a little more clarity?

It doesn't need a too in-depth review, it's a WIP. Forgot to put that in the PR title.

[therealklanni]

I think I named it that way because that's how it's named in the github response, so for ease of use. I don't mind changing it, but we have to consider how it will affect all our existing user data.

[troym9731]

I'll just revert this and add a comment in the model line explaining that it is the github user created_at date 👍

[troym9731]

Could you hop on this branch, run
MIX_ENV=test mix ecto.reset
and then
mix test test/controllers/user_controller_test.exs:8
and see if you get the same error as I do in the PR description?

[therealklanni]

Yeah, I'll take a look tonight

[troym9731]

Ok, I just updated the plugs you wrote to simply be an Auth plug, where the main one dealing with the session is a module plug, but the rest are simply function plugs. Let me know what you think, and if you are ok with it, then I can delete the unused files.

I also commented out all the auto generated tests. Right now, all the tests that I wrote are passing, which means I might set up Travis next.

[troym9731]

Also, I'm thinking we should get rid of the admin controllers and just collapse it into the normal ones. Seems odd to have to hit a different endpoint if you are an admin even though admins and members will be basically be using the same app. I have an idea for a simple permission system that should work.

[therealklanni]

I don't believe the admin controllers constitute a different endpoint, they are just different controllers to handle the routes differently for admins. Check router.ex to see how they're being used. Let me know if I'm misunderstanding something.

[therealklanni]

We might want to consider making a generic error.json and insert the status code/message?

[troym9731]

This is how Phoenix wants you to structure your views. We're pattern matching on "403.json", and the .json I believe is necessary to know to encode the output as such.

[therealklanni]

I would rather stick with the generic/spec HTTP status messages, i.e. "Unauthorized" and "Forbidden" (below). As a general rule, I prefer not to be very specific about what is wrong (any more than necessary), for security reasons.

[therealklanni]

Also, we should probably work out a standard format for responses.

{
  data: ...
  status: ...
  page?: ... if we want to page results? (probably not a big concern)
}

{
  status: 401
  message: 'Unauthorized'
}

❓

Let's discuss more in Slack when you have time.

[troym9731]

Oh I'm fine with the generic HTTP status messages. I just kept what was there before when trying to access a page without being signed in. Hmmm, I wonder if we need to return the status like that? Or if Phoenix knows to return that already?

But yeah, I'm fine with formatting it with {data: ..., status: ..., message: ...}

[therealklanni]

Yeah, I don't know, I haven't looked at what the actual response looks like, so maybe Phoenix adds some stuff automatically?

[troym9731]

I don't think it's possible to have one route lead to two different actions. Every route can only belong to one action. There might be a way we can hijack that, but I'm not sure we want to. So if there are specific actions that only admins can do e.g., destroy, then that can live in an admin controller that will run through admin auth plugs to verify. But outside of that, if the admin controller has the same action like index as the normal user controller, /users can only lead to one of those controllers.

That being said, we can also just attach those plugs to a specific action in the user controller like in user_controller.ex:6, and I think that is the simpler approach.

[therealklanni]

Not sure what you mean, that's literally how it was set up when it was serving views. It's more than one "route", but for a single endpoint (e.g. /users is accessible as a "user", or with extra features as an "admin").

We may be talking about the same thing here, but you seem to think we weren't already doing this? Dunno. Message me on Slack and we can talk in more detail.

[therealklanni]

Yeah, each action can only exist once per route, but that's what the router.ex does, it tells Phoenix which routes to serve depending on the plugs that resolve. So if the authorized user is an "admin" the router points /user/1/edit to the admin/user_controller.ex controller instead of the normal user_controller for that route, which provides edit, create, update, etc, that only admins can do. Authed non-admin users can hit the non-admin update to update themselves only.

Anyway, let's talk more in Slack, it will be easier to get on the same page.

[therealklanni]

I recommend rebasing on master, I added the Heroku app.json, which is needed for "review apps".

[troym9731]

Ok, rebased