Merge branch 'ide-commit-form-improvements' into 'master'

Improve Web IDE commit form

Closes #47307

See merge request gitlab-org/gitlab-ce!19883

app/assets/javascripts/ide/components/commit_sidebar/actions.vue

@@ -10,7 +10,7 @@ export default {
   },
   computed: {
     ...mapState(['currentBranchId', 'changedFiles', 'stagedFiles']),
-    ...mapGetters(['currentProject']),
+    ...mapGetters(['currentProject', 'currentBranch']),
     commitToCurrentBranchText() {
       return sprintf(
         __('Commit to %{branchName} branch'),
@@ -22,17 +22,30 @@ export default {
       return this.changedFiles.length > 0 && this.stagedFiles.length > 0;
     },
   },
+  watch: {
+    disableMergeRequestRadio() {
+      this.updateSelectedCommitAction();
+    },
+  },
   mounted() {
-    if (this.disableMergeRequestRadio) {
-      this.updateCommitAction(consts.COMMIT_TO_CURRENT_BRANCH);
-    }
+    this.updateSelectedCommitAction();
   },
   methods: {
     ...mapActions('commit', ['updateCommitAction']),
+    updateSelectedCommitAction() {
+      if (this.currentBranch && !this.currentBranch.can_push) {
+        this.updateCommitAction(consts.COMMIT_TO_NEW_BRANCH);
+      } else if (this.disableMergeRequestRadio) {
+        this.updateCommitAction(consts.COMMIT_TO_CURRENT_BRANCH);
+      }
+    },
   },
   commitToCurrentBranch: consts.COMMIT_TO_CURRENT_BRANCH,
   commitToNewBranch: consts.COMMIT_TO_NEW_BRANCH,
   commitToNewBranchMR: consts.COMMIT_TO_NEW_BRANCH_MR,
+  currentBranchPermissionsTooltip: __(
+    "This option is disabled as you don't have write permissions for the current branch",
+  ),
 };
 </script>
 
@@ -40,9 +53,11 @@ export default {
   <div class="append-bottom-15 ide-commit-radios">
     <radio-group
       :value="$options.commitToCurrentBranch"
-      :checked="true"
+      :disabled="currentBranch && !currentBranch.can_push"
+      :title="$options.currentBranchPermissionsTooltip"
     >
       <span
+        class="ide-radio-label"
         v-html="commitToCurrentBranchText"
       >
       </span>
@@ -56,6 +71,7 @@ export default {
       v-if="currentProject.merge_requests_enabled"
       :value="$options.commitToNewBranchMR"
       :label="__('Create a new branch and merge request')"
+      :title="__('This option is disabled while you still have unstaged changes')"
       :show-input="true"
       :disabled="disableMergeRequestRadio"
     />

app/assets/javascripts/ide/components/commit_sidebar/form.vue

@@ -24,7 +24,7 @@ export default {
     ...mapState(['changedFiles', 'stagedFiles', 'currentActivityView', 'lastCommitMsg']),
     ...mapState('commit', ['commitMessage', 'submitCommitLoading']),
     ...mapGetters(['hasChanges']),
-    ...mapGetters('commit', ['commitButtonDisabled', 'discardDraftButtonDisabled']),
+    ...mapGetters('commit', ['discardDraftButtonDisabled', 'preBuiltCommitMessage']),
     overviewText() {
       return sprintf(
         __(
@@ -36,6 +36,9 @@ export default {
         },
       );
     },
+    commitButtonText() {
+      return this.stagedFiles.length ? __('Commit') : __('Stage & Commit');
+    },
   },
   watch: {
     currentActivityView() {
@@ -136,14 +139,14 @@ export default {
         </transition>
         <commit-message-field
           :text="commitMessage"
+          :placeholder="preBuiltCommitMessage"
           @input="updateCommitMessage"
         />
         <div class="clearfix prepend-top-15">
           <actions />
           <loading-button
             :loading="submitCommitLoading"
-            :disabled="commitButtonDisabled"
-            :label="__('Commit')"
+            :label="commitButtonText"
             container-class="btn btn-success btn-sm float-left"
             @click="commitChanges"
           />

app/assets/javascripts/ide/components/commit_sidebar/message_field.vue

@@ -16,6 +16,10 @@ export default {
       type: String,
       required: true,
     },
+    placeholder: {
+      type: String,
+      required: true,
+    },
   },
   data() {
     return {
@@ -114,7 +118,7 @@ export default {
         </div>
         <textarea
           ref="textarea"
-          :placeholder="__('Write a commit message...')"
+          :placeholder="placeholder"
           :value="text"
           class="note-textarea ide-commit-message-textarea"
           name="commit-message"

app/assets/javascripts/ide/components/commit_sidebar/radio_group.vue

 <script>
 import { mapActions, mapState, mapGetters } from 'vuex';
-import { __ } from '~/locale';
 import tooltip from '~/vue_shared/directives/tooltip';
 
 export default {
@@ -32,14 +31,17 @@ export default {
       required: false,
       default: false,
     },
+    title: {
+      type: String,
+      required: false,
+      default: '',
+    },
   },
   computed: {
     ...mapState('commit', ['commitAction']),
     ...mapGetters('commit', ['newBranchName']),
     tooltipTitle() {
-      return this.disabled
-        ? __('This option is disabled while you still have unstaged changes')
-        : '';
+      return this.disabled ? this.title : '';
     },
   },
   methods: {

app/assets/javascripts/ide/components/repo_commit_section.vue

@@ -28,7 +28,7 @@ export default {
     ]),
     ...mapState('commit', ['commitMessage', 'submitCommitLoading']),
     ...mapGetters(['lastOpenedFile', 'hasChanges', 'someUncommitedChanges', 'activeFile']),
-    ...mapGetters('commit', ['commitButtonDisabled', 'discardDraftButtonDisabled']),
+    ...mapGetters('commit', ['discardDraftButtonDisabled']),
     showStageUnstageArea() {
       return !!(this.someUncommitedChanges || this.lastCommitMsg || !this.unusedSeal);
     },

app/assets/javascripts/ide/stores/getters.js

@@ -82,10 +82,13 @@ export const getStagedFilesCountForPath = state => path =>
   getChangesCountForFiles(state.stagedFiles, path);
 
 export const lastCommit = (state, getters) => {
-  const branch = getters.currentProject && getters.currentProject.branches[state.currentBranchId];
+  const branch = getters.currentProject && getters.currentBranch;
 
   return branch ? branch.commit : null;
 };
 
+export const currentBranch = (state, getters) =>
+  getters.currentProject && getters.currentProject.branches[state.currentBranchId];
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

app/assets/javascripts/ide/stores/modules/commit/actions.js

@@ -103,17 +103,24 @@ export const updateFilesAfterCommit = ({ commit, dispatch, rootState }, { data }
 
 export const commitChanges = ({ commit, state, getters, dispatch, rootState, rootGetters }) => {
   const newBranch = state.commitAction !== consts.COMMIT_TO_CURRENT_BRANCH;
-  const payload = createCommitPayload({
-    branch: getters.branchName,
-    newBranch,
-    state,
-    rootState,
-  });
+  const stageFilesPromise = rootState.stagedFiles.length
+    ? Promise.resolve()
+    : dispatch('stageAllChanges', null, { root: true });
 
   commit(types.UPDATE_LOADING, true);
 
-  return service
-    .commit(rootState.currentProjectId, payload)
+  return stageFilesPromise
+    .then(() => {
+      const payload = createCommitPayload({
+        branch: getters.branchName,
+        newBranch,
+        getters,
+        state,
+        rootState,
+      });
+
+      return service.commit(rootState.currentProjectId, payload);
+    })
     .then(({ data }) => {
       commit(types.UPDATE_LOADING, false);
 

app/assets/javascripts/ide/stores/modules/commit/getters.js

+import { sprintf, n__ } from '../../../../locale';
 import * as consts from './constants';
 
 const BRANCH_SUFFIX_COUNT = 5;
@@ -5,9 +6,6 @@ const BRANCH_SUFFIX_COUNT = 5;
 export const discardDraftButtonDisabled = state =>
   state.commitMessage === '' || state.submitCommitLoading;
 
-export const commitButtonDisabled = (state, getters, rootState) =>
-  getters.discardDraftButtonDisabled || !rootState.stagedFiles.length;
-
 export const newBranchName = (state, _, rootState) =>
   `${gon.current_username}-${rootState.currentBranchId}-patch-${`${new Date().getTime()}`.substr(
     -BRANCH_SUFFIX_COUNT,
@@ -28,5 +26,18 @@ export const branchName = (state, getters, rootState) => {
   return rootState.currentBranchId;
 };
 
+export const preBuiltCommitMessage = (state, _, rootState) => {
+  if (state.commitMessage) return state.commitMessage;
+
+  const files = (rootState.stagedFiles.length
+    ? rootState.stagedFiles
+    : rootState.changedFiles
+  ).reduce((acc, val) => acc.concat(val.path), []);
+
+  return sprintf(n__('Update %{files}', 'Update %{files} files', files.length), {
+    files: files.join(', '),
+  });
+};
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

app/assets/javascripts/ide/stores/utils.js

@@ -105,9 +105,9 @@ export const setPageTitle = title => {
   document.title = title;
 };
 
-export const createCommitPayload = ({ branch, newBranch, state, rootState }) => ({
+export const createCommitPayload = ({ branch, getters, newBranch, state, rootState }) => ({
   branch,
-  commit_message: state.commitMessage,
+  commit_message: state.commitMessage || getters.preBuiltCommitMessage,
   actions: rootState.stagedFiles.map(f => ({
     action: f.tempFile ? 'create' : 'update',
     file_path: f.path,

spec/javascripts/ide/components/commit_sidebar/form_spec.js

@@ -16,6 +16,7 @@ describe('IDE commit form', () => {
 
     store.state.changedFiles.push('test');
     store.state.currentProjectId = 'abcproject';
+    store.state.currentBranchId = 'master';
     Vue.set(store.state.projects, 'abcproject', { ...projectData });
 
     vm = createComponentWithStore(Component, store).$mount();
@@ -146,4 +147,16 @@ describe('IDE commit form', () => {
       });
     });
   });
+
+  describe('commitButtonText', () => {
+    it('returns commit text when staged files exist', () => {
+      vm.$store.state.stagedFiles.push('testing');
+
+      expect(vm.commitButtonText).toBe('Commit');
+    });
+
+    it('returns stage & commit text when staged files do not exist', () => {
+      expect(vm.commitButtonText).toBe('Stage & Commit');
+    });
+  });
 });

spec/javascripts/ide/components/commit_sidebar/message_field_spec.js

@@ -13,6 +13,7 @@ describe('IDE commit message field', () => {
       Component,
       {
         text: '',
+        placeholder: 'testing',
       },
       '#app',
     );

spec/javascripts/ide/components/commit_sidebar/radio_group_spec.js

@@ -114,4 +114,19 @@ describe('IDE commit sidebar radio group', () => {
       });
     });
   });
+
+  describe('tooltipTitle', () => {
+    it('returns title when disabled', () => {
+      vm.title = 'test title';
+      vm.disabled = true;
+
+      expect(vm.tooltipTitle).toBe('test title');
+    });
+
+    it('returns blank when not disabled', () => {
+      vm.title = 'test title';
+
+      expect(vm.tooltipTitle).not.toBe('test title');
+    });
+  });
 });

spec/javascripts/ide/mock_data.js

@@ -8,6 +8,7 @@ export const projectData = {
   branches: {
     master: {
       treeId: 'abcproject/master',
+      can_push: true,
     },
   },
   mergeRequests: {},

spec/javascripts/ide/stores/getters_spec.js

@@ -147,12 +147,11 @@ describe('IDE store getters', () => {
       const commitTitle = 'Example commit title';
       const localGetters = {
         currentProject: {
-          branches: {
-            'example-branch': {
-              commit: {
-                title: commitTitle,
-              },
-            },
+          name: 'test-project',
+        },
+        currentBranch: {
+          commit: {
+            title: commitTitle,
           },
         },
       };
@@ -161,4 +160,23 @@ describe('IDE store getters', () => {
       expect(getters.lastCommit(localState, localGetters).title).toBe(commitTitle);
     });
   });
+
+  describe('currentBranch', () => {
+    it('returns current projects branch', () => {
+      const localGetters = {
+        currentProject: {
+          branches: {
+            master: {
+              name: 'master',
+            },
+          },
+        },
+      };
+      localState.currentBranchId = 'master';
+
+      expect(getters.currentBranch(localState, localGetters)).toEqual({
+        name: 'master',
+      });
+    });
+  });
 });

spec/javascripts/ide/stores/modules/commit/getters_spec.js

@@ -29,46 +29,6 @@ describe('IDE commit module getters', () => {
     });
   });
 
-  describe('commitButtonDisabled', () => {
-    const localGetters = {
-      discardDraftButtonDisabled: false,
-    };
-    const rootState = {
-      stagedFiles: ['a'],
-    };
-
-    it('returns false when discardDraftButtonDisabled is false & stagedFiles is not empty', () => {
-      expect(
-        getters.commitButtonDisabled(state, localGetters, rootState),
-      ).toBeFalsy();
-    });
-
-    it('returns true when discardDraftButtonDisabled is false & stagedFiles is empty', () => {
-      rootState.stagedFiles.length = 0;
-
-      expect(
-        getters.commitButtonDisabled(state, localGetters, rootState),
-      ).toBeTruthy();
-    });
-
-    it('returns true when discardDraftButtonDisabled is true', () => {
-      localGetters.discardDraftButtonDisabled = true;
-
-      expect(
-        getters.commitButtonDisabled(state, localGetters, rootState),
-      ).toBeTruthy();
-    });
-
-    it('returns true when discardDraftButtonDisabled is false & changedFiles is not empty', () => {
-      localGetters.discardDraftButtonDisabled = false;
-      rootState.stagedFiles.length = 0;
-
-      expect(
-        getters.commitButtonDisabled(state, localGetters, rootState),
-      ).toBeTruthy();
-    });
-  });
-
   describe('newBranchName', () => {
     it('includes username, currentBranchId, patch & random number', () => {
       gon.current_username = 'username';
@@ -108,9 +68,7 @@ describe('IDE commit module getters', () => {
         });
 
         it('uses newBranchName when not empty', () => {
-          expect(getters.branchName(state, localGetters, rootState)).toBe(
-            'state-newBranchName',
-          );
+          expect(getters.branchName(state, localGetters, rootState)).toBe('state-newBranchName');
         });
 
         it('uses getters newBranchName when state newBranchName is empty', () => {
@@ -118,11 +76,53 @@ describe('IDE commit module getters', () => {
             newBranchName: '',
           });
 
-          expect(getters.branchName(state, localGetters, rootState)).toBe(
-            'newBranchName',
-          );
+          expect(getters.branchName(state, localGetters, rootState)).toBe('newBranchName');
         });
       });
     });
   });
+
+  describe('preBuiltCommitMessage', () => {
+    let rootState = {};
+
+    beforeEach(() => {
+      rootState.changedFiles = [];
+      rootState.stagedFiles = [];
+    });
+
+    afterEach(() => {
+      rootState = {};
+    });
+
+    it('returns commitMessage when set', () => {
+      state.commitMessage = 'test commit message';
+
+      expect(getters.preBuiltCommitMessage(state, null, rootState)).toBe('test commit message');
+    });
+
+    ['changedFiles', 'stagedFiles'].forEach(key => {
+      it('returns commitMessage with updated file', () => {
+        rootState[key].push({
+          path: 'test-file',
+        });
+
+        expect(getters.preBuiltCommitMessage(state, null, rootState)).toBe('Update test-file');
+      });
+
+      it('returns commitMessage with updated files', () => {
+        rootState[key].push(
+          {
+            path: 'test-file',
+          },
+          {
+            path: 'index.js',
+          },
+        );
+
+        expect(getters.preBuiltCommitMessage(state, null, rootState)).toBe(
+          'Update test-file, index.js files',
+        );
+      });
+    });
+  });
 });

spec/javascripts/ide/stores/utils_spec.js

@@ -94,6 +94,7 @@ describe('Multi-file store utils', () => {
         newBranch: false,
         state,
         rootState,
+        getters: {},
       });
 
       expect(payload).toEqual({
@@ -118,5 +119,58 @@ describe('Multi-file store utils', () => {
         start_branch: undefined,
       });
     });
+
+    it('uses prebuilt commit message when commit message is empty', () => {
+      const rootState = {
+        stagedFiles: [
+          {
+            ...file('staged'),
+            path: 'staged',
+            content: 'updated file content',
+            lastCommitSha: '123456789',
+          },
+          {
+            ...file('newFile'),
+            path: 'added',
+            tempFile: true,
+            content: 'new file content',
+            base64: true,
+            lastCommitSha: '123456789',
+          },
+        ],
+        currentBranchId: 'master',
+      };
+      const payload = utils.createCommitPayload({
+        branch: 'master',
+        newBranch: false,
+        state: {},
+        rootState,
+        getters: {
+          preBuiltCommitMessage: 'prebuilt test commit message',
+        },
+      });
+
+      expect(payload).toEqual({
+        branch: 'master',
+        commit_message: 'prebuilt test commit message',
+        actions: [
+          {
+            action: 'update',
+            file_path: 'staged',
+            content: 'updated file content',
+            encoding: 'text',
+            last_commit_id: '123456789',
+          },
+          {
+            action: 'create',
+            file_path: 'added',
+            content: 'new file content',
+            encoding: 'base64',
+            last_commit_id: '123456789',
+          },
+        ],
+        start_branch: undefined,
+      });
+    });
   });
 });