[#67] Refactor ApiController to return JSON so thesis can redirect to the proper page.#111
[#67] Refactor ApiController to return JSON so thesis can redirect to the proper page.#111silasjmatson wants to merge 3 commits intomasterfrom
Conversation
to the proper page. * Change index on thesis_pages.slug to be unique
| contents_params | ||
| |> Enum.map(fn(x) -> content_changeset(x, page, preloaded_contents) end) | ||
| |> Enum.each(fn(x) -> repo.insert_or_update!(x) end) | ||
| |> Enum.map(&(content_changeset(&1, page, preloaded_contents))) |
There was a problem hiding this comment.
I think we should use Ecto.Multi here (future suggestion).
Thoughts?
There was a problem hiding this comment.
I agree...can you file an issue?
| inserted_at: page.inserted_at, | ||
| updated_at: page.updated_at | ||
| } | ||
| end |
There was a problem hiding this comment.
Are we missing the description here?
There was a problem hiding this comment.
It looks like it -- note that the javascript only makes use of the slug in the response currently, at any rate.
lib/thesis/views/api_view.ex
Outdated
| def render("page.json", nil), do: %{} | ||
|
|
||
| def render("page_contents.json", page_contents) | ||
| when is_nil(page_contents), do: [] |
There was a problem hiding this comment.
@yulolimum I like keeping line-length under 80 characters -- I have a guide in my editor that shows when a line is over that length.
There was a problem hiding this comment.
gotcha - i have mine set to 9999, just like most of my z-indexes
¯_(ツ)_/¯
lib/thesis/views/api_view.ex
Outdated
| def render("page_contents.json", page_contents) when is_list(page_contents) do | ||
| Enum.map(page_contents, fn(page_content) -> | ||
| render("page_content.json", page_content) | ||
| end) |
| title: page.title, | ||
| description: page.description, | ||
| redirect_url: page.redirect_url, | ||
| page_contents: render("page_contents.json", assigns[:page_contents]), |
There was a problem hiding this comment.
Note that page_contents will always be an empty list here, as we aren't assigning :page_contents in the controller -- this is only here for possible future need.
|
@jamonholmgren Note that this includes a new migration -- does hex have a way of doing a post-install/update message so we can alert the user to re-run |
jamonholmgren
left a comment
jamonholmgren
left a comment
There was a problem hiding this comment.
I'm happy to chat about these changes sometime. Kind of busy with Chain React prep, but hit me up on Slack.
| end | ||
|
|
||
| defp parameterize_slug(changeset) do | ||
| if {:data, slug} = fetch_field(changeset, :slug) do |
There was a problem hiding this comment.
Can you change this to a case statement? For some reason I like that better in this case, like you did in ecto_store.
| @doc """ | ||
| Updates the given page (identified by its slug) with the given map of | ||
| string keys and Thesis.PageContent structs. Returns `:ok`. | ||
| string keys and Thesis.PageContent structs. Returns tuple `{:ok, page}`. |
There was a problem hiding this comment.
These are breaking changes. But I think we should do it now, for sure. It'll be harder to change later.
|
|
||
| iex> import Thesis.Utilities | ||
| iex> slugify("Jamon is so cool!") | ||
| "Jamon-is-so-cool" |
| use Ecto.Migration | ||
|
|
||
| def up do | ||
| # Rreate old non-unique index |
There was a problem hiding this comment.
I think you mean Remove not Rreate. :D
| drop index(:thesis_pages, [:slug]) | ||
|
|
||
| # Create a Unique Index | ||
| create unique_index(:thesis_pages, [:slug]) |
There was a problem hiding this comment.
This will fail in code bases that already include duplicate slugs. We should probably fail with a friendly error here. You can detect duplicates or just catch the error and fail gracefully (recommended).
|
@silasjmatson I'll update this. |
3 participants
Fixes #67
Note that this changes the API a bit, but I do think it'll make some of the other issues a bit easier to tackle, namely #68