Automatically assign issue credit for committing patches

The code that makes the commit comments is http://cgit.drupalcode.org/versioncontrol_project/tree/versioncontrol_pr...

Rather than a comment hook, this might be done in a node update hook, since the comment is actually a node save. That would also be a good time to make sure mentioned usernames are checked for credit, if we want that in addition to the maintainer saving the issue.

Personally, I think the distinction between issue credit (helped fix the issue) and committing (reviewed and committed) makes sense. But I could see the simplicity of lumping everything together into one credit type being nice, too.

Yes, it could (a) live on a node hook, but the event processor plugins were created for this; so I would suggest to (b) create one more of them instead, i.e. at the same level than git_commits_as_comment and git_issue_mapper plugins; another option would be to (c) just extend the git_commits_as_comment plugin to handle this, but that is also stretching a little more its purpose than the originally intended.

Again, my suggestion would be to choose (b), and now that we would have a third case, it may be a good time to try to move some more functionality to the abstract parent class (VersioncontrolEventProcessorGitOperationBase).

I have a situation that may be relevant to this scenario: Drupal.org has a Your Commits tab which shows commits that you participated. Due to the way it gives credit based on commit messages there are some commits that are not shown in this tab, like this one for Domain Access (I believe it's not being computed because the period is a valid username character).

@joelwallis Currently we are not parsing the commit message to generate the mapping behind "Your commits" view, instead it just relies on the git commit's committer and the author, which in the case you mentioned is the maintainer; please kindly ask him to review the related documentation which is now easier to do with the "Credit & committing" section.

Said that, the last comment seems to be unrelated, and even in the case that we already are parsing for credit, it will also fail, since it would be using the wrong format, i.e. not the suggested one.

The code in VersioncontrolEventProcessorGitIssueMapper & VersioncontrolEventProcessorGitCommitsComment is already similar enough, I’m not eager to add another one.

We can finally deploy #443000: When viewing an issue, display a list of commits that reference that issue # to populate the versioncontrol_project_issue_operations table. If things happen in the right order, that can be used to find the author(s) and committer(s) of the issue’s commits on save.

That deployment is done and we have a big table relating Git commits to issues.

A fix for this is now deployed.

It doesn’t automatically update the credit right away, it sets the defaults on the “Credit & committing” fieldset, as if the user uploaded a patch. Credit is all saved by the maintainer when updating the issue.

This was a bit easier to implement, don’t have to worry about node edit conflicts with the automatically assigned credit. And commits almost always happen before the maintainer’s last update. The big downside is that if the maintainer won’t get the checkbox auto-checked until the page load after the commits are processed. So saving a stale form can happen.

If I'm reading #10 correctly, you're saying there's a race condition which may cause the maintainer (committer) to not be assigned credit?

Unfortunately yes, I think that's the lesser evil than having the maintainer resolve a node edit conflict.

Thanks for clarifying!

How can we inform/train maintainers that they should reload the page after pushing the commit? (That's what I understand from #10.)

I as a maintainer am used to looking at an issue, then write a message while committing (and pushing), and then post my comment after having pushed. Without reloading the page.
I suspect I'm not alone.

Conflict module does not do well with multi-value fields. For example, #2552405: A patch uploaded by user X in comment N mysteriously appeared in a comment by user Y in comment N+4 after cross-posting with user Z. If it could merge each edit’s new values, that would help this work well.

In an ideal world, I’d like to see the page auto-update when cross-posting happens, or at least notify if there are new comments since the page was opened. But that probably should be backlogged until after a D8 migration.

So I just made a commit for the first time since this was deployed, and I followed my normal workflow which is:

1. Draft credit, metadata changes, and a committer review.
2. Commit locally.
3. Push my commits.
4. Save my drafted comment and metadata changes.

As described above, this workflow did not grant me credit, because I did not reload the issue node between my commit and posting the comment.

@Cottser and I were discussing whether there was another workflow that would allow us to post only one comment and still give the committer credit. However, this doesn't work because we have to fill out the crediting field to generate the commit message correctly mentioning the issue contributors. So the committer still needs to at least start filling out their issue comment before pushing their commit, and has to comment a second time to have the default of their own credit.

The other workaround @drumm, @Cottser and I discussed is that the committer would check themselves in their comment before they push the commit, but then we end up with a committer in every commit message which we could already have done.

@drumm said he would file a followup issue for this but in the meanwhile we have a situation where issue credit is going to be skewed depending on whether a committer knows to post a second comment after commit (or whether they happen to for some other reason.)

I was pinged about this at #mwds2017, and +1 to the workflow outlined in #15, which is also my own workflow, and that of all the other core committers, AFAIK.

Per @Cottser, it also emails the committer letting them know they have credited themselves on the issue. :)

Yeah, no need to email me about a thing I just did. :) NO. MORE. EMAILS. :P So if committer username == newly credited username, kill the email notification. This may be resolved by solving the problem in a different way as indicated by Neil at MWDS.

#2552405: A patch uploaded by user X in comment N mysteriously appeared in a comment by user Y in comment N+4 after cross-posting with user Z is the issue I thought might be related. But I opted to go with a more-direct fix.

On node save, this does another check for commits that were made after the form was loaded. If those have happened and have uids, the credit is piled in.

This should be working well now.

Thanks @drumm!