Merge branch '32894-sensitive-information-in-description-history-frontend' into 'master'

Delete description change history - Frontend

See merge request gitlab-org/gitlab!23568

app/assets/javascripts/notes/constants.js

@@ -18,6 +18,7 @@ export const HISTORY_ONLY_FILTER_VALUE = 2;
 export const DISCUSSION_FILTERS_DEFAULT_VALUE = 0;
 export const DISCUSSION_TAB_LABEL = 'show';
 export const NOTE_UNDERSCORE = 'note_';
+export const TIME_DIFFERENCE_VALUE = 10;
 
 export const NOTEABLE_TYPE_MAPPING = {
   Issue: ISSUE_NOTEABLE_TYPE,

app/assets/javascripts/notes/mixins/description_version_history.js

@@ -3,10 +3,12 @@
 export default {
   computed: {
     canSeeDescriptionVersion() {},
+    canDeleteDescriptionVersion() {},
     shouldShowDescriptionVersion() {},
     descriptionVersionToggleIcon() {},
   },
   methods: {
     toggleDescriptionVersion() {},
+    deleteDescriptionVersion() {},
   },
 };

app/assets/javascripts/notes/stores/actions.js

@@ -491,23 +491,66 @@ export const convertToDiscussion = ({ commit }, noteId) =>
 export const removeConvertedDiscussion = ({ commit }, noteId) =>
   commit(types.REMOVE_CONVERTED_DISCUSSION, noteId);
 
-export const fetchDescriptionVersion = (_, { endpoint, startingVersion }) => {
+export const setCurrentDiscussionId = ({ commit }, discussionId) =>
+  commit(types.SET_CURRENT_DISCUSSION_ID, discussionId);
+
+export const fetchDescriptionVersion = ({ dispatch }, { endpoint, startingVersion }) => {
   let requestUrl = endpoint;
 
   if (startingVersion) {
     requestUrl = mergeUrlParams({ start_version_id: startingVersion }, requestUrl);
   }
+  dispatch('requestDescriptionVersion');
 
   return axios
     .get(requestUrl)
-    .then(res => res.data)
-    .catch(() => {
+    .then(res => {
+      dispatch('receiveDescriptionVersion', res.data);
+    })
+    .catch(error => {
+      dispatch('receiveDescriptionVersionError', error);
       Flash(__('Something went wrong while fetching description changes. Please try again.'));
     });
 };
 
-export const setCurrentDiscussionId = ({ commit }, discussionId) =>
-  commit(types.SET_CURRENT_DISCUSSION_ID, discussionId);
+export const requestDescriptionVersion = ({ commit }) => {
+  commit(types.REQUEST_DESCRIPTION_VERSION);
+};
+export const receiveDescriptionVersion = ({ commit }, descriptionVersion) => {
+  commit(types.RECEIVE_DESCRIPTION_VERSION, descriptionVersion);
+};
+export const receiveDescriptionVersionError = ({ commit }, error) => {
+  commit(types.RECEIVE_DESCRIPTION_VERSION_ERROR, error);
+};
+
+export const softDeleteDescriptionVersion = ({ dispatch }, { endpoint, startingVersion }) => {
+  let requestUrl = endpoint;
+
+  if (startingVersion) {
+    requestUrl = mergeUrlParams({ start_version_id: startingVersion }, requestUrl);
+  }
+  dispatch('requestDeleteDescriptionVersion');
+
+  return axios
+    .delete(requestUrl)
+    .then(() => {
+      dispatch('receiveDeleteDescriptionVersion');
+    })
+    .catch(error => {
+      dispatch('receiveDeleteDescriptionVersionError', error);
+      Flash(__('Something went wrong while deleting description changes. Please try again.'));
+    });
+};
+
+export const requestDeleteDescriptionVersion = ({ commit }) => {
+  commit(types.REQUEST_DELETE_DESCRIPTION_VERSION);
+};
+export const receiveDeleteDescriptionVersion = ({ commit }) => {
+  commit(types.RECEIVE_DELETE_DESCRIPTION_VERSION, __('Deleted'));
+};
+export const receiveDeleteDescriptionVersionError = ({ commit }, error) => {
+  commit(types.RECEIVE_DELETE_DESCRIPTION_VERSION_ERROR, error);
+};
 
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};

app/assets/javascripts/notes/stores/collapse_utils.js

-import { DESCRIPTION_TYPE } from '../constants';
+import { DESCRIPTION_TYPE, TIME_DIFFERENCE_VALUE } from '../constants';
 
 /**
  * Checks the time difference between two notes from their 'created_at' dates
@@ -45,7 +45,11 @@ export const collapseSystemNotes = notes => {
         const timeDifferenceMinutes = getTimeDifferenceMinutes(lastDescriptionSystemNote, note);
 
         // are they less than 10 minutes apart from the same user?
-        if (timeDifferenceMinutes > 10 || note.author.id !== lastDescriptionSystemNote.author.id) {
+        if (
+          timeDifferenceMinutes > TIME_DIFFERENCE_VALUE ||
+          note.author.id !== lastDescriptionSystemNote.author.id ||
+          lastDescriptionSystemNote.description_version_deleted
+        ) {
           // update the previous system note
           lastDescriptionSystemNote = note;
           lastDescriptionSystemNoteIndex = acc.length;

app/assets/javascripts/notes/stores/modules/index.js

@@ -14,6 +14,7 @@ export default () => ({
     isToggleStateButtonLoading: false,
     isNotesFetched: false,
     isLoading: true,
+    isLoadingDescriptionVersion: false,
 
     // holds endpoints and permissions provided through haml
     notesData: {
@@ -27,6 +28,7 @@ export default () => ({
     commentsDisabled: false,
     resolvableDiscussionsCount: 0,
     unresolvedDiscussionsCount: 0,
+    descriptionVersion: null,
   },
   actions,
   getters,

app/assets/javascripts/notes/stores/mutation_types.js

@@ -31,3 +31,11 @@ export const SET_CURRENT_DISCUSSION_ID = 'SET_CURRENT_DISCUSSION_ID';
 export const CLOSE_ISSUE = 'CLOSE_ISSUE';
 export const REOPEN_ISSUE = 'REOPEN_ISSUE';
 export const TOGGLE_STATE_BUTTON_LOADING = 'TOGGLE_STATE_BUTTON_LOADING';
+
+// Description version
+export const REQUEST_DESCRIPTION_VERSION = 'REQUEST_DESCRIPTION_VERSION';
+export const RECEIVE_DESCRIPTION_VERSION = 'RECEIVE_DESCRIPTION_VERSION';
+export const RECEIVE_DESCRIPTION_VERSION_ERROR = 'RECEIVE_DESCRIPTION_VERSION_ERROR';
+export const REQUEST_DELETE_DESCRIPTION_VERSION = 'REQUEST_DELETE_DESCRIPTION_VERSION';
+export const RECEIVE_DELETE_DESCRIPTION_VERSION = 'RECEIVE_DELETE_DESCRIPTION_VERSION';
+export const RECEIVE_DELETE_DESCRIPTION_VERSION_ERROR = 'RECEIVE_DELETE_DESCRIPTION_VERSION_ERROR';

app/assets/javascripts/notes/stores/mutations.js

@@ -284,4 +284,25 @@ export default {
   [types.SET_CURRENT_DISCUSSION_ID](state, discussionId) {
     state.currentDiscussionId = discussionId;
   },
+
+  [types.REQUEST_DESCRIPTION_VERSION](state) {
+    state.isLoadingDescriptionVersion = true;
+  },
+  [types.RECEIVE_DESCRIPTION_VERSION](state, descriptionVersion) {
+    state.isLoadingDescriptionVersion = false;
+    state.descriptionVersion = descriptionVersion;
+  },
+  [types.RECEIVE_DESCRIPTION_VERSION_ERROR](state) {
+    state.isLoadingDescriptionVersion = false;
+  },
+  [types.REQUEST_DELETE_DESCRIPTION_VERSION](state) {
+    state.isLoadingDescriptionVersion = true;
+  },
+  [types.RECEIVE_DELETE_DESCRIPTION_VERSION](state, descriptionVersion) {
+    state.isLoadingDescriptionVersion = false;
+    state.descriptionVersion = descriptionVersion;
+  },
+  [types.RECEIVE_DELETE_DESCRIPTION_VERSION_ERROR](state) {
+    state.isLoadingDescriptionVersion = false;
+  },
 };

app/assets/javascripts/vue_shared/components/notes/system_note.vue

@@ -17,11 +17,12 @@
  *   />
  */
 import $ from 'jquery';
-import { mapGetters, mapActions } from 'vuex';
-import { GlSkeletonLoading } from '@gitlab/ui';
+import { mapGetters, mapActions, mapState } from 'vuex';
+import { GlButton, GlSkeletonLoading, GlTooltipDirective } from '@gitlab/ui';
 import descriptionVersionHistoryMixin from 'ee_else_ce/notes/mixins/description_version_history';
 import noteHeader from '~/notes/components/note_header.vue';
 import Icon from '~/vue_shared/components/icon.vue';
+import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
 import TimelineEntryItem from './timeline_entry_item.vue';
 import { spriteIcon } from '../../../lib/utils/common_utils';
 import initMRPopovers from '~/mr_popover/';
@@ -34,9 +35,13 @@ export default {
     Icon,
     noteHeader,
     TimelineEntryItem,
+    GlButton,
     GlSkeletonLoading,
   },
-  mixins: [descriptionVersionHistoryMixin],
+  directives: {
+    GlTooltip: GlTooltipDirective,
+  },
+  mixins: [descriptionVersionHistoryMixin, glFeatureFlagsMixin()],
   props: {
     note: {
       type: Object,
@@ -50,6 +55,7 @@ export default {
   },
   computed: {
     ...mapGetters(['targetNoteHash']),
+    ...mapState(['descriptionVersion', 'isLoadingDescriptionVersion']),
     noteAnchorId() {
       return `note_${this.note.id}`;
     },
@@ -80,7 +86,7 @@ export default {
     initMRPopovers(this.$el.querySelectorAll('.gfm-merge_request'));
   },
   methods: {
-    ...mapActions(['fetchDescriptionVersion']),
+    ...mapActions(['fetchDescriptionVersion', 'softDeleteDescriptionVersion']),
   },
 };
 </script>
@@ -122,6 +128,16 @@ export default {
             <gl-skeleton-loading />
           </pre>
           <pre v-else class="wrapper mt-2" v-html="descriptionVersion"></pre>
+          <gl-button
+            v-if="canDeleteDescriptionVersion"
+            ref="deleteDescriptionVersionButton"
+            v-gl-tooltip
+            :title="__('Remove description history')"
+            class="btn-transparent delete-description-history"
+            @click="deleteDescriptionVersion"
+          >
+            <icon name="remove" />
+          </gl-button>
         </div>
       </div>
     </div>

app/assets/stylesheets/pages/notes.scss

@@ -311,13 +311,18 @@ $note-form-margin-left: 72px;
       overflow: hidden;
 
       .description-version {
+        position: relative;
+
+        .btn.delete-description-history {
+          position: absolute;
+          top: 18px;
+          right: 0;
+        }
+
         pre {
           max-height: $dropdown-max-height-lg;
           white-space: pre-wrap;
-
-          &.loading-state {
-            height: 94px;
-          }
+          padding-right: 30px;
         }
       }
 

ee/app/assets/javascripts/notes/mixins/description_version_history.js

 export default {
   data() {
     return {
-      isLoadingDescriptionVersion: false,
       isDescriptionVersionExpanded: false,
-      descriptionVersion: '',
     };
   },
   computed: {
     canSeeDescriptionVersion() {
-      return Boolean(this.note.description_diff_path && this.note.description_version_id);
+      return Boolean(
+        this.note.description_diff_path &&
+          this.note.description_version_id &&
+          !this.note.description_version_deleted,
+      );
+    },
+    canDeleteDescriptionVersion() {
+      return this.note.can_delete_description_version;
     },
     shouldShowDescriptionVersion() {
       return this.canSeeDescriptionVersion && this.isDescriptionVersionExpanded;
@@ -25,14 +30,16 @@ export default {
         return false;
       }
 
-      this.isLoadingDescriptionVersion = true;
       const endpoint = this.note.description_diff_path;
       const startingVersion = this.note.start_description_version_id;
 
-      return this.fetchDescriptionVersion({ endpoint, startingVersion }).then(diff => {
-        this.isLoadingDescriptionVersion = false;
-        this.descriptionVersion = diff;
-      });
+      return this.fetchDescriptionVersion({ endpoint, startingVersion });
+    },
+    deleteDescriptionVersion() {
+      const endpoint = this.note.delete_description_version_path;
+      const startingVersion = this.note.start_description_version_id;
+
+      return this.softDeleteDescriptionVersion({ endpoint, startingVersion });
     },
   },
 };

ee/app/models/ee/description_version.rb

@@ -39,6 +39,8 @@ module EE
         issuable_description_versions.where('id BETWEEN ? AND ?', start_id, self.id)
 
       description_versions.update_all(deleted_at: Time.now)
+
+      issuable&.expire_note_etag_cache
     end
 
     def deleted?

ee/changelogs/unreleased/32894-sensitive-information-in-description-history-frontend.yml

+---
+title: Delete description change history - Frontend
+merge_request: 23568
+author:
+type: changed

ee/spec/frontend/vue_shared/components/notes/system_note_spec.js

+import { mount } from '@vue/test-utils';
+import axios from '~/lib/utils/axios_utils';
+import MockAdapter from 'axios-mock-adapter';
+import IssueSystemNote from '~/vue_shared/components/notes/system_note.vue';
+import createStore from '~/notes/stores';
+import waitForPromises from 'helpers/wait_for_promises';
+
+describe('system note component', () => {
+  let wrapper;
+  let props;
+  let mock;
+
+  const diffData = '<span class="idiff">Description</span><span class="idiff addition">Diff</span>';
+
+  function mockFetchDiff() {
+    mock.onGet('/path/to/diff').replyOnce(200, diffData);
+  }
+
+  function mockDeleteDiff() {
+    mock.onDelete('/path/to/diff/1').replyOnce(200);
+  }
+
+  const findBlankBtn = () => wrapper.find('.note-headline-light .btn-blank');
+
+  const findDescriptionVersion = () => wrapper.find('.description-version');
+
+  beforeEach(() => {
+    props = {
+      note: {
+        id: '1424',
+        author: {
+          id: 1,
+          name: 'Root',
+          username: 'root',
+          state: 'active',
+          avatar_url: 'path',
+          path: '/root',
+        },
+        note_html: '<p dir="auto">closed</p>',
+        system_note_icon_name: 'status_closed',
+        created_at: '2017-08-02T10:51:58.559Z',
+        description_version_id: 1,
+        description_diff_path: 'path/to/diff',
+        delete_description_version_path: 'path/to/diff/1',
+        can_delete_description_version: true,
+        description_version_deleted: false,
+      },
+    };
+
+    const store = createStore();
+    store.dispatch('setTargetNoteHash', `note_${props.note.id}`);
+
+    mock = new MockAdapter(axios);
+
+    wrapper = mount(IssueSystemNote, {
+      store,
+      propsData: props,
+      provide: {
+        glFeatures: { saveDescriptionVersions: true, descriptionDiffs: true },
+      },
+    });
+  });
+
+  afterEach(() => {
+    mock.restore();
+    wrapper.destroy();
+  });
+
+  it('should display button to toggle description diff, description version does not display', () => {
+    const button = findBlankBtn();
+    expect(button.exists()).toBe(true);
+    expect(button.text()).toContain('Compare with previous version');
+    expect(findDescriptionVersion().exists()).toBe(false);
+  });
+
+  it('click on button to toggle description diff displays description diff with delete icon button', done => {
+    mockFetchDiff();
+    expect(findDescriptionVersion().exists()).toBe(false);
+
+    const button = findBlankBtn();
+    button.trigger('click');
+    return wrapper.vm
+      .$nextTick()
+      .then(() => waitForPromises())
+      .then(() => {
+        expect(findDescriptionVersion().exists()).toBe(true);
+        expect(findDescriptionVersion().html()).toContain(diffData);
+        expect(
+          wrapper
+            .find('.description-version button.delete-description-history svg.ic-remove')
+            .exists(),
+        ).toBe(true);
+        done();
+      });
+  });
+
+  it('click on delete icon button deletes description diff', done => {
+    mockFetchDiff();
+    mockDeleteDiff();
+    const button = findBlankBtn();
+    button.trigger('click');
+    return wrapper.vm
+      .$nextTick()
+      .then(() => waitForPromises())
+      .then(() => {
+        const deleteButton = wrapper.find({ ref: 'deleteDescriptionVersionButton' });
+        deleteButton.trigger('click');
+      })
+      .then(() => waitForPromises())
+      .then(() => {
+        expect(findDescriptionVersion().text()).toContain('Deleted');
+        done();
+      });
+  });
+});

ee/spec/models/ee/description_version_spec.rb

@@ -53,6 +53,14 @@ describe DescriptionVersion do
         .count
     end
 
+    it 'expires issuable etag cache' do
+      version = epic.description_versions.last
+
+      expect(epic).to receive(:expire_note_etag_cache)
+
+      version.delete!
+    end
+
     context 'when start_id is not present' do
       it 'only soft deletes description_version' do
         version = epic.description_versions.last

locale/gitlab.pot

@@ -15869,6 +15869,9 @@ msgstr ""
 msgid "Remove child epic from an epic"
 msgstr ""
 
+msgid "Remove description history"
+msgstr ""
+
 msgid "Remove due date"
 msgstr ""
 
@@ -17792,6 +17795,9 @@ msgstr ""
 msgid "Something went wrong while closing the %{issuable}. Please try again later"
 msgstr ""
 
+msgid "Something went wrong while deleting description changes. Please try again."
+msgstr ""
+
 msgid "Something went wrong while deleting the image."
 msgstr ""