Improve by zj nice catches

doc/api/pipeline_schedules.md

@@ -290,12 +290,12 @@ Create a new variable of a pipeline schedule.
 POST /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables
 ```
 
-| Attribute     | Type    | required | Description              |
-|---------------|---------|----------|--------------------------|
-| `id`          | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user      |
-| `pipeline_schedule_id`          | integer | yes      | The pipeline schedule id |
-| `key`       | string  | yes      | The `key` of a variable; must have no more than 255 characters; only `A-Z`, `a-z`, `0-9`, and `_` are allowed |
-| `value`     | string  | yes      | The `value` of a variable |
+| Attribute              | Type           | required | Description              |
+|------------------------|----------------|----------|--------------------------|
+| `id`                   | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user      |
+| `pipeline_schedule_id` | integer        | yes      | The pipeline schedule id |
+| `key`                  | string         | yes      | The `key` of a variable; must have no more than 255 characters; only `A-Z`, `a-z`, `0-9`, and `_` are allowed |
+| `value`                | string         | yes      | The `value` of a variable |
 
 ```sh
 curl --request POST --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" --form "key=NEW_VARIABLE" --form "value=new value" "https://gitlab.example.com/api/v4/projects/29/pipeline_schedules/13/variables"
@@ -316,12 +316,12 @@ Updates the variable of a pipeline schedule.
 PUT /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key
 ```
 
-| Attribute     | Type    | required | Description              |
-|---------------|---------|----------|--------------------------|
-| `id`          | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user      |
-| `pipeline_schedule_id`          | integer | yes      | The pipeline schedule id |
-| `key`       | string  | yes      | The `key` of a variable   |
-| `value`     | string  | yes      | The `value` of a variable |
+| Attribute              | Type           | required | Description              |
+|------------------------|----------------|----------|--------------------------|
+| `id`                   | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user      |
+| `pipeline_schedule_id` | integer        | yes      | The pipeline schedule id |
+| `key`                  | string         | yes      | The `key` of a variable   |
+| `value`                | string         | yes      | The `value` of a variable |
 
 ```sh
 curl --request PUT --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" --form "value=updated value" "https://gitlab.example.com/api/v4/projects/29/pipeline_schedules/13/variables/NEW_VARIABLE"
@@ -342,18 +342,21 @@ Delete the variable of a pipeline schedule.
 DELETE /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key
 ```
 
-| Attribute      | Type    | required | Description              |
-|----------------|---------|----------|--------------------------|
-| `id`           | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user      |
-| `pipeline_schedule_id`          | integer | yes      | The pipeline schedule id |
-| `key`     | string  | yes      | The `key` of a variable |
+| Attribute              | Type           | required | Description              |
+|------------------------|----------------|----------|--------------------------|
+| `id`                   | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user      |
+| `pipeline_schedule_id` | integer        | yes      | The pipeline schedule id |
+| `key`                  | string         | yes      | The `key` of a variable |
 
 ```sh
 curl --request DELETE --header "PRIVATE-TOKEN: k5ESFgWY2Qf5xEvDcFxZ" "https://gitlab.example.com/api/v4/projects/29/pipeline_schedules/13/variables/NEW_VARIABLE"
 ```
 
 ```json
-// Empty
+{
+    "key": "NEW_VARIABLE",
+    "value": "updated value"
+}
 ```
 
 [ce-34518]: https://gitlab.com/gitlab-org/gitlab-ce/issues/34518
\ No newline at end of file

lib/api/entities.rb

@@ -819,7 +819,7 @@ module API
 
     class Variable < Grape::Entity
       expose :key, :value
-      expose :protected?, as: :protected, if: -> (entity, options) { entity.respond_to?(:protected?) }
+      expose :protected?, as: :protected, if: -> (entity, _) { entity.respond_to?(:protected?) }
     end
 
     class Pipeline < PipelineBasic

lib/api/pipeline_schedules.rb

@@ -33,8 +33,6 @@ module API
       get ':id/pipeline_schedules/:pipeline_schedule_id' do
         authorize! :read_pipeline_schedule, user_project
 
-        not_found!('PipelineSchedule') unless pipeline_schedule
-
         present pipeline_schedule, with: Entities::PipelineScheduleDetails
       end
 
@@ -75,8 +73,6 @@ module API
       end
       put ':id/pipeline_schedules/:pipeline_schedule_id' do
         authorize! :read_pipeline_schedule, user_project
-
-        not_found!('PipelineSchedule') unless pipeline_schedule
         authorize! :update_pipeline_schedule, pipeline_schedule
 
         if pipeline_schedule.update(declared_params(include_missing: false))
@@ -94,8 +90,6 @@ module API
       end
       post ':id/pipeline_schedules/:pipeline_schedule_id/take_ownership' do
         authorize! :read_pipeline_schedule, user_project
-
-        not_found!('PipelineSchedule') unless pipeline_schedule
         authorize! :update_pipeline_schedule, pipeline_schedule
 
         if pipeline_schedule.own!(current_user)
@@ -113,8 +107,6 @@ module API
       end
       delete ':id/pipeline_schedules/:pipeline_schedule_id' do
         authorize! :read_pipeline_schedule, user_project
-
-        not_found!('PipelineSchedule') unless pipeline_schedule
         authorize! :admin_pipeline_schedule, pipeline_schedule
 
         destroy_conditionally!(pipeline_schedule)
@@ -130,8 +122,6 @@ module API
       end
       post ':id/pipeline_schedules/:pipeline_schedule_id/variables' do
         authorize! :read_pipeline_schedule, user_project
-
-        not_found!('PipelineSchedule') unless pipeline_schedule
         authorize! :update_pipeline_schedule, pipeline_schedule
 
         variable_params = declared_params(include_missing: false)
@@ -153,17 +143,12 @@ module API
       end
       put ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do
         authorize! :read_pipeline_schedule, user_project
-
-        not_found!('PipelineSchedule') unless pipeline_schedule
         authorize! :update_pipeline_schedule, pipeline_schedule
 
-        variable = pipeline_schedule.variables.find_by(key: params[:key])
-        not_found!('Variable') unless variable
-
-        if variable.update(declared_params(include_missing: false))
-          present variable, with: Entities::Variable
+        if pipeline_schedule_variable.update(declared_params(include_missing: false))
+          present pipeline_schedule_variable, with: Entities::Variable
         else
-          render_validation_error!(variable)
+          render_validation_error!(pipeline_schedule_variable)
         end
       end
 
@@ -176,15 +161,10 @@ module API
       end
       delete ':id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do
         authorize! :read_pipeline_schedule, user_project
-
-        not_found!('PipelineSchedule') unless pipeline_schedule
         authorize! :admin_pipeline_schedule, pipeline_schedule
 
-        variable = pipeline_schedule.variables.find_by(key: params[:key])
-        not_found!('Variable') unless variable
-
         status :accepted
-        present variable.destroy, with: Entities::Variable
+        present pipeline_schedule_variable.destroy, with: Entities::Variable
       end
     end
 
@@ -194,6 +174,15 @@ module API
           user_project.pipeline_schedules
                       .preload(:owner, :last_pipeline)
                       .find_by(id: params.delete(:pipeline_schedule_id))
+
+        @pipeline_schedule || not_found!('Pipeline Schedule')
+      end
+
+      def pipeline_schedule_variable
+        @pipeline_schedule_variable ||=
+          pipeline_schedule.variables.find_by(key: params[:key])
+
+        @pipeline_schedule_variable || not_found!('Pipeline Schedule Variable')
       end
     end
   end

spec/requests/api/pipeline_schedules_spec.rb

@@ -303,7 +303,7 @@ describe API::PipelineSchedules do
   describe 'POST /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables' do
     let(:params) { attributes_for(:ci_pipeline_schedule_variable) }
 
-    let(:pipeline_schedule) do
+    set(:pipeline_schedule) do
       create(:ci_pipeline_schedule, project: project, owner: developer)
     end
 
@@ -359,7 +359,7 @@ describe API::PipelineSchedules do
   end
 
   describe 'PUT /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do
-    let(:pipeline_schedule) do
+    set(:pipeline_schedule) do
       create(:ci_pipeline_schedule, project: project, owner: developer)
     end
 
@@ -398,7 +398,7 @@ describe API::PipelineSchedules do
   describe 'DELETE /projects/:id/pipeline_schedules/:pipeline_schedule_id/variables/:key' do
     let(:master) { create(:user) }
 
-    let!(:pipeline_schedule) do
+    set(:pipeline_schedule) do
       create(:ci_pipeline_schedule, project: project, owner: developer)
     end
 
@@ -427,7 +427,7 @@ describe API::PipelineSchedules do
       end
     end
 
-    context 'authenticated user with invalid permissions' do # TODO:
+    context 'authenticated user with invalid permissions' do
       let!(:pipeline_schedule) { create(:ci_pipeline_schedule, project: project, owner: master) }
 
       it 'does not delete pipeline_schedule_variable' do