Credo error: Function is too complex (CC is 10, max is 9)

[Cleop]

In our event controller we have the following error from credo:

Function is too complex (CC is 10, max is 9)

For this function:

  def new(conn, payload) do
    headers = Enum.into(conn.req_headers, %{})
    x_github_event = Map.get(headers, "x-github-event")
    github_webhook_action = payload["action"]

    case EventType.get_event_type(x_github_event, github_webhook_action) do
      :new_installation ->
        token = @github_api.get_installation_token(payload["installation"]["id"])
        issues = @github_api.get_issues(token, payload, 1, [])
        comments = @github_api.get_comments(token, payload)

        conn
        |> put_status(200)
        |> json(%{ok: "new installation"})

      :issue_created ->
        issue_params = %{
          issue_id: payload["issue"]["id"],
          title: payload["issue"]["title"],
          comments: [
            %{
              comment_id: "#{payload["issue"]["id"]}_1",
              versions: [%{author: payload["issue"]["user"]["login"]}]
            }
          ]
        }

        changeset = Issue.changeset(%Issue{}, issue_params)
        issue = Repo.insert!(changeset)

        comment = payload["issue"]["body"]
        version_id = issue.comments
                     |> List.first()
                     |> Map.get(:versions)
                     |> List.first()
                     |> Map.get(:id)
        content = Poison.encode!(%{version_id => comment})
        @s3_api.save_comment(issue.issue_id, content)

        conn
        |> put_status(200)
        |> json(%{ok: "issue created"})

      :issue_edited ->
        issue_id = payload["issue"]["id"]

        if Map.has_key?(payload["changes"], "title") do
            issue = Repo.get_by!(Issue, issue_id: issue_id)
            issue = Changeset.change issue, title: payload["issue"]["title"]
            Repo.update!(issue)
        end

        if Map.has_key?(payload["changes"], "title") do
          comment = payload["issue"]["body"]
          author = payload["sender"]["login"]
          add_comment_version(issue_id, "#{issue_id}_1", comment, author)
        end

        conn
        |> put_status(200)
        |> json(%{ok: "issue edited"})

      :comment_created ->
        issue_id = payload["issue"]["id"]
        issue = Repo.get_by!(Issue, issue_id: issue_id)
        comment_params = %{
          comment_id: "#{payload["comment"]["id"]}",
          versions: [%{author: payload["comment"]["user"]["login"]}]
        }
        changeset = Ecto.build_assoc(issue, :comments, comment_params)
        comment = Repo.insert!(changeset)

        version = List.first(comment.versions)
        update_s3_file(issue_id, version.id, payload["comment"]["body"])

        conn
        |> put_status(200)
        |> json(%{ok: "comment created"})

      :comment_edited ->
        issue_id = payload["issue"]["id"]
        comment_id = payload["comment"]["id"]
        comment = payload["comment"]["body"]
        author = payload["sender"]["login"]
        add_comment_version(issue_id, comment_id, comment, author)

        conn
        |> put_status(200)
        |> json(%{ok: "comment edited"})

      :comment_deleted ->
        comment_id = payload["comment"]["id"]
        author = payload["sender"]["login"]
        comment = Repo.get_by!(Comment, comment_id: "#{comment_id}")
        changeset = Comment.changeset(comment)
        changeset = changeset
                    |> Changeset.put_change(:deleted, true)
                    |> Changeset.put_change(:deleted_by, author)
        Repo.update!(changeset)

        conn
        |> put_status(200)
        |> json(%{ok: "comment deleted"})

      _ -> nil
    end
  end

Admittedly it is a big function but this error message doesn't give us much insight into what about the function credo doesn't like. We need to determine if it's the number of cases or what's within them so that we can resolve this.

This is not the first time we've encountered this on this project when we've had to include the multiple actions in a case statement.

[nelsonic]

@Cleop we can split out the cases into their own handler functions which accept the payload...

[Cleop]

My proposed solution: In event_type.ex change the atoms to functions

  defp type("issue_comment", action) do
    case action do
      "created" -> :comment_created
      "edited" -> :comment_edited
      "deleted" -> comment_deleted(payload, conn)
    end
  end

E.g. function:

  defp comment_deleted(conn, payload) do
    comment_id = payload["comment"]["id"]
    author = payload["sender"]["login"]
    comment = Repo.get_by!(Comment, comment_id: "#{comment_id}")
    changeset = Comment.changeset(comment)
    changeset = changeset
                |> Changeset.put_change(:deleted, true)
                |> Changeset.put_change(:deleted_by, author)
    Repo.update!(changeset)

    conn
    |> put_status(200)
    |> json(%{ok: "comment deleted"})
  end

And simply call the get_event_type function which will funnel down into the individual functions in the event_controller.

EventType.get_event_type(x_github_event, github_webhook_action, conn, payload)

[Cleop]

I've implemented this and put the handlers in their own file for neatness.

There is this warning though when you run the server:

which I can lose by doing this:

However, @SimonLab these lines were in place before I refactored, should they be kept? I'm not sure why they're there given that they're not being used but I don't want to take them out and break something!

[Cleop]

The atoms remaining in event type lower the test coverage because we're not testing for them in the event_controller_test because they don't have a payload etc. yet because we haven't implemented the functionality for them.

Either I can remove them and the codecov will be as usual or I can leave them in but accept a lower code coverage until we implement the functionality for them and they are no longer atoms. The atoms are: installation_repositories, issues - closed/reopened.

@nelsonic, @SimonLab - what do you think?

[nelsonic]

@Cleop yeah, comment out the code and include a link to the issue where the feature is described in the "backlog" so that we can return to work on them later.

[Cleop]

Closing as merged in and the issue is technical so can't be tested.