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

[DouglasLutz]

Fixes #588

@gemantzu @doomspork I'm coming from rails and don't have experience with elixir/phoenix so if there is a better way to do this I'm open for ideas :D

[maartenvanvliet]

Agreed with @gemantzu, using proper slugs would be preferable. There's no uniqueness enforced on company name either, so if we have duplicates they would result in unreachable urls.

[doomspork]

@DouglasLutz I'm glad you've decided to jump into Elixir head first and get right to contributing :tada:

I think @gemantzu and @maartenvanvliet have good points. Would you like some help making these changes or do you want to give it a shot and we can go from there?

Thanks again for getting involved, we're stoked to have you :grinning:

[DouglasLutz]

Thanks for the feedback, I made some research and think I can create a slug for the companies

[gemantzu]

@DouglasLutz need any help with this? Can we provide support somehow if you are stuck?

[DouglasLutz]

@DouglasLutz need any help with this? Can we provide support somehow if you are stuck?

@gemantzu Sorry, have been a little busy this week but yes, I got stuck in the assurance of generating unique slugs. I thought about appending a number in the slug if the slug is already in use, but this would add queries to company model and I don't know if it's a good thing to do so I think I'll take your suggestion of adding the company id to the slug.

[gemantzu]

@DouglasLutz no worries, I know how it is with work and life sometimes. I just thought I'd check in to see if there was anything I could help with. There's no rush.

If you want to push up a partial solution I'd be happy to take a look. We can bounce ideas off each other if need be but I'm sure whatever you decide on is fine.

We don't need to be too worried about performance. We can always come back and make improvements.

[gemantzu]

@doomspork a good solution to me is provided by the phoenix book.

1. we add the slug field, non-unique
2. we add the field in the ecto schema
3. we add a new pipe in the changeset pipeline for companies, where we produce the slug based on the company name (replace spaces with -, downcase, etc)
4. we create a new Phoenix.Param implementation for Company like so:

defimpl Phoenix.Param, for: Companies.Schema.Company do
  def to_param(%{slug: slug, id: id}) do
    "#{id}-#{slug}"
  end
end

Then, when we call Routes.company_path, the url provided is in the form of /companies/1-plataformatec 5. we create a new behaviour implementation for Ecto.Type for our ids like so:

defmodule Companies.Permalink do
  @behaviour Ecto.Type
  
  def type, do: :id
  
  def cast(binary) when is_binary(binary) do
     case Integer.parse(binary) do
       {int, _} when int > 0 -> {:ok, int}
       _ -> :error
     end
  end
  
  def cast(integer) when is_integer(integer) do
    {:ok, integer}
  end
  
  def cast(_) do
    :error
  end

  def dump(integer) when is_integer(integer) do
    {:ok, integer}
  end
  
  def load(integer) when is_integer(integer) do
    {:ok, integer}
  end
end

6. then we finally modify the default way our primary keys work:

@primary_key {:id, Rumbl.Multimedia.Permalink, autogenerate: true}
schema "companies" do
...

The end result is that we don't have to do anything about the context methods, they work as intended.

[doomspork]

@gemantzu if we default to putting a number on the slug then moving to slugs makes little sense to me. People are attempting to file companies by their exact name, they will never be able to guess the internal primary key for a company. Right or am I confused?

[DouglasLutz]

Also if in this case the purpose of adding slugs is to people put the name of the company in the url, a simpler solution could be by doing something like this in the controller. And also this would not break the existing routes.

def show(conn, %{"id" => id}) do
    case Integer.parse(id) do
      :error ->
        redirect(conn, to: "/en/companies?search[text]=#{id}")
      {_num, _remainder} ->
        company = Companies.get!(id, preloads: [:jobs, :industry])
        render(conn, "show.html", company: company)
    end
  end

Of course this would not solve the cases of someone trying to go to /companies/plataformatec/edit but I feel that if people are already in a company#show page and want to edit it they would just add the /edit in the end of url.

@doomspork @gemantzu @maartenvanvliet What are your thoughts on this?

[doomspork]

@DouglasLutz your proposed got me thinking more about moving this into the controller. We want to support slugs and ids in URL so we should see if we can't figure that out before moving this to a search redirect.

One issue we have is figure out which type we have and passing it down to the query. What if we change get!/3 (which uses Repo.get!/3) to both use and be called get_by!/3, something this like:

@doc """
iex> get_by!(id: 123)
%Company{}

iex> get_by!(id: 456)
** (Ecto.NoResultsError)

iex> get_by!(slug: "example-company")
%Company{}

"""
def get_by!(predicates, 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_by!(predicates)
end

In our controller could do something like this:

def show(conn, %{"id" => id_or_slug}) do
  case Integer.parse(id_or_slug) do
    {int_id, _remainder} ->
      %{slug: slug} = Companies.get!(id: int_id)
      redirect(conn, to: Routes.company_path(conn, :show, slug))

    :error ->
      company = Companies.get!(slug: id_or_slug)
      render(conn, "show.html", company: company)
  end
end

What do you think? Since we will get more slugs going forward than id type URLs we'll need to be considerate of the Integer.parse/1 performance.

[gemantzu]

@DouglasLutz thanks again for working on this. Are you done? Should we check again the code and functionality?

[doomspork]

@DouglasLutz this is nearly 3 years old and there's quite a few conflicts. I'm going to close but I think we should look to see if this change is still applicable and if so, open a new PR with the feedback incorporated 🎉