Commit 710e0dde authored by Paul Slaughter's avatar Paul Slaughter Committed by Phil Hughes

Fix IDE commit to use start_ref

**Why?**
The branch HEAD could be changed since the
IDE was opened. This leads to user's unintentionally
creating commits that overwrite other changes.

https://gitlab.com/gitlab-org/gitlab-ce/issues/59023
parent e3eeb779
......@@ -56,7 +56,13 @@ export default {
return Api.branchSingle(projectId, currentBranchId);
},
commit(projectId, payload) {
return Api.commitMultiple(projectId, payload);
// Currently the `commit` endpoint does not support `start_sha` so we
// have to make the request in the FE. This is not ideal and will be
// resolved soon. https://gitlab.com/gitlab-org/gitlab-ce/issues/59023
const { branch, start_sha: ref } = payload;
const branchPromise = ref ? Api.createBranch(projectId, { ref, branch }) : Promise.resolve();
return branchPromise.then(() => Api.commitMultiple(projectId, payload));
},
getFiles(projectUrl, branchId) {
const url = `${projectUrl}/files/${branchId}`;
......
......@@ -123,6 +123,7 @@ export const commitChanges = ({ commit, state, getters, dispatch, rootState, roo
getters,
state,
rootState,
rootGetters,
});
return service.commit(rootState.currentProjectId, payload);
......
......@@ -135,7 +135,14 @@ export const getCommitFiles = stagedFiles =>
});
}, []);
export const createCommitPayload = ({ branch, getters, newBranch, state, rootState }) => ({
export const createCommitPayload = ({
branch,
getters,
newBranch,
state,
rootState,
rootGetters,
}) => ({
branch,
commit_message: state.commitMessage || getters.preBuiltCommitMessage,
actions: getCommitFiles(rootState.stagedFiles).map(f => ({
......@@ -146,7 +153,7 @@ export const createCommitPayload = ({ branch, getters, newBranch, state, rootSta
encoding: f.base64 ? 'base64' : 'text',
last_commit_id: newBranch || f.deleted || f.prevPath ? undefined : f.lastCommitSha,
})),
start_branch: newBranch ? rootState.currentBranchId : undefined,
start_sha: newBranch ? rootGetters.lastCommit.short_id : undefined,
});
export const createNewMergeRequestUrl = (projectUrl, source, target) =>
......
---
title: Fix IDE commit using latest ref in branch and overriding contents
merge_request: 29769
author:
type: fixed
import services from '~/ide/services';
import Api from '~/api';
jest.mock('~/api');
const TEST_PROJECT_ID = 'alice/wonderland';
const TEST_BRANCH = 'master-patch-123';
const TEST_COMMIT_SHA = '123456789';
describe('IDE services', () => {
describe('commit', () => {
let payload;
beforeEach(() => {
payload = {
branch: TEST_BRANCH,
commit_message: 'Hello world',
actions: [],
start_sha: undefined,
};
Api.createBranch.mockReturnValue(Promise.resolve());
Api.commitMultiple.mockReturnValue(Promise.resolve());
});
describe.each`
startSha | shouldCreateBranch
${undefined} | ${false}
${TEST_COMMIT_SHA} | ${true}
`('when start_sha is $startSha', ({ startSha, shouldCreateBranch }) => {
beforeEach(() => {
payload.start_sha = startSha;
return services.commit(TEST_PROJECT_ID, payload);
});
if (shouldCreateBranch) {
it('should create branch', () => {
expect(Api.createBranch).toHaveBeenCalledWith(TEST_PROJECT_ID, {
ref: TEST_COMMIT_SHA,
branch: TEST_BRANCH,
});
});
} else {
it('should not create branch', () => {
expect(Api.createBranch).not.toHaveBeenCalled();
});
}
it('should commit', () => {
expect(Api.commitMultiple).toHaveBeenCalledWith(TEST_PROJECT_ID, payload);
});
});
});
});
......@@ -7,6 +7,8 @@ import consts from '~/ide/stores/modules/commit/constants';
import { commitActionTypes } from '~/ide/constants';
import { resetStore, file } from 'spec/ide/helpers';
const TEST_COMMIT_SHA = '123456789';
describe('IDE commit module actions', () => {
beforeEach(() => {
spyOn(router, 'push');
......@@ -136,6 +138,9 @@ describe('IDE commit module actions', () => {
branches: {
master: {
workingReference: '',
commit: {
short_id: TEST_COMMIT_SHA,
},
},
},
};
......@@ -236,6 +241,9 @@ describe('IDE commit module actions', () => {
branches: {
master: {
workingReference: '1',
commit: {
short_id: TEST_COMMIT_SHA,
},
},
},
};
......@@ -244,7 +252,7 @@ describe('IDE commit module actions', () => {
...file('changed'),
type: 'blob',
active: true,
lastCommitSha: '123456789',
lastCommitSha: TEST_COMMIT_SHA,
};
store.state.stagedFiles.push(f);
store.state.changedFiles = [
......@@ -303,7 +311,7 @@ describe('IDE commit module actions', () => {
previous_path: undefined,
},
],
start_branch: 'master',
start_sha: TEST_COMMIT_SHA,
});
done();
......@@ -326,11 +334,11 @@ describe('IDE commit module actions', () => {
file_path: jasmine.anything(),
content: undefined,
encoding: jasmine.anything(),
last_commit_id: '123456789',
last_commit_id: TEST_COMMIT_SHA,
previous_path: undefined,
},
],
start_branch: undefined,
start_sha: undefined,
});
done();
......
......@@ -132,7 +132,7 @@ describe('Multi-file store utils', () => {
previous_path: undefined,
},
],
start_branch: undefined,
start_sha: undefined,
});
});
......@@ -187,7 +187,7 @@ describe('Multi-file store utils', () => {
previous_path: undefined,
},
],
start_branch: undefined,
start_sha: undefined,
});
});
});
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment