Improve design based on review

app/models/ci/build.rb

@@ -360,10 +360,9 @@ module Ci
     end
 
     def artifacts_expire_in=(value)
+      self.artifacts_expire_at =
         if value
-        self.artifacts_expire_at = Time.now + ChronicDuration.parse(value)
-      else
-        self.artifacts_expire_at = nil
+          Time.now + ChronicDuration.parse(value)
         end
     end
 

doc/api/builds.md

@@ -446,7 +446,7 @@ Example of response
 
 ## Keep artifacts
 
-Prevents artifacts from being deleted when expiration is set
+Prevents artifacts from being deleted when expiration is set.
 
 ```
 POST /projects/:id/builds/:build_id/artifacts/keep
@@ -459,13 +459,13 @@ Parameters
 | `id`        | integer | yes      | The ID of a project |
 | `build_id`  | integer | yes      | The ID of a build   |
 
-Example of request
+Example request:
 
 ```
 curl -X POST -H "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v3/projects/1/builds/1/artifacts/keep"
 ```
 
-Example of response
+Example response:
 
 ```json
 {

lib/api/builds.rb

@@ -167,10 +167,11 @@ module API
                        user_can_download_artifacts: can?(current_user, :download_build_artifacts, user_project)
       end
 
-      # Keep the artifacts to prevent them to be deleted
+      # Keep the artifacts to prevent them from being deleted
       #
       # Parameters:
-      #   id (required) - The ID of a build
+      #   id (required) - the id of a project
+      #   build_id (required) - The ID of a build
       # Example Request:
       #   POST /projects/:id/builds/:build_id/artifacts/keep
       post ':id/builds/:build_id/artifacts/keep' do

lib/ci/api/entities.rb

@@ -20,7 +20,7 @@ module Ci
         expose :name, :token, :stage
         expose :project_id
         expose :project_name
-        expose :artifacts_file, using: ArtifactFile, if: lambda { |build, opts| build.artifacts? }
+        expose :artifacts_file, using: ArtifactFile, if: ->(build, _) { build.artifacts? }
       end
 
       class BuildDetails < Build
@@ -29,7 +29,7 @@ module Ci
         expose :before_sha
         expose :allow_git_fetch
         expose :token
-        expose :artifacts_expire_at, if: lambda { |build, opts| build.artifacts? }
+        expose :artifacts_expire_at, if: ->(build, _) { build.artifacts? }
 
         expose :options do |model|
           model.options

spec/features/builds_spec.rb

@@ -106,7 +106,7 @@ describe "Builds" do
       context 'no expire date defined' do
         let(:expire_at) { nil }
 
-        it 'should not have the Keep button' do
+        it 'does not have the Keep button' do
           page.within('.artifacts') do
             expect(page).not_to have_content 'Keep'
           end
@@ -116,7 +116,7 @@ describe "Builds" do
       context 'when expire date is defined' do
         let(:expire_at) { Time.now + 7.days }
 
-        it 'should keep artifacts when Keep button is clicked' do
+        it 'keeps artifacts when Keep button is clicked' do
           page.within('.artifacts') do
             expect(page).to have_content 'The artifacts will be removed'
             click_link 'Keep'
@@ -130,7 +130,7 @@ describe "Builds" do
       context 'when artifacts expired' do
         let(:expire_at) { Time.now - 7.days }
 
-        it 'should not have the Keep button' do
+        it 'does not have the Keep button' do
           page.within('.artifacts') do
             expect(page).to have_content 'The artifacts were removed'
             expect(page).not_to have_link 'Keep'
@@ -150,8 +150,6 @@ describe "Builds" do
         expect(page).to have_link 'Raw'
       end
     end
-
-    context ''
   end
 
   describe "POST /:project/builds/:id/cancel" do

spec/models/build_spec.rb

@@ -467,6 +467,7 @@ describe Ci::Build, models: true do
 
     it 'when assigning valid duration' do
       build.artifacts_expire_in = '7 days'
+
       is_expected.to be_within(10).of(7.days.to_i)
     end
 
@@ -477,6 +478,7 @@ describe Ci::Build, models: true do
 
     it 'when resseting value' do
       build.artifacts_expire_in = nil
+
       is_expected.to be_nil
     end
   end
@@ -486,6 +488,7 @@ describe Ci::Build, models: true do
 
     it 'to reset expire_at' do
       build.keep_artifacts!
+
       expect(build.artifacts_expire_at).to be_nil
     end
   end

spec/requests/api/builds_spec.rb

@@ -253,17 +253,16 @@ describe API::API, api: true  do
                project: project, pipeline: pipeline, artifacts_expire_at: Time.now + 7.days)
       end
 
-      it 'should keep artifacts' do
+      it 'keeps artifacts' do
         expect(response.status).to eq 200
-        build.reload
-        expect(build.artifacts_expire_at).to be_nil
+        expect(build.reload.artifacts_expire_at).to be_nil
       end
     end
 
     context 'no artifacts' do
       let(:build) { create(:ci_build, project: project, pipeline: pipeline) }
 
-      it 'should respond with not found' do
+      it 'responds with not found' do
         expect(response.status).to eq 404
       end
     end

spec/requests/ci/api/builds_spec.rb

@@ -364,7 +364,7 @@ describe Ci::API::API do
             end
           end
 
-          context 'expire date' do
+          context 'with an expire date' do
             let!(:artifacts) { file_upload }
 
             let(:post_data) do
@@ -377,7 +377,7 @@ describe Ci::API::API do
               post(post_url, post_data, headers_with_token)
             end
 
-            context 'updates when specified' do
+            context 'with an expire_in given' do
               let(:expire_in) { '7 days' }
 
               it do
@@ -388,7 +388,7 @@ describe Ci::API::API do
               end
             end
 
-            context 'ignores if not specified' do
+            context 'with no expire_in given' do
               let(:expire_in) { nil }
 
               it do

spec/workers/expire_build_artifacts_worker_spec.rb

@@ -3,7 +3,7 @@ require 'spec_helper'
 describe ExpireBuildArtifactsWorker do
   include RepoHelpers
 
-  let(:worker) { ExpireBuildArtifactsWorker.new }
+  let(:worker) { described_class.new }
 
   describe '#perform' do
     context 'with expired artifacts' do
@@ -11,9 +11,10 @@ describe ExpireBuildArtifactsWorker do
 
       it do
         expect_any_instance_of(Ci::Build).to receive(:erase_artifacts!)
+
         worker.perform
-        build.reload
-        expect(build.artifacts_expired?).to be_truthy
+
+        expect(build.reload.artifacts_expired?).to be_truthy
       end
     end
 
@@ -22,9 +23,10 @@ describe ExpireBuildArtifactsWorker do
 
       it do
         expect_any_instance_of(Ci::Build).not_to receive(:erase_artifacts!)
+
         worker.perform
-        build.reload
-        expect(build.artifacts_expired?).to be_falsey
+
+        expect(build.reload.artifacts_expired?).to be_falsey
       end
     end
 
@@ -33,6 +35,7 @@ describe ExpireBuildArtifactsWorker do
 
       it do
         expect_any_instance_of(Ci::Build).not_to receive(:erase_artifacts!)
+
         worker.perform
       end
     end
@@ -47,9 +50,10 @@ describe ExpireBuildArtifactsWorker do
 
       it do
         expect_any_instance_of(Ci::Build).not_to receive(:erase_artifacts!)
+
         worker.perform
-        build.reload
-        expect(build.artifacts_expired?).to be_truthy
+
+        expect(build.reload.artifacts_expired?).to be_truthy
       end
     end
   end