Distinguish official vs non-official reviews, add tool tips, and upgr…

[william-allspice]

This Pull Request is a follow up to #31886:

1. Adds a UI indicator between official (green) and unofficial (grey) approved pull requests on the Pull Request page (as suggested by @kdumontnu )
2. Adds tooltips adding clarity to the type and status of a review on the Pull Request page (as suggested by @kdumontnu)
3. Updates text adding more clarity to required approvals (as suggested by @kdumontnu)
4. Updates text on the branch settings page explaining what branch approval limitations (as suggested by @yp05327)

Official approval:

Unofficial approval:

Rejected approval:

Stale approval:

Requested review tooltip:

Updated text for approvals:

Updated text for allowlisted/whitelisted approvals:

Protected branch settings text:

Comments list:

[william-allspice]

Note: I made the decision to prioritize the unofficial vs official indicator over stale. So an unofficial reviewer will not see a stale indication. To me, this made the most sense since seeing why a review doesn't count towards a protected branches approval limit felt more pertinent then seeing it was stale.

Let me know if you disagree with this logic.

[kdumontnu]

Minor suggestion - thanks!

[yp05327]

In the code, it is whitelist, and for the UI, it is allowlist..... Just similar to activities, the naming has no consistency.
But as there are already two approves, maybe it is ok.

And another thing, it seems that you didn't changed the icon color here?
I think it is in templates/repo/issue/view_content/comments.tmpl, but there's no change in this PR.
ps: screenshot is copied from #31886 (comment)

[william-allspice]

@yp05327 thank you for pointing out the discrepancy with the comment list.

The way the system exists today, an "official" review will become "unofficial" as soon as an approved user submits a second review. I'm not sure displaying a grey check mark for an official reviewers previous review makes sense. The tool tip would definitely not. As far as I can tell, the real use for the Official boolean is just to optimize a SQL count query here: https://github.com/go-gitea/gitea/blob/main/models/issues/pull.go#L384

Furthermore, we currently don't show a different color for stale reviews. You can see here, its either red or green (no yellow): https://github.com/go-gitea/gitea/blob/main/templates/repo/issue/view_content/comments.tmpl#L383 I'm not certain if that difference is intentional.

In my opinion, further modification of the official boolean feels out of scope for this change. But please let me know if you feel differently.

[kdumontnu]

@yp05327 thank you for pointing out the discrepancy with the comment list.

The way the system exists today, an "official" review will become "unofficial" as soon as an approved user submits a second review. I'm not sure displaying a grey check mark for an official reviewers previous review makes sense. The tool tip would definitely not. As far as I can tell, the real use for the Official boolean is just to optimize a SQL count query here: https://github.com/go-gitea/gitea/blob/main/models/issues/pull.go#L384

I actually think this might be intended behavior (or at least acceptable). Once the user leaves a second review the previous one is no longer "official". It doesn't necessarily indicate the reviewer doesn't have sufficient permissions (for instance, if they approve, then reject, the previous approval doesn't count). I still think it's better than showing green for everyone.

Also, the other intention of official approver boolean was to continue to count the approval even if that user is removed from the project later (though, to be fair I'm not sure why someone would want this behavior).

[yp05327]

There are two tables. One is Review, and the other one is Comment.

The way the system exists today, an "official" review will become "unofficial" as soon as an approved user submits a second review.

This is recorded in Review table, and can only have one record. So it will be changed when user submits a second one.
Then what I said above, it is in Comment table, and can have several records. They are not the same thing.

Wait, comment saved the review id, not the details, so it will be changed...... IMO, this should not be changed for comments.
@lunny @wolfogre
How do you think about this.

[kdumontnu]

There are two tables. One is Review, and the other one is Comment.

The way the system exists today, an "official" review will become "unofficial" as soon as an approved user submits a second review.

This is recorded in Review table, and can only have one record. So it will be changed when user submits a second one. Then what I said above, it is in Comment table, and can have several records. They are not the same thing.

Wait, comment saved the review id, not the details, so it will be changed...... IMO, this should not be changed for comments. @lunny @wolfogre How do you think about this.

My thought here was that as soon as the user leaves an updated review, the previous one would change to grey (unofficial). This reflects the database state & should be pretty clear/intuitive. The "green" checks clearly indicate which reviews are being counted to the total. Grey can be because the user doesn't have sufficient permissions or because they have left a second/third review.

I think it would be much more confusing as it currently is, where I can "approve" and see a green check, then "reject" and have a red x - it's slightly ambiguous that my previous green check doesn't count toward approvals. Marking it grey helps imo.

[kdumontnu]

Leaving additional review to reflect the latest changes.

[lunny]

There are two tables. One is Review, and the other one is Comment.

The way the system exists today, an "official" review will become "unofficial" as soon as an approved user submits a second review.

This is recorded in Review table, and can only have one record. So it will be changed when user submits a second one. Then what I said above, it is in Comment table, and can have several records. They are not the same thing.

Wait, comment saved the review id, not the details, so it will be changed...... IMO, this should not be changed for comments. @lunny @wolfogre How do you think about this.

That should be another issue/bug?

[yp05327]

That should be another issue/bug?

I don't think so.
The operation history (the comment mentioned above) should not be changed, but it can be changed now.
If a PR introduced a known bug before merge, I don't think it should be another bug/issue.

[william-allspice]

That should be another issue/bug?

I don't think so. The operation history (the comment mentioned above) should not be changed, but it can be changed now. If a PR introduced a known bug before merge, I don't think it should be another bug/issue.

Just for clarity and to wrap this PR up.

The other issue/bug we are discussing is the implementation where a previously approved Official=True review will become Official=False after an official reviewer leaves an additional review. I believe this is the case because of the use of Official in a count query: https://github.com/go-gitea/gitea/blob/main/models/issues/pull.go#L384. As an aside, I believe we could accomplish the same result keeping Official=True and just using a SQL DISTINCT keyword on the ReviewerId of a Review.

This is existing behavior and NOT being modified in this PR. In my opinion, modifying or discussing changes to this behavior is outside of the scope of this PR.

The change reflected in this PR is that the comments list will also show a grey checkmark for all approved reviews that do not count towards the protected branches minimum approval limit. This includes approvals that may have previously counted, but no longer do. With these changes, a user will be able to count the green checkmarks in the comment list and they will match the approval count in the merge pr section. These suggestions were made by @yp05327 and concurred by @kdumontnu previously in this PR.

I'd really like to get this PR over the finish line so users can quickly determine which approvals do and do not count towards their protected branch limit. Are there any additional changes needed to get this merged?

[lunny]

OK. So this change may result in a new inconsistent between comment approval status and sidebar approval status. I think this is what @yp05327 is concerned about.

[kdumontnu]

OK. So this change may result in a new inconsistent between comment approval status and sidebar approval status. I think this is what @yp05327 is concerned about.

I don’t think there is any issue here, especially with the latest change. @yp05327 can you approve if you’re in agreement?

as soon as the user leaves an updated review, the previous one will change to grey checkbox (unofficial). This reflects the database state & should be pretty clear/intuitive. The "green" checks clearly indicate which reviews are being counted to the total. Grey can be because the user doesn't have sufficient permissions or because they have left a second/third review.

imo, past reviews changing from green to grey is a good thing. It reflects what is counted towards approvals.

[lunny]

OK. So this change may result in a new inconsistent between comment approval status and sidebar approval status. I think this is what @yp05327 is concerned about.

I don’t think there is any issue here, especially with the latest change. @yp05327 can you approve if you’re in agreement?

as soon as the user leaves an updated review, the previous one will change to grey checkbox (unofficial). This reflects the database state & should be pretty clear/intuitive. The "green" checks clearly indicate which reviews are being counted to the total. Grey can be because the user doesn't have sufficient permissions or because they have left a second/third review.

imo, past reviews changing from green to grey is a good thing. It reflects what is counted towards approvals.

Yes. So maybe the approval status in the comment render template should be also changed to keep it consistent.

[william-allspice]

OK. So this change may result in a new inconsistent between comment approval status and sidebar approval status. I think this is what @yp05327 is concerned about.

I don’t think there is any issue here, especially with the latest change. @yp05327 can you approve if you’re in agreement?

as soon as the user leaves an updated review, the previous one will change to grey checkbox (unofficial). This reflects the database state & should be pretty clear/intuitive. The "green" checks clearly indicate which reviews are being counted to the total. Grey can be because the user doesn't have sufficient permissions or because they have left a second/third review.

imo, past reviews changing from green to grey is a good thing. It reflects what is counted towards approvals.

Yes. So maybe the approval status in the comment render template should be also changed to keep it consistent.

Awesome. It sounds like we are all on the same page.

Here is a screen shot showing how this PR will make sure your latest review on the right hand side and in the comments list will match. And how older, dismissed, reviews will be grey.