When connecting github user, associate with tasks and comments #956
Conversation
begedin
changed the title
begedin
force-pushed
the
941-942-connect-github-user-to-tasks-comments
branch
from
8449717
to
906e539
Compare
begedin
force-pushed
the
941-942-connect-github-user-to-tasks-comments
branch
from
906e539
to
c6a504d
Compare
| @@ -50,6 +53,8 @@ defmodule CodeCorps.GitHub.User do | |||
| Multi.new | |||
| |> Multi.update(:user, changeset) | |||
| |> Multi.run(:installations, fn %{user: %User{} = user} -> user |> associate_installations() end) | |||
| |> Multi.run(:tasks, fn %{user: %User{} = user} -> user |> associate_tasks() end) | |||
| |> Multi.run(:comments, fn %{user: %User{} = user} -> user |> associate_comments() end) | |||
There was a problem hiding this comment.
I get why you chained more Multi.runs on here but I'm deeply concerned about the number of tasks/comments that we may be associating here. I had originally envisioned this process being fairly asynchronous and displaying to the user something to the effect of "your account is syncing with GitHub", even going so far as to be blocking.
This sync problem is going to be even larger when we sync large repos from GitHub to projects.
I know this adds some non-trivial complexity here. We could probably get away with this as a first pass, MVP and all. But this is something we should really circle back to, at the very least.
There was a problem hiding this comment.
Reading that these are Repo.update_all the operation may not be too slow. We should still write down an issue to benchmark certainly.
Syncing projects does seem like a different beast. That we need an issue for.
|
My review above can be considered a 👍 if my questions are a non-issue right now. This can be merged if so. |
|
Do you want to create an issue for project sync async? |
|
Created #959 |
Closes #941, closes #942
This PR adds 2 additional steps to the connect process. One associates existing tasks with the newly connected user, while the other does the same with comments.
There is a decision to be made about the user previously associated with these. Should the record be deleted, or should something else be done with it?
That's something that can be done as a separate issue, however.