Reorganize GitHub modules

[joshsmith]

Problem

Much of the GitHub syncing can be reorganized as follows:

• [x] CommentSyncer and TaskSyncer need to move out of Event; structure could be like:
  • sync/issue/issue.ex Sync.Issue (main entry point for handling the syncing of an issue)
  • sync/issue/body_parser.ex Sync.Issue.BodyParser (parses issue body)
  • sync/issue/task/task.ex Sync.Issue.Task (syncs the issue to a Task)
  • sync/issue/task/changeset.ex Sync.Issue.Task.Changeset (deals with changesets)
  • sync/issue/github_issue/github_issue.ex Sync.Issue.GithubIssue (syncs the issue to a GithubIssue)
  • etc.
• [x] ChangesetBuilders
  • [x] need to move out event to wherever syncing goes
  • [x] map can be removed
  • [x] naming needs to be rethought to reflect that these are changesets for Code Corps models (Task, Comment, etc)
• [ ] GithubRepo installation events need to have common/repo_finder (renamed to repo_linker)
• [x] CommentDeleter needs to move out of Event
• [x] CommentDeleter needs to delete GithubComment
• [x] ChangesetBuilders need create_ and update_ functions made public, build_ fns removed, unit tests added
• [ ] Replace instances of find_or_init and commit with insert_or_update and find

[begedin]

Re:

GithubRepo installation events need to have common/repo_finder (renamed to repo_linker)

Not sure if this is still applicable.

The main Sync module makes extensive use of the existing Sync.Utils.RepoFinder. The approach towards using it is that a sync step is expected to fail if none is found.

The InstallationRepositories event has an internal method for finding a repo, which works differently due to a find or create approach which is necessary here.

The Installation event preloads all Repos as the final step and then processes each of them individually. There is no find_or_create in there. We explicitly now which repo is to be created, which updated, which deleted.

The Matched/Unmatched user helper modules do not deal with processing repos.

If we try really hard, we could force the installation repos and installation repo loading into RepoFinder, but from my POV, unless I'm missing something, it will make the code less manageable.

Re:

renamed to repo_linker

At some point, we probably had the idea of the module actually linking a provided record with a repo, but the current module actually does just try to find the repo, returning an ok or error tuple.

I feel RepoFinder is more suitable than RepoLinker, considering that.

Re:

Replace instances of find_or_init and commit with insert_or_update and find

There is only one instance left in CodeCorps.GitHub.Sync.Issue.GithubIssue

payload
|> find_or_init()
|> GithubIssue.changeset(payload |> Adapters.Issue.to_issue())
|> Changeset.put_assoc(:github_user, github_user)
|> Changeset.put_assoc(:github_repo, github_repo)
|> maybe_put_github_pull_request(github_pull_request)
|> Repo.insert_or_update()

Looking at all of our other code, I'm seeing inconsistencies here

• We use GithubIssue.changeset, then add to it
• We try to associate regardless of existing or new record
• We have a maybe

In our other modules,

• we have a Changeset helper module
• we do a find, if nil, Changeset.create_changeset, if found, Changeset.update_changeset
• create_changeset associates everything, update_changeset only the associations allowed to change post-creation

I'm actually inclined to switch to this other approach instead of leaving this here.

[joshsmith]

@begedin what's the ultimate actionable takeaway of the above comment? I see an inclination but not confident about execution.

[begedin]

With the recent changes in the PR, I'd say

GithubRepo installation events need to have common/repo_finder (renamed to repo_linker)

This is obsolete and has been dealt with differently

renamed to repo_linker

We now have a common "Finder" module, used for everything, though that might go obsolete to

CodeCorps.GitHub.Sync.Issue.GithubIssue

Seems ok for now, but we may change it up a bit later. Not sure

[joshsmith]

Is this issue actually relevant now after the merge of your PR?