Updated: Comment #14

Problem/Motivation

The current commit messages generated by Dreditor credit anyone who has uploaded a new version of a patch, even if they have just done an easy reroll. By contrast, reviewers are almost never credited, even if they have provided significant feedback across multiple reviews, because doing so requires special care on behalf of a committer to manually adjust the commit message by hand to include more people. This seems unfair. It also means reviewers don't share the responsibility for the patch when just looking at the commit log.

We want to be able to recognize more contributions than committed someone else's work, or touched a patch in an issue that eventually got committed somewhere.

Comments are sometimes more than one kind of thing, both a review of a previous thing, and a proposal for something different, and a patch.

We do not want to make more work for people. We do not want someone to feel like they should go through lots of comments on lots of issues an categorize them, or +1 it.

We do not want to introduce a change to the issue queue that makes the issue queues more combative (less collaborative).

Proposed resolution

General proposal:

Categorize comment as a review (and/or some other type of non-code contribution), so data about more contribution types can exist.
(This might later be able to be parsed for specific credit in auto-generated commit messages.)

Method of categorization proposal:

  • owner manual categorization.
  • automatic. (Alternative to manual type categorization: automatically guess what type of contribution a comment had based on stats about the comment: if there was a status change, how long the comment was, what types of files were attached to the comment.)
  • crowd sourced (non-comment owners could categorize other user's comments)

types

(what to have in the select list

  • Research (how something is done currently in Drupal and why it's broken; how other projects accomplish something.)
  • Proposal (new idea, specific architectural direction, etc.)
  • Solution (a patch, code snippet or file that constitutes the solution or part of the solution to the problem in the issue summary)
  • Review (feedback on another's work that pushes the issue forward in some way)
  • Issue management (helping to set tags/priorities, updating issue summaries, etc. e.g. (mentors) prepping for a sprint)

Examples:

Maybe link to specific comments in other issues.

Description Type
Updating title, changing priority, adding tags Issue management
Reading comments, identifying remaining tasks, updating the issue summary Issue management
Attaching and embedding image showing a bug Issue management
Attaching and embedding image showing before and after screenshots Issue management
Commenting to evaluate after screenshots Review
Commenting to evaluate a change (expressed via words, or mock up, or patch) to the UI Review
Attaching and embedding image file of a UI mockup Proposal
Patch which adds an image file Patch
Comment with some code snip-its suggesting a way to do something Proposal
Comment with some code snip-its pointing out a problem with a previous patch and making a suggestion of a better way Review, Proposal
? ?
? ?

Implementation ideas

  • Multiple select,
  • Default to something sane,
    • goes to a value guessed based on status change/state
    • takes into account the type of file uploaded
    • like a default a user picks and is stored in their profile.

Remaining tasks

Discuss. Agree.

User interface changes

TBD

API changes

TBD

Files: 

Comments

ianthomas_uk’s picture

Points to consider:
- The list of names can already be long, so is probably worth moving to the end of the list.
- What constitutes a review?
- How do we keep track of who has reviewed an issue without creating more work?

I don't really like the 'reviewed by' phrase, as that suggested that reviews that set a patch back to needs work aren't worth as much, but I wanted to name one person (occasionally more) who had reviewed the patch and agreed it was ready for and worthy of commit. This will usually be the person who most recently changed the issue status to RTBC, although that's not really true when RTBC patch need to be rerolled which is a nuisance. Maybe 'seconded by' would be better?

'thanks to' credits everyone not already credited, on the assumption that all issue comments are generally helpful, or at least intended to be.

ianthomas_uk’s picture

Title: [policy, no patch] » [policy, no patch] Credit reviewers in commit messages
sun’s picture

I think this issue should be "moved" to Dreditor's issue queue on GitHub, since it is actually Dreditor that automatically generates the current commit messages.

There might even be an existing issue already. (At least I vaguely remember a related one.)


Just to share some backstory and details upfront:

  1. Dreditor simply scans the issue for comments containing a patch and extracts the author names from those comments.
  2. For issue participants without patches, it is not possible to make a differentiation between reviewers (a.k.a. sign-offs) vs. "thanks to", because yeah, that requires Artificial Intelligence. ;-)
  3. Dreditor did actually list everyone at the beginning, but that approach did not work out; e.g., on issues with 300+ comments.
  4. Likewise, it sometimes caused even people to get credited for a change, whose comments were actually out of scope, unhelpful, or otherwise non-productive.
  5. I strongly advise to not change the basic formatting of commit messages, since we still want to be able to parse contributors out of the commit log for various use-cases — the current parsing logic is complex enough already now.
  6. There are certainly way to improve the current situation, but it requires to come up with clever ideas in order to (semi-)automate the list of people to credit, so as to make it work on a larger scale. — Maintainers who're committing changes on a very frequent basis usually do not have much time for reviewing + committing (many) change proposals, so their time is best spent on getting things done, and thus they should not have to deal with having to manually compose a list like this for every single issue.
ianthomas_uk’s picture

Issue summary: View changes

this issue should be "moved" to Dreditor's issue queue

While Dreditor will likely be responsible for the implementation, I think the policy itself should be agreed on here.

Dreditor did actually list everyone at the beginning, but that approach did not work out; e.g., on issues with 300+ comments.

Do you mean it wasn't able to find all the names, or the commit message was just too long? A workaround would be to add 'et al' on long issues (which isn't a great workaround, I realise).

Likewise, it sometimes caused even people to get credited for a change, whose comments were actually out of scope, unhelpful, or otherwise non-productive.

The same happens with patches (e.g. uploaded to the wrong issue, or people uploading rerolls that just drop any conflicted blocks). I don't think there's any way around that without AI, and even then it would sometimes be debatable who should/shouldn't be credited. I think we just have to accept that sometimes we'll thank people who were unhelpful.

I strongly advise to not change the basic formatting of commit messages ... current parsing logic is complex enough already

We could keep the 'by a, b, c' where it is now, but that's already a usability problem when looking through a list of commits (e.g. quickly tell me what was has been changed in HEAD recently from http://drupalcode.org/project/drupal.git alone, without following any links). I would have raised that as it's own issue if I wasn't already opening this proposal to change the message format.

requires to come up with clever ideas in order to (semi-)automate the list of people

Absolutely. Even adding a few seconds work to every commit quickly becomes a lot of work when multiplied by the number of commits made on d.o.

YesCT’s picture

I suspect there are related issues, and this might even be a duplicate. We should search for other issues to get a sense of history of conversations about how to recognize reviewers.

ianthomas_uk’s picture

I couldn't find any duplicate issues, but did find a couple that are related. Actually, the duplicate is a thread on groups.drupal.org, but I'd already written this so I'll post to summarise the other issues.

#543178: Tweak commit message algorithm where sun/webchick looked at ways to pick most helpful commentors based on frequency of comments.

#1583840: Define conventions around drupal core git interaction is more about whether sandbox commit messages follow conventions, but I wanted to copy some quotes here:

@dww: After spending hours, sometimes 100s of them, on a given change, we leave it up to the over-worked core maintainers to construct the right commit message, which they often do totally improv and rushed, they instantly commit / push with 0 input/review from the community, and then it's locked that way forever.

In terms of the commit message itself, a very simple improvement might be to include a section in the recommended issue summary template where everyone participating in an issue can be constructing a proper, well-formatted, clear, concise commit message to accompany the issue.
- Derek Wright; #1583840-1; 16th May 2012

I don't like this option in most cases, as it is more manual work and it has a tendancy to get out of date.

Attribution is a harder problem since Git natively only supports a single author and committer. I don't think the Linux approach of "signed-off-by" conventions at the bottom of the commits is the way to go, nor the "Issue #X by Y, Z" we do now. In both cases, you can't fix it after the fact. Our convention is particularly problematic since it eats up valuable space in what should be the very short 1-line (e.g. ~50 chars) summary of what changed. I think sdboyer's proposal for git notes is the only sane way to actually do attribution right given our situation. However, I honestly think that deserves to be spun out into a separate issue (which can be linked from here) since it's a hefty conversation to have on its own.
- by dww

catch says: I'd quite like to move to something like this, and completely agree we should use one method of attribution and stick to it (at least for things like contributor stats), and that author of commit messages won't work for us.

Gábor Hojtsy’s picture

What about distributing this to the commenters who say check a box "This comment is a review" and those would get into the dreditor user list?

webchick’s picture

Agreed; something like that is really the only way for this to work sustainably. Particularly in the core queue, committers are simply not going to be able to read all 300 comments and adequately pick out the ones that feel "commit-mention-worthy."

Could also perhaps accomplish a similar thing with "plus 1" feature on comments (there's an issue for this somewhere that I can't find atm). Then Dreditor / D.o could auto-generate suggested commit messages based on both patch + a threshold of voting. Would also be useful in other areas of Drupal.org such as support requests in forums.

webchick’s picture

Hm. #42232: Help Maintainers Manage Issue Priority by Encouraging Voting looks like it might be the voting feature issue.

fago’s picture

As mentioned in #2288727-34: [meta] Provide credit to organizations / customers who contribute to Drupal issues, this has been discussed on Twitter a bit: https://twitter.com/dries/status/502805736020537344. I posted some arguments for doing it at #2288727-35: [meta] Provide credit to organizations / customers who contribute to Drupal issues, citing them here:

  • it actually takes more knowledge for being able to tell whether something is a good chance or not than writing it
  • doing proper reviews takes a lot of time, sometimes even more than writing it
  • the lack of any credits for reviews does not encourage people to review more code. That's in particular problematic as most people would probably consider writing patches being more fun anyway.

What about distributing this to the commenters who say check a box "This comment is a review" and those would get into the dreditor user list?

Yeah, some sort of automation seems to be necessary here. I don't think a review should require a certain threshold of votes to be considered though as imo not only particular good reviews should get credit, but all reviews. Compared to code, a simple typo fix counts as contribution being mentioned - thus we should do likewise for reviews and count all of them. That becomes a bit problematic though, as it might be difficult to determine what exactly makes a review a review. I'd propose to define that as reviewing the whole patch and build that into the checkbox text on the comment. E.g.:
"This comment is a review."
Description: Only check this box, when you've reviewed all changes introduced by the patch."

Then, the number of reviews could be used for generating the ordering of mentioned reviewers.

sun’s picture

Any chance to postpone this + related issues until after the D8 beta, please?

I'd really like to think through these topics some more, but happen to be in "hurry-up/last-minute" mode for a couple of weeks already, so not really able to spare the time to truly sit down and evaluate the proposal(s) right now, even though I'd love to.

The change won't have an effect on D8 either way ('cos >90% is done/committed already).

webchick’s picture

In Dries's AMS keynote, he also mentioned the possibility of crediting other non-technical contributions, including providing mockups, user interface testing, etc. I'd like to come up with a workable plan for that which complements #2288727: [meta] Provide credit to organizations / customers who contribute to Drupal issues.

Should I expand this issue to include that scope as well, or file a separate meta?

ianthomas_uk’s picture

I think there will be enough common problems that they need to be addressed on one issue, so we should expand this one.

Up until now there's been a general assumption that the data created should live in git, but I don't see why that actually needs to be the case. Most of the commit messages are auto-generated based on the issue thread anyway so why not track this entirely on d.o? That would allow us to credit people for tasks that don't lead directly to a patch.

Perhaps we want something compatible with Mozilla Open Badges? http://openbadges.org/

webchick’s picture

Title: [policy, no patch] Credit reviewers in commit messages » [policy, no patch] Allow crediting reviewers (and other non-coders) as first-class contribitors
Issue summary: View changes
Related issues: +#2295411: Auto-generate Git attribution info / commit messages on Drupal.org
FileSize
84.52 KB

There's another issue talking about auto-generating Dreditor-esque commit messages that's pretty far along over at #2295411: Auto-generate Git attribution info / commit messages on Drupal.org (hm. bad title given its current direction I guess), so agreed we should make this issue less about how the commit message gets specifically formatted (in theory, that's a pretty trivial change to make once that larger feature's deployed) and more about how we go about adding the data to comments to allow this kind of information to be used in an auto-generated commit message in the first place. Therefore, slightly hijacking the issue title/summary accordingly. (Please feel free to revert if you disagree with these changes; really not trying to step on your toes and happy to start another meta issue instead.)

I had a lengthy discussion at Pacific Northwest Drupal Summit this weekend with @jhodgdon, @YesCT, @bwinett, and a few other folks. I apologize in advance for the length of this comment, but I want to try and capture the discussion here for transparency/further discussion.

Our initial direction started along the lines of the idea proposed in Dries's Amsterdam keynote, which set out broad categories of contributions, of which "Review" could be one:

'Type of contribution' drop-down on issue comments

(Sorry about the poor quality; had to screenshot that from YouTube. ;))

The general idea was to be able to self-select what type of comment this is, so that we could pull stats like "# of design mockups contributed by person X" and "# of people doing user testing in the X project queue." (and, of course, optionally add them to the commit message as well). In general, just to add more visibility around what sorts of things people are doing, as well as to help folks like patch reviewers, designers, issue triagers, etc. (who do very important work but currently feel very "second class" compared to developers) feel less marginalized.

We played around with what that list might actually be. Our first take was something like 20 items (obviously way too granular and long). Our second take was more like 7-8, but it, like the mockup pictured above, was an uncomfortable mix of taxonomies. "What" the comment itself was versus "what expertise" the person brought to the table. This brought up some questions like "How is 'usability testing' not just a type of 'review'?" and even "If 'Documentation' is a separate thing from 'Patch', it feels like we're still calling it 'lesser than' because otherwise in most cases it would just be a first-class 'patch'."

What we finally settled on (and I don't think any of us are married to this, though we did talk it through for like 2 hours so it's hopefully not completely inane :D) was separating these two things out, into two separate multivalue fields (or possibly a single field that's "hierarchical select"):

Comment Type

- Proposal (new idea, specific architectural direction, etc.)
- Patch (a patch :) — deliberately "patch" and not "code" so as not to leave out Docs, etc. patches)
- Review (feedback on another's work that pushes the issue forward in some way)
- Issue management (helping to set tags/priorities, updating issue summaries, etc. e.g. core mentors prepping for a sprint)

(There may be things missing from this list, but ideally we want to keep it to ~5 or fewer things.)

Contribution Type

- Accessibility
- Usability
- Markup/CSS
- Automated testing
- Documentation
- Development
- etc.

(We didn't spend as much time on this list, so this one is mostly hand-waving at the moment. The key thing is to try and represent a fuller list of the types of contribution that happen, especially highlighting those groups who feel marginalized atm.)

In this way you could submit a "Usability" + "Proposal" (aka a design mockup) or a "Usability" + "Patch" (implementing the design) or a "Usability" + "Review" (user testing). And we could even show these things on your profile too, like:

Projects

Project foo (xxx commits, yyy reviews, zzz proposals, 000 issue management)
Project bar (xx commits, y reviews, z proposals, 000 issue management)

...

Contributions

Usability: 1234 contributions
Accessibility: 67 contributions
(or badges for this, or a tag cloud, or whatever gets implemented ultimately)
...

I haven't yet taken time to mock this all up, because I wanted to first float the idea past some folks (including tkoleary, who designed the stuff in Dries's keynote) and see if it even made sense first.

webchick’s picture

Also, I should point out that there is some "prior art" in the Prairie group around this general topic, albeit more on the side of making *other* people do the categorizing https://groups.drupal.org/node/225824 Not sure if this was consulted in Kevin's research for the keynote or not.

Screenshot:

+1 button, with a selection of categories.

nod_’s picture

I don't think the "patch" type is correct. It doesn't exclude docs but it does exclude mockups and design. If we put mockups/design in "proposal" it means they're endless discussion topics (bikesheds) instead of contributions waiting to be reviewed (constructive feedback). And because patch really is about code (doesn't matter if it's PHP/JS/CSS or comments, I don't think anyone will be fooled) people who can't implement their design are still second class contributors. Or we get rid of the patch type and have mockups/design/patches as "proposal" because there shouldn't be any difference between a graphical proposal (mockup) and a code proposal (patch).

What about "deliverable"? maybe to business but something along the lines of that. Because patch does suppose we're changing code somewhere.

Other than that, looks good to me. As long as we can set default values for those fields in the user profile.

tkoleary’s picture

@YesCT and @tvn and I had a lengthy discussion on this in Amsterdam that covered both reputation and categorization

I understand that there has been a lot of controversy around introducing ranking or voting on comments but I think we should strongly consider using what Tatiana has built (in some form). Experience from stack exchange, bugzilla etc. shows that this type of tool is incredibly valuable as a lubricant to the ideation process. There is no question that they are doing it better than us and this kind of "off the island" thinking is needed.

Having said that we do need to think about where categorization ends and voting begins because they are currently a little conflated in Reputation module which currently lets you plus one users by both category and activity (code review).

Another thing reputation does not take in to account is overall activity.

The robust-karma score represents the overall value of a user's contributions: the quality component ensures some thought and care in the preparation of contributions, and the participation side ensures that the contributor is very active

http://buildingreputation.com/writings/2010/02/on_karma.html

With a robust karma system contributors will get a better sense of the subject matter experts which *should* lend weight in the discussion to those who are more likely to effect change but there are other factors that frustrate consensus building. A karma, reputation, badge or ranking system will not mitigate the bikeshedding problem.

Bikeshedding is what complexity theorists call a "positive feedback loop" (where positive means "increasing in intensity" not "getting better"). The issue queue feedback loop looks like this:

The more topical an issue the more comments it gets, the more comments the more likely that someone will say something provocative, the more provocative comments the more heated the debate, the more heated the more polarized, the more polarized the further it is from resolution, the further from resolution the more discouraged contributors get, the more discouraged they are the less they want to spend time in the queue, the less time they spend in the queue the greater the percentage of people who enjoy endless heated debate etc. ad infinitum.

Given all of that it seems we need a mechanism that:

  1. Discourages provocative comments
  2. Facilitates consensus

There is a code of conduct that addresses #1 but a code of conduct alone is clearly insufficient. There need to be more guardrails—perhaps a "flag as inappropriate" field—as well as positive reinforcement of behaviors that should be encouraged.

One approach to all this would be that "plus ones" in comments could be categorized qualitatively without reference to the subject of the comment. For example:

Qualities:

Helpful
Informative
Thoughtful
Creative
Consensus building

Subject matter could then be dealt with separately with an autocomplete field to allow for a much longer list of unqualified categories similar to Angie's "contribution type"

Category

Code
Design
Accessibility
Usability
Documentation
Testing

The combination of the two could then give an accurate picture of both the type and quality of the contribution eg. 30 plus ones for "creative" on a comment tagged with "design" would clearly indicate value (to be used both for Karma and reward as in Dries proposed system). An added benefit of this is that the categories for reputation themselves suggest to the user expected behavior.

Unlike in Reputation, the total plus-ones on a comment would not be displayed, only whether you had plus-oned it and for what quality. The reputation/karma score itself should probably accompany the avatar and be a calculated value that takes activity into account to mitigate "fly-bys" and to remove the potential confusion that in plus-oneing a comment you are voting for the idea in the comment rather than for the reputation of the user.

To further mitigate bikeshedding I think it might be worthwhile to also consider comment voting as well as other things like comment truncation, which would allow you to quickly bypass comments you might consider off-topic based on the teaser, or the individual's reputation.

We could also consider muting (per user) of comments or of users, provided we show some visual affordance that muted comments are there so they can be expanded if needed. This would provide a way to filter out comments or users who are disruptive, provocative or just off topic in a way that is not offensive to anyone.

jhodgdon’s picture

[EDIT: note - this was in response to nod's comment above]

Um. Just because someone marks their contribution as a "proposal" doesn't mean it is subject to any more endless bikesheds than it would have been before this change -- I don't see how this changes in any way whether or how patches or comments or design mockups are reviewed and commented upon. We will still have "needs review" status, issue summaries, etc. We are just adding a field to each comment where you can self-decide what type of a contribution you are making.

I also think that you've missed the point a bit. The point of being able to mark your contributions up in this way is that we are going to ****track and recognize all types of contributions****. For instance, Joe makes a ton of UI mockups, so he gets recognition on his profile for Usability/Proposal, and everyone can see at a glance that he's a UI expert. Mary does a ton of patch reviews, and everyone can see that on her profile. Jane makes one commit that she's given authorship for in contrib, and that can be seen too as it always was, but who do you think is going to be recognized as the ones who had more contributions, the one with 1 commit or the ones with hundreds of other types of contributions?

So the point here is that currently we are only tracking commits on your user profile, which means that people who do lots of reviews or lots of proposals are not getting any recognition at all for helping to move issues forward. Only people whose patches were significant enough that they were recognized in the commit message (Core) or with authorship (contrib) are getting recognition. That is precisely what we want to change with this proposal, and putting all 4-6 types of contribution on the same basis in the same drop-down... how is that saying "patches are the only thing we value"?

jhodgdon’s picture

Regarding #17 - I am strongly against the idea of voting, karma, or +1 buttons on issue comments and issue contributions.

I think this type of thing makes sense in the case of a stack exchange type of thing, where you want the "best answer" to bubble up to the top. But the last thing I want to see on issues is the ability for some people to think that their contributions are not valued because no one gave them a +1. The point of this issue is that we want to make it clear that we do value all reviews, design mockups, patches, and etc. on issues, not that some contributions are somehow more valuable than others.

And here's another user story: I'm reviewing/committing docs patches. I spend several minutes reviewing the patch, decide it's good, and commit it. I do not go back and put a +1 on the user who contributed it or the other one who reviewed it previously or the other one who suggested a better way to do it. None of those people gets any "karma" for issues, although their contributions were valuable.

This is a likely scenario. It already takes a LOT of my time to review/commit docs patches. I absolutely do not want to have another thing I need to do in order to give proper recognition to people who contributed to the patch. Please do not do this.

nod_’s picture

Patches are proposal then, why would we need to differentiate them?

YesCT’s picture

Expertise area (frontend, field api, testing, usability, accessibility, performance, security, etc) is still something different, and comes from a lot of different places in our issues: components and tags. Cleaning those up would be a different issue. visualizing the types of contributions per topic area or skill type might be possible now that our issues have json. There are some visualization tools being worked on outside of d.o already.

How to display types of contributions (reviews, proposals, issue management) on profiles is a separate issue. For example: #2281763: Make Drupal.org user profiles more robust. But we do need to get the data so we can display it. Once we have the data, it will enable more ways of displaying it than just d.o profiles.

Our issue queues are different than a place for questions and answers like stack overflow. Our issues have comments that cannot be floated up via a ranking, because we are not looking for the best answer; we are trying to develop an acceptable solution. Our comments are timeline dependent. Threading comments is something else that might be useful, but again a separate issue. Once we have implemented and committed an acceptable solution, if another better solution is suggested, those show up as separate issues (follow-up or related issues).

+1's might be useful for a variety of things but again, that's really a separate issue, with it's own set of troubles.

I'm afraid of having the purpose of this issue being: to make our issues queues more of a place that builds consensus. That is more something like #2154143: Evaluating Procid, a tool to help the drupal community improve the consensus building process for d.o issues.

To focus on this issue:
Maybe it would help the discussion to have examples of contributions and then identify ways they would be categorized. (We could start a list in the summary here.. I did.)

Alternatives we also discussed:
Not having type "proposal" and just having people use "review" for those things. (but then, if there is only an issue report, what is a first proposed resolution... it's not a review yet, since there is nothing to review)
Not having type "proposal" or type "review" and just having "feedback". (I personally really wanted us to be able to use the word "review" for a type of contribution). (I think "feedback" starts to approximate "comment" and goes toward the kind of thing that the name cloud of contributors that d8mi uses, which is based on comments.)

I really think that review is the special thing we want to be able to pick out here. Whether it is a review of something communicated with an image, or review of an idea expressed as a patch, or a review of a general solution communicated with just words. Usability review, Accessibility review, Performance profiling, Architectural review ... these are all reviews.

webchick’s picture

Also adding a related issue about +1 voting on comments. Lots of good thoughts here, but they belong over there, not here. This one is just about tracking comment metadata so we can discern a patch review (and possibly a few other non-tech contributions, but I'm prepared to drop them for now if it's going to interfere with reviewers getting credit) from something else.

YesCT’s picture

Issue summary: View changes
yoroy’s picture

From my non-code contributors perspective, the 4 main buckets of Proposal, Patch, Review, Management make sense and seem to fully capture (on a high level) what's happening in an issue.
But I agree with nod_ that 'Patch' seems to narrow (of the 4 words it is by far the most specific and concret).
Maybe 'Implementation' is a better (wider) alternative for 'Patch'.

jhodgdon’s picture

While "patch" may seem overly specific, in practice uploading a patch is how we make our Proposals concrete. So if I can summarize the discussion we had over the weekend:
- If you make a coding suggestion or an architectural suggestion or a design mockup or an update that could go into the "Proposed Resolution" section of the issue summary, this is a Proposal.
- If you make a proposal in the form of uploading a patch file that implements any of these types of suggestion, this is a Patch.
- If you add a comment about a Proposal or Patch, or test something, or upload a screen shot of something in action, this is a Review.
- If you update the issue status, edit the issue summary, provide a link to a novice contributor to the coding standards or "how to patch" docs, this is Issue Management.

Because there is possibly overlap between these 4 choices, the proposal here is to have this be a multi-select.

tkoleary’s picture

+1 to Roy, "patch" is too narrow.

If, for example I upload an SVG file for a new icon, that is most definitely an implementation but it is not a patch. I would be ok with implementation or something else equally broad like "solution", as opposed to "proposal". The distinction between proposal and solution could be:

Proposal:

A description of a possible solution to the problem outlined in the issue summary eg.

We should change the extend icon in the toolbar from a plug to a puzzle piece to disambiguate it from things related to 'power'

We should use the :before CSS pseudo-element to add icons to the toolbar

Solution

A patch, code snippet or file that constitutes the solution or part of the solution to the problem in the issue summary eg.

Here is an SVG file for the puzzle piece icon in the toolbar

here is a snippet of CSS for adding the SVG file to the toolbar using the :before pseudo-element

Drawing the line at part of a solution rather than the entire solution (patch) is much more inclusive to designers and front-end developers who may be very comfortable writing a sippet of CSS or uploading a file but who may not be set up to generate a patch (ie. have a fully up-to-date local install of the dev branch of core and the tools and/or knowledge of how to make a patch to Drupal).

Another advantage of the above solution is that it also makes it easier to programmatically pre-populate the (likely) correct category if we choose to do that. The rule could be something like:

  • if the "code" tag is used or if a file upload of type ".SVG or .patch" is added pre-populate "solution"
  • if a file upload of type .jpg or .gif is added pre-populate "proposal"
  • if a file of type .png is uploaded (the pivot that could be either a mockup or an actual icon) pre-populate neither or both.

For the multi select I suggest we use the Select2 library with dismissible pills as has also been proposed for core.

To Angie's comment about keeping this decoupled from karma, I can see the sense in that but we do need to think of them together at the meta level because they both add functionality to the comment "component" increasing the number of possible interactions and we need to keep noise at a minimum and make sure that interactions do not present duplicate or conflicting functionality, which is a danger here.

jhodgdon’s picture

+1 for "Solution" instead of "Patch". Good points!

I don't think we need something as complicated as Select2 for this issue, and I believe it would complicate it getting done. This is a simple list of, what, 4 choices?

webchick’s picture

Yeah, the Select2 would be if we *also* wanted to add "taxonomy" to what "kind" of Solution, etc. (e.g. UX, accessibility, etc.)

However, I think the list of 4 is a simple starting point and we could always add to it from there.

webchick’s picture

However, I did also want to point people here at the work going on at #2295411-44: Auto-generate Git attribution info / commit messages on Drupal.org. I actually think this UI might do more than anything else to get reviewers credit on things.

I therefore think I'm going to hold off marking this up until I talk with Dries and get some of his thoughts on the goals we want to hit with this feature.

catch’s picture

If we use git notes, since that can be edited without changing git history, I'd suggest the following:

1. We can auto-generate the message, and it'll be correct 95% of the time per #2295411: Auto-generate Git attribution info / commit messages on Drupal.org.

2. If we can auto-generate it, we can also have a script do this retrospectively for previous commits against core - as long as there's an issue nid associated with the commit that should be possible. That'd retrospectively fix some of the missing reviewer credit etc.

3. If we can auto-generate it, then I think the git notes part doesn't even necessarily have to be done by a core committer - we could automate it with a git commit hook, and then manually change things if we need to. Can always have the git commit hook not change anything if it's already there.

4. That leaves the current commit message - could remove attribution from that, or keep it to patch authors in the sense that's similar to what you'd see if we were merging pull requests.

chx’s picture

> Proposal, Patch, Review, Management

I would like to add Research. Researching how something is done currently in Drupal and why it's broken (that's fun! see the stackneg issue) or see how other projects do it (finding the array cartesian idea in hoa for example is everything but trivial). These do not necessarily fall under proposal -- it can be just bug report, suggestion etc.

jhodgdon’s picture

RE #31 - good idea "Research". Motion seconded.

webchick’s picture

Issue summary: View changes

Motion carried. :) Updated the issue summary for both Research + Solution.

webchick’s picture

Also, as far as specific format of commit message/credit vs. git notes et al, see #2323715: [policy, no patch] Determine format for commit credit for individuals/organizations/customers.

joshuami’s picture

mgifford’s picture

Issue tags: +maintain

We do need to have more folks read books like Building Web Reputation Systems before we can have an informed discussion about the advantages/disadvantages about adding +1 buttons on comments.

Right now the only reputation system we have is patches in Core & modules maintained. That's really not very useful. Reputation has a real concrete value for folks and is a great incentive for people to contribute more.

I've got 8 mentions in Core commits this month (from the first of the month) which is pretty amazing. But many of them are just re-rolls of other folks patches. Things that got stalled. I went through, wanted to nudge issues along and so just re-rolled a few patches to help push an issue along. Often these re-rolls take 5 minutes or less.

Now doing a full & proper review.. That really should take quite a lot longer.. Sadly, there's very little recognition of that work. Much of the work I do in Drupal has nothing to do with the patches, but that's really the only way that my reputation in the community is recorded.

I'd love a way to acknowledge folks who summarize the threads in the issue summary so that we don't have to read through all of the discussions and there's a clear list of what needs to be done before it could be marked RTBC.

Anyways, great to see this discussion moving along.

webchick’s picture

Hm. Did you mean for that to be a comment on #42232: Help Maintainers Manage Issue Priority by Encouraging Voting or another related issue? This issue is not about voting, nor any kind of karma system. It's about better identifying the types of contributions people make in the resolution of an issue (including adjusting issue summary data).

mgifford’s picture

I was going back to #17 & #19 where voting was being discussed. It was probably a tangent then which I brought up again. Sorry.

We do have a reputation or karma system right now, we just aren't calling it that. Mentions in Core are a type of reputation system. Number of modules we maintain. Official volunteer positions. We don't talk about it that way, but it is.

I do understand that we need to do a better job identifying the types of contributions made in the resolution of issues. How we choose to abstract that out in order to recognize those who are contributing is going to be a challenging task.

However, we are doing this because we want to recognize people's contributions so that they will have a greater incentive to participate in the future.

YesCT’s picture

oseldman’s picture

Title: [policy, no patch] Allow crediting reviewers (and other non-coders) as first-class contribitors » [policy, no patch] Allow crediting reviewers (and other non-coders) as first-class contributors
drumm’s picture

With #2369159: Extend crediting UI to include organizations & customers now deployed, Drupal.org will store credit for issues as maintainers update the issues.

mgifford’s picture

Great to hear about this progress @drumm.

joshuami’s picture

kattekrab’s picture

Is this issue still in progress?

xjm’s picture

Since April (when effulgentsia and I joined the committer team and org credits were deployed on d.o), all six Drupal 8 committers have been treating all substantial issue contributions as first-class contributions. That is, anyone who posts a helpful issue report, suggestion, or review receives issue credit in the crediting field and is mentioned in the commit message for the issue, based on the committer's best judgment. Where possible, we also include credit for contributions like mentoring and issue triage.

So, in terms of the title of this issue, Drupal 8 core at least has already adopted the policy that all substantive contributions to an issue are treated equally. We do not distinguish between different types of contribution as the issue summary proposes, because Drupal.org does not support such categorization, which to me seems just fine. Contribution is contribution.

There are still improvements that can be made to reduce some of the burden on committers for this and improve the contribution credit tools overall, and there are open issues for any number of proposed improvements:
https://www.drupal.org/project/issues/search?issue_tags=d.o%20crediting%...

Hope that answers the question, @kattekrab! I'm not sure what should be done with this particular issue, though.

P.S. for everyone: If you did not receive credit for a particular issue but aren't sure why, reach out to the committer. Sometimes we miss someone, and we can update the issue credit in those cases. In other cases, we can give suggestions on how to post a more helpful review/comment/etc. to ensure you get credit for your future contributions.

chx’s picture

Yes contribution is contribution but just for the archives let me note that good, insightful reviews are way more valuable than a patch. Writing software is easy, reviewing / understanding / maintaining is hard.

dawehner’s picture

You are absoutely right, we should not try to categorize contributions, we want to communicate that each contribution is equally valuable, even I agree with chx 100% that good reviews are much harder and actually more valueable, not just because of its rarity.

kattekrab’s picture

@xjm - awesome! Thanks so much for the update. This is really great news.

and ++ @dawehner and @chx for acknowledging reviewers.

and ++ "contribution is contribution"

/me = happy little vegemite.

I think that means - for this issue specifically - we could call it fixed? I'm going to change the status to Needs Review, anyone who agrees this is resolved, could move to RTBC, and perhaps @ianthomas_uk - if you're happy you could mark it fixed?

Or if there is further work to do, we should create new specific, actionable issues, and reference them here as related issues.

kattekrab’s picture

Status: Active » Needs review
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Let's follow the plan of kattekrab

xjm’s picture

Assigned: Unassigned » xjm
Status: Reviewed & tested by the community » Needs work
Issue tags: -maintain +d.o crediting contributions, +Needs issue summary update

After thinking about it, I think what we should do is put a page in the handbook. I started looking into this over the weekend.

Setting NW since that needs to be completed and since the current summary does not reflect the implemented solution. :)

Thanks @kattekrab and @dawehner!

David_Rothstein’s picture

Title: [policy, no patch] Allow crediting reviewers (and other non-coders) as first-class contributors » [meta] Allow crediting reviewers (and other non-coders) as first-class contributors

The documentation here and here already recommends crediting non-code contributions, but it could be improved.

However I don't believe this can be considered fixed based on the idea that maintainers will assign credit manually. That's the solution that has been tried for the past decade or so, and it doesn't work. The new UI helps, but only so much; I think webchick's comment in #8 is still correct:

Particularly in the core queue, committers are simply not going to be able to read all 300 comments and adequately pick out the ones that feel "commit-mention-worthy."

Although we are now trying to do that in the core queue, I can testify from first-hand experience that it's extremely time-consuming, error-prone, and subjective. Two different people giving commit credits for the same issue (e.g. D7 and D8) often come up with different ideas of which non-code contributions to credit.

And I'm not sure how much contrib maintainers are doing this at all...

Meanwhile, the issue credits apparently now determine the order in which organizations get listed at https://www.drupal.org/drupal-services, which ups the stakes significantly and is a responsibility I really don't want to have.

We should have some kind of automation or crowdsourcing for this, and it looks like the discussion in this issue has basically been a meta issue to come up with an overall plan for both those topics, but with no resolution yet.

  1. Automation:
    • Can we consider just having the UI check all the credit boxes by default (for everyone who commented on an issue)? The boxes could still be unchecked manually if desired. But at least this way we'd default to crediting extra people rather than not enough.
    • Or failing that, could we at least check more of them by default? If you upload a screenshot (but not a patch file) to an issue, or if you use <code> or <?php tags in the comment body, that's almost certainly a meaningful review and your box should be automatically checked in that case. (See also tkoleary's comment in #26.)
  2. Crowdsourcing:
    It seems like a lot of the proposals in this issue for categorizing comments by contribution type are a way to crowdsource the classification of who should get credit on the issue (while still letting the maintainer make the final determination).

    Also as jhodgdon wrote in #18:

    The point of being able to mark your contributions up in this way is that we are going to ****track and recognize all types of contributions****. For instance, Joe makes a ton of UI mockups, so he gets recognition on his profile for Usability/Proposal, and everyone can see at a glance that he's a UI expert. Mary does a ton of patch reviews, and everyone can see that on her profile.

    Agreed that would be really useful. I question whether people would actually bother filling this out (or if they would fill it out accurately), but if it's visible prominently then they may have an incentive to do that.

xjm’s picture

Can we consider just having the UI check all the credit boxes by default (for everyone who commented on an issue)? The boxes could still be unchecked manually if desired. But at least this way we'd default to crediting extra people rather than not enough.

Or failing that, could we at least check more of them by default? If you upload a screenshot (but not a patch file) to an issue, or if you use or <?php tags in the comment body, that's almost certainly a meaningful review and your box should be automatically checked in that case. (See also tkoleary's comment in #26.)

Well, right now, the boxes can't be unchecked if a patch is uploaded; they'll just be rechecked and the changed credit saved the next time a maintainer comments on the issue. So that bug would need to be fixed first.

Also, in general, I'd be opposed to auto-credit for comments because it is so very gameable. We're already seeing the marketplace ordering impact the nature of contributions. I think that maintianer discretion should be involved, and granting credit is a positive action, whereas taking away credit would feel punitive and overall be more work IMO.

My process is thus:

  1. I scan the summary and do my own review of my patch first.
  2. I look at the credit field and scan down the summary of patch files, comments, and other files per contributor.
  3. For each name on the list, I search on the page for the user's name on the issue and glance over their replies. (I'd love a little feature that let me click on their name in the crediting field and scroll through a summary of their comments in a modal or something. I'd also like a pony.)
  4. As I glance at each comment, I consider:
    • Anything unproductive or inappropriate doesn't get credit.
    • Comments like "I tested and it works" or "Looks good" or tweaking issue metadata don't get credit by themselves.
    • Tweaking the issue metadata by itself doesn't get credit by itself.
    • If someone updates an issue summary, I check the revision to see what the update was. Trivial tweaks don't get credit, but adding or clarifying information does.
    • Productive questions and feedback get credit.
    • Code reviews that are more than just nitpicks get credit. If it's just nitpicks I make a judgment call.
  5. I err on the side of granting credit if I'm on the fence about a comment.

It takes a minute or so on a short issue; maybe as much as 10 mins on a long issue, but fortunately those are rarer. As webchick says, a 300-comment mega issue is onerous to credit and for those it really helps to have a recommended commit message crowdsourced on the issue, but those issues are also only a tiny percentage of the patches we commit. I don't find it too burdensome because it's always been part of committing a patch for me, and it also makes sure I am aware of what's gone into getting the issue done, which in turn helps me consider things during my review and make sure no aspects were overlooked.

One suggestion that's been made is opening it up to maintainers with the "maintain issues" permission, which at least spreads the responsibility to 10x more people for core who are trusted for the project. I'd be in favor of doing that as a start, once we have guidelines in place for what gets credited. I think crowdsourcing somehow what gets checked by default or not may be a better long-term solution. However, I think maintainers making the final call is a good thing.

catch’s picture

Replying to #46/#47.

For commit credit, and by extension git blame, I do think it's useful to differentiate different types of contributions to issues, let's say we have:

Designed by tkoleary
User experience tested by Bojhan
Coded by Wim Leers
Reviewed by dawehner
Committed by webchick

Then if I'm looking at the git history in two years, and have a specific question, I get some kind of idea of who I should ask, without having to know the likely roles of each of those people through personal interaction.

Also I think it was Jennifer mentioned on irc (not sure if there's an issue) about automatically crediting committers when they commit patches (which pre-supposes some level of review of the patch beforehand too). In terms of helping to fix an issue, then sure assigning credit for actually committing them is fine. However right now there's a big difference between a commit by me, where the message doesn't include 'catch', and one where it includes 'catch' - because the latter only applies if I either rolled patches or as of the recent change, did active reviews during the lifetime of the issue. Once you add 'catch' to the end of the list of participants for every core issue I commit, that distinction completely disappears.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.