Gitea: RFC: Refactoring the review system

[3l73]

RFC: Refocturing the review system

This issue I an request for comment and about changing the whole review system.

A colleague of mine recommend to introduce rebuild the review system as a new feature in it's own. This allows use to use current infrastructure and setup a structure in depend from the current code.

Since I will follow his recommendation, some terms will differ from what you currently know.

Let's talk about audit's.
A pull request can contain one or more of it. An audit is done by an author and contains a list of audit tasks.
The audit it self can reject or agree the pull request.

An audit task can be connect to a change done by the developer.
It contains comments and a status:
Done? True/False

As an audit must not connect to an technical change, this allows the developer starting general diskussions about the code.

If a task is completed, the author of the audit mark it as completed or done.
The Administrator, can mark all task as done and overwrite the authors decision.

If an audit contains task that are not marked as done, it is not fulfilled.
As a consequence the pull request could not merged into the selected branch.

An audit can created by a human person or a bot/hook.

For a example a bot could check the "code smell" and block the pull request.
In this case the bot will update the audit after all pull request.

The administrator can decide what audit have to e fulfilled in order to merge, and how many audit's have to be done and completed.

As you see, additional information is required, in order to decide if an audit task is required or optional.

What additional should be checked?

• if a code changed, removed or inside another file, an audit task has to be updated

What is your opinion?
Have you ideas what should be extended?

---

Features that need to be done

• Pull Request should not be able to merged, if one ore more audits are denied

Features that are nice to have

• TBD

Features to be discussed

• Administrator allowed to override status of an audit

[saitho]

cc @jonasfranz

[jonasfranz]

What's the difference between the current review system and the audits? If I understood correctly, you want to add a feature to mark change requests as done?

[3l73]

There are some differences:

1. The internal structure
   
   
   
   1. You can add task that have to be fulfilled even without an connection to code, that is currently not possible
   
   
   2. An audit has a status that said, fulfilled or still something not done
   
   
   3. A change has to be accepted
   
   
2. An audit task has a status, comment are connected to the audit task and a single entity. Currently it is confusing how it handled
3. Easy to understand. If audits contains task that are not fulfilled... Audit is not accepted
4. Currently if someone change the code, what the reviewer written to the code is in some way "lost"
5. The author of the audit has to accept the change

I had a look at the source of the current system, so most of the points are more connected to the internal aspect.

The system not really change, but allows some interesting things.

Call it an audit instead of a review comes from a discussion with a colleague.

His thoughts goes into direction to introduce the feature and switch it at a point when it is functional

[davidsvantesson]

I work in an industry with a lot of quality standards, so I think I get some of the points you are making. Regarding the naming (review, audit or something else) I think that has very little with the tool to do, you can call it what you like.

Some comments:

• You can use pull request templates to add e.g. default checklists
• With a tool like LGTM that is used here (but more advanced rules), you can set up specific rules for what is required to merge.
• I think the approved / request changes scheme is quite good in general, but might be limiting for some people and cases.

Is this issue inspired by Gerrit? (A tool I dislike but for other reasons). They have something called review labels and you can add custom review labels. It allows to set different values depending on label and you can configure different rules for disabling merge. Could that fulfill what you are looking for?

4\. Currently if someone change the code, what the reviewer written to the code is in some way "lost"

The review system needs to be improved, for sure.

[3l73]

The name audit was used as a placeholder.
If you call it review it is fine too. As you say, it is just a word.

Since I have no experience with Gerrit, I can say it is not inspired by it.

Template can be an option.
There were some situations, that leads a colleague and me to place a comment and start general discussions.
I am personally not a friend having a discussion over a review tool. A direct talk is the most best way in this cases. Though there are times where it is not possible to have a personal discussion, so leave a comment and block the merge would be a nice feature.

Currently it is possible to merge a pull request even if there is a denied review.
In some cases this is good if a reviewer is not possible to agree because of vacation or other circumstances.
A solution could be that the administrator accept the review or he can assing it to another developer who can mark the other review as accepted.

In the end a pull request should not be merged if some reviews are not accepted.

What kind of regulations should applied on a project should decided by the owner or admin of the project.

[davidsvantesson]

Currently it is possible to merge a pull request even if there is a denied review.

Apparently. I don't think that is good behavior, should at least be a setting.

[3l73]

I would collect all ideas at the top if this issue, so that they are located at on place.

[gerdemann]

I would definitely have two more features that I miss a lot at the moment:

1. comments on the code in the individual commits (https://github.com/go-gitea/gitea/issues/4898)
2. automatically add default reviewers when creating a PR

[davidsvantesson]

Now (coming release) there is a setting to block merge if there are rejected reviews.

[3l73]

Now (coming release) there is a setting to block merge if there are rejected reviews.

I like that feature.
May be we need a role to allow override a rejected Pull-Request in case the developer is not able to agree the Pull-Request (illness, vacation or what ever).

By the way, I miss the "Delete branch after merge" feature.

[6543]

@3l73 to the new feature RepoAdmin is able to merge eafen review is not complete or status checks failed ...

[6543]

@3l73 and for the other things ... the easiest way is a ticket system trigerd by webhook witch set status check to false until tasks are completed

[davidsvantesson]

@6543 How would you do the ticket system?

Maybe there should be a possibility to make issue comments editable by other users (currently only writer of comment and repo admin can edit).

[6543]

@davidsvantesson

Maybe there should be a possibility to make issue comments editable by other users

dont think so ... could be abused

ticket system

I dont know a lot of ticket systems and its integration ... for example zammad has webhooks ...
And like CI status use tiket status ...

[davidsvantesson]

I don't mean anyone can edit anytime, there must be some way to set that.
The problem is today the issue or pull creator makes the first post with maybe a list or something, and noone else can edit that. There could for example be a way to set "make editable by others", so if you have a review protocol other persons can edit while the review procedes.

[6543]

@davidsvantesson like githubs [ ] Allow Maintainer to edit option on pulls?