refactor updater

app/Http/Controllers/UpdateController.php

@@ -3,66 +3,58 @@
 namespace App\Http\Controllers;
 
 use App\Services\PackageManager;
+use Composer\CaBundle\CaBundle;
 use Composer\Semver\Comparator;
 use Exception;
+use GuzzleHttp\Client;
+use GuzzleHttp\Exception\RequestException;
 use Illuminate\Contracts\Console\Kernel as Artisan;
 use Illuminate\Filesystem\Filesystem;
-use Illuminate\Http\Request;
 use Illuminate\Support\Arr;
 use Symfony\Component\Finder\SplFileInfo;
 
 class UpdateController extends Controller
 {
-    protected $currentVersion;
-    protected $updateSource;
-    protected $guzzle;
-    protected $error;
-    protected $info = [];
+    const SPEC = 2;
 
-    public function __construct(\GuzzleHttp\Client $guzzle)
+    public function showUpdatePage(Client $client)
     {
-        $this->updateSource = config('app.update_source');
-        $this->currentVersion = config('app.version');
-        $this->guzzle = $guzzle;
+        $info = $this->getUpdateInfo($client);
+        $canUpdate = $this->canUpdate(Arr::get($info, 'info'));
+
+        return view('admin.update', [
+            'info' => [
+                'latest' => Arr::get($info, 'info.latest'),
+                'current' => config('app.version'),
+            ],
+            'error' => Arr::get($info, 'error', $canUpdate['reason']),
+            'can_update' => $canUpdate['can'],
+        ]);
     }
 
-    public function showUpdatePage()
-    {
-        $info = [
-            'latest' => Arr::get($this->getUpdateInfo(), 'latest'),
-            'current' => $this->currentVersion,
-        ];
-        $error = $this->error;
-        $extra = ['canUpdate' => $this->canUpdate()];
-
-        return view('admin.update', compact('info', 'error', 'extra'));
-    }
-
-    public function download(Request $request, PackageManager $package, Filesystem $filesystem)
-    {
-        if (!$this->canUpdate()) {
-            return json([]);
+    public function download(
+        PackageManager $package,
+        Filesystem $filesystem,
+        Client $client
+    ) {
+        $info = $this->getUpdateInfo($client);
+        if (!$info['ok'] || !$this->canUpdate($info['info'])['can']) {
+            return json(trans('admin.update.info.up-to-date'), 1);
         }
 
-        $path = storage_path('packages/bs_'.$this->info['latest'].'.zip');
-        switch ($request->get('action')) {
-            case 'download':
-                try {
-                    $package->download($this->info['url'], $path)->extract(base_path());
+        $info = $info['info'];
+        $path = storage_path('packages/bs_'.$info['latest'].'.zip');
+        try {
+            $package->download($info['url'], $path)->extract(base_path());
 
-                    // Delete options cache. This allows us to update the version info which is recorded as an option.
-                    $filesystem->delete(storage_path('options.php'));
+            // Delete options cache. This allows us to update the version.
+            $filesystem->delete(storage_path('options.php'));
 
-                    return json(trans('admin.update.complete'), 0);
-                } catch (Exception $e) {
-                    report($e);
+            return json(trans('admin.update.complete'), 0);
+        } catch (Exception $e) {
+            report($e);
 
-                    return json($e->getMessage(), 1);
-                }
-            case 'progress':
-                return $package->progress();
-            default:
-                return json(trans('general.illegal-parameters'), 1);
+            return json($e->getMessage(), 1);
         }
     }
 
@@ -87,43 +79,37 @@ class UpdateController extends Controller
         return view('setup.updates.success');
     }
 
-    protected function getUpdateInfo()
+    protected function getUpdateInfo(Client $client)
     {
-        $acceptableSpec = 2;
-        if (app()->runningUnitTests() || !$this->info) {
-            try {
-                $json = $this->guzzle->request(
-                    'GET',
-                    $this->updateSource,
-                    ['verify' => \Composer\CaBundle\CaBundle::getSystemCaRootBundlePath()]
-                )->getBody();
-                $info = json_decode($json, true);
-                if (Arr::get($info, 'spec') == $acceptableSpec) {
-                    $this->info = $info;
-                } else {
-                    $this->error = trans('admin.update.errors.spec');
-                }
-            } catch (Exception $e) {
-                $this->error = $e->getMessage();
+        try {
+            $response = $client->request('GET', config('app.update_source'), [
+                'verify' => CaBundle::getSystemCaRootBundlePath(),
+            ]);
+            $info = json_decode($response->getBody(), true);
+            if (Arr::get($info, 'spec') === self::SPEC) {
+                return ['ok' => true, 'info' => $info];
+            } else {
+                return ['ok' => false, 'error' => trans('admin.update.errors.spec')];
             }
+        } catch (RequestException $e) {
+            return ['ok' => false, 'error' => $e->getMessage()];
         }
-
-        return $this->info;
     }
 
-    protected function canUpdate()
+    protected function canUpdate($info = [])
     {
-        $this->getUpdateInfo();
-
-        $php = Arr::get($this->info, 'php');
+        $php = Arr::get($info, 'php');
         preg_match('/(\d+\.\d+\.\d+)/', PHP_VERSION, $matches);
         $version = $matches[1];
         if (Comparator::lessThan($version, $php)) {
-            $this->error = trans('admin.update.errors.php', ['version' => $php]);
-
-            return false;
+            return [
+                'can' => false,
+                'reason' => trans('admin.update.errors.php', ['version' => $php]),
+            ];
         }
 
-        return Comparator::greaterThan(Arr::get($this->info, 'latest'), $this->currentVersion);
+        $can = Comparator::greaterThan(Arr::get($info, 'latest'), config('app.version'));
+
+        return ['can' => $can, 'reason' => ''];
     }
 }

app/Http/View/Composers/FootComposer.php

@@ -2,7 +2,6 @@
 
 namespace App\Http\View\Composers;
 
-use App\Models\User;
 use App\Services\Translations\JavaScript;
 use App\Services\Webpack;
 use Illuminate\Contracts\Events\Dispatcher;

app/Services/PackageManager.php

@@ -4,17 +4,17 @@ declare(strict_types=1);
 
 namespace App\Services;
 
-use Cache;
 use Exception;
+use GuzzleHttp\Client;
 use Illuminate\Filesystem\Filesystem;
 use ZipArchive;
 
 class PackageManager
 {
-    protected $guzzle;
     protected $path;
-    protected $cacheKey;
-    protected $onProgress;
+
+    /** @var Client */
+    protected $guzzle;
 
     /** @var Filesystem */
     protected $filesystem;
@@ -23,35 +23,28 @@ class PackageManager
     protected $zipper;
 
     public function __construct(
-        \GuzzleHttp\Client $guzzle,
+        Client $guzzle,
         Filesystem $filesystem,
         ZipArchive $zipper
     ) {
         $this->guzzle = $guzzle;
         $this->filesystem = $filesystem;
         $this->zipper = $zipper;
-        $this->onProgress = function ($total, $done) {
-            Cache::put($this->cacheKey, serialize(['total' => $total, 'done' => $done]));
-        };
     }
 
     public function download(string $url, string $path, $shasum = null): self
     {
         $this->path = $path;
-        $this->cacheKey = "download_$url";
-        Cache::forget($this->cacheKey);
         try {
             $this->guzzle->request('GET', $url, [
                 'sink' => $path,
-                'progress' => $this->onProgress,
                 'verify' => \Composer\CaBundle\CaBundle::getSystemCaRootBundlePath(),
             ]);
         } catch (Exception $e) {
             throw new Exception(trans('admin.download.errors.download', ['error' => $e->getMessage()]));
         }
 
-        Cache::forget($this->cacheKey);
-        if (is_string($shasum) && sha1_file($path) != strtolower($shasum)) {
+        if (is_string($shasum) && sha1_file($path) !== strtolower($shasum)) {
             $this->filesystem->delete($path);
             throw new Exception(trans('admin.download.errors.shasum'));
         }
@@ -71,14 +64,4 @@ class PackageManager
             throw new Exception(trans('admin.download.errors.unzip'));
         }
     }
-
-    public function progress(): float
-    {
-        $progress = unserialize(Cache::get($this->cacheKey));
-        if ($progress['total'] == 0) {
-            return 0;
-        } else {
-            return $progress['done'] / $progress['total'];
-        }
-    }
 }

resources/assets/src/scripts/route.ts

@@ -88,8 +88,9 @@ export default [
   },
   {
     path: 'admin/update',
-    component: () => import('../views/admin/Update.vue'),
-    el: '#update-button',
+    module: [
+      () => import('../views/admin/Update'),
+    ],
   },
   {
     path: 'auth/login',

resources/assets/src/views/admin/Update.ts

@@ -0,0 +1,22 @@
+import { post, ResponseBody } from '../../scripts/net'
+import { showModal } from '../../scripts/notify'
+import { trans } from '../../scripts/i18n'
+
+export default async function handler(event: MouseEvent) {
+  const button = event.target as HTMLButtonElement
+  button.disabled = true
+
+  const text = button.textContent
+  button.innerHTML = `<i class="fas fa-spinner fa-spin"></i> ${trans('admin.downloading')}`
+
+  const { code, message }: ResponseBody = await post('/admin/update/download')
+  button.textContent = text
+  button.disabled = false
+  await showModal({ mode: 'alert', text: message })
+  if (code === 0) {
+    location.href = '/'
+  }
+}
+
+const button = document.querySelector<HTMLButtonElement>('#update')
+button?.addEventListener('click', handler)

resources/assets/src/views/admin/Update.vue

@@ -1,102 +0,0 @@
-<template>
-  <span>
-    <button
-      v-if="!updating"
-      class="btn btn-primary"
-      :disabled="!canUpdate"
-      @click="update"
-    >
-      {{ $t('admin.updateButton') }}
-    </button>
-    <button v-else disabled class="btn btn-primary">
-      <i class="fa fa-spinner fa-spin" /> {{ $t('admin.downloading') }}
-    </button>
-
-    <div
-      id="modal-download"
-      class="modal fade"
-      tabindex="-1"
-      role="dialog"
-    >
-      <div class="modal-dialog">
-        <div class="modal-content">
-          <div class="modal-header">
-            <h4 v-t="'admin.downloading'" class="modal-title" />
-          </div>
-          <div class="modal-body">
-            <div class="progress">
-              <div
-                class="progress-bar progress-bar-striped bg-primary"
-                role="progressbar"
-                aria-valuenow="0"
-                aria-valuemin="0"
-                aria-valuemax="100"
-                :style="{ width: `${percentage}%` }"
-              >
-                <span>{{ percentage }}</span>%
-              </div>
-            </div>
-          </div>
-        </div>
-      </div>
-    </div>
-  </span>
-</template>
-
-<script>
-import emitMounted from '../../components/mixins/emitMounted'
-import { showModal } from '../../scripts/notify'
-
-const POLLING_INTERVAL = 500
-
-export default {
-  name: 'UpdateButton',
-  mixins: [
-    emitMounted,
-  ],
-  data: () => ({
-    canUpdate: blessing.extra.canUpdate,
-    updating: false,
-    percentage: 0,
-  }),
-  methods: {
-    async update() {
-      this.updating = true
-      $('#modal-download').modal({
-        backdrop: 'static',
-        keyboard: false,
-      })
-
-      setTimeout(() => this.polling(), POLLING_INTERVAL)
-      const { code, message } = await this.$http.post(
-        '/admin/update/download',
-        { action: 'download' },
-      )
-      this.updating = false
-      if (code) {
-        showModal({
-          mode: 'alert',
-          text: message,
-          type: 'danger',
-          okButtonType: 'outline-light',
-        })
-        return
-      }
-      await showModal({
-        mode: 'alert',
-        text: this.$t('admin.updateCompleted'),
-        okButtonType: 'success',
-      })
-      window.location = blessing.base_url
-    },
-    async polling() {
-      const percentage = await this.$http.get(
-        '/admin/update/download',
-        { action: 'progress' },
-      )
-      this.percentage = ~~percentage * 100
-      this.updating && setTimeout(this.polling, POLLING_INTERVAL)
-    },
-  },
-}
-</script>

resources/assets/tests/views/admin/Update.test.ts

@@ -1,58 +1,24 @@
-import Vue from 'vue'
-import { mount } from '@vue/test-utils'
 import { flushPromises } from '../../utils'
 import { showModal } from '@/scripts/notify'
-import Update from '@/views/admin/Update.vue'
+import { post } from '@/scripts/net'
+import handler from '@/views/admin/Update'
 
 jest.mock('@/scripts/notify')
+jest.mock('@/scripts/net')
 
-afterEach(() => {
-  window.blessing.extra = { canUpdate: true }
-})
-test('button should be disabled if update is unavailable', () => {
-  window.blessing.extra = { canUpdate: false }
-  const wrapper = mount(Update)
-  expect(wrapper.find('button').attributes('disabled')).toBe('disabled')
-})
+test('click button', async () => {
+  post.mockResolvedValueOnce({ code: 1, message: 'failed' })
+    .mockResolvedValue({ code: 0, message: 'ok' })
 
-test('perform update', async () => {
-  window.$ = jest.fn(() => ({
-    modal() {},
-  }))
-  Vue.prototype.$http.post
-    .mockResolvedValueOnce({ code: 1, message: 'fail' })
-    .mockResolvedValue({})
-  const wrapper = mount(Update)
-  const button = wrapper.find('button')
+  const button = document.createElement('button')
+  button.addEventListener('click', handler)
 
-  button.trigger('click')
+  const event = new MouseEvent('click')
+  button.dispatchEvent(event)
   await flushPromises()
-  expect(showModal).toBeCalledWith({
-    mode: 'alert',
-    text: 'fail',
-    type: 'danger',
-    okButtonType: 'outline-light',
-  })
+  expect(showModal).toBeCalledWith({ mode: 'alert', text: 'failed' })
 
-  button.trigger('click')
-  jest.runOnlyPendingTimers()
+  button.dispatchEvent(event)
   await flushPromises()
-  expect(Vue.prototype.$http.get).toBeCalledWith(
-    '/admin/update/download',
-    { action: 'progress' },
-  )
-  expect(Vue.prototype.$http.post).toBeCalledWith(
-    '/admin/update/download',
-    { action: 'download' },
-  )
-})
-
-test('polling for querying download progress', async () => {
-  const wrapper = mount<Vue & { polling(): Promise<void> }>(Update)
-  wrapper.setData({ updating: true })
-  await wrapper.vm.polling()
-  expect(Vue.prototype.$http.get).toBeCalledWith(
-    '/admin/update/download',
-    { action: 'progress' },
-  )
+  expect(showModal).toBeCalledWith({ mode: 'alert', text: 'ok' })
 })

resources/views/admin/update.twig

@@ -10,28 +10,28 @@
           <h3 class="card-title">{{ trans('admin.update.info.title') }}</h3>
         </div>
         <div class="card-body">
-          {% if extra.canUpdate %}
-          <div class="callout callout-info">
-            {{ trans('admin.update.info.available') }}
-          </div>
-          <table class="table">
-            <tbody>
-              <tr>
-                <td class="key">{{ trans('admin.update.info.versions.latest') }}</td>
-                <td class="value">
-                  v{{ info.latest }}
-                </td>
-              </tr>
-              <tr>
-                <td class="key">{{ trans('admin.update.info.versions.current') }}</td>
-                <td class="value">
-                  v{{ info.current }}
-                </td>
-              </tr>
-            </tbody>
-          </table>
+          {% if can_update %}
+            <div class="callout callout-info">
+              {{ trans('admin.update.info.available') }}
+            </div>
+            <table class="table">
+              <tbody>
+                <tr>
+                  <td class="key">{{ trans('admin.update.info.versions.latest') }}</td>
+                  <td class="value">
+                    v{{ info.latest }}
+                  </td>
+                </tr>
+                <tr>
+                  <td class="key">{{ trans('admin.update.info.versions.current') }}</td>
+                  <td class="value">
+                    v{{ info.current }}
+                  </td>
+                </tr>
+              </tbody>
+            </table>
           {% else %}
-            {% if error is not empty %}
+            {% if error %}
               <div class="callout callout-danger">
                 {{ trans('admin.update.errors.connection', {error: error}) }}
               </div>
@@ -54,7 +54,15 @@
           {% endif %}
         </div>
         <div class="card-footer">
-          <span id="update-button"></span>
+          {% if can_update %}
+            <button class="btn bg-primary" id="update">
+              {{ trans('admin.update.info.button') }}
+            </button>
+          {% else %}
+            <button class="btn bg-primary" disabled="disabled">
+              {{ trans('admin.update.info.button') }}
+            </button>
+          {% endif %}
           <a
             target="_blank"
             class="btn btn-default float-right"
@@ -73,9 +81,7 @@
           </h3>
         </div>
         <div class="card-body">
-          {% for text in trans('admin.update.cautions.text')|split('\n') %}
-            <p>{{ text }}</p>
-          {% endfor %}
+          {{ trans('admin.update.cautions.text')|nl2br }}
           <a target="_blank" href="https://blessing.netlify.com/update-sources.html">
             {{ trans('admin.update.cautions.link') }}
           </a>
@@ -84,9 +90,3 @@
     </div>
   </div>
 {% endblock %}
-
-{% block before_foot %}
-  <script>
-    blessing.extra = {{ extra|json_encode|raw }}
-  </script>
-{% endblock %}

tests/HttpTest/ControllersTest/UpdateControllerTest.php

@@ -36,8 +36,6 @@ class UpdateControllerTest extends TestCase
         // Missing `spec` field
         $this->appendToGuzzleQueue([
             new Response(200, [], $this->mockFakeUpdateInfo('8.9.3', ['spec' => 0])),
-            // Weird. Don't remove the following line, or the tests will fail.
-            new Response(200, [], $this->mockFakeUpdateInfo('8.9.3', ['php' => '100.0.0'])),
         ]);
         $this->get('/admin/update')->assertSee(trans('admin.update.errors.spec'));
 
@@ -59,8 +57,14 @@ class UpdateControllerTest extends TestCase
         $this->setupGuzzleClientMock();
 
         // Already up-to-date
+        $this->appendToGuzzleQueue([
+            new Response(200, [], $this->mockFakeUpdateInfo('1.2.3')),
+        ]);
         $this->getJson('/admin/update/download')
-            ->assertDontSee(trans('general.illegal-parameters'));
+            ->assertJson([
+                'code' => 1,
+                'message' => trans('admin.update.info.up-to-date'),
+            ]);
 
         // Download
         $this->appendToGuzzleQueue([
@@ -70,31 +74,17 @@ class UpdateControllerTest extends TestCase
         $this->mock(PackageManager::class, function ($mock) {
             $mock->shouldReceive('download')->andThrow(new \Exception('ddd'));
         });
-        $this->getJson('/admin/update/download?action=download')
-            ->assertJson(['code' => 1]);
+        $this->getJson('/admin/update/download')->assertJson(['code' => 1]);
         $this->mock(PackageManager::class, function ($mock) {
             $mock->shouldReceive('download')->andReturnSelf();
             $mock->shouldReceive('extract')->andReturn(true);
-            $mock->shouldReceive('progress');
         });
         $this->mock(\Illuminate\Filesystem\Filesystem::class, function ($mock) {
             $mock->shouldReceive('delete')->with(storage_path('options.php'))->once();
             $mock->shouldReceive('exists')->with(storage_path('install.lock'))->andReturn(true);
         });
-        $this->getJson('/admin/update/download?action=download')
+        $this->getJson('/admin/update/download')
             ->assertJson(['code' => 0, 'message' => trans('admin.update.complete')]);
-
-        // Get download progress
-        $this->getJson('/admin/update/download?action=progress')
-            ->assertSee('0');
-
-        // Invalid action
-        $this->appendToGuzzleQueue(200, [], $this->mockFakeUpdateInfo('8.9.3'));
-        $this->getJson('/admin/update/download?action=no')
-            ->assertJson([
-                'code' => 1,
-                'message' => trans('general.illegal-parameters'),
-            ]);
     }
 
     public function testUpdate()

tests/ServicesTest/PackageManagerTest.php

@@ -3,7 +3,6 @@
 namespace Tests;
 
 use App\Services\PackageManager;
-use Cache;
 use Exception;
 use GuzzleHttp\Client;
 use GuzzleHttp\Exception\RequestException;
@@ -12,7 +11,6 @@ use GuzzleHttp\HandlerStack;
 use GuzzleHttp\Psr7\Request;
 use GuzzleHttp\Psr7\Response;
 use Illuminate\Filesystem\Filesystem;
-use ReflectionClass;
 use ZipArchive;
 
 class PackageManagerTest extends TestCase
@@ -74,33 +72,4 @@ class PackageManagerTest extends TestCase
         $this->expectException(Exception::class);
         $package->extract('dest');
     }
-
-    public function testProgress()
-    {
-        $package = resolve(PackageManager::class);
-        $reflect = new ReflectionClass($package);
-        $property = $reflect->getProperty('cacheKey');
-        $property->setAccessible(true);
-        $property->setValue($package, 'key');
-
-        Cache::put('key', serialize(['total' => 0, 'done' => 0]));
-        $this->assertEquals(0, $package->progress());
-
-        Cache::put('key', serialize(['total' => 2, 'done' => 1]));
-        $this->assertEquals(0.5, $package->progress());
-    }
-
-    public function testOnProgress()
-    {
-        $package = resolve(PackageManager::class);
-        $reflect = new ReflectionClass($package);
-        $property = $reflect->getProperty('cacheKey');
-        $property->setAccessible(true);
-        $property->setValue($package, 'key');
-        $closure = $reflect->getProperty('onProgress');
-        $closure->setAccessible(true);
-
-        Cache::shouldReceive('put')->with('key', serialize(['total' => 5, 'done' => 4]));
-        call_user_func($closure->getValue($package), 5, 4);
-    }
 }