Problem/Motivation

There're some cases we have a patch in issue, and it looks okay. But we have some unclear comments. It needs further feedback from maintainers or others.

In those cases, we can't set to:

"Needs Works" - this is implied the patch does not look good.
"Needs Review" - this is implied the patch is okay and no needs for further discussion. (Mainly focus on code reviews). Sometimes when you marked issues as it, it also assumed you agreed with the patch and directions.

For example:
Case 1:
https://www.drupal.org/node/2588013#comment-10858396

This is a new patch with new idea, it's more like asking for feedback than actual final reviews. And then Comments #40 & #44 are wrong rerolls.

Case 2:
https://www.drupal.org/node/2725255

In this case, we have to pick one solution before an actual patch. Any patches work and unrelated comments can be noises to contributors / reviewers. You waste time on it if you working for Plan A but we picked plan B at final.

Case 3:
https://www.drupal.org/node/1849750

Some tasks are very hard to implement. It prevents this case happened: https://www.drupal.org/node/2707291
We may spend 10 hours to create a patch and another 10 hours to do code reviews but rejected with ONE min of UX reviews. (Many cases can be review without a patch)

Case 4:
https://www.drupal.org/node/1931632

In few tasks & new features requests, if not all of these people say Yes, usually it won't move forward: Drupal Comitters, Subsystem maintainers, experts in that sections, few contributors with hidden power to say no.

Proposed resolution

Add new status 'Needs feedback'.

Comments

droplet created an issue. See original summary.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented

Needs review doesn't imply the patch is good. It just means the patch needs more eyes. To me, review implies I am going to give a feedback, whatever it is negative or positive; I don't see any need to add a new status, when the new one is practically covered by an old one.

mlhess’s picture

mlhess CreditAttribution: mlhess as a volunteer commented
Status: Active » Closed (outdated)
droplet’s picture

droplet CreditAttribution: droplet commented
Title: Add "Needs Feedback" Status » Split "Needs Review" status into "Needs Feedback" & "Needs Code Review"
Status: Closed (outdated) » Active

@kiamlaluno,

No, Needs Review will trigger some reviewers to do functionality tests and then they usually marked as RTBC. It's not what we wanted. Patch works don't mean it's correct direction. A clear issue Status could help these reviewers to skip this issue. and the right person jumps in to give feedback and prevent reviewers set back to Needs Work because it's just can't apply the patch.

one more:
https://www.drupal.org/node/2830882#comment-12138857

I have no idea why I need to code a patch if we haven't unclear directions.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Category: Plan » Feature request

I still don't feel it's a necessary change.
The values for the Status field are set in code (features/drupalorg_issues/drupalorg_issues.features.field_base.inc). If this change will be considered necessary or useful, it's a feature request for the custom modules used for drupal.org.

apaderno’s picture

apaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
CreditAttribution: apaderno as a volunteer commented
Project: Drupal.org site moderators » Drupal.org customizations
Version: » 7.x-3.x-dev
Component: Site organization » User interface

I meant this project.

ivnish’s picture

ivnish CreditAttribution: ivnish commented

+

Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Title: Split "Needs Review" status into "Needs Feedback" & "Needs Code Review" » Add status "Needs Feedback" to issues
Status: Active » Needs review

I still don't feel it's a necessary change.

Me too. I'm ambivalent. On one hand it may be useful on the other it can be confusing. I suggest to leave the "Needs Review" status. We have been using this for a long time... Let's add an additional "Needs Feedback".

    'settings' => array(
      'allowed_values' => array(
        1 => 'Active',
        13 => 'Needs work',
        8 => 'Needs review',
+      19 => 'Needs Feedback',
        14 => 'Reviewed & tested by the community',
        15 => 'Patch (to be ported)',
        2 => 'Fixed',
        4 => 'Postponed',
        16 => 'Postponed (maintainer needs more info)',
        3 => 'Closed (duplicate)',
        5 => 'Closed (won\'t fix)',
        6 => 'Closed (works as designed)',
        18 => 'Closed (cannot reproduce)',
        17 => 'Closed (outdated)',
        7 => 'Closed (fixed)',
      ),
      'allowed_values_function' => '',
Krzysztof Domański’s picture

Krzysztof Domański
Location Poland
CreditAttribution: Krzysztof Domański at abventor commented
Status: Needs review » Active
quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue summary: View changes

+ 1 Right now I think the proposal, as applied to the cases presented, is a good idea.

It is true that using NR could work but I have seen many cases, even with a decent IS or last comment, that someone reviews a patch or does a reroll when something else needed to be solved first.

For background, I reviewed the definitions for Needs review and Needs work and the both emphasize the presence of patch and that is what is to reviewed or worked on. Whereas, 'needs feedback' is a very quick and efficient way to inform the reviewer to look elsewhere for what is needed. And hopefully that is clearly stated in the IS.