search

code-corps / code-corps-api

Elixir/Phoenix API for Code Corps.
https://www.codecorps.org
MIT License
234 stars 86 forks source link

include tasks in view #1302

Open snewcomer opened 6 years ago

snewcomer commented 6 years ago

WIP

So this is a first stab at including tasks in the task_list view and would remove the need for this line taskLists.forEach((taskList) => get(taskList, 'tasks').reload()); in the tasks index route.

A couple of notes:

  1. This is using a separate TaskIncludedView with just the attrs needed for the tasks index route. Also note the has_github_pull_request and overall_status is not in the attrs. I can add back, but wasn't seeing it on the index route.
  2. This should just work on the frontend. Further refactors to the tasks index route and task detail route will follow.

Progress on: #1156

joshsmith commented 6 years ago

@begedin is this something we can talk about sync some soon? I'd like to actually step through this a little more explicitly to wrap my head around it.

begedin commented 6 years ago

@joshsmith Posted a comment which we could conver into an issue at https://github.com/code-corps/code-corps-api/pull/1349#issuecomment-354760129 but I would agree that this is wort standup time.

joshsmith commented 6 years ago

@begedin wasn't our conclusion that this is necessary to do, we just need clarity on what the client is going to need so we can design our API more intentionally?

joshsmith commented 6 years ago

kind of like graphql? I would love that. I worked on a project and eventually there was 5 different views for the same resource, each giving you a different subset of data. It was slightly hard to manage.

In fact, JSON API provides mechanisms to do exactly this: render subsets of data based on what the client requests.

begedin commented 6 years ago

@begedin wasn't our conclusion that this is necessary to do, we just need clarity on what the client is going to need so we can design our API more intentionally?

Including conversation parts in conversation is easier to merge ahead of time, since it doesn't alter much in our architecture, so I would say #1349 is good to go for now, as something we do before adding support for explicit includes.

However, this one is harder to decide on, since it involves adding new subtypes of views, which is an architecture change. It still seems to be an improvement in performance, but I'm not sure what else it entails.