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

Jump to bottom

Replace TaskList with jsonapi #1187

Closed
wants to merge 6 commits into from
Closed

Replace TaskList with jsonapi #1187

wants to merge 6 commits into from

Conversation

snewcomer
Copy link
Contributor

@snewcomer snewcomer commented Nov 13, 2017 •
edited
Loading

This is a WIP. A few PR's to jsonapi may be in order. Need to investigate more.

Progress on: #1139

@snewcomer snewcomer self-assigned this Nov 13, 2017
@snewcomer snewcomer requested a review from begedin November 13, 2017 15:41
snewcomer
snewcomer commented Nov 13, 2017
View reviewed changes
config/config.exs Outdated
@@ -84,6 +84,9 @@ config :sentry,

config :code_corps, :processor, CodeCorps.Processor.Async

config :jsonapi,
remove_links: true
Copy link
Contributor Author

@snewcomer snewcomer Nov 13, 2017 •
edited by begedin
Loading

Choose a reason for hiding this comment

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

underscore_to_dash is giving me this error. Perhaps a PR.

** (Protocol.UndefinedError) protocol Enumerable not implemented for #DateTime<2017-11-13 15:38:42Z>. This protocol is implemented for: DBConnection.PrepareStream, DBConnection.Stream, 
Date.Range, Ecto.Adapters.SQL.Stream, File.Stream, Function, GenEvent.Stream, HashDict, HashSet, IO.Stream, List, Map, MapSet, Postgrex.Stream, Range, Scrivener.Page, Stream, Timex.Interval

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like problems with serializing a date. We'll have to look into it. Any open or closed issues related to date serialization?

Copy link
Contributor

Choose a reason for hiding this comment

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

Narrowing down to which field is creating the problem would likely help. Maybe disable one attribute at a time to see what happens?

lib/code_corps_web/views/task_list_view.ex Outdated
has_many :tasks, serializer: CodeCorpsWeb.TaskView, identifiers: :always
def render("show.json-api", %{data: task_list, conn: conn, params: params}) do
__MODULE__.show(task_list, conn, params)
end
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would rather just call the show/index action directly from the controller, but was getting failing controller tests around expected connection to have a response but no response was set/sent. Please verify that you assign to "conn" after a request. Will have to investigate more.

test/lib/code_corps_web/views/task_list_view_test.exs Outdated
@@ -7,7 +7,7 @@ defmodule CodeCorpsWeb.TaskListViewTest do
task = insert(:task, order: 1000, task_list: task_list)

task_list = CodeCorpsWeb.TaskListController.preload(task_list)
rendered_json = render(CodeCorpsWeb.TaskListView, "show.json-api", data: task_list)
rendered_json = render(CodeCorpsWeb.TaskListView, "show.json-api", %{data: task_list, conn: %Plug.Conn{}, params: task_list.id})

expected_json = %{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@begedin this is failing. I returns a map with atom keys. Should the json be a map with string keys or doesn't it matter?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have keys such as "pull-requests", so I'm not sure they can be atoms. Is this configurable in jsonapi?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you said underscore_to_dash: true, is giving you errors above. We need that to. Our Ember client is expecting dashes.

@snewcomer snewcomer mentioned this pull request Nov 17, 2017
Closed
begedin
begedin reviewed Nov 20, 2017
View reviewed changes
Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

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

These comments are pending for a couple of days now. Not sure how I missed it. Sorry about that. I hope they help.

test/lib/code_corps_web/views/task_list_view_test.exs Outdated
@@ -7,7 +7,7 @@ defmodule CodeCorpsWeb.TaskListViewTest do
task = insert(:task, order: 1000, task_list: task_list)

task_list = CodeCorpsWeb.TaskListController.preload(task_list)
rendered_json = render(CodeCorpsWeb.TaskListView, "show.json-api", data: task_list)
rendered_json = render(CodeCorpsWeb.TaskListView, "show.json-api", %{data: task_list, conn: %Plug.Conn{}, params: task_list.id})

expected_json = %{
Copy link
Contributor

Choose a reason for hiding this comment

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

We have keys such as "pull-requests", so I'm not sure they can be atoms. Is this configurable in jsonapi?

test/lib/code_corps_web/views/task_list_view_test.exs Outdated
@@ -7,7 +7,7 @@ defmodule CodeCorpsWeb.TaskListViewTest do
task = insert(:task, order: 1000, task_list: task_list)

task_list = CodeCorpsWeb.TaskListController.preload(task_list)
rendered_json = render(CodeCorpsWeb.TaskListView, "show.json-api", data: task_list)
rendered_json = render(CodeCorpsWeb.TaskListView, "show.json-api", %{data: task_list, conn: %Plug.Conn{}, params: task_list.id})

expected_json = %{
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you said underscore_to_dash: true, is giving you errors above. We need that to. Our Ember client is expecting dashes.

config/config.exs Outdated
@@ -84,6 +84,9 @@ config :sentry,

config :code_corps, :processor, CodeCorps.Processor.Async

config :jsonapi,
remove_links: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like problems with serializing a date. We'll have to look into it. Any open or closed issues related to date serialization?

config/config.exs Outdated
@@ -84,6 +84,9 @@ config :sentry,

config :code_corps, :processor, CodeCorps.Processor.Async

config :jsonapi,
remove_links: true
Copy link
Contributor

Choose a reason for hiding this comment

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

Narrowing down to which field is creating the problem would likely help. Maybe disable one attribute at a time to see what happens?

@snewcomer snewcomer force-pushed the jsonapi branch 3 times, most recently from ac30789 to 5dd7826 Compare December 13, 2017 15:04
@snewcomer
Copy link
Contributor Author

snewcomer commented Dec 13, 2017 •
edited
Loading

@begedin @joshsmith So I got a good working version for task_list. However, it kind of creates a snowball effect b/c ProjectView / TaskView needs to be changed. Then those relationship views need to be updated.

I wanted to know your thoughts on tackling this. It's a big refactor so just wanted to ping you guys to make sure its a ok. I can try to get it done today...

@snewcomer
Copy link
Contributor Author

task_list

Here is the latest w/o includes...

@snewcomer
Copy link
Contributor Author

task_list_include

and about net w/ the includes flag

@snewcomer
Copy link
Contributor Author

@joshsmith So here is where I am at. I separated out the project and task views to their own files specific for the task_list view so to avoid a huge refactor. These relationships are now included in the view as well.

The only piece of housekeeping is ensuring the custom attribute methods that are defined on the view can be included in the attributes...which may be a PR to jsonapi.

@snewcomer
Copy link
Contributor Author

@begedin @joshsmith Just to add to what the goal is...It would be great to use the include key, whether it is jsonapi or ja-serializer. So maybe I close this and start a PR to use the include key w/ ja-serializer....or I keep going with this PR. Lmk your thoughts.

@snewcomer
Copy link
Contributor Author

Going to close this in favor of #1302 for right now.

@snewcomer snewcomer closed this Dec 15, 2017
@snewcomer snewcomer deleted the jsonapi branch February 8, 2019 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@begedin begedin begedin left review comments

Assignees

@snewcomer snewcomer

Labels
Difficulty: Medium in progress needs code review Priority: Medium Skill: Elixir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@snewcomer @begedin