Allow more maintainers to grant credit

Credit on issues can currently assigned by project maintainers with “Write to VCS” access only. All the maintainers of the Project Applications queue have “Maintain issues”.

I think it would be okay to broaden access to assign credit to everyone with “Maintain issues”. For core this would broaden the number of people saving credit from the 13 committers (some retired, like me) to about 80 people, I think corresponding to people who have been in MAINTAINERS.txt. I’d want a comment from a core committer before going ahead and doing this.

I’d like to avoid special behavior per-project, which is why I wanted to check on the impact on core before going ahead and changing the behavior.

It's possible to see and use the option to give credit, and for it not to work? (And this is expected?)

Yes, the UI shows for all logged-in users since it doubles as the commit message/command generator. Maintainers who are able to save credit on Drupal.org see a couple UI additions, the “Giving credit to …” text at the top of the fieldset, and “As a project maintainer…” help at the bottom of the fieldset.

we could simply give project maintainers VCS access even though they won't use it

Yes, that would be a quick way to solve this for project applications.

I think we still want to consider whether to actually also allow this for subsystem maintainers in core, so setting back to active.

Since this needs feedback, I think this is a good status.

I'm all for sub system maintainers to get credit for reviewing applications, most but likely not all are also project maintainers(though I know I wasn't for a year or two after becoming a core subsystem maintainer, thanks Brian).

Everyone can now get issue credit when they review project applications, regardless of sub system maintainership, which is a core thing.

Currently you are a maintainer who can assign issue credit if you have “Write to VCS” maintainership of the project. This is in line with who gets to write the commit messages mentioning names when committing.

It may be helpful to have more maintainers be able to update the credit on issues. In particular, core has everyone from MAINTAINERS.txt able to “Maintain issues.” (That would have been a solution to the original issue report here, but that is solved another way now.)

I think this has the most impact for core, and I’m comfortable with contrib following core’s lead here.

Ok may have misunderstood and still do what and from whom the feedback is needed for this issue?

Mostly I’m looking for feedback from the expert creditors, the core maintainers.

Commitment from subsystem maintainers to follow the same crediting standards would be good to hear.

@alexpott and I were just discussing this at Front End United, and were both lamenting the fact that more trusted people don't have access to adjust this field. In our opinion, it would be wonderful for everyone with "Maintain issues" permissions to have the ability to adjust credit, not only for empowerment reasons, but because it would save a TON of time at commit-time, so we could focus on that, and let the people actually involved with any given issue decide the crediting.

So, HUGE +1. :) Not sure what to set this to, but basically "RTBC but needs a patch."

Makes total sense to me. My reading of @xjm in #8 is also that she was in agreement :)

Sounds like this is agreed on. Adding the solution to the issue summary:

Broaden access to assign credit to everyone with “Maintain issues” maintainership of a project, in addition to “Write to VCS”.

Ok, just made the meetings component a thing, so having this to go in tandem with it would make it AMAZING.

Since I suspect this is only a 1-2 line change (famous last words...) going to see if I can try my hand at a patch.

This seems to be all that's needed.

Test site: https://initiatives-drupal.dev.devdrupal.org/project/drupal/issues/2969420

The set up:
Dries is a project maintainer with full access.
bacon is a project maintainer with only "Maintain issues" permissions.
Dries checks off credit to both he and webchick in #4.

Without the patch:
bacon cannot affect issue credit.

With the patch:
bacon unchecks credit for webchick in #5.

Unfortunately the credit messages aren't showing up, and there are errors at the top of the form, but I suspect that's to do with the limited set of content on dev environments vs. something in the patch specifically, since it's literally a one-line change.

+++ b/drupalorg_project/drupalorg_project.module
@@ -720,7 +720,7 @@ function drupalorg_project_form_node_form_alter(&$form, &$form_state) {
-        $is_maintainer = !empty($maintainer_access['write to vcs']);
+        $is_maintainer = !empty($maintainer_access['maintain issues']);

We could maintain the existing behaviour by doing:

// Allow users with either 'write to vcs' or 'maintain issues' to assign issue credit.
$is_maintainer = !(empty($maintainer_access['write to vcs']) && empty($maintainer_access['maintain issues']));

There's probably not too many instances where someone has 'write to vcs' and not 'maintain issues' - but it is possible. Here's a test to prove the logic is as expected... https://3v4l.org/rTQeS

Oooh, good call. Here's a patch that incorporates that improved logic, along with a comment, cos I would never have deduced that behaviour from a quick skim. :)

Also spun off #2978223: Issue credit form looks to non-maintainers like they could do something with it but they in fact can't since this one-liner took AGES longer than it should've due to the current UX for this fieldset for non-maintainers.

For me this is harder to read. As a human I would have said: "Allow to set credit if they have permission A or permission B". Putting an AND in there seems weird :(

@dawehner is right for reasons beyond me I was over complex.

!empty($maintainer_access['write to vcs']) || !empty($maintainer_access['maintain issues'])
is way better cause then you get to read the OR :) @dawhener++

Oh great, that's much more legible. :) I kept the comment there because why not... it's a long function, and it's not clear until later what $is_maintainer is used for.

Looks good, but there is one other place the maintainer is considered, the last time a maintainer who can grant credit commented is used in figuring out the defaults. Currently, comments made after a maintainer last commented get credit by default if they uploaded a file; along with the credits assigned when that maintainer commented also being defaults.

Since this changes when a crediting maintainer last commented, pay attention as the defaults can be a bit thrown off by this change. (I think implementing #2972225: Move from opt in to opt out for contribution attribution first wouldn't avoid this, just make it differently inconsistent.)

I moved the comment up, along with a single array to keep track of what these permissions are. And converted the two uses to use that array.

This has been deployed, thanks!

Followup to this: #3509194: Allow all project maintainers to grant issue credit