MIM-864 mongoosepush exposes phoenix initial endpoint #144
Conversation
There was a problem hiding this comment.
Thanks for the PR! In general, the code does what it suppose to, just few minor things to change, mostly formatting. I'm not sure if mix format has been used here, if not, let's do so.
Quite a few files are missing newline character at the end of last line - if that was intentional, I recommend this read: https://unix.stackexchange.com/questions/18743/whats-the-point-in-adding-a-new-line-to-the-end-of-a-file :)
config/dev.exs
Outdated
| debug_errors: true, | ||
| code_reloader: false, | ||
| check_origin: false, | ||
| server: true |
There was a problem hiding this comment.
It was unintentional and corrected with mix format. Anyway thank you for comprehensive reading :)
lib/mongoose_push/application.ex
Outdated
| @@ -32,7 +32,7 @@ defmodule MongoosePush.Application do | |||
| MongoosePush.Telemetry.attach_all() | |||
|
|
|||
| # Define workers and child supervisors to be supervised | |||
| children = children() | |||
| children = children() ++ [MongoosePushWeb.Endpoint] | |||
There was a problem hiding this comment.
Maybe let's rename children to service_children or something like that in order to indicate that those are not "all possible children" anymore.
| @@ -0,0 +1,7 @@ | |||
| defmodule MongoosePushWeb.DummyController do | |||
There was a problem hiding this comment.
This file uses 4 spaces indentation, while other use double. We should normalize to double-spaces.
| def handle(conn, params) do | ||
| json(conn, %{body: params}) | ||
| end | ||
| end |
There was a problem hiding this comment.
Missing end-of-line character.
lib/mongoose_push_web/endpoint.ex
Outdated
| # | ||
| # You should set gzip to true if you are running phx.digest | ||
| # when deploying your static files in production. | ||
| plug Plug.Static, |
There was a problem hiding this comment.
I don't think we need any static data, so we can remove this plug.
| @@ -0,0 +1,13 @@ | |||
| defmodule MongoosePushWeb.DummyControllerTest do | |||
There was a problem hiding this comment.
This file uses 4 spaces indentation, while other use double. We should normalize to double-spaces.
| conn = post(conn, "/api/dummy", [key: "value"]) | ||
| assert json_response(conn,200) == %{"body" => %{"key"=>"value"}} | ||
| end | ||
| end |
There was a problem hiding this comment.
Missing end-of-line character.
| @@ -0,0 +1,2 @@ | |||
| {:ok, _} = Application.ensure_all_started(:mongoose_push) | |||
| ExUnit.start() | |||
There was a problem hiding this comment.
Missing end-of-line character.
There was a problem hiding this comment.
Also, it's recommended to use ExUnit.start(capture: true) in order to hide logs as long tests are green.
There was a problem hiding this comment.
Thanks for the changes @leszke! I left some comments to parts of the code which I think is not required by MongoosePush and a question regarding Phoenix's JSON library.
lib/mongoose_push_web/endpoint.ex
Outdated
| @session_options [ | ||
| store: :cookie, | ||
| key: "_mongoose_push_key", | ||
| signing_salt: "URIBGSwL" | ||
| ] |
There was a problem hiding this comment.
From what I understand, we don't need this in MongoosePush since there is no session needed.
There was a problem hiding this comment.
@rslota what do you think?
There was a problem hiding this comment.
Yeah, it seems that we can remove this along with entire Plug.Session.
lib/mongoose_push_web/endpoint.ex
Outdated
| plug Plug.Static, | ||
| at: "/", | ||
| from: :mongoose_push, | ||
| gzip: false, | ||
| only: ~w(css fonts images js favicon.ico robots.txt) |
There was a problem hiding this comment.
This one is also not needed. We'll probably don't have static files to serve.
There was a problem hiding this comment.
@rslota what do you think?
There was a problem hiding this comment.
Yeah, also left comment for that :)
| plug Plug.Parsers, | ||
| parsers: [:urlencoded, :multipart, :json], | ||
| pass: ["*/*"], | ||
| json_decoder: Phoenix.json_library() |
There was a problem hiding this comment.
Do we know what is the default json encoder? I don't see it configured and I think it's better to use the one which is already used in the project. As far as I know MongoosePush already uses Jason. It can be configured by adding the following line:
config :phoenix, :json_library, Jasonto config/config.exs
leszke
deleted the
mim-864-mongoosepush-exposes-phoenix-initial-endpoint
branch
From story description:
As MongoosePush Client (HTTP API user)
Given MongoosePush is running
When I send a dummy POST request to MongoosePush on Phoenix backed endpoint
Then I get dummy response
Notes:
Both HTTP and HTTPS protocols should be supported
Both HTTP 1.1 and HTTP 2.0 protocols should be supported
Temporary we need to expose those two endpoints on different ports then current API
Phoenix should be configured with no Ecto and no Views.
Phoenix Websockets should be disabled
No need for exposing new endpoints in release Docker container
Dummy Controller and dummy entry in Router should be created in order to propose initial file structure - we need to keep in mind protocol versions split etc.