Are these merge conflicts due to needing a rebase or what? I know I need to review #805 first.

@joshsmith I'm not seeing any merge conflicts here. Did you fix them? EIther way, no point in reviewing this until we agree #805 is good to merge, since any changes to the process there will likely have to be applied to the process here as well.

There are conflicts again.

And there will be as long as #805 keeps getting modified. My vote is to leave this alone until that one is done. I've spent close to a day rebasing and resolving here, when it can't be merged anyway until we deal with #805

Also, while I don't have time right this moment to write a failing test, posting a comment gets a 403 error because plug CodeCorpsWeb.Plug.IdsToIntegers is not being used in the CommentController (and wasn't clear to me that it needed to be used). This suggests something is wrong with our testing, or even just the way we're processing IDs generally.

Unrelated – but it's only started happening here – when I run mix phx.server I now get tests running automatically every time the code changes, as though I were running mix test.watch or something. I see that mix_test_watch is set to run only in :dev, which may be part of the problem. But it makes it impossible to do debugging because the tests are just clouding the log output.

@joshsmith Regarding id's to integers, I should have written about this, but simply put, I forgot, so it's a mistake on my part. Sorry.

I encountered this about two weeks back. The phoenix API does not convert numbers in received request bodies into integers. However, elixir does not auto-cast strings to integers in any way, so comparison in those cases is troublesome.

The problem with that is that ember's json api adapter, casts all ids to strings, because, while our ids are all integers, some apis might have uuids, slugs, etc, for ids, so that's the safest approach from ember's POV.

Even for us Repo.get(User, 1) and Repo.get(User, "1") is interchangeable. It's just this specific case of comparing a record id against a param value, where the param value will always be string by default is when it fails.

That's why I concluded that the better place to handle the conversion is in the API, even though it may make sense to do it in our ember app's serializers to.

As you can see, the issue has been identified and resolved in the task pr, but I didn't get a chance to propagate it to this comment PR yet.

All that being said, even though there are arguments for implementing the fix for the issue this way, through the plug, I'm still not completely sure this is the best approach, so I'm open for discussion and suggestions.

@begedin I decided to merge this now so we can circle back to the user issues as a separate PR.