Merge pull request #1134 from jescalada/remove-getMissingData

refactor: replace `getMissingData` action with `checkEmptyBranch`

Author: jescalada

src/proxy/chain.ts

@@ -5,6 +5,7 @@ import { attemptAutoApproval, attemptAutoRejection } from './actions/autoActions
 
 const pushActionChain: ((req: any, action: Action) => Promise<Action>)[] = [
   proc.push.parsePush,
+  proc.push.checkEmptyBranch,
   proc.push.checkRepoInAuthorisedList,
   proc.push.checkCommitMessages,
   proc.push.checkAuthorEmails,
@@ -13,7 +14,6 @@ const pushActionChain: ((req: any, action: Action) => Promise<Action>)[] = [
   proc.push.writePack,
   proc.push.checkHiddenCommits,
   proc.push.checkIfWaitingAuth,
-  proc.push.getMissingData,
   proc.push.preReceive,
   proc.push.getDiff,
   // run before clear remote

src/proxy/processors/push-action/checkEmptyBranch.ts

@@ -0,0 +1,42 @@
+import { Action, Step } from '../../actions';
+import simpleGit from 'simple-git';
+import { EMPTY_COMMIT_HASH } from '../constants';
+
+const isEmptyBranch = async (action: Action) => {
+  if (action.commitFrom === EMPTY_COMMIT_HASH) {
+    try {
+      const git = simpleGit(`${action.proxyGitPath}/${action.repoName}`);
+
+      const type = await git.raw(['cat-file', '-t', action.commitTo || '']);
+      return type.trim() === 'commit';
+    } catch (err) {
+      console.log(`Commit ${action.commitTo} not found: ${err}`);
+    }
+  }
+
+  return false;
+};
+
+const exec = async (req: any, action: Action): Promise<Action> => {
+  const step = new Step('checkEmptyBranch');
+
+  if (action.commitData && action.commitData.length > 0) {
+    return action;
+  }
+
+  if (await isEmptyBranch(action)) {
+    step.setError('Push blocked: Empty branch. Please make a commit before pushing a new branch.');
+    action.addStep(step);
+    step.error = true;
+    return action;
+  } else {
+    step.setError('Push blocked: Commit data not found. Please contact an administrator for support.');
+    action.addStep(step);
+    step.error = true;
+    return action;
+  }
+};
+
+exec.displayName = 'checkEmptyBranch.exec';
+
+export { exec };

src/proxy/processors/push-action/checkUserPushPermission.ts

@@ -8,7 +8,9 @@ const exec = async (req: any, action: Action): Promise<Action> => {
   const user = action.user;
 
   if (!user) {
-    console.log('Action has no user set. This may be due to a fast-forward ref update. Deferring to getMissingData action.');
+    step.setError('Push blocked: User not found. Please contact an administrator for support.');
+    action.addStep(step);
+    step.error = true;
     return action;
   }
 
@@ -17,8 +19,7 @@ const exec = async (req: any, action: Action): Promise<Action> => {
 
 /**
  * Helper that validates the user's push permission.
- * This can be used by other actions that need it. For example, when the user is missing from the commit data,
- * validation is deferred to getMissingData, but the logic is the same.
+ * This can be used by other actions that need it.
  * @param {string} user The user to validate
  * @param {Action} action The action object
  * @param {Step} step The step object

src/proxy/processors/push-action/getMissingData.ts

@@ -14,7 +14,7 @@ import { exec as checkCommitMessages } from './checkCommitMessages';
 import { exec as checkAuthorEmails } from './checkAuthorEmails';
 import { exec as checkUserPushPermission } from './checkUserPushPermission';
 import { exec as clearBareClone } from './clearBareClone';
-import { exec as getMissingData } from './getMissingData';
+import { exec as checkEmptyBranch } from './checkEmptyBranch';
 
 export {
   parsePush,
@@ -33,5 +33,5 @@ export {
   checkAuthorEmails,
   checkUserPushPermission,
   clearBareClone,
-  getMissingData,
+  checkEmptyBranch,
 };

src/proxy/processors/push-action/index.ts

@@ -18,6 +18,7 @@ const mockLoader = {
 const initMockPushProcessors = (sinon) => {
   const mockPushProcessors = {
     parsePush: sinon.stub(),
+    checkEmptyBranch: sinon.stub(),
     audit: sinon.stub(),
     checkRepoInAuthorisedList: sinon.stub(),
     checkCommitMessages: sinon.stub(),
@@ -33,9 +34,9 @@ const initMockPushProcessors = (sinon) => {
     clearBareClone: sinon.stub(),
     scanDiff: sinon.stub(),
     blockForAuth: sinon.stub(),
-    getMissingData: sinon.stub(),
   };
   mockPushProcessors.parsePush.displayName = 'parsePush';
+  mockPushProcessors.checkEmptyBranch.displayName = 'checkEmptyBranch';
   mockPushProcessors.audit.displayName = 'audit';
   mockPushProcessors.checkRepoInAuthorisedList.displayName = 'checkRepoInAuthorisedList';
   mockPushProcessors.checkCommitMessages.displayName = 'checkCommitMessages';
@@ -51,7 +52,6 @@ const initMockPushProcessors = (sinon) => {
   mockPushProcessors.clearBareClone.displayName = 'clearBareClone';
   mockPushProcessors.scanDiff.displayName = 'scanDiff';
   mockPushProcessors.blockForAuth.displayName = 'blockForAuth';
-  mockPushProcessors.getMissingData.displayName = 'getMissingData';
   return mockPushProcessors;
 };
 const mockPreProcessors = {
@@ -127,6 +127,7 @@ describe('proxy chain', function () {
     const continuingAction = { type: 'push', continue: () => true, allowPush: false };
     mockPreProcessors.parseAction.resolves({ type: 'push' });
     mockPushProcessors.parsePush.resolves(continuingAction);
+    mockPushProcessors.checkEmptyBranch.resolves(continuingAction);
     mockPushProcessors.checkRepoInAuthorisedList.resolves(continuingAction);
     mockPushProcessors.checkCommitMessages.resolves(continuingAction);
     mockPushProcessors.checkAuthorEmails.resolves(continuingAction);
@@ -152,7 +153,7 @@ describe('proxy chain', function () {
     expect(mockPushProcessors.pullRemote.called).to.be.true;
     expect(mockPushProcessors.checkHiddenCommits.called).to.be.true;
     expect(mockPushProcessors.writePack.called).to.be.true;
-    expect(mockPushProcessors.getMissingData.called).to.be.false;
+    expect(mockPushProcessors.checkEmptyBranch.called).to.be.true;
     expect(mockPushProcessors.audit.called).to.be.true;
 
     expect(result.type).to.equal('push');
@@ -165,6 +166,7 @@ describe('proxy chain', function () {
     const continuingAction = { type: 'push', continue: () => true, allowPush: false };
     mockPreProcessors.parseAction.resolves({ type: 'push' });
     mockPushProcessors.parsePush.resolves(continuingAction);
+    mockPushProcessors.checkEmptyBranch.resolves(continuingAction);
     mockPushProcessors.checkRepoInAuthorisedList.resolves(continuingAction);
     mockPushProcessors.checkCommitMessages.resolves(continuingAction);
     mockPushProcessors.checkAuthorEmails.resolves(continuingAction);
@@ -182,6 +184,7 @@ describe('proxy chain', function () {
 
     expect(mockPreProcessors.parseAction.called).to.be.true;
     expect(mockPushProcessors.parsePush.called).to.be.true;
+    expect(mockPushProcessors.checkEmptyBranch.called).to.be.true;
     expect(mockPushProcessors.checkRepoInAuthorisedList.called).to.be.true;
     expect(mockPushProcessors.checkCommitMessages.called).to.be.true;
     expect(mockPushProcessors.checkAuthorEmails.called).to.be.true;
@@ -190,7 +193,6 @@ describe('proxy chain', function () {
     expect(mockPushProcessors.pullRemote.called).to.be.true;
     expect(mockPushProcessors.checkHiddenCommits.called).to.be.true;
     expect(mockPushProcessors.writePack.called).to.be.true;
-    expect(mockPushProcessors.getMissingData.called).to.be.false;
     expect(mockPushProcessors.audit.called).to.be.true;
 
     expect(result.type).to.equal('push');
@@ -203,6 +205,7 @@ describe('proxy chain', function () {
     const continuingAction = { type: 'push', continue: () => true, allowPush: false };
     mockPreProcessors.parseAction.resolves({ type: 'push' });
     mockPushProcessors.parsePush.resolves(continuingAction);
+    mockPushProcessors.checkEmptyBranch.resolves(continuingAction);
     mockPushProcessors.checkRepoInAuthorisedList.resolves(continuingAction);
     mockPushProcessors.checkCommitMessages.resolves(continuingAction);
     mockPushProcessors.checkAuthorEmails.resolves(continuingAction);
@@ -217,12 +220,12 @@ describe('proxy chain', function () {
     mockPushProcessors.clearBareClone.resolves(continuingAction);
     mockPushProcessors.scanDiff.resolves(continuingAction);
     mockPushProcessors.blockForAuth.resolves(continuingAction);
-    mockPushProcessors.getMissingData.resolves(continuingAction);
 
     const result = await chain.executeChain(req);
 
     expect(mockPreProcessors.parseAction.called).to.be.true;
     expect(mockPushProcessors.parsePush.called).to.be.true;
+    expect(mockPushProcessors.checkEmptyBranch.called).to.be.true;
     expect(mockPushProcessors.checkRepoInAuthorisedList.called).to.be.true;
     expect(mockPushProcessors.checkCommitMessages.called).to.be.true;
     expect(mockPushProcessors.checkAuthorEmails.called).to.be.true;
@@ -238,7 +241,6 @@ describe('proxy chain', function () {
     expect(mockPushProcessors.scanDiff.called).to.be.true;
     expect(mockPushProcessors.blockForAuth.called).to.be.true;
     expect(mockPushProcessors.audit.called).to.be.true;
-    expect(mockPushProcessors.getMissingData.called).to.be.true;
 
     expect(result.type).to.equal('push');
     expect(result.allowPush).to.be.false;
@@ -299,6 +301,7 @@ describe('proxy chain', function () {
 
     mockPreProcessors.parseAction.resolves(action);
     mockPushProcessors.parsePush.resolves(action);
+    mockPushProcessors.checkEmptyBranch.resolves(action);
     mockPushProcessors.checkRepoInAuthorisedList.resolves(action);
     mockPushProcessors.checkCommitMessages.resolves(action);
     mockPushProcessors.checkAuthorEmails.resolves(action);
@@ -320,7 +323,6 @@ describe('proxy chain', function () {
     mockPushProcessors.clearBareClone.resolves(action);
     mockPushProcessors.scanDiff.resolves(action);
     mockPushProcessors.blockForAuth.resolves(action);
-    mockPushProcessors.getMissingData.resolves(action);
     const dbStub = sinon.stub(db, 'authorise').resolves(true);
 
     const result = await chain.executeChain(req);
@@ -347,6 +349,7 @@ describe('proxy chain', function () {
 
     mockPreProcessors.parseAction.resolves(action);
     mockPushProcessors.parsePush.resolves(action);
+    mockPushProcessors.checkEmptyBranch.resolves(action);
     mockPushProcessors.checkRepoInAuthorisedList.resolves(action);
     mockPushProcessors.checkCommitMessages.resolves(action);
     mockPushProcessors.checkAuthorEmails.resolves(action);
@@ -368,7 +371,6 @@ describe('proxy chain', function () {
     mockPushProcessors.clearBareClone.resolves(action);
     mockPushProcessors.scanDiff.resolves(action);
     mockPushProcessors.blockForAuth.resolves(action);
-    mockPushProcessors.getMissingData.resolves(action);
 
     const dbStub = sinon.stub(db, 'reject').resolves(true);
 
@@ -396,6 +398,7 @@ describe('proxy chain', function () {
 
     mockPreProcessors.parseAction.resolves(action);
     mockPushProcessors.parsePush.resolves(action);
+    mockPushProcessors.checkEmptyBranch.resolves(action);
     mockPushProcessors.checkRepoInAuthorisedList.resolves(action);
     mockPushProcessors.checkCommitMessages.resolves(action);
     mockPushProcessors.checkAuthorEmails.resolves(action);
@@ -417,7 +420,6 @@ describe('proxy chain', function () {
     mockPushProcessors.clearBareClone.resolves(action);
     mockPushProcessors.scanDiff.resolves(action);
     mockPushProcessors.blockForAuth.resolves(action);
-    mockPushProcessors.getMissingData.resolves(action);
 
     const error = new Error('Database error');
 
@@ -444,6 +446,7 @@ describe('proxy chain', function () {
 
     mockPreProcessors.parseAction.resolves(action);
     mockPushProcessors.parsePush.resolves(action);
+    mockPushProcessors.checkEmptyBranch.resolves(action);
     mockPushProcessors.checkRepoInAuthorisedList.resolves(action);
     mockPushProcessors.checkCommitMessages.resolves(action);
     mockPushProcessors.checkAuthorEmails.resolves(action);
@@ -465,7 +468,6 @@ describe('proxy chain', function () {
     mockPushProcessors.clearBareClone.resolves(action);
     mockPushProcessors.scanDiff.resolves(action);
     mockPushProcessors.blockForAuth.resolves(action);
-    mockPushProcessors.getMissingData.resolves(action);
 
     const error = new Error('Database error');
 

test/chain.test.js

@@ -97,8 +97,9 @@ describe('checkCommitMessages', () => {
     });
 
     it('should not error when commit data is empty', async () => {
-      // Empty commit data is a valid scenario that happens when making a branch from an unapproved commit
-      // This is remedied in the getMissingData.exec action  
+      // Empty commit data happens when making a branch from an unapproved commit
+      // or when pushing an empty branch or deleting a branch
+      // This is handled in the checkEmptyBranch.exec action  
       action.commitData = [];
       const result = await exec(req, action);