Fix changeset errors in markdown/body with issue changeset builder

[joshsmith]

What's in this PR?

This fixes the issue with changeset errors from blank markdown (and therefore blank body) values that had an attempted fix in #963. It turns out that we're not using the changesets defined in the model itself, but instead specifically putting changes into a changeset and then validating.

@begedin I think it's worth addressing whether or not we want this logic to be split apart so separately, as it was pretty confusing; even in code review I didn't expect that this was using some other changeset. Any thoughts here?

Would like to hear @landongrindheim's thoughts on this, too.

[begedin]

The idea/concept of a changeset, is that you have a function which casts and validates input for a specific action.

The Task.create_changesetspecific action is creation of a task from code corps.

The GitHub.Event.Issues.ChangesetBuilder.build specific action is "creating a task when handling an Issues event.

In hindsight, I believe we should consider changing the name of the module from ChangesetBuilder into, maybe, IssueToTaskChangeset or something along the lines, but I don't feel great about bunching up multiple changesets into one, since I feel it defeats the purpose of the changeset concept in the first place.

The issue here is that we named our changeset based on our initial (mis)understanding of what the purpose of changesets is.

EDIT:

Looking at the code for handling the issues event, I'm not seeing a part that would misguide the developer into thinking that Task.create_changeset is the one being used here.

The mistake comes from the assumption that all database inserts go through the same changeset function, which is, IMHO, incorrect. While the most basic validations are often common across all insertions, at the very least, there will be custom casts based on the origin.

ja_resource, for example, operated on a similar (but IMO much worse) assumption ModelName.changeset, is the "one changeset to rule them all", through which ALL database operations go and that's the main reason we're moving away from it.

[begedin]

This looks good to me.

Something to consider, and I believe I've posted a comment in a related discussion somewhere,

#963 removed the requirement for a task to have a body when a user is creating/updating it from CodeCorps

I don't think that requirement is needed anyway, but I will say it again, just to put the option out there, since we are using multiple different changesets, we can add the requirement back in the case of input from CodeCorps, while keeping it off in the case of syncing from Github.

That's the main strength of changesets - different validations and restrictions on the same schema in different scenarios.

[joshsmith]

@begedin understood and agreed about changesets. I think maybe we could just be more explicit about documenting it, and likely should have been much more explicit about the requirements in the issue.