Issue/101 REST API: Chapter Management

[eteubert]

1. Calling a plug directly in https://github.com/podlove/radiator/compare/issue/101-api-chapter-management?expand=1#diff-d0559d752a76cec019da187d4034d879R43 feels strange but the general idea of short-circuiting here should be ok.

2. I added

def get_permission(_user = %Auth.User{}, _subject = %Chapter{}), do: nil

so the permission check on chapter always fails and falls back to the parent (Audio). Is that the intended place? Should there be a general def get_permission(_user = %Auth.User{}, _subject), do: nil catchall?

closes #101

[monkeydom]

Yeah - I think probably for short circuiting errors a raise with a corrsponding catch in the error view is probably more appropriate.

[monkeydom]

Some notes to be addressed (all inline explained):

• Choice of id for chapters (if not changed to the proposed start then the id needs to be reported in the GraphQL api as well to be able to be modified)
• Missing audio_id field on get/result
• Missing :show - if that was intentionally, it needs to be removed more thoroughly in the docs as well, if kept I think reporting audio_id in show.json is important too
• Leakage of image files

[monkeydom]

As mentioned before, this is probably better done using a raise and catch the exception the way the original not found is done

[eteubert]

Ready for a second review. A few notes:

1. Regarding calling RadiatorWeb.Api.FallbackController directly in ChaptersController: I don't think we can raise in a plug as it breaks the typespec contract and Dialyzer complains. Tried researching a bit for similar implementations but couldn't find any, so I left it as it is for now.
2. Leaking files in arc: I am surprised arc_ecto doesn't handle this automatically. My current solution for chapter images works "good enough" as far as my testing goes but our aim should be a generic changeset step that we can throw in everywhere we use an arc_ecto attribute. See arc_ecto: Create changeset step to auto-delete images #166

[monkeydom]

two comments and didn't compile and test, but looks fine to me now.

[monkeydom]

ah - i see what you did there. Interesting pattern.

[monkeydom]

Hmm, shouldn't we somehow just refer to the assigned fallbackcontroller? e.g. something like

apply(@phoenix_fallback, :call, [error])

? - in the middle of some other change now, can't verify if that is the way this should work. but the falback seems to be in the phoenix_fallback attribute

[eteubert]

Thanks for pointing out @phoenix_fallback. However that's a tuple, either {:module, mod} or {:function, fun}, depending on type of plug used.

What I came up with:

  defp apply_action_fallback(conn, response) do
    case @phoenix_fallback do
      {:module, module} -> apply(module, :call, [conn, response]) |> halt()
    end
  end

Then I can

  defp assign_chapter(conn, _) do
    with {:ok, chapter} <-
           conn
           |> current_user()
           |> Editor.get_chapter(conn.assigns[:audio], conn.params["start"]) do
      conn
      |> assign(:chapter, chapter)
    else
      response -> apply_action_fallback(conn, response)
    end
  end

It's at least removes the direct reference to the plug module name, which is nice. What bothers me is that I cannot add the {:function, fun} case because Dialyzer determines it's a module and complains the match is pointless. Anyway, still happier with this than before.