Fix migration script for layouts

migrator/index.js

@@ -19,6 +19,7 @@ const {
   deleteIfUnchanged,
   removeOldPatternIncludesFromSassFile,
   updateUnbrandedLayouts,
+  upgradeLayoutIfUnchanged,
   upgradeIfPossible
 } = require('./migration-steps')
 
@@ -81,6 +82,8 @@ const filesToDeleteIfUnchanged = [
   'app/assets/sass/unbranded.scss',
   'app/views/includes/breadcrumb_examples.html',
   'app/views/includes/cookie-banner.html',
+  'app/views/includes/head.html',
+  'app/views/includes/scripts.html',
   'app/views/layout_unbranded.html'
 ]
 
@@ -118,19 +121,6 @@ async function prepareMigration (kitDependency, projectDirectory) {
   ])
 }
 
-// Special case app/views/layout.html, as it has moved in prototype
-// starter files, but we don't want to move for existing users
-async function upgradeLayoutIfUnchanged () {
-  const results = await upgradeIfUnchanged(
-    ['app/views/layout.html'],
-    'app/views/layouts/main.html',
-    () => deleteIfUnchanged([
-      'app/views/includes/head.html',
-      'app/views/includes/scripts.html'
-    ]))
-  return results.flat()
-}
-
 async function migrate () {
   await logger.setup()
 
@@ -143,8 +133,8 @@ async function migrate () {
       prepareSass('app/assets/sass/application.scss'),
       deleteUnusedFiles(filesToDelete),
       deleteUnusedDirectories(directoriesToDelete),
-      upgradeIfUnchanged(filesToUpdateIfUnchanged, '', upgradeIfPossible),
-      upgradeLayoutIfUnchanged(),
+      upgradeIfUnchanged(filesToUpdateIfUnchanged, upgradeIfPossible),
+      upgradeLayoutIfUnchanged('app/views/layout.html', 'app/views/layouts/main.html'),
       updateUnbrandedLayouts('app/views'),
       deleteIfUnchanged(filesToDeleteIfUnchanged),
       deleteIfUnchanged(patternsToDeleteIfUnchanged)

migrator/migration-steps.js

@@ -206,16 +206,37 @@ async function deleteIfUnchanged (filePaths) {
   }))
 }
 
-async function upgradeIfUnchanged (filePaths, starterFilePath, additionalStep) {
-  return Promise.all(filePaths.map(async filePath => {
+// Special case app/views/layout.html, as it has moved in prototype
+// starter files, but we don't want to move for existing users
+async function upgradeLayoutIfUnchanged (filePath, starterFilePath) {
+  const matchFound = await matchAgainstOldVersions(filePath)
+  const reporter = await addReporter(`Overwrite ${filePath}`)
+
+  let result = false
+  try {
+    if (matchFound) {
+      await copyFileFromStarter(starterFilePath, filePath)
+      result = true
+    }
+  } catch (e) {
+    await verboseLog(e.message)
+    await verboseLog(e.stack)
+  }
+
+  await reporter(result)
+  return result
+}
+
+async function upgradeIfUnchanged (filePaths, additionalStep) {
+  const results = await Promise.all(filePaths.map(async filePath => {
     const matchFound = await matchAgainstOldVersions(filePath)
 
     const reporter = await addReporter(`Overwrite ${filePath}`)
 
     let result = false
     try {
       if (matchFound) {
-        await copyFileFromStarter(starterFilePath || filePath, filePath)
+        await copyFileFromStarter(filePath)
       }
       if (additionalStep) {
         result = await additionalStep(filePath, matchFound)
@@ -230,6 +251,7 @@ async function upgradeIfUnchanged (filePaths, starterFilePath, additionalStep) {
     await reporter(result)
     return result
   }))
+  return results
 }
 
 async function updateUnbrandedLayouts (dir) {
@@ -252,6 +274,7 @@ module.exports = {
   deleteUnusedDirectories,
   deleteEmptyDirectories,
   deleteIfUnchanged,
+  upgradeLayoutIfUnchanged,
   upgradeIfUnchanged,
   updateUnbrandedLayouts,
   upgradeIfPossible

migrator/migration-steps.spec.js

@@ -49,14 +49,17 @@ const config = require('../lib/config')
 const { projectDir, starterDir, appDir } = require('../lib/utils/paths')
 
 const migrationSteps = require('./migration-steps')
-const { preflightChecks, deleteIfUnchanged, removeOldPatternIncludesFromSassFile } = require('./migration-steps')
 const {
+  preflightChecks,
+  deleteIfUnchanged,
+  removeOldPatternIncludesFromSassFile,
+  upgradeIfUnchanged,
+  upgradeLayoutIfUnchanged,
   migrateConfig,
   prepareAppRoutes,
   prepareSass,
   deleteUnusedFiles,
-  deleteUnusedDirectories,
-  upgradeIfUnchanged
+  deleteUnusedDirectories
 } = migrationSteps
 
 describe('migration steps', () => {
@@ -277,20 +280,46 @@ describe('migration steps', () => {
     expect(mockReporter).toHaveBeenNthCalledWith(3, true)
   })
 
+  it('upgrade unchanged application.js and fail changed filters.js', async () => {
+    const appFile = 'application.js'
+    const filtersFile = 'filters.js'
+    const additionalStep = jest.fn()
+      .mockImplementationOnce(async () => true)
+      .mockImplementationOnce(async () => false)
+    const result = await upgradeIfUnchanged([appFile, filtersFile], additionalStep)
+
+    expect(result).toEqual([true, false])
+
+    expect(fileHelpers.matchAgainstOldVersions).toHaveBeenCalledTimes(2)
+    expect(fileHelpers.matchAgainstOldVersions.mock.calls).toEqual([
+      [appFile], // First call
+      [filtersFile] // Second call
+    ])
+
+    expect(reporter.addReporter).toHaveBeenCalledTimes(2)
+    expect(reporter.addReporter.mock.calls).toEqual([
+      [`Overwrite ${appFile}`], // First call
+      [`Overwrite ${filtersFile}`] // Second call
+    ])
+
+    expect(mockReporter).toHaveBeenCalledTimes(2)
+    expect(mockReporter.mock.calls).toEqual([
+      [true], // First call
+      [false] // Second call
+    ])
+  })
+
   it('upgrade if unchanged layout', async () => {
     const layout = 'app/views/layout.html'
-    const additionalStep = jest.fn().mockResolvedValue(true)
+    const starterLayout = 'app/views/layouts/main.html'
 
-    const result = await upgradeIfUnchanged([layout], layout, additionalStep)
+    const result = await upgradeLayoutIfUnchanged(layout, starterLayout)
 
     expect(result).toBeTruthy()
 
     expect(fileHelpers.matchAgainstOldVersions).toHaveBeenCalledTimes(1)
     expect(fileHelpers.matchAgainstOldVersions).toHaveBeenCalledWith(layout)
 
-    expect(additionalStep).toHaveBeenCalledTimes(1)
-    expect(additionalStep).toHaveBeenCalled()
-
     expect(reporter.addReporter).toHaveBeenCalledTimes(1)
     expect(reporter.addReporter).toHaveBeenCalledWith(`Overwrite ${layout}`)
 
@@ -300,25 +329,22 @@ describe('migration steps', () => {
 
   it('report if changed layout', async () => {
     const layout = 'app/views/layout.html'
-    const additionalStep = jest.fn().mockResolvedValue(true)
+    const starterLayout = 'app/views/layouts/main.html'
 
     fileHelpers.matchAgainstOldVersions.mockReturnValue(false)
 
-    const result = await upgradeIfUnchanged([layout], layout, additionalStep)
+    const result = await upgradeLayoutIfUnchanged(layout, starterLayout)
 
-    expect(result).toBeTruthy()
+    expect(result).toBeFalsy()
 
     expect(fileHelpers.matchAgainstOldVersions).toHaveBeenCalledTimes(1)
     expect(fileHelpers.matchAgainstOldVersions).toHaveBeenCalledWith(layout)
 
-    expect(additionalStep).toHaveBeenCalledTimes(1)
-    expect(additionalStep).toHaveBeenCalled()
-
     expect(reporter.addReporter).toHaveBeenCalledTimes(1)
     expect(reporter.addReporter).toHaveBeenCalledWith(`Overwrite ${layout}`)
 
     expect(mockReporter).toHaveBeenCalledTimes(1)
-    expect(mockReporter).toHaveBeenCalledWith(true)
+    expect(mockReporter).toHaveBeenCalledWith(false)
   })
 
   it('delete if unchanged unbranded layout', async () => {

migrator/migrator.spec.js

@@ -21,6 +21,7 @@ jest.mock('./migration-steps', () => {
     deleteUnusedDirectories: jest.fn().mockResolvedValue(true),
     deleteEmptyDirectories: jest.fn().mockResolvedValue([true]),
     upgradeIfUnchanged: jest.fn(),
+    upgradeLayoutIfUnchanged: jest.fn().mockResolvedValue(true),
     upgradeIfPossible: jest.fn(),
     updateUnbrandedLayouts: jest.fn().mockResolvedValue(true),
     deleteIfUnchanged: jest.fn().mockResolvedValue([true, true, true]),