From 5cb4c901bbdc0f3ef73b53bdfa06196af42cdff9 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Sun, 7 Jun 2020 10:03:36 +0800 Subject: [PATCH 1/2] Document code design of approval rules In this development document, major parts of the application being used to implement the approval rules feature is explained in a high level manner. The goal is to help contributors understand the code design easier and to also help those familiar with the codebase to see if there are parts to improve. This doesn't document the entire feature yet as it's missing other parts specified in the TODO section of the document. --- doc/development/approval_rules.md | 271 ++++++++++++++++++++++++++++++ 1 file changed, 271 insertions(+) create mode 100644 doc/development/approval_rules.md diff --git a/doc/development/approval_rules.md b/doc/development/approval_rules.md new file mode 100644 index 00000000000000..8cd85ed3d4f496 --- /dev/null +++ b/doc/development/approval_rules.md @@ -0,0 +1,271 @@ +# Approval Rules **(STARTER)** + +This document explains the backend design and flow of all related functionality +about merge request approval rules. + +This should help contributors to understand the code design easier and to also +help see if there are parts to improve as the feature and its implementation +evolves. + +It's intentional that it doesn't contain too much implementation detail as they +can change often. The code should explain those things better. The components +mentioned here are the major parts of the application for the approval rules +feature to work. + +**NOTE:** This is a living document and should be updated accordingly when parts +of the codebase touched in this document changed/removed or when new components +are added. + +## Data Model + +```mermaid +erDiagram + Project ||--o{ MergeRequest: " " + Project ||--o{ ApprovalProjectRule: " " + ApprovalProjectRule }o--o{ User: " " + ApprovalProjectRule }o--o{ Group: " " + ApprovalProjectRule }o--o{ ProtectedBranch: " " + MergeRequest ||--|| ApprovalState: " " + ApprovalState ||--o{ ApprovalWrappedRule: " " + MergeRequest ||--o{ Approval: " " + MergeRequest ||--o{ ApprovalMergeRequestRule: " " + ApprovalMergeRequestRule }o--o{ User: " " + ApprovalMergeRequestRule }o--o{ Group: " " + ApprovalMergeRequestRule ||--o| ApprovalProjectRule: " " +``` + +### `Project` and `MergeRequest` + +They are defined in `ee/app/models/ee/project.rb` and `ee/app/models/ee/merge_request.rb`. +They extend the non-EE versions since approval rules is an EE only feature. +Associations and other related stuff to merge request approvals are defined here. + +### `ApprovalState` + +```mermaid +erDiagram + MergeRequest ||--|| ApprovalState: " " +``` + +It is defined in `ee/app/models/approval_state.rb`. It's not an actual +ActiveRecord model. This class encapsulates all logic related to the state +of the approvals for a certain merge request like: + +- knowing the approval rules that are applicable to the merge request based on + its target branch +- knowing the approval rules that are applicable to a certain target branch +- checking if all rules were approved +- checking if approval is required +- knowing how many approvals were given or still required + +It gets the approval rules data from the project (`ApprovalProjectRule`) or the +merge request (`ApprovalMergeRequestRule`) and wrap it as `ApprovalWrappedRule`. + +### `ApprovalProjectRule` + +```mermaid +erDiagram + Project ||--o{ ApprovalProjectRule: " " + ApprovalProjectRule }o--o{ User: " " + ApprovalProjectRule }o--o{ Group: " " + ApprovalProjectRule }o--o{ ProtectedBranch: " " +``` + +This model is defined in `ee/app/models/approval_project_rule.rb`. It has the +following attributes: + +A record is created/updated/deleted when an approval rule is added/edited/removed +via project settings or the [project level approvals API](../api/merge_request_approvals.md#project-level-mr-approvals). The `ApprovalState` model get these records when approval rules are not +overwritten. + +The `protected_branches` attribute is set and used when a rule is scoped to +protected branches. See [Scoped to Protected Branch doc](../user/project/merge_requests/merge_request_approvals.md#scoped-to-protected-branch-premium) +for more information about the feature. + +### `ApprovalMergeRequestRule` + +```mermaid +erDiagram + MergeRequest ||--o{ ApprovalMergeRequestRule: " " + ApprovalMergeRequestRule }o--o{ User: " " + ApprovalMergeRequestRule }o--o{ Group: " " + ApprovalMergeRequestRule ||--o| ApprovalProjectRule: " " +``` + +This is defined in `ee/app/models/approval_merge_request_rule.rb`. A record is +created/updated/deleted when a rule is added/edited/removed via merge request +create/edit form or the [merge request level approvals API](../api/merge_request_approvals.md#merge-request-level-mr-approvals). + +The `approval_project_rule` is set when it is based from an existing `ApprovalProjectRule`. + +An `ApprovalMergeRequestRule` doesn't have `protected_branches` as it inherits +them from the `approval_project_rule` if not overridden. + +### `ApprovalWrappedRule` + +```mermaid +erDiagram + ApprovalState ||--o{ ApprovalWrappedRule: " " +``` + +Defined in `ee/app/modes/approval_wrapped_rule.rb` and is not an ActiveRecord +model. It's used to wrap an `ApprovalProjectRule` or `ApprovalMergeRequestRule` +for common interface. It also has the following sub types: + +- `ApprovalWrappedAnyApprovalRule` - for wrapping an `any_approver` rule +- `ApprovalWrappedCodeOwnerRule` - for wrapping a `code_owner` rule + +This class delegates most of the responsibilities to the approval rule it wraps +but it's also responsible for: + +- checking if the approval rule is approved +- knowing how many approvals were given or still required for the approval rule + +It gets this information from the approval rule and the `Approval` records from +the merge request. + +### `Approval` + +```mermaid +erDiagram + MergeRequest ||--o{ Approval: " " +``` + +This is defined in `ee/app/models/approval.rb`. This model is responsible for +storing information about an approval made on a merge request. Whenever an +approval is given/revoked, a record is created/deleted. + +## Controllers and Services + +The following controllers and services below are being utilized for the approval +rules feature to work. + +### `API::ProjectApprovalSettings` + +Defined in `ee/lib/api/project_approval_settings.rb`. + +This is a private API used for the following: + +- listing the approval rules in project settings +- creating/updating/deleting rules in project settings +- listing the approval rules on create merge request form + +### `Projects::MergeRequests::CreationsController` + +Defined in `app/controllers/projects/merge_requests/creations_controller.rb`. + +The `create` action of this controller is used when create merge request form is +submitted. It accepts the `approval_rules_attributes` parameter for creating/updating/deleting +`ApprovalMergeRequestRule` records. Passes the parameter along when it executes +`MergeRequests::CreateService`. + +### `Projects::MergeRequestsController` + +Defined in `app/controllers/projects/merge_requests_controller.rb`. + +The `update` action of this controller is used when edit merge request form is +submitted. It's like `Projects::MergeRequests::CreationsController` but it executes +`MergeRequests::UpdateService` instead. + +### `API::MergeRequestApprovals` + +Defined in `ee/lib/api/merge_request_approvals.rb`. + +The [Approvals API endpoint](../api/merge_request_approvals.md#get-configuration-1) +is requested when merge request page loads. + +The `/projects/:id/merge_requests/:merge_request_iid/approval_settings` is a +private API endpoint used for the following: + +- listing the approval rules on edit merge request form +- listing the approval rules on the merge request page + +When approving/unapproving MR via UI and API, the [Approve Merge Request](../api/merge_request_approvals.md#approve-merge-request) and the [Unapprove Merge Request](../api/merge_request_approvals.md#unapprove-merge-request) +are requested. They execute `MergeRequests::ApprovalService` and `MergeRequests::RemoveApprovalService` +accordingly. + +### `API::ProjectApprovalRules` and `API::MergeRequestApprovalRules` + +Defined in `ee/lib/api/project_approval_rules.rb` and `ee/lib/api/merge_request_approval_rules.rb`. + +Used to list/create/update/delete project and merge request level rules via +[Merge request approvals API](../api/merge_request_approvals.md). + +Executes `ApprovalRules::CreateService`, `ApprovalRules::UpdateService`, +`ApprovalRules::ProjectRuleDestroyService`, and `ApprovalRules::MergeRequestRuleDestroyService` +accordingly. + +### `ApprovalRules::ParamsFilteringService` + +Defined in `ee/app/services/approval_rules/params_filtering_service.rb`. + +This service is called only when `MergeRequests::CreateService` and +`MergeRequests::UpdateService` are executed. + +It is responsible for parsing `approval_rules_attributes` parameter to: + +- remove it when user can't update approval rules +- filter the user IDs whether they are members of the project or not +- filter the group IDs whether they are visible to user +- identify the `any_approver` rule +- append hidden groups to it when specified +- append user defined inapplicable (rules that does not apply to MR's target + branch) approval rules + +## Flow + +These flowcharts should help explain the flow from the controllers down to the +models for different functionalities. + +Some CRUD API endpoints are intentionally skipped because they are pretty +straightforward. + +### Creating a merge request with approval rules via web UI + +```mermaid +graph LR + Projects::MergeRequests::CreationsController --> MergeRequests::CreateService + MergeRequests::CreateService --> ApprovalRules::ParamsFilteringService + ApprovalRules::ParamsFilteringService --> MergeRequests::CreateService + MergeRequests::CreateService --> MergeRequest + MergeRequest --> db[(Database)] + MergeRequest --> User + MergeRequest --> Group + MergeRequest --> ApprovalProjectRule + User --> db[(Database)] + Group --> db[(Database)] + ApprovalProjectRule --> db[(Database)] +``` + +When updating, same flow is followed but it starts at `Projects::MergeRequestsController` +and executes `MergeRequests::UpdateService` instead. + +### Viewing the merge request approval rules on MR page + +```mermaid +graph LR + API::MergeRequestApprovals --> MergeRequest + MergeRequest --> ApprovalState + ApprovalState --> id1{approval rules are overridden} + id1{approval rules are overridden} --> |No| ApprovalProjectRule & ApprovalMergeRequestRule + id1{approval rules are overridden} --> |Yes| ApprovalMergeRequestRule + ApprovalState --> ApprovalWrappedRule + ApprovalWrappedRule --> Approval +``` + +### Approving a merge request + +```mermaid +graph LR + API::MergeRequestApprovals --> MergeRequests::ApprovalService + MergeRequests::ApprovalService --> Approval + Approval --> db[(Database)] +``` + +When unapproving, same flow is followed but the `MergeRequests::RemoveApprovalService` +is executed instead. + +## TODO + +1. Add information related to other rule types (e.g. `code_owner` and `report_approver`). +1. Add information about side effects of approving/unapproving merge request. -- GitLab From 058964c34eb8053678e01be8e4dc51fa011937e5 Mon Sep 17 00:00:00 2001 From: Patrick Bajao Date: Fri, 19 Jun 2020 10:25:20 +0800 Subject: [PATCH 2/2] Add link to development doc index --- doc/development/README.md | 1 + doc/development/approval_rules.md | 119 ++++++++++++++++-------------- 2 files changed, 65 insertions(+), 55 deletions(-) diff --git a/doc/development/README.md b/doc/development/README.md index d77b5d3eea4cae..88abc638ac6b5c 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -100,6 +100,7 @@ Complementary reads: - [Renaming features](renaming_features.md) - [Windows Development on GCP](windows.md) - [Code Intelligence](code_intelligence/index.md) +- [Approval Rules](approval_rules.md) ## Performance guides diff --git a/doc/development/approval_rules.md b/doc/development/approval_rules.md index 8cd85ed3d4f496..65df82721defa5 100644 --- a/doc/development/approval_rules.md +++ b/doc/development/approval_rules.md @@ -1,7 +1,7 @@ # Approval Rules **(STARTER)** This document explains the backend design and flow of all related functionality -about merge request approval rules. +about [merge request approval rules](../user/project/merge_requests/merge_request_approvals.md). This should help contributors to understand the code design easier and to also help see if there are parts to improve as the feature and its implementation @@ -12,7 +12,8 @@ can change often. The code should explain those things better. The components mentioned here are the major parts of the application for the approval rules feature to work. -**NOTE:** This is a living document and should be updated accordingly when parts +NOTE: **Note:** +This is a living document and should be updated accordingly when parts of the codebase touched in this document changed/removed or when new components are added. @@ -36,9 +37,10 @@ erDiagram ### `Project` and `MergeRequest` -They are defined in `ee/app/models/ee/project.rb` and `ee/app/models/ee/merge_request.rb`. -They extend the non-EE versions since approval rules is an EE only feature. -Associations and other related stuff to merge request approvals are defined here. +`Project` and `MergeRequest` models are defined in `ee/app/models/ee/project.rb` +and `ee/app/models/ee/merge_request.rb`. They extend the non-EE versions since +approval rules is an EE only feature. Associations and other related stuff to +merge request approvals are defined here. ### `ApprovalState` @@ -47,16 +49,16 @@ erDiagram MergeRequest ||--|| ApprovalState: " " ``` -It is defined in `ee/app/models/approval_state.rb`. It's not an actual -ActiveRecord model. This class encapsulates all logic related to the state -of the approvals for a certain merge request like: +`ApprovalState` class is defined in `ee/app/models/approval_state.rb`. It's not +an actual `ActiveRecord` model. This class encapsulates all logic related to the +state of the approvals for a certain merge request like: -- knowing the approval rules that are applicable to the merge request based on - its target branch -- knowing the approval rules that are applicable to a certain target branch -- checking if all rules were approved -- checking if approval is required -- knowing how many approvals were given or still required +- Knowing the approval rules that are applicable to the merge request based on + its target branch. +- Knowing the approval rules that are applicable to a certain target branch. +- Checking if all rules were approved. +- Checking if approval is required. +- Knowing how many approvals were given or still required. It gets the approval rules data from the project (`ApprovalProjectRule`) or the merge request (`ApprovalMergeRequestRule`) and wrap it as `ApprovalWrappedRule`. @@ -71,11 +73,11 @@ erDiagram ApprovalProjectRule }o--o{ ProtectedBranch: " " ``` -This model is defined in `ee/app/models/approval_project_rule.rb`. It has the -following attributes: +`ApprovalProjectRule` model is defined in `ee/app/models/approval_project_rule.rb`. A record is created/updated/deleted when an approval rule is added/edited/removed -via project settings or the [project level approvals API](../api/merge_request_approvals.md#project-level-mr-approvals). The `ApprovalState` model get these records when approval rules are not +via project settings or the [project level approvals API](../api/merge_request_approvals.md#project-level-mr-approvals). +The `ApprovalState` model get these records when approval rules are not overwritten. The `protected_branches` attribute is set and used when a rule is scoped to @@ -92,9 +94,10 @@ erDiagram ApprovalMergeRequestRule ||--o| ApprovalProjectRule: " " ``` -This is defined in `ee/app/models/approval_merge_request_rule.rb`. A record is -created/updated/deleted when a rule is added/edited/removed via merge request -create/edit form or the [merge request level approvals API](../api/merge_request_approvals.md#merge-request-level-mr-approvals). +`ApprovalMergeRequestRule` model is defined in `ee/app/models/approval_merge_request_rule.rb`. + +A record is created/updated/deleted when a rule is added/edited/removed via merge +request create/edit form or the [merge request level approvals API](../api/merge_request_approvals.md#merge-request-level-mr-approvals). The `approval_project_rule` is set when it is based from an existing `ApprovalProjectRule`. @@ -108,18 +111,19 @@ erDiagram ApprovalState ||--o{ ApprovalWrappedRule: " " ``` -Defined in `ee/app/modes/approval_wrapped_rule.rb` and is not an ActiveRecord -model. It's used to wrap an `ApprovalProjectRule` or `ApprovalMergeRequestRule` -for common interface. It also has the following sub types: +`ApprovalWrappedRule` is defined in `ee/app/modes/approval_wrapped_rule.rb` and +is not an `ActiveRecord` model. It's used to wrap an `ApprovalProjectRule` or +`ApprovalMergeRequestRule` for common interface. It also has the following sub +types: -- `ApprovalWrappedAnyApprovalRule` - for wrapping an `any_approver` rule -- `ApprovalWrappedCodeOwnerRule` - for wrapping a `code_owner` rule +- `ApprovalWrappedAnyApprovalRule` - for wrapping an `any_approver` rule. +- `ApprovalWrappedCodeOwnerRule` - for wrapping a `code_owner` rule. This class delegates most of the responsibilities to the approval rule it wraps but it's also responsible for: -- checking if the approval rule is approved -- knowing how many approvals were given or still required for the approval rule +- Checking if the approval rule is approved. +- Knowing how many approvals were given or still required for the approval rule. It gets this information from the approval rule and the `Approval` records from the merge request. @@ -131,9 +135,9 @@ erDiagram MergeRequest ||--o{ Approval: " " ``` -This is defined in `ee/app/models/approval.rb`. This model is responsible for -storing information about an approval made on a merge request. Whenever an -approval is given/revoked, a record is created/deleted. +`Approval` model is defined in `ee/app/models/approval.rb`. This model is +responsible for storing information about an approval made on a merge request. +Whenever an approval is given/revoked, a record is created/deleted. ## Controllers and Services @@ -142,26 +146,26 @@ rules feature to work. ### `API::ProjectApprovalSettings` -Defined in `ee/lib/api/project_approval_settings.rb`. +This private API is defined in `ee/lib/api/project_approval_settings.rb`. -This is a private API used for the following: +This is used for the following: -- listing the approval rules in project settings -- creating/updating/deleting rules in project settings -- listing the approval rules on create merge request form +- Listing the approval rules in project settings. +- Creating/updating/deleting rules in project settings. +- Listing the approval rules on create merge request form. ### `Projects::MergeRequests::CreationsController` -Defined in `app/controllers/projects/merge_requests/creations_controller.rb`. +This controller is defined in `app/controllers/projects/merge_requests/creations_controller.rb`. The `create` action of this controller is used when create merge request form is submitted. It accepts the `approval_rules_attributes` parameter for creating/updating/deleting -`ApprovalMergeRequestRule` records. Passes the parameter along when it executes +`ApprovalMergeRequestRule` records. It passes the parameter along when it executes `MergeRequests::CreateService`. ### `Projects::MergeRequestsController` -Defined in `app/controllers/projects/merge_requests_controller.rb`. +This controller is defined in `app/controllers/projects/merge_requests_controller.rb`. The `update` action of this controller is used when edit merge request form is submitted. It's like `Projects::MergeRequests::CreationsController` but it executes @@ -169,7 +173,7 @@ submitted. It's like `Projects::MergeRequests::CreationsController` but it execu ### `API::MergeRequestApprovals` -Defined in `ee/lib/api/merge_request_approvals.rb`. +This API is defined in `ee/lib/api/merge_request_approvals.rb`. The [Approvals API endpoint](../api/merge_request_approvals.md#get-configuration-1) is requested when merge request page loads. @@ -177,16 +181,18 @@ is requested when merge request page loads. The `/projects/:id/merge_requests/:merge_request_iid/approval_settings` is a private API endpoint used for the following: -- listing the approval rules on edit merge request form -- listing the approval rules on the merge request page +- Listing the approval rules on edit merge request form. +- Listing the approval rules on the merge request page. -When approving/unapproving MR via UI and API, the [Approve Merge Request](../api/merge_request_approvals.md#approve-merge-request) and the [Unapprove Merge Request](../api/merge_request_approvals.md#unapprove-merge-request) -are requested. They execute `MergeRequests::ApprovalService` and `MergeRequests::RemoveApprovalService` -accordingly. +When approving/unapproving MR via UI and API, the [Approve Merge Request](../api/merge_request_approvals.md#approve-merge-request) +API endpoint and the [Unapprove Merge Request](../api/merge_request_approvals.md#unapprove-merge-request) +API endpoint are requested. They execute `MergeRequests::ApprovalService` and +`MergeRequests::RemoveApprovalService` accordingly. ### `API::ProjectApprovalRules` and `API::MergeRequestApprovalRules` -Defined in `ee/lib/api/project_approval_rules.rb` and `ee/lib/api/merge_request_approval_rules.rb`. +These APIs are defined in `ee/lib/api/project_approval_rules.rb` and +`ee/lib/api/merge_request_approval_rules.rb`. Used to list/create/update/delete project and merge request level rules via [Merge request approvals API](../api/merge_request_approvals.md). @@ -197,20 +203,20 @@ accordingly. ### `ApprovalRules::ParamsFilteringService` -Defined in `ee/app/services/approval_rules/params_filtering_service.rb`. +This service is defined in `ee/app/services/approval_rules/params_filtering_service.rb`. -This service is called only when `MergeRequests::CreateService` and +It is called only when `MergeRequests::CreateService` and `MergeRequests::UpdateService` are executed. It is responsible for parsing `approval_rules_attributes` parameter to: -- remove it when user can't update approval rules -- filter the user IDs whether they are members of the project or not -- filter the group IDs whether they are visible to user -- identify the `any_approver` rule -- append hidden groups to it when specified -- append user defined inapplicable (rules that does not apply to MR's target - branch) approval rules +- Remove it when user can't update approval rules. +- Filter the user IDs whether they are members of the project or not. +- Filter the group IDs whether they are visible to user. +- Identify the `any_approver` rule. +- Append hidden groups to it when specified. +- Append user defined inapplicable (rules that does not apply to MR's target + branch) approval rules. ## Flow @@ -240,7 +246,7 @@ graph LR When updating, same flow is followed but it starts at `Projects::MergeRequestsController` and executes `MergeRequests::UpdateService` instead. -### Viewing the merge request approval rules on MR page +### Viewing the merge request approval rules on an MR page ```mermaid graph LR @@ -253,6 +259,9 @@ graph LR ApprovalWrappedRule --> Approval ``` +This flow gets initiated by the frontend component. The data returned will +then be used to display information on the MR widget. + ### Approving a merge request ```mermaid -- GitLab