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.

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

Include tasks in task_list view #1146

Closed
wants to merge 1 commit into from
Closed

Include tasks in task_list view #1146

wants to merge 1 commit into from

Conversation

snewcomer
Copy link
Contributor

@snewcomer snewcomer commented Nov 2, 2017

Add include key to tasks for the task_list view

References

Fixes #

Progress on: #

@@ -28,7 +28,7 @@ defmodule CodeCorpsWeb.TaskListController do
end
end

@preloads [:tasks]
@preloads [{:tasks, [:comments, :github_pull_request, :task_skills, :user_task]}]
Copy link
Contributor Author

@snewcomer snewcomer Nov 2, 2017

Choose a reason for hiding this comment

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

This is using nested preloads. I am not sure if we should just add the whole list of task preloads or not. Was just adding them based on the feedback from the tests. @begedin thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah these are the ones marked with identifier: always

Copy link
Contributor Author

@snewcomer snewcomer Nov 3, 2017

Choose a reason for hiding this comment

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

@joshsmith would you also include the task's relationships?

Copy link
Contributor

Choose a reason for hiding this comment

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

The real answer here is that it doesn't much matter what I think, but what the performance implications are. Testing this out in the browser with Scout running and doing more precise benchmarking will combined yield a better answer.

@snewcomer
Copy link
Contributor Author

Closing this in favor of #1187

@snewcomer snewcomer closed this Nov 17, 2017
@snewcomer snewcomer deleted the include-tasks branch November 17, 2017 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants