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.

Sign up for GitHub

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

[WIP] Change url companies param on :show route to name attribute #594

Closed
wants to merge 7 commits into from
Closed

[WIP] Change url companies param on :show route to name attribute #594

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fd9cca2
Change url companies param on :show route to name attribute
DouglasLutz Mar 4, 2020
b4d66f0
Change template link to new url
DouglasLutz Mar 4, 2020
87168dd
Add slug field to company
DouglasLutz Mar 5, 2020
d093ae0
Remove data migration
DouglasLutz Mar 9, 2020
30a35a0
Change index to unique index
DouglasLutz Mar 18, 2020
014a304
Change slug generation
DouglasLutz Mar 18, 2020
6288667
Remove get_by_slug! methot/change get! method to accept slugs
DouglasLutz Mar 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions lib/companies/companies.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,14 +90,18 @@ defmodule Companies.Companies do
** (Ecto.NoResultsError)

"""
def get!(id, opts \\ []) do
def get!(key, opts \\ []) do
preloads = Keyword.get(opts, :preloads, [])

from(c in Company)
|> preload(^preloads)
|> from()
|> where([c], is_nil(c.removed_pending_change_id))
|> Repo.get!(id)
query = from(c in Company) |> preload(^preloads) |> from() |> where([c], is_nil(c.removed_pending_change_id))

final_query =
case Integer.parse(key) do
:error -> where(query, [c], c.slug == ^key)
{int_id, _remainder} -> where(query, [c], c.id == ^int_id or c.slug == ^key)
end

Repo.one!(final_query)
end

@doc """
Expand Down
23 changes: 22 additions & 1 deletion lib/companies/schema/company.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,15 @@ defmodule Companies.Schema.Company do

alias Companies.Schema.{Industry, Job, PendingChange}

@derive {Phoenix.Param, key: :slug}
Copy link
Member

Choose a reason for hiding this comment

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

I like this :)

schema "companies" do
field :blog, :string
field :description, :string
field :github, :string
field :location, :string
field :name, :string
field :url, :string
field :slug, :string

belongs_to :industry, Industry
has_many :jobs, Job, defaults: [removed_pending_change_id: nil]
Expand All @@ -25,7 +27,26 @@ defmodule Companies.Schema.Company do
@doc false
def changeset(company, attrs) do
company
|> cast(attrs, [:name, :description, :url, :github, :location, :blog, :industry_id])
|> cast(attrs, [:name, :description, :url, :github, :location, :blog, :industry_id, :slug])
|> validate_required([:name, :description, :url, :industry_id])
Copy link
Member

Choose a reason for hiding this comment

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

The slug is unique in the database by a constraint but this constraint is not handled in the changeset.

Another thing to deal with is how pending changes are managed in the code. When a company is created a pending_change is inserted in the database. When that change is approved, the company will be inserted in the companies table and could fail on the uniqueness constraint.

It would be preferable imo to deal with duplicate slugs earlier, perhaps by adding an unsafe_validate_unique and adding postfixes to the slugs to make them unique.

This would not entirely be foolproof (e.g. multiple pending changes could have the same slug) but could go a long way.

Copy link
Collaborator

@gemantzu gemantzu Mar 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

we could add the company id in the slug just to be safe. e.g. Plataformatec turns into 1-plataformatec (the id here is random).

Copy link
Member

Choose a reason for hiding this comment

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

Crazy idea feel free to shoot this down: what if we don't enforce the uniqueness in the changeset (or we create a changeset without it) and instead defer the work of creating unique slugs onto admins approving changes. In the UI to approve a company we could show whether the slug was already taken and require the admin to make a unique one.

Copy link
Member

Choose a reason for hiding this comment

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

@maartenvanvliet / @gemantzu how do you both feel about moving generation of things like slugs (or password hashes) into the changeset pipeline like such?

def changeset(company, attrs) do
  company
  |> cast(attrs, [:name, :description, :url, :github, :location, :blog, :industry_id, :slug])
  |> validate_required([:name, :description, :url, :industry_id])
  |> generate_slug()
  |> unique_constraint(:slug)
end

# when there is an existing non-nil `id` in the data, we don't want to regenerate a slug even on name change
# that would break any potential backlinks.
defp generate_slug(%Ecto.Changeset{data: %{id: id}} = changeset) when not is_nil(id) do
  changeset
end

defp generate_slug(%Ecto.Changeset{changes: %{name: name}} = changeset) do
  slug =
    name
    |> String.replace(~r/['’]s/u, "s")
    |> String.downcase()
    |> String.replace(~r/([^a-z0-9가-힣])+/, "-")
    |> String.replace(" ", "-")

  put_change(changeset, :slug, slug)
end

|> generate_slug()
|> unique_constraint(:slug, message: "Company slug already exists")
end

defp generate_slug(%Ecto.Changeset{data: %{id: id}} = changeset) when not is_nil(id) do
changeset
end

defp generate_slug(%Ecto.Changeset{changes: %{name: name}} = changeset) do
slug =
name
|> String.replace(~r/['’]s/u, "s")
|> String.downcase()
|> String.replace(~r/([^a-z0-9가-힣])+/, "-")
|> String.replace(" ", "-")

put_change(changeset, :slug, slug)
end

defp generate_slug(changeset), do: changeset
end
16 changes: 8 additions & 8 deletions lib/companies_web/controllers/company_controller.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,20 +37,20 @@ defmodule CompaniesWeb.CompanyController do
end
end

def show(conn, %{"id" => id}) do
company = Companies.get!(id, preloads: [:jobs, :industry])
def show(conn, %{"slug" => slug}) do
company = Companies.get!(slug, preloads: [:jobs, :industry])
render(conn, "show.html", company: company)
end

def edit(conn, %{"id" => id}) do
company = Companies.get!(id)
def edit(conn, %{"slug" => slug}) do
company = Companies.get!(slug)
changeset = Companies.change(company)
industries = Industries.all()
render(conn, "edit.html", company: company, changeset: changeset, industries: industries)
end

def update(conn, %{"id" => id, "company" => company_params}) do
company = Companies.get!(id)
def update(conn, %{"slug" => slug, "company" => company_params}) do
company = Companies.get!(slug)

case Companies.update(company, company_params, current_user(conn)) do
{:ok, _company} ->
Expand All @@ -64,8 +64,8 @@ defmodule CompaniesWeb.CompanyController do
end
end

def delete(conn, %{"id" => id}) do
company = Companies.get!(id)
def delete(conn, %{"slug" => slug}) do
company = Companies.get!(slug)
{:ok, _company} = Companies.delete(company, current_user(conn))

conn
Expand Down
4 changes: 2 additions & 2 deletions lib/companies_web/router.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,13 @@ defmodule CompaniesWeb.Router do
scope "/" do
pipe_through [:auth]

resources "/companies", CompanyController, except: [:index, :show]
resources "/companies", CompanyController, except: [:index, :show], param: "slug"
resources "/jobs", JobController, except: [:index, :show]
resources "/users", UserController, only: [:edit, :update]
end

get "/", CompanyController, :recent
resources "/companies", CompanyController, only: [:index, :show]
resources "/companies", CompanyController, only: [:index, :show], param: "slug"
Copy link
Member

Choose a reason for hiding this comment

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

Just to be aware of this, the old url's based on integers would no longer work with this change. We could implement a fallback but this is also fine.

get "/jobs", JobController, :index
get "/profile", UserController, :profile
get "/for_hire", UserController, :for_hire
Expand Down
11 changes: 11 additions & 0 deletions priv/repo/migrations/20200305171000_add_slug_to_companies.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
defmodule Companies.Repo.Migrations.AddSlugToCompanies do
use Ecto.Migration

def change do
alter table(:companies) do
add :slug, :string
end

create unique_index(:companies, [:slug])
end
end
1 change: 1 addition & 0 deletions priv/repo/seeds.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ technology_consulting = Repo.insert!(%Industry{name: "Technology Consulting"})
plataformatec =
Repo.insert!(%Company{
name: "Plataformatec",
slug: "plataformatec",
description: """
Project inception, coaching, tailored projects, general consulting. Sponsor of Elixir, employer to Elixir's BDFL.
""",
Expand Down
15 changes: 8 additions & 7 deletions test/companies/companies_test.exs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,21 +66,22 @@ defmodule Companies.CompaniesTest do
end

describe "get!/2" do
test "retrieves a company by id" do
%{id: id} = insert(:company, name: "ZULU")
test "retrieves a company by given key" do
%{id: id, slug: slug} = insert(:company, name: "ZULU")

assert %{id: ^id} = Companies.get!(id)
assert %{id: ^id} = Companies.get!("#{id}")
assert %{id: ^id} = Companies.get!(slug)
end

test "preloads given associations" do
company = insert(:company, name: "ZULU")

assert %{jobs: []} = Companies.get!(company.id, preloads: [:jobs])
assert %{jobs: []} = Companies.get!("#{company.id}", preloads: [:jobs])
end

test "raises for unknown id" do
assert_raise Ecto.NoResultsError, fn ->
Companies.get!(1000, preloads: [:jobs])
Companies.get!("#{1000}", preloads: [:jobs])
end
end
end
Expand All @@ -89,13 +90,13 @@ defmodule Companies.CompaniesTest do
test "retrieves by id" do
%{id: company_id} = insert(:company)

assert %{id: ^company_id} = Companies.get!(company_id)
assert %{id: ^company_id} = Companies.get!("#{company_id}")
end

test "does not retrieve deleted record" do
company = insert(:company, %{removed_pending_change: build(:pending_change)})

assert_raise Ecto.NoResultsError, fn -> Companies.get!(company.id) end
assert_raise Ecto.NoResultsError, fn -> Companies.get!("#{company.id}") end
end
end

Expand Down
1 change: 1 addition & 0 deletions test/support/factories.ex
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ defmodule Companies.Factory do
description: "A test company",
industry: insert(:industry),
name: sequence(:name, &"Test Company #{&1}"),
slug: sequence(:slug, &"test-company-#{&1}"),
url: "www.example.com"
}
end
Expand Down