Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add database of shared links #1098

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Add database of shared links #1098

wants to merge 4 commits into from

Conversation

Rachelcoll
Copy link

This is just a draft, currently there are several problems yet to fix:

  • Unable to use URL shortener at backend, cannot deploy yourls on my site that generate shortened url. Currently POST request to create new shared links will return the UUID instead

Copy link
Member

@RichDom2185 RichDom2185 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for working on this! Did a quick look, just some comments below for now.

Also, a minor nit: the name data is very generic and a name that reflects more closely to what it does (e.g. state) might be preferred.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't commit this as this is a local development setup, thanks!

Comment on lines +8 to +17
schema "shared_programs" do
field(:data, :map)
field(:uuid, Ecto.UUID)

timestamps()
end

@doc false
def changeset(shared_program, attrs) do
shared_program
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing definitions for required and optional fields (take a look at the other models in the codebase to see how they're implemented).

Comment on lines +6 to +7
add(:uuid, :uuid)
add(:data, :map)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add constraints and/or indices to this table (e.g. non-null, unique, primary key, etc.)?

@@ -210,6 +210,12 @@ defmodule CadetWeb.Router do
# pipe_through :api
# end

scope "/v2", CadetWeb do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't create a separate scope is possible, since this endpoint is grouped as part of the other routes.

@@ -210,6 +210,12 @@ defmodule CadetWeb.Router do
# pipe_through :api
# end

scope "/v2", CadetWeb do
pipe_through(:api)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should pipe through the proper authentication middleware like the other endpoints.

scope "/v2", CadetWeb do
pipe_through(:api)

resources("/shared_programs", SharedProgramController, only: [:index, :show, :create, :delete])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we supporting :index and :delete? Only create and read operations are meant to be supported right?

Comment on lines +63 to +84
defp ping_url_shortener(uuid) do
url = "https://localhost:8000/yourls-api.php"

params = %{
signature: "5eef899abd",
action: "shorturl",
format: "json",
keyword: uuid,
url: "http://localhost:8000/playground#uuid=#{uuid}"
}

case HTTPoison.post(url, {:form, params}) do
{:ok, %{status_code: 200, body: body}} ->
{:ok, body}

{:error, %HTTPoison.Error{reason: reason}} ->
{:error, reason}

other ->
{:error, "Unexpected response: #{inspect(other)}"}
end
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove hardcoding.

Comment on lines +26 to +37
# case ping_url_shortener(uuid) do
# {:ok, shortened_url} ->
# conn
# |> put_status(:created)
# |> put_resp_header("location", ~s"/api/shared_programs/#{shared_program}")
# # |> render(:show, shared_program: shared_program)
# |> render("show.json", %{uuid: uuid, shortened_url: shortened_url})
# {:error, reason} ->
# conn
# |> put_status(:internal_server_error)
# |> json(%{error: "Failed to shorten URL: #{reason}"})
# end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commented code should either be explained with a comment or removed if unused.

Comment on lines +17 to +19
# change to current server of source academy
# server = "http://localhost:8000"
# url =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this for?

action_fallback(CadetWeb.FallbackController)

def index(conn, _params) do
shared_programs = SharedPrograms.list_shared_programs()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should not expose this. Very dangerous and breaks privacy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants