Skip to content

G
gitlab-ce
Commit c0417474 authored by Jesse Hall's avatar Jesse Hall Committed by Bob Van Landuyt
Browse files
Options

Users can apply multiple suggestions at once.

parent 759bcf35
Show whitespace changes
Inline Side-by-side
Showing with 2070 additions and 395 deletions
app/assets/javascripts/api.js
View file @ c0417474
......@@ -38,6 +38,7 @@ const Api = {
userPostStatusPath: '/api/:version/user/status',
commitPath: '/api/:version/projects/:id/repository/commits',
applySuggestionPath: '/api/:version/suggestions/:id/apply',
applySuggestionBatchPath: '/api/:version/suggestions/batch_apply',
commitPipelinesPath: '/:project_id/commit/:sha/pipelines',
branchSinglePath: '/api/:version/projects/:id/repository/branches/:branch',
createBranchPath: '/api/:version/projects/:id/repository/branches',
......@@ -322,6 +323,12 @@ const Api = {
return axios.put(url);
},
applySuggestionBatch(ids) {
const url = Api.buildUrl(Api.applySuggestionBatchPath);
return axios.put(url, { ids });
},
commitPipelines(projectId, sha) {
const encodedProjectId = projectId
.split('/')
......
app/assets/javascripts/notes/components/note_body.vue
View file @ c0417474
<script>
import { mapActions, mapGetters } from 'vuex';
import { mapActions, mapGetters, mapState } from 'vuex';
import $ from 'jquery';
import '~/behaviors/markdown/render_gfm';
import noteEditedText from './note_edited_text.vue';
......@@ -50,6 +50,9 @@ export default {
return this.getDiscussion(this.note.discussion_id);
},
...mapState({
batchSuggestionsInfo: state => state.notes.batchSuggestionsInfo,
}),
noteBody() {
return this.note.note;
},
......@@ -79,7 +82,12 @@ export default {
}
},
methods: {
...mapActions(['submitSuggestion']),
...mapActions([
'submitSuggestion',
'submitSuggestionBatch',
'addSuggestionInfoToBatch',
'removeSuggestionInfoFromBatch',
]),
renderGFM() {
$(this.$refs['note-body']).renderGFM();
},
......@@ -96,6 +104,17 @@ export default {
callback,
);
},
applySuggestionBatch({ flashContainer }) {
return this.submitSuggestionBatch({ flashContainer });
},
addSuggestionToBatch(suggestionId) {
const { discussion_id: discussionId, id: noteId } = this.note;
this.addSuggestionInfoToBatch({ suggestionId, discussionId, noteId });
},
removeSuggestionFromBatch(suggestionId) {
this.removeSuggestionInfoFromBatch(suggestionId);
},
},
};
</script>
......@@ -105,10 +124,14 @@ export default {
<suggestions
v-if="hasSuggestion && !isEditing"
:suggestions="note.suggestions"
:batch-suggestions-info="batchSuggestionsInfo"
:note-html="note.note_html"
:line-type="lineType"
:help-page-path="helpPagePath"
@apply="applySuggestion"
@applyBatch="applySuggestionBatch"
@addToBatch="addSuggestionToBatch"
@removeFromBatch="removeSuggestionFromBatch"
/>
<div v-else class="note-text md" v-html="note.note_html"></div>
<note-form
......
app/assets/javascripts/notes/stores/actions.js
View file @ c0417474
......@@ -524,12 +524,55 @@ export const submitSuggestion = (
const defaultMessage = __(
'Something went wrong while applying the suggestion. Please try again.',
);
const flashMessage = err.response.data ? `${err.response.data.message}.` : defaultMessage;
const errorMessage = err.response.data?.message;
const flashMessage = errorMessage || defaultMessage;
Flash(__(flashMessage), 'alert', flashContainer);
});
};
export const submitSuggestionBatch = ({ commit, dispatch, state }, { flashContainer }) => {
const suggestionIds = state.batchSuggestionsInfo.map(({ suggestionId }) => suggestionId);
const applyAllSuggestions = () =>
state.batchSuggestionsInfo.map(suggestionInfo =>
commit(types.APPLY_SUGGESTION, suggestionInfo),
);
const resolveAllDiscussions = () =>
state.batchSuggestionsInfo.map(suggestionInfo => {
const { discussionId } = suggestionInfo;
return dispatch('resolveDiscussion', { discussionId }).catch(() => {});
});
commit(types.SET_APPLYING_BATCH_STATE, true);
return Api.applySuggestionBatch(suggestionIds)
.then(() => Promise.all(applyAllSuggestions()))
.then(() => Promise.all(resolveAllDiscussions()))
.then(() => commit(types.CLEAR_SUGGESTION_BATCH))
.catch(err => {
const defaultMessage = __(
'Something went wrong while applying the batch of suggestions. Please try again.',
);
const errorMessage = err.response.data?.message;
const flashMessage = errorMessage || defaultMessage;
Flash(__(flashMessage), 'alert', flashContainer);
})
.finally(() => commit(types.SET_APPLYING_BATCH_STATE, false));
};
export const addSuggestionInfoToBatch = ({ commit }, { suggestionId, noteId, discussionId }) =>
commit(types.ADD_SUGGESTION_TO_BATCH, { suggestionId, noteId, discussionId });
export const removeSuggestionInfoFromBatch = ({ commit }, suggestionId) =>
commit(types.REMOVE_SUGGESTION_FROM_BATCH, suggestionId);
export const convertToDiscussion = ({ commit }, noteId) =>
commit(types.CONVERT_TO_DISCUSSION, noteId);
......
app/assets/javascripts/notes/stores/modules/index.js
View file @ c0417474
......@@ -11,6 +11,7 @@ export default () => ({
targetNoteHash: null,
lastFetchedAt: null,
currentDiscussionId: null,
batchSuggestionsInfo: [],
// View layer
isToggleStateButtonLoading: false,
......
app/assets/javascripts/notes/stores/mutation_types.js
View file @ c0417474
......@@ -17,6 +17,10 @@ export const SET_NOTES_FETCHED_STATE = 'SET_NOTES_FETCHED_STATE';
export const SET_NOTES_LOADING_STATE = 'SET_NOTES_LOADING_STATE';
export const DISABLE_COMMENTS = 'DISABLE_COMMENTS';
export const APPLY_SUGGESTION = 'APPLY_SUGGESTION';
export const SET_APPLYING_BATCH_STATE = 'SET_APPLYING_BATCH_STATE';
export const ADD_SUGGESTION_TO_BATCH = 'ADD_SUGGESTION_TO_BATCH';
export const REMOVE_SUGGESTION_FROM_BATCH = 'REMOVE_SUGGESTION_FROM_BATCH';
export const CLEAR_SUGGESTION_BATCH = 'CLEAR_SUGGESTION_BATCH';
export const CONVERT_TO_DISCUSSION = 'CONVERT_TO_DISCUSSION';
export const REMOVE_CONVERTED_DISCUSSION = 'REMOVE_CONVERTED_DISCUSSION';
......
app/assets/javascripts/notes/stores/mutations.js
View file @ c0417474
......@@ -225,6 +225,39 @@ export default {
}));
},
[types.SET_APPLYING_BATCH_STATE](state, isApplyingBatch) {
state.batchSuggestionsInfo.forEach(suggestionInfo => {
const { discussionId, noteId, suggestionId } = suggestionInfo;
const noteObj = utils.findNoteObjectById(state.discussions, discussionId);
const comment = utils.findNoteObjectById(noteObj.notes, noteId);
comment.suggestions = comment.suggestions.map(suggestion => ({
...suggestion,
is_applying_batch: suggestion.id === suggestionId && isApplyingBatch,
}));
});
},
[types.ADD_SUGGESTION_TO_BATCH](state, { noteId, discussionId, suggestionId }) {
state.batchSuggestionsInfo.push({
suggestionId,
noteId,
discussionId,
});
},
[types.REMOVE_SUGGESTION_FROM_BATCH](state, id) {
const index = state.batchSuggestionsInfo.findIndex(({ suggestionId }) => suggestionId === id);
if (index !== -1) {
state.batchSuggestionsInfo.splice(index, 1);
}
},
[types.CLEAR_SUGGESTION_BATCH](state) {
state.batchSuggestionsInfo.splice(0, state.batchSuggestionsInfo.length);
},
[types.UPDATE_DISCUSSION](state, noteData) {
const note = noteData;
const selectedDiscussion = state.discussions.find(disc => disc.id === note.id);
......
app/assets/javascripts/vue_shared/components/markdown/suggestion_diff.vue
View file @ c0417474
......@@ -13,6 +13,11 @@ export default {
type: Object,
required: true,
},
batchSuggestionsInfo: {
type: Array,
required: false,
default: () => [],
},
disabled: {
type: Boolean,
required: false,
......@@ -24,6 +29,14 @@ export default {
},
},
computed: {
batchSuggestionsCount() {
return this.batchSuggestionsInfo.length;
},
isBatched() {
return Boolean(
this.batchSuggestionsInfo.find(({ suggestionId }) => suggestionId === this.suggestion.id),
);
},
lines() {
return selectDiffLines(this.suggestion.diff_lines);
},
......@@ -32,6 +45,15 @@ export default {
applySuggestion(callback) {
this.$emit('apply', { suggestionId: this.suggestion.id, callback });
},
applySuggestionBatch() {
this.$emit('applyBatch');
},
addSuggestionToBatch() {
this.$emit('addToBatch', this.suggestion.id);
},
removeSuggestionFromBatch() {
this.$emit('removeFromBatch', this.suggestion.id);
},
},
};
</script>
......@@ -42,8 +64,14 @@ export default {
class="qa-suggestion-diff-header js-suggestion-diff-header"
:can-apply="suggestion.appliable && suggestion.current_user.can_apply && !disabled"
:is-applied="suggestion.applied"
:is-batched="isBatched"
:is-applying-batch="suggestion.is_applying_batch"
:batch-suggestions-count="batchSuggestionsCount"
:help-page-path="helpPagePath"
@apply="applySuggestion"
@applyBatch="applySuggestionBatch"
@addToBatch="addSuggestionToBatch"
@removeFromBatch="removeSuggestionFromBatch"
/>
<table class="mb-3 md-suggestion-diff js-syntax-highlight code">
<tbody>
......
app/assets/javascripts/vue_shared/components/markdown/suggestion_diff_header.vue
View file @ c0417474
<script>
import { GlDeprecatedButton, GlLoadingIcon, GlTooltipDirective } from '@gitlab/ui';
import Icon from '~/vue_shared/components/icon.vue';
import { __ } from '~/locale';
export default {
components: { Icon, GlDeprecatedButton, GlLoadingIcon },
directives: { 'gl-tooltip': GlTooltipDirective },
props: {
batchSuggestionsCount: {
type: Number,
required: false,
default: 0,
},
canApply: {
type: Boolean,
required: false,
......@@ -16,6 +22,16 @@ export default {
required: true,
default: false,
},
isBatched: {
type: Boolean,
required: false,
default: false,
},
isApplyingBatch: {
type: Boolean,
required: false,
default: false,
},
helpPagePath: {
type: String,
required: true,
......@@ -23,17 +39,38 @@ export default {
},
data() {
return {
isApplying: false,
isApplyingSingle: false,
};
},
computed: {
isApplying() {
return this.isApplyingSingle || this.isApplyingBatch;
},
applyingSuggestionsMessage() {
if (this.isApplyingSingle || this.batchSuggestionsCount < 2) {
return __('Applying suggestion...');
}
return __('Applying suggestions...');
},
},
methods: {
applySuggestion() {
if (!this.canApply) return;
this.isApplying = true;
this.isApplyingSingle = true;
this.$emit('apply', this.applySuggestionCallback);
},
applySuggestionCallback() {
this.isApplying = false;
this.isApplyingSingle = false;
},
applySuggestionBatch() {
if (!this.canApply) return;
this.$emit('applyBatch');
},
addSuggestionToBatch() {
this.$emit('addToBatch');
},
removeSuggestionFromBatch() {
this.$emit('removeFromBatch');
},
},
};
......@@ -47,15 +84,43 @@ export default {
<icon name="question-o" css-classes="link-highlight" />
</a>
</div>
<span v-if="isApplied" class="badge badge-success">{{ __('Applied') }}</span>
<div v-if="isApplying" class="d-flex align-items-center text-secondary">
<div v-if="isApplied" class="badge badge-success">{{ __('Applied') }}</div>
<div v-else-if="isApplying" class="d-flex align-items-center text-secondary">
<gl-loading-icon class="d-flex-center mr-2" />
<span>{{ __('Applying suggestion') }}</span>
<span>{{ applyingSuggestionsMessage }}</span>
</div>
<div v-else-if="canApply && isBatched" class="d-flex align-items-center">
<gl-deprecated-button
class="btn-inverted js-remove-from-batch-btn btn-grouped"
:disabled="isApplying"
@click="removeSuggestionFromBatch"
>
{{ __('Remove from batch') }}
</gl-deprecated-button>
<gl-deprecated-button
v-gl-tooltip.viewport="__('This also resolves all related threads')"
class="btn-inverted js-apply-batch-btn btn-grouped"
:disabled="isApplying"
variant="success"
@click="applySuggestionBatch"
>
{{ __('Apply suggestions') }}
<span class="badge badge-pill badge-pill-success">
{{ batchSuggestionsCount }}
</span>
</gl-deprecated-button>
</div>
<div v-else-if="canApply" class="d-flex align-items-center">
<gl-deprecated-button
class="btn-inverted js-add-to-batch-btn btn-grouped"
:disabled="isApplying"
@click="addSuggestionToBatch"
>
{{ __('Add suggestion to batch') }}
</gl-deprecated-button>
<gl-deprecated-button
v-else-if="canApply"
v-gl-tooltip.viewport="__('This also resolves the discussion')"
class="btn-inverted js-apply-btn"
v-gl-tooltip.viewport="__('This also resolves the thread')"
class="btn-inverted js-apply-btn btn-grouped"
:disabled="isApplying"
variant="success"
@click="applySuggestion"
......@@ -63,4 +128,5 @@ export default {
{{ __('Apply suggestion') }}
</gl-deprecated-button>
</div>
</div>
</template>
app/assets/javascripts/vue_shared/components/markdown/suggestions.vue
View file @ c0417474
......@@ -16,6 +16,11 @@ export default {
required: false,
default: () => [],
},
batchSuggestionsInfo: {
type: Array,
required: false,
default: () => [],
},
noteHtml: {
type: String,
required: true,
......@@ -68,18 +73,30 @@ export default {
this.isRendered = true;
},
generateDiff(suggestionIndex) {
const { suggestions, disabled, helpPagePath } = this;
const { suggestions, disabled, batchSuggestionsInfo, helpPagePath } = this;
const suggestion =
suggestions && suggestions[suggestionIndex] ? suggestions[suggestionIndex] : {};
const SuggestionDiffComponent = Vue.extend(SuggestionDiff);
const suggestionDiff = new SuggestionDiffComponent({
propsData: { disabled, suggestion, helpPagePath },
propsData: { disabled, suggestion, batchSuggestionsInfo, helpPagePath },
});
suggestionDiff.$on('apply', ({ suggestionId, callback }) => {
this.$emit('apply', { suggestionId, callback, flashContainer: this.$el });
});
suggestionDiff.$on('applyBatch', () => {
this.$emit('applyBatch', { flashContainer: this.$el });
});
suggestionDiff.$on('addToBatch', suggestionId => {
this.$emit('addToBatch', suggestionId);
});
suggestionDiff.$on('removeFromBatch', suggestionId => {
this.$emit('removeFromBatch', suggestionId);
});
return suggestionDiff;
},
reset() {
......
app/assets/stylesheets/framework/badges.scss
View file @ c0417474
......@@ -3,4 +3,12 @@
background-color: $badge-bg;
color: $gray-800;
vertical-align: baseline;
// Do not use this!
// This is a temporary workaround until the new GlBadge component
// is available: https://gitlab.com/gitlab-org/gitlab-ui/-/issues/481
&.badge-pill-success {
background-color: rgba($green-500, 0.2);
color: $green;
}
}
app/services/suggestions/apply_service.rb
View file @ c0417474
......@@ -2,109 +2,49 @@
module Suggestions
class ApplyService < ::BaseService
DEFAULT_SUGGESTION_COMMIT_MESSAGE = 'Apply suggestion to %{file_path}'
PLACEHOLDERS = {
'project_path' => ->(suggestion, user) { suggestion.project.path },
'project_name' => ->(suggestion, user) { suggestion.project.name },
'file_path' => ->(suggestion, user) { suggestion.file_path },
'branch_name' => ->(suggestion, user) { suggestion.branch },
'username' => ->(suggestion, user) { user.username },
'user_full_name' => ->(suggestion, user) { user.name }
}.freeze
# This regex is built dynamically using the keys from the PLACEHOLDER struct.
# So, we can easily add new placeholder just by modifying the PLACEHOLDER hash.
# This regex will build the new PLACEHOLDER_REGEX with the new information
PLACEHOLDERS_REGEX = Regexp.union(PLACEHOLDERS.keys.map { |key| Regexp.new(Regexp.escape(key)) }).freeze
attr_reader :current_user
def initialize(current_user)
def initialize(current_user, *suggestions)
@current_user = current_user
@suggestion_set = Gitlab::Suggestions::SuggestionSet.new(suggestions)
end
def execute(suggestion)
unless suggestion.appliable?(cached: false)
return error('Suggestion is not appliable')
def execute
if suggestion_set.valid?
result
else
error(suggestion_set.error_message)
end
unless latest_source_head?(suggestion)
return error('The file has been changed')
end
diff_file = suggestion.diff_file
unless diff_file
return error('The file was not found')
end
private
params = file_update_params(suggestion, diff_file)
result = ::Files::UpdateService.new(suggestion.project, current_user, params).execute
attr_reader :current_user, :suggestion_set
if result[:status] == :success
suggestion.update(commit_id: result[:result], applied: true)
def result
multi_service.execute.tap do |result|
update_suggestions(result)
end
result
rescue Files::UpdateService::FileChangedError
error('The file has been changed')
end
private
def update_suggestions(result)
return unless result[:status] == :success
# Checks whether the latest source branch HEAD matches with
# the position HEAD we're using to update the file content. Since
# the persisted HEAD is updated async (for MergeRequest),
# it's more consistent to fetch this data directly from the
# repository.
def latest_source_head?(suggestion)
suggestion.position.head_sha == suggestion.noteable.source_branch_sha
Suggestion.id_in(suggestion_set.suggestions)
.update_all(commit_id: result[:result], applied: true)
end
def file_update_params(suggestion, diff_file)
blob = diff_file.new_blob
project = suggestion.project
file_path = suggestion.file_path
branch_name = suggestion.branch
file_content = new_file_content(suggestion, blob)
commit_message = processed_suggestion_commit_message(suggestion)
file_last_commit =
Gitlab::Git::Commit.last_for_path(project.repository,
blob.commit_id,
blob.path)
{
file_path: file_path,
branch_name: branch_name,
start_branch: branch_name,
def multi_service
params = {
commit_message: commit_message,
file_content: file_content,
last_commit_sha: file_last_commit&.id
branch_name: suggestion_set.branch,
start_branch: suggestion_set.branch,
actions: suggestion_set.actions
}
end
def new_file_content(suggestion, blob)
range = suggestion.from_line_index..suggestion.to_line_index
blob.load_all_data!
content = blob.data.lines
content[range] = suggestion.to_content
content.join
end
def suggestion_commit_message(project)
project.suggestion_commit_message.presence || DEFAULT_SUGGESTION_COMMIT_MESSAGE
::Files::MultiService.new(suggestion_set.project, current_user, params)
end
def processed_suggestion_commit_message(suggestion)
message = suggestion_commit_message(suggestion.project)
Gitlab::StringPlaceholderReplacer.replace_string_placeholders(message, PLACEHOLDERS_REGEX) do |key|
PLACEHOLDERS[key].call(suggestion, current_user)
end
def commit_message
Gitlab::Suggestions::CommitMessage.new(current_user, suggestion_set).message
end
end
end
app/views/projects/_merge_request_merge_suggestions_settings.html.haml
View file @ c0417474
......@@ -9,9 +9,9 @@
anchor: 'configure-the-commit-message-for-applied-suggestions'),
target: '_blank'
.mb-2
= form.text_field :suggestion_commit_message, class: 'form-control mb-2', placeholder: Suggestions::ApplyService::DEFAULT_SUGGESTION_COMMIT_MESSAGE
= form.text_field :suggestion_commit_message, class: 'form-control mb-2', placeholder: Gitlab::Suggestions::CommitMessage::DEFAULT_SUGGESTION_COMMIT_MESSAGE
%p.form-text.text-muted
= s_('ProjectSettings|The variables GitLab supports:')
- Suggestions::ApplyService::PLACEHOLDERS.keys.each do |placeholder|
- Gitlab::Suggestions::CommitMessage::PLACEHOLDERS.keys.each do |placeholder|
%code
= "%{#{placeholder}}".html_safe
changelogs/unreleased/25486-batch-suggestions.yml 0 → 100644
View file @ c0417474
---
title: User can apply multiple suggestions at the same time.
merge_request: 22439
author: Jesse Hall
type: added
doc/user/discussions/index.md
View file @ c0417474
......@@ -459,32 +459,60 @@ instead of the usual three.
> [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/13086) in GitLab 12.7.
GitLab uses `Apply suggestion to %{file_path}` by default as commit messages
when applying Suggestions. This commit message can be customized to
follow any guidelines you might have. To do so, expand the **Merge requests**
GitLab uses a default commit message
when applying Suggestions: `Apply %{suggestions_count} suggestion(s) to %{files_count} file(s)`
For example, consider that a user applied 3 suggestions to 2 different files, the default commit message will be: **Apply 3 suggestion(s) to 2 file(s)**
These commit messages can be customized to follow any guidelines you might have. To do so, expand the **Merge requests**
tab within your project's **General** settings and change the
**Merge suggestions** text:
![Custom commit message for applied Suggestions](img/suggestions_custom_commit_messages_v12_7.png)
![Custom commit message for applied Suggestions](img/suggestions_custom_commit_messages_v13_1.jpg)
You can also use following variables besides static text:
| Variable | Description | Output example |
|---|---|---|
| `%{branch_name}` | The name of the branch the Suggestion(s) was(were) applied to. | `my-feature-branch` |
| `%{files_count}` | The number of file(s) to which Suggestion(s) was(were) applied.| **2** |
| `%{file_paths}` | The path(s) of the file(s) Suggestion(s) was(were) applied to. Paths are separated by commas.| `docs/index.md, docs/about.md` |
| `%{project_path}` | The project path. | `my-group/my-project` |
| `%{project_name}` | The human-readable name of the project. | **My Project** |
| `%{file_path}` | The path of the file the Suggestion is applied to. | `docs/index.md` |
| `%{branch_name}` | The name of the branch the Suggestion is applied on. | `my-feature-branch` |
| `%{username}` | The username of the user applying the Suggestion. | `user_1` |
| `%{user_full_name}` | The full name of the user applying the Suggestion. | **User 1** |
| `%{suggestions_count}` | The number of Suggestions applied.| **3** |
| `%{username}` | The username of the user applying Suggestion(s). | `user_1` |
| `%{user_full_name}` | The full name of the user applying Suggestion(s). | **User 1** |
For example, to customize the commit message to output
**Addresses user_1's review**, set the custom text to
`Addresses %{username}'s review`.
NOTE: **Note:**
Custom commit messages for each applied Suggestion will be
introduced by [#25381](https://gitlab.com/gitlab-org/gitlab/-/issues/25381).
Custom commit messages for each applied Suggestion (and for batch Suggestions) will be
introduced by [#25381](https://gitlab.com/gitlab-org/gitlab/issues/25381).
### Batch Suggestions
> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/25486) in GitLab 13.1.
You can apply multiple suggestions at once to reduce the number of commits added
to your branch to address your reviewers' requests.
1. To start a batch of suggestions that will be applied with a single commit, click **Add suggestion to batch**:
![A code change suggestion displayed, with the button to add the suggestion to a batch highlighted.](img/add_first_suggestion_to_batch_v13_1.jpg "Add a suggestion to a batch")
1. Add as many additional suggestions to the batch as you wish:
![A code change suggestion displayed, with the button to add an additional suggestion to a batch highlighted.](img/add_another_suggestion_to_batch_v13_1.jpg "Add another suggestion to a batch")
1. To remove suggestions, click **Remove from batch**:
![A code change suggestion displayed, with the button to remove that suggestion from its batch highlighted.](img/remove_suggestion_from_batch_v13_1.jpg "Remove a suggestion from a batch")
1. Having added all the suggestions to your liking, when ready, click **Apply suggestions**:
![A code change suggestion displayed, with the button to apply the batch of suggestions highlighted.](img/apply_batch_of_suggestions_v13_1.jpg "Apply a batch of suggestions")
## Start a thread by replying to a standard comment
......
lib/api/suggestions.rb
View file @ c0417474
......@@ -14,18 +14,51 @@ module API
put ':id/apply' do
suggestion = Suggestion.find_by_id(params[:id])
not_found! unless suggestion
authorize! :apply_suggestion, suggestion
if suggestion
apply_suggestions(suggestion, current_user)
else
render_api_error!(_('Suggestion is not applicable as the suggestion was not found.'), :not_found)
end
end
desc 'Apply multiple suggestion patches in the Merge Request where they were created' do
success Entities::Suggestion
end
params do
requires :ids, type: Array[String], desc: "An array of suggestion ID's"
end
put 'batch_apply' do
ids = params[:ids]
suggestions = Suggestion.id_in(ids)
result = ::Suggestions::ApplyService.new(current_user).execute(suggestion)
if suggestions.size == ids.length
apply_suggestions(suggestions, current_user)
else
render_api_error!(_('Suggestions are not applicable as one or more suggestions were not found.'), :not_found)
end
end
end
helpers do
def apply_suggestions(suggestions, current_user)
authorize_suggestions(*suggestions)
result = ::Suggestions::ApplyService.new(current_user, *suggestions).execute
if result[:status] == :success
present suggestion, with: Entities::Suggestion, current_user: current_user
present suggestions, with: Entities::Suggestion, current_user: current_user
else
http_status = result[:http_status] || 400
http_status = result[:http_status] || :bad_request
render_api_error!(result[:message], http_status)
end
end
def authorize_suggestions(*suggestions)
suggestions.each do |suggestion|
authorize! :apply_suggestion, suggestion
end
end
end
end
end
lib/gitlab/suggestions/commit_message.rb 0 → 100644
View file @ c0417474
# frozen_string_literal: true
module Gitlab
module Suggestions
class CommitMessage
DEFAULT_SUGGESTION_COMMIT_MESSAGE =
'Apply %{suggestions_count} suggestion(s) to %{files_count} file(s)'
def initialize(user, suggestion_set)
@user = user
@suggestion_set = suggestion_set
end
def message
project = suggestion_set.project
user_defined_message = project.suggestion_commit_message.presence
message = user_defined_message || DEFAULT_SUGGESTION_COMMIT_MESSAGE
Gitlab::StringPlaceholderReplacer
.replace_string_placeholders(message, PLACEHOLDERS_REGEX) do |key|
PLACEHOLDERS[key].call(user, suggestion_set)
end
end
def self.format_paths(paths)
paths.sort.join(', ')
end
private_class_method :format_paths
private
attr_reader :user, :suggestion_set
PLACEHOLDERS = {
'branch_name' => ->(user, suggestion_set) { suggestion_set.branch },
'files_count' => ->(user, suggestion_set) { suggestion_set.file_paths.length },
'file_paths' => ->(user, suggestion_set) { format_paths(suggestion_set.file_paths) },
'project_name' => ->(user, suggestion_set) { suggestion_set.project.name },
'project_path' => ->(user, suggestion_set) { suggestion_set.project.path },
'user_full_name' => ->(user, suggestion_set) { user.name },
'username' => ->(user, suggestion_set) { user.username },
'suggestions_count' => ->(user, suggestion_set) { suggestion_set.suggestions.size }
}.freeze
# This regex is built dynamically using the keys from the PLACEHOLDER struct.
# So, we can easily add new placeholder just by modifying the PLACEHOLDER hash.
# This regex will build the new PLACEHOLDER_REGEX with the new information
PLACEHOLDERS_REGEX = Regexp.union(PLACEHOLDERS.keys.map do |key|
Regexp.new(Regexp.escape(key))
end).freeze
end
end
end
lib/gitlab/suggestions/file_suggestion.rb 0 → 100644
View file @ c0417474
# frozen_string_literal: true
module Gitlab
module Suggestions
class FileSuggestion
include Gitlab::Utils::StrongMemoize
SuggestionForDifferentFileError = Class.new(StandardError)
def initialize
@suggestions = []
end
def add_suggestion(new_suggestion)
if for_different_file?(new_suggestion)
raise SuggestionForDifferentFileError,
'Only add suggestions for the same file.'
end
suggestions << new_suggestion
end
def line_conflict?
strong_memoize(:line_conflict) do
_line_conflict?
end
end
def new_content
@new_content ||= _new_content
end
def file_path
@file_path ||= _file_path
end
private
attr_accessor :suggestions
def blob
first_suggestion&.diff_file&.new_blob
end
def blob_data_lines
blob.load_all_data!
blob.data.lines
end
def current_content
@current_content ||= blob.nil? ? [''] : blob_data_lines
end
def _new_content
current_content.tap do |content|
suggestions.each do |suggestion|
range = line_range(suggestion)
content[range] = suggestion.to_content
end
end.join
end
def line_range(suggestion)
suggestion.from_line_index..suggestion.to_line_index
end
def for_different_file?(suggestion)
file_path && file_path != suggestion_file_path(suggestion)
end
def suggestion_file_path(suggestion)
suggestion&.diff_file&.file_path
end
def first_suggestion
suggestions.first
end
def _file_path
suggestion_file_path(first_suggestion)
end
def _line_conflict?
has_conflict = false
suggestions.each_with_object([]) do |suggestion, ranges|
range_in_test = line_range(suggestion)
if has_range_conflict?(range_in_test, ranges)
has_conflict = true
break
end
ranges << range_in_test
end
has_conflict
end
def has_range_conflict?(range_in_test, ranges)
ranges.any? do |range|
range.overlaps?(range_in_test)
end
end
end
end
end
lib/gitlab/suggestions/suggestion_set.rb 0 → 100644
View file @ c0417474
# frozen_string_literal: true
module Gitlab
module Suggestions
class SuggestionSet
attr_reader :suggestions
def initialize(suggestions)
@suggestions = suggestions
end
def project
first_suggestion.project
end
def branch
first_suggestion.branch
end
def valid?
error_message.nil?
end
def error_message
@error_message ||= _error_message
end
def actions
@actions ||= suggestions_per_file.map do |file_path, file_suggestion|
{
action: 'update',
file_path: file_path,
content: file_suggestion.new_content
}
end
end
def file_paths
@file_paths ||= suggestions.map(&:file_path).uniq
end
private
def first_suggestion
suggestions.first
end
def suggestions_per_file
@suggestions_per_file ||= _suggestions_per_file
end
def _suggestions_per_file
suggestions.each_with_object({}) do |suggestion, result|
file_path = suggestion.diff_file.file_path
file_suggestion = result[file_path] ||= FileSuggestion.new
file_suggestion.add_suggestion(suggestion)
end
end
def file_suggestions
suggestions_per_file.values
end
def first_file_suggestion
file_suggestions.first
end
def _error_message
suggestions.each do |suggestion|
message = error_for_suggestion(suggestion)
return message if message
end
has_line_conflict = file_suggestions.any? do |file_suggestion|
file_suggestion.line_conflict?
end
if has_line_conflict
return _('Suggestions are not applicable as their lines cannot overlap.')
end
nil
end
def error_for_suggestion(suggestion)
unless suggestion.diff_file
return _('A file was not found.')
end
unless on_same_branch?(suggestion)
return _('Suggestions must all be on the same branch.')
end
unless suggestion.appliable?(cached: false)
return _('A suggestion is not applicable.')
end
unless latest_source_head?(suggestion)
return _('A file has been changed.')
end
nil
end
def on_same_branch?(suggestion)
branch == suggestion.branch
end
# Checks whether the latest source branch HEAD matches with
# the position HEAD we're using to update the file content. Since
# the persisted HEAD is updated async (for MergeRequest),
# it's more consistent to fetch this data directly from the
# repository.
def latest_source_head?(suggestion)
suggestion.position.head_sha == suggestion.noteable.source_branch_sha
end
end
end
end
locale/gitlab.pot
View file @ c0417474
......@@ -941,6 +941,12 @@ msgstr ""
msgid "A deleted user"
msgstr ""
msgid "A file has been changed."
msgstr ""
msgid "A file was not found."
msgstr ""
msgid "A file with '%{file_name}' already exists in %{branch} branch"
msgstr ""
......@@ -1007,6 +1013,9 @@ msgstr ""
msgid "A subscription will trigger a new pipeline on the default branch of this project when a pipeline successfully completes for a new tag on the %{default_branch_docs} of the subscribed project."
msgstr ""
msgid "A suggestion is not applicable."
msgstr ""
msgid "A terraform report was generated in your pipelines."
msgstr ""
......@@ -1366,6 +1375,9 @@ msgstr ""
msgid "Add strikethrough text"
msgstr ""
msgid "Add suggestion to batch"
msgstr ""
msgid "Add system hook"
msgstr ""
......@@ -2568,6 +2580,9 @@ msgstr ""
msgid "Apply suggestion"
msgstr ""
msgid "Apply suggestions"
msgstr ""
msgid "Apply template"
msgstr ""
......@@ -2586,7 +2601,10 @@ msgstr ""
msgid "Applying multiple commands"
msgstr ""
msgid "Applying suggestion"
msgid "Applying suggestion..."
msgstr ""
msgid "Applying suggestions..."
msgstr ""
msgid "Approval rules"
......@@ -18265,6 +18283,9 @@ msgstr ""
msgid "Remove fork relationship"
msgstr ""
msgid "Remove from batch"
msgstr ""
msgid "Remove from board"
msgstr ""
......@@ -20502,6 +20523,9 @@ msgstr ""
msgid "Something went wrong while adding your award. Please try again."
msgstr ""
msgid "Something went wrong while applying the batch of suggestions. Please try again."
msgstr ""
msgid "Something went wrong while applying the suggestion. Please try again."
msgstr ""
......@@ -21459,6 +21483,18 @@ msgstr ""
msgid "SuggestedColors|Very pale orange"
msgstr ""
msgid "Suggestion is not applicable as the suggestion was not found."
msgstr ""
msgid "Suggestions are not applicable as one or more suggestions were not found."
msgstr ""
msgid "Suggestions are not applicable as their lines cannot overlap."
msgstr ""
msgid "Suggestions must all be on the same branch."
msgstr ""
msgid "Suggestions:"
msgstr ""
......@@ -22428,7 +22464,10 @@ msgstr ""
msgid "This action can lead to data loss. To prevent accidental actions we ask you to confirm your intention."
msgstr ""
msgid "This also resolves the discussion"
msgid "This also resolves all related threads"
msgstr ""
msgid "This also resolves the thread"
msgstr ""
msgid "This application was created by %{link_to_owner}."
......
spec/features/merge_request/user_suggests_changes_on_diff_spec.rb
View file @ c0417474
......@@ -93,6 +93,100 @@ describe 'User comments on a diff', :js do
end
end
context 'applying suggestions in batches' do
def hash(path)
diff_file = merge_request.diffs(paths: [path]).diff_files.first
Digest::SHA1.hexdigest(diff_file.file_path)
end
file1 = 'files/ruby/popen.rb'
file2 = 'files/ruby/regex.rb'
let(:files) do
[
{
hash: hash(file1),
line_code: "#{hash(file1)}_12_12"
},
{
hash: hash(file2),
line_code: "#{hash(file2)}_21_21"
}
]
end
it 'can add and remove suggestions from a batch' do
files.each_with_index do |file, index|
page.within("[id='#{file[:hash]}']") do
find("button[title='Show full file']").click
wait_for_requests
click_diff_line(find("[id='#{file[:line_code]}']"))
page.within('.js-discussion-note-form') do
fill_in('note_note', with: "```suggestion\n# change to a comment\n```")
click_button('Add comment now')
wait_for_requests
end
end
page.within("[id='#{file[:hash]}']") do
expect(page).not_to have_content('Applied')
click_button('Add suggestion to batch')
wait_for_requests
expect(page).to have_content('Remove from batch')
expect(page).to have_content("Apply suggestions #{index + 1}")
end
end
page.within("[id='#{files[0][:hash]}']") do
click_button('Remove from batch')
wait_for_requests
expect(page).to have_content('Apply suggestion')
expect(page).to have_content('Add suggestion to batch')
end
page.within("[id='#{files[1][:hash]}']") do
expect(page).to have_content('Remove from batch')
expect(page).to have_content('Apply suggestions 1')
end
end
it 'can apply multiple suggestions as a batch' do
files.each_with_index do |file, index|
page.within("[id='#{file[:hash]}']") do
find("button[title='Show full file']").click
wait_for_requests
click_diff_line(find("[id='#{file[:line_code]}']"))
page.within('.js-discussion-note-form') do
fill_in('note_note', with: "```suggestion\n# change to a comment\n```")
click_button('Add comment now')
wait_for_requests
end
end
page.within("[id='#{file[:hash]}']") do
click_button('Add suggestion to batch')
wait_for_requests
end
end
expect(page).not_to have_content('Applied')
page.within("[id='#{files[0][:hash]}']") do
click_button('Apply suggestions 2')
wait_for_requests
end
expect(page).to have_content('Applied').twice
end
end
context 'multiple suggestions in expanded lines' do
# https://gitlab.com/gitlab-org/gitlab/issues/38277
it 'suggestions are appliable', :quarantine do
......
spec/frontend/notes/mock_data.js
View file @ c0417474
......@@ -1254,3 +1254,16 @@ export const discussionFiltersMock = [
value: 2,
},
];
export const batchSuggestionsInfoMock = [
{
suggestionId: 'a123',
noteId: 'b456',
discussionId: 'c789',
},
{
suggestionId: 'a001',
noteId: 'b002',
discussionId: 'c003',
},
];
spec/frontend/notes/stores/actions_spec.js
View file @ c0417474
......@@ -15,6 +15,7 @@ import {
userDataMock,
noteableDataMock,
individualNote,
batchSuggestionsInfoMock,
} from '../mock_data';
import axios from '~/lib/utils/axios_utils';
......@@ -890,7 +891,23 @@ describe('Actions Notes Store', () => {
testSubmitSuggestion(done, () => {
expect(commit).not.toHaveBeenCalled();
expect(dispatch).not.toHaveBeenCalled();
expect(Flash).toHaveBeenCalledWith(`${TEST_ERROR_MESSAGE}.`, 'alert', flashContainer);
expect(Flash).toHaveBeenCalledWith(TEST_ERROR_MESSAGE, 'alert', flashContainer);
});
});
it('when service fails, and no error message available, uses default message', done => {
const response = { response: 'foo' };
Api.applySuggestion.mockReturnValue(Promise.reject(response));
testSubmitSuggestion(done, () => {
expect(commit).not.toHaveBeenCalled();
expect(dispatch).not.toHaveBeenCalled();
expect(Flash).toHaveBeenCalledWith(
'Something went wrong while applying the suggestion. Please try again.',
'alert',
flashContainer,
);
});
});
......@@ -903,6 +920,130 @@ describe('Actions Notes Store', () => {
});
});
describe('submitSuggestionBatch', () => {
const discussionIds = batchSuggestionsInfoMock.map(({ discussionId }) => discussionId);
const batchSuggestionsInfo = batchSuggestionsInfoMock;
let flashContainer;
beforeEach(() => {
jest.spyOn(Api, 'applySuggestionBatch');
dispatch.mockReturnValue(Promise.resolve());
Api.applySuggestionBatch.mockReturnValue(Promise.resolve());
state = { batchSuggestionsInfo };
flashContainer = {};
});
const testSubmitSuggestionBatch = (done, expectFn) => {
actions
.submitSuggestionBatch({ commit, dispatch, state }, { flashContainer })
.then(expectFn)
.then(done)
.catch(done.fail);
};
it('when service succeeds, commits, resolves discussions, resets batch and applying batch state', done => {
testSubmitSuggestionBatch(done, () => {
expect(commit.mock.calls).toEqual([
[mutationTypes.SET_APPLYING_BATCH_STATE, true],
[mutationTypes.APPLY_SUGGESTION, batchSuggestionsInfo[0]],
[mutationTypes.APPLY_SUGGESTION, batchSuggestionsInfo[1]],
[mutationTypes.CLEAR_SUGGESTION_BATCH],
[mutationTypes.SET_APPLYING_BATCH_STATE, false],
]);
expect(dispatch.mock.calls).toEqual([
['resolveDiscussion', { discussionId: discussionIds[0] }],
['resolveDiscussion', { discussionId: discussionIds[1] }],
]);
expect(Flash).not.toHaveBeenCalled();
});
});
it('when service fails, flashes error message, resets applying batch state', done => {
const response = { response: { data: { message: TEST_ERROR_MESSAGE } } };
Api.applySuggestionBatch.mockReturnValue(Promise.reject(response));
testSubmitSuggestionBatch(done, () => {
expect(commit.mock.calls).toEqual([
[mutationTypes.SET_APPLYING_BATCH_STATE, true],
[mutationTypes.SET_APPLYING_BATCH_STATE, false],
]);
expect(dispatch).not.toHaveBeenCalled();
expect(Flash).toHaveBeenCalledWith(TEST_ERROR_MESSAGE, 'alert', flashContainer);
});
});
it('when service fails, and no error message available, uses default message', done => {
const response = { response: 'foo' };
Api.applySuggestionBatch.mockReturnValue(Promise.reject(response));
testSubmitSuggestionBatch(done, () => {
expect(commit.mock.calls).toEqual([
[mutationTypes.SET_APPLYING_BATCH_STATE, true],
[mutationTypes.SET_APPLYING_BATCH_STATE, false],
]);
expect(dispatch).not.toHaveBeenCalled();
expect(Flash).toHaveBeenCalledWith(
'Something went wrong while applying the batch of suggestions. Please try again.',
'alert',
flashContainer,
);
});
});
it('when resolve discussions fails, fails gracefully, resets batch and applying batch state', done => {
dispatch.mockReturnValue(Promise.reject());
testSubmitSuggestionBatch(done, () => {
expect(commit.mock.calls).toEqual([
[mutationTypes.SET_APPLYING_BATCH_STATE, true],
[mutationTypes.APPLY_SUGGESTION, batchSuggestionsInfo[0]],
[mutationTypes.APPLY_SUGGESTION, batchSuggestionsInfo[1]],
[mutationTypes.CLEAR_SUGGESTION_BATCH],
[mutationTypes.SET_APPLYING_BATCH_STATE, false],
]);
expect(Flash).not.toHaveBeenCalled();
});
});
});
describe('addSuggestionInfoToBatch', () => {
const suggestionInfo = batchSuggestionsInfoMock[0];
it("adds a suggestion's info to the current batch", done => {
testAction(
actions.addSuggestionInfoToBatch,
suggestionInfo,
{ batchSuggestionsInfo: [] },
[{ type: 'ADD_SUGGESTION_TO_BATCH', payload: suggestionInfo }],
[],
done,
);
});
});
describe('removeSuggestionInfoFromBatch', () => {
const suggestionInfo = batchSuggestionsInfoMock[0];
it("removes a suggestion's info the current batch", done => {
testAction(
actions.removeSuggestionInfoFromBatch,
suggestionInfo.suggestionId,
{ batchSuggestionsInfo: [suggestionInfo] },
[{ type: 'REMOVE_SUGGESTION_FROM_BATCH', payload: suggestionInfo.suggestionId }],
[],
done,
);
});
});
describe('filterDiscussion', () => {
const path = 'some-discussion-path';
const filter = 0;
......
spec/frontend/notes/stores/mutation_spec.js
View file @ c0417474
......@@ -9,6 +9,7 @@ import {
noteableDataMock,
individualNote,
notesWithDescriptionChanges,
batchSuggestionsInfoMock,
} from '../mock_data';
const RESOLVED_NOTE = { resolvable: true, resolved: true };
......@@ -700,4 +701,108 @@ describe('Notes Store mutations', () => {
expect(state.isToggleBlockedIssueWarning).toEqual(false);
});
});
describe('SET_APPLYING_BATCH_STATE', () => {
const buildDiscussions = suggestionsInfo => {
const suggestions = suggestionsInfo.map(({ suggestionId }) => ({ id: suggestionId }));
const notes = suggestionsInfo.map(({ noteId }, index) => ({
id: noteId,
suggestions: [suggestions[index]],
}));
return suggestionsInfo.map(({ discussionId }, index) => ({
id: discussionId,
notes: [notes[index]],
}));
};
let state;
let batchedSuggestionInfo;
let discussions;
let suggestions;
beforeEach(() => {
[batchedSuggestionInfo] = batchSuggestionsInfoMock;
suggestions = batchSuggestionsInfoMock.map(({ suggestionId }) => ({ id: suggestionId }));
discussions = buildDiscussions(batchSuggestionsInfoMock);
state = {
batchSuggestionsInfo: [batchedSuggestionInfo],
discussions,
};
});
it('sets is_applying_batch to a boolean value for all batched suggestions', () => {
mutations.SET_APPLYING_BATCH_STATE(state, true);
const updatedSuggestion = {
...suggestions[0],
is_applying_batch: true,
};
const expectedSuggestions = [updatedSuggestion, suggestions[1]];
const actualSuggestions = state.discussions
.map(discussion => discussion.notes.map(n => n.suggestions))
.flat(2);
expect(actualSuggestions).toEqual(expectedSuggestions);
});
});
describe('ADD_SUGGESTION_TO_BATCH', () => {
let state;
beforeEach(() => {
state = { batchSuggestionsInfo: [] };
});
it("adds a suggestion's info to a batch", () => {
const suggestionInfo = {
suggestionId: 'a123',
noteId: 'b456',
discussionId: 'c789',
};
mutations.ADD_SUGGESTION_TO_BATCH(state, suggestionInfo);
expect(state.batchSuggestionsInfo).toEqual([suggestionInfo]);
});
});
describe('REMOVE_SUGGESTION_FROM_BATCH', () => {
let state;
let suggestionInfo1;
let suggestionInfo2;
beforeEach(() => {
[suggestionInfo1, suggestionInfo2] = batchSuggestionsInfoMock;
state = {
batchSuggestionsInfo: [suggestionInfo1, suggestionInfo2],
};
});
it("removes a suggestion's info from a batch", () => {
mutations.REMOVE_SUGGESTION_FROM_BATCH(state, suggestionInfo1.suggestionId);
expect(state.batchSuggestionsInfo).toEqual([suggestionInfo2]);
});
});
describe('CLEAR_SUGGESTION_BATCH', () => {
let state;
beforeEach(() => {
state = {
batchSuggestionsInfo: batchSuggestionsInfoMock,
};
});
it('removes info for all suggestions from a batch', () => {
mutations.CLEAR_SUGGESTION_BATCH(state);
expect(state.batchSuggestionsInfo.length).toEqual(0);
});
});
});
spec/frontend/vue_shared/components/markdown/__snapshots__/suggestion_diff_spec.js.snap
View file @ c0417474
......@@ -5,8 +5,11 @@ exports[`Suggestion Diff component matches snapshot 1`] = `
class="md-suggestion"
>
<suggestion-diff-header-stub
batchsuggestionscount="1"
class="qa-suggestion-diff-header js-suggestion-diff-header"
helppagepath="path_to_docs"
isapplyingbatch="true"
isbatched="true"
/>
<table
......
spec/frontend/vue_shared/components/markdown/suggestion_diff_header_spec.js
View file @ c0417474
......@@ -3,8 +3,11 @@ import { shallowMount } from '@vue/test-utils';
import SuggestionDiffHeader from '~/vue_shared/components/markdown/suggestion_diff_header.vue';
const DEFAULT_PROPS = {
batchSuggestionsCount: 2,
canApply: true,
isApplied: false,
isBatched: false,
isApplyingBatch: false,
helpPagePath: 'path_to_docs',
};
......@@ -25,6 +28,9 @@ describe('Suggestion Diff component', () => {
});
const findApplyButton = () => wrapper.find('.js-apply-btn');
const findApplyBatchButton = () => wrapper.find('.js-apply-batch-btn');
const findAddToBatchButton = () => wrapper.find('.js-add-to-batch-btn');
const findRemoveFromBatchButton = () => wrapper.find('.js-remove-from-batch-btn');
const findHeader = () => wrapper.find('.js-suggestion-diff-header');
const findHelpButton = () => wrapper.find('.js-help-btn');
const findLoading = () => wrapper.find(GlLoadingIcon);
......@@ -44,19 +50,24 @@ describe('Suggestion Diff component', () => {
expect(findHelpButton().exists()).toBe(true);
});
it('renders an apply button', () => {
it('renders apply suggestion and add to batch buttons', () => {
createComponent();
const applyBtn = findApplyButton();
const addToBatchBtn = findAddToBatchButton();
expect(applyBtn.exists()).toBe(true);
expect(applyBtn.html().includes('Apply suggestion')).toBe(true);
expect(addToBatchBtn.exists()).toBe(true);
expect(addToBatchBtn.html().includes('Add suggestion to batch')).toBe(true);
});
it('does not render an apply button if `canApply` is set to false', () => {
it('does not render apply suggestion and add to batch buttons if `canApply` is set to false', () => {
createComponent({ canApply: false });
expect(findApplyButton().exists()).toBe(false);
expect(findAddToBatchButton().exists()).toBe(false);
});
describe('when apply suggestion is clicked', () => {
......@@ -73,13 +84,14 @@ describe('Suggestion Diff component', () => {
});
});
it('hides apply button', () => {
it('hides apply suggestion and add to batch buttons', () => {
expect(findApplyButton().exists()).toBe(false);
expect(findAddToBatchButton().exists()).toBe(false);
});
it('shows loading', () => {
expect(findLoading().exists()).toBe(true);
expect(wrapper.text()).toContain('Applying suggestion');
expect(wrapper.text()).toContain('Applying suggestion...');
});
it('when callback of apply is called, hides loading', () => {
......@@ -93,4 +105,104 @@ describe('Suggestion Diff component', () => {
});
});
});
describe('when add to batch is clicked', () => {
it('emits addToBatch', () => {
createComponent();
findAddToBatchButton().vm.$emit('click');
expect(wrapper.emittedByOrder()).toContainEqual({
name: 'addToBatch',
args: [],
});
});
});
describe('when remove from batch is clicked', () => {
it('emits removeFromBatch', () => {
createComponent({ isBatched: true });
findRemoveFromBatchButton().vm.$emit('click');
expect(wrapper.emittedByOrder()).toContainEqual({
name: 'removeFromBatch',
args: [],
});
});
});
describe('apply suggestions is clicked', () => {
it('emits applyBatch', () => {
createComponent({ isBatched: true });
findApplyBatchButton().vm.$emit('click');
expect(wrapper.emittedByOrder()).toContainEqual({
name: 'applyBatch',
args: [],
});
});
});
describe('when isBatched is true', () => {
it('shows remove from batch and apply batch buttons and displays the batch count', () => {
createComponent({
batchSuggestionsCount: 9,
isBatched: true,
});
const applyBatchBtn = findApplyBatchButton();
const removeFromBatchBtn = findRemoveFromBatchButton();
expect(removeFromBatchBtn.exists()).toBe(true);
expect(removeFromBatchBtn.html().includes('Remove from batch')).toBe(true);
expect(applyBatchBtn.exists()).toBe(true);
expect(applyBatchBtn.html().includes('Apply suggestions')).toBe(true);
expect(applyBatchBtn.html().includes(String('9'))).toBe(true);
});
it('hides add to batch and apply buttons', () => {
createComponent({
isBatched: true,
});
expect(findApplyButton().exists()).toBe(false);
expect(findAddToBatchButton().exists()).toBe(false);
});
describe('when isBatched and isApplyingBatch are true', () => {
it('shows loading', () => {
createComponent({
isBatched: true,
isApplyingBatch: true,
});
expect(findLoading().exists()).toBe(true);
expect(wrapper.text()).toContain('Applying suggestions...');
});
it('adjusts message for batch with single suggestion', () => {
createComponent({
batchSuggestionsCount: 1,
isBatched: true,
isApplyingBatch: true,
});
expect(findLoading().exists()).toBe(true);
expect(wrapper.text()).toContain('Applying suggestion...');
});
it('hides remove from batch and apply suggestions buttons', () => {
createComponent({
isBatched: true,
isApplyingBatch: true,
});
expect(findRemoveFromBatchButton().exists()).toBe(false);
expect(findApplyBatchButton().exists()).toBe(false);
});
});
});
});
spec/frontend/vue_shared/components/markdown/suggestion_diff_spec.js
View file @ c0417474
......@@ -3,9 +3,10 @@ import SuggestionDiffComponent from '~/vue_shared/components/markdown/suggestion
import SuggestionDiffHeader from '~/vue_shared/components/markdown/suggestion_diff_header.vue';
import SuggestionDiffRow from '~/vue_shared/components/markdown/suggestion_diff_row.vue';
const suggestionId = 1;
const MOCK_DATA = {
suggestion: {
id: 1,
id: suggestionId,
diff_lines: [
{
can_receive_suggestion: false,
......@@ -38,8 +39,10 @@ const MOCK_DATA = {
type: 'new',
},
],
is_applying_batch: true,
},
helpPagePath: 'path_to_docs',
batchSuggestionsInfo: [{ suggestionId }],
};
describe('Suggestion Diff component', () => {
......@@ -70,17 +73,24 @@ describe('Suggestion Diff component', () => {
expect(wrapper.findAll(SuggestionDiffRow)).toHaveLength(3);
});
it('emits apply event on sugestion diff header apply', () => {
wrapper.find(SuggestionDiffHeader).vm.$emit('apply', 'test-event');
it.each`
event | childArgs | args
${'apply'} | ${['test-event']} | ${[{ callback: 'test-event', suggestionId }]}
${'applyBatch'} | ${[]} | ${[]}
${'addToBatch'} | ${[]} | ${[suggestionId]}
${'removeFromBatch'} | ${[]} | ${[suggestionId]}
`('emits $event event on sugestion diff header $event', ({ event, childArgs, args }) => {
wrapper.find(SuggestionDiffHeader).vm.$emit(event, ...childArgs);
expect(wrapper.emitted('apply')).toBeDefined();
expect(wrapper.emitted('apply')).toEqual([
[
{
callback: 'test-event',
suggestionId: 1,
},
],
]);
expect(wrapper.emitted(event)).toBeDefined();
expect(wrapper.emitted(event)).toEqual([args]);
});
it('passes suggestion batch props to suggestion diff header', () => {
expect(wrapper.find(SuggestionDiffHeader).props()).toMatchObject({
batchSuggestionsCount: 1,
isBatched: true,
isApplyingBatch: MOCK_DATA.suggestion.is_applying_batch,
});
});
});
spec/lib/gitlab/suggestions/commit_message_spec.rb 0 → 100644
View file @ c0417474
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Suggestions::CommitMessage do
def create_suggestion(file_path, new_line, to_content)
position = Gitlab::Diff::Position.new(old_path: file_path,
new_path: file_path,
old_line: nil,
new_line: new_line,
diff_refs: merge_request.diff_refs)
diff_note = create(:diff_note_on_merge_request,
noteable: merge_request,
position: position,
project: project)
create(:suggestion,
:content_from_repo,
note: diff_note,
to_content: to_content)
end
let_it_be(:user) do
create(:user, :commit_email, name: 'Test User', username: 'test.user')
end
let_it_be(:project) do
create(:project, :repository, path: 'project-1', name: 'Project_1')
end
let_it_be(:merge_request) do
create(:merge_request, source_project: project, target_project: project)
end
let_it_be(:suggestion_set) do
suggestion1 = create_suggestion('files/ruby/popen.rb', 9, '*** SUGGESTION 1 ***')
suggestion2 = create_suggestion('files/ruby/popen.rb', 13, '*** SUGGESTION 2 ***')
suggestion3 = create_suggestion('files/ruby/regex.rb', 22, '*** SUGGESTION 3 ***')
Gitlab::Suggestions::SuggestionSet.new([suggestion1, suggestion2, suggestion3])
end
describe '#message' do
before do
# Updating the suggestion_commit_message on a project shared across specs
# avoids recreating the repository for each spec.
project.update!(suggestion_commit_message: message)
end
context 'when a custom commit message is not specified' do
let(:expected_message) { 'Apply 3 suggestion(s) to 2 file(s)' }
context 'and is nil' do
let(:message) { nil }
it 'uses the default commit message' do
expect(described_class
.new(user, suggestion_set)
.message).to eq(expected_message)
end
end
context 'and is an empty string' do
let(:message) { '' }
it 'uses the default commit message' do
expect(described_class
.new(user, suggestion_set)
.message).to eq(expected_message)
end
end
end
context 'is specified and includes all placeholders' do
let(:message) do
'*** %{branch_name} %{files_count} %{file_paths} %{project_name} %{project_path} %{user_full_name} %{username} %{suggestions_count} ***'
end
it 'generates a custom commit message' do
expect(Gitlab::Suggestions::CommitMessage
.new(user, suggestion_set)
.message).to eq('*** master 2 files/ruby/popen.rb, files/ruby/regex.rb Project_1 project-1 Test User test.user 3 ***')
end
end
end
end
spec/lib/gitlab/suggestions/file_suggestion_spec.rb 0 → 100644
View file @ c0417474
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Suggestions::FileSuggestion do
def create_suggestion(new_line, to_content)
position = Gitlab::Diff::Position.new(old_path: file_path,
new_path: file_path,
old_line: nil,
new_line: new_line,
diff_refs: merge_request.diff_refs)
diff_note = create(:diff_note_on_merge_request,
noteable: merge_request,
position: position,
project: project)
create(:suggestion,
:content_from_repo,
note: diff_note,
to_content: to_content)
end
let_it_be(:user) { create(:user) }
let_it_be(:file_path) { 'files/ruby/popen.rb'}
let_it_be(:project) { create(:project, :repository) }
let_it_be(:merge_request) do
create(:merge_request, source_project: project, target_project: project)
end
let_it_be(:suggestion1) do
create_suggestion(9, " *** SUGGESTION 1 ***\n")
end
let_it_be(:suggestion2) do
create_suggestion(15, " *** SUGGESTION 2 ***\n")
end
let(:file_suggestion) { described_class.new }
describe '#add_suggestion' do
it 'succeeds when adding a suggestion for the same file as the original' do
file_suggestion.add_suggestion(suggestion1)
expect { file_suggestion.add_suggestion(suggestion2) }.not_to raise_error
end
it 'raises an error when adding a suggestion for a different file' do
allow(suggestion2)
.to(receive_message_chain(:diff_file, :file_path)
.and_return('path/to/different/file'))
file_suggestion.add_suggestion(suggestion1)
expect { file_suggestion.add_suggestion(suggestion2) }.to(
raise_error(described_class::SuggestionForDifferentFileError)
)
end
end
describe '#line_conflict' do
def stub_suggestions(line_index_spans)
fake_suggestions = line_index_spans.map do |span|
double("Suggestion",
from_line_index: span[:from_line_index],
to_line_index: span[:to_line_index])
end
allow(file_suggestion).to(receive(:suggestions).and_return(fake_suggestions))
end
context 'when line ranges do not overlap' do
it 'return false' do
stub_suggestions(
[
{
from_line_index: 0,
to_line_index: 10
},
{
from_line_index: 11,
to_line_index: 20
}
]
)
expect(file_suggestion.line_conflict?).to be(false)
end
end
context 'when line ranges are identical' do
it 'returns true' do
stub_suggestions(
[
{
from_line_index: 0,
to_line_index: 10
},
{
from_line_index: 0,
to_line_index: 10
}
]
)
expect(file_suggestion.line_conflict?).to be(true)
end
end
context 'when one range starts, and the other ends, on the same line' do
it 'returns true' do
stub_suggestions(
[
{
from_line_index: 0,
to_line_index: 10
},
{
from_line_index: 10,
to_line_index: 20
}
]
)
expect(file_suggestion.line_conflict?).to be(true)
end
end
context 'when one line range contains the other' do
it 'returns true' do
stub_suggestions(
[
{
from_line_index: 0,
to_line_index: 10
},
{
from_line_index: 5,
to_line_index: 7
}
]
)
expect(file_suggestion.line_conflict?).to be(true)
end
end
context 'when line ranges overlap' do
it 'returns true' do
stub_suggestions(
[
{
from_line_index: 0,
to_line_index: 10
},
{
from_line_index: 8,
to_line_index: 15
}
]
)
expect(file_suggestion.line_conflict?).to be(true)
end
end
context 'when no suggestions have been added' do
it 'returns false' do
expect(file_suggestion.line_conflict?).to be(false)
end
end
end
describe '#new_content' do
it 'returns a blob with the suggestions applied to it' do
file_suggestion.add_suggestion(suggestion1)
file_suggestion.add_suggestion(suggestion2)
expected_content = <<-CONTENT.strip_heredoc
require 'fileutils'
require 'open3'
module Popen
extend self
def popen(cmd, path=nil)
unless cmd.is_a?(Array)
*** SUGGESTION 1 ***
end
path ||= Dir.pwd
vars = {
*** SUGGESTION 2 ***
}
options = {
chdir: path
}
unless File.directory?(path)
FileUtils.mkdir_p(path)
end
@cmd_output = ""
@cmd_status = 0
Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
@cmd_output << stdout.read
@cmd_output << stderr.read
@cmd_status = wait_thr.value.exitstatus
end
return @cmd_output, @cmd_status
end
end
CONTENT
expect(file_suggestion.new_content).to eq(expected_content)
end
it 'returns an empty string when no suggestions have been added' do
expect(file_suggestion.new_content).to eq('')
end
end
describe '#file_path' do
it 'returns the path of the file associated with the suggestions' do
file_suggestion.add_suggestion(suggestion1)
expect(file_suggestion.file_path).to eq(file_path)
end
it 'returns nil if no suggestions have been added' do
expect(file_suggestion.file_path).to be(nil)
end
end
end
spec/lib/gitlab/suggestions/suggestion_set_spec.rb 0 → 100644
View file @ c0417474
# frozen_string_literal: true
require 'spec_helper'
describe Gitlab::Suggestions::SuggestionSet do
def create_suggestion(file_path, new_line, to_content)
position = Gitlab::Diff::Position.new(old_path: file_path,
new_path: file_path,
old_line: nil,
new_line: new_line,
diff_refs: merge_request.diff_refs)
diff_note = create(:diff_note_on_merge_request,
noteable: merge_request,
position: position,
project: project)
create(:suggestion,
:content_from_repo,
note: diff_note,
to_content: to_content)
end
let_it_be(:user) { create(:user) }
let_it_be(:project) { create(:project, :repository) }
let_it_be(:merge_request) do
create(:merge_request, source_project: project, target_project: project)
end
let_it_be(:suggestion) { create(:suggestion)}
let_it_be(:suggestion2) do
create_suggestion('files/ruby/popen.rb', 13, "*** SUGGESTION 2 ***")
end
let_it_be(:suggestion3) do
create_suggestion('files/ruby/regex.rb', 22, "*** SUGGESTION 3 ***")
end
let_it_be(:unappliable_suggestion) { create(:suggestion, :unappliable) }
let(:suggestion_set) { described_class.new([suggestion]) }
describe '#project' do
it 'returns the project associated with the suggestions' do
expected_project = suggestion.project
expect(suggestion_set.project).to be(expected_project)
end
end
describe '#branch' do
it 'returns the branch associated with the suggestions' do
expected_branch = suggestion.branch
expect(suggestion_set.branch).to be(expected_branch)
end
end
describe '#valid?' do
it 'returns true if no errors are found' do
expect(suggestion_set.valid?).to be(true)
end
it 'returns false if an error is found' do
suggestion_set = described_class.new([unappliable_suggestion])
expect(suggestion_set.valid?).to be(false)
end
end
describe '#error_message' do
it 'returns an error message if an error is found' do
suggestion_set = described_class.new([unappliable_suggestion])
expect(suggestion_set.error_message).to be_a(String)
end
it 'returns nil if no errors are found' do
expect(suggestion_set.error_message).to be(nil)
end
end
describe '#actions' do
it 'returns an array of hashes with proper key/value pairs' do
first_action = suggestion_set.actions.first
file_path, file_suggestion = suggestion_set
.send(:suggestions_per_file).first
expect(first_action[:action]).to be('update')
expect(first_action[:file_path]).to eq(file_path)
expect(first_action[:content]).to eq(file_suggestion.new_content)
end
end
describe '#file_paths' do
it 'returns an array of unique file paths associated with the suggestions' do
suggestion_set = described_class.new([suggestion, suggestion2, suggestion3])
expected_paths = %w(files/ruby/popen.rb files/ruby/regex.rb)
actual_paths = suggestion_set.file_paths
expect(actual_paths.sort).to eq(expected_paths)
end
end
end
spec/requests/api/suggestions_spec.rb
View file @ c0417474
......@@ -7,8 +7,7 @@ describe API::Suggestions do
let(:user) { create(:user) }
let(:merge_request) do
create(:merge_request, source_project: project,
target_project: project)
create(:merge_request, source_project: project, target_project: project)
end
let(:position) do
......@@ -19,26 +18,45 @@ describe API::Suggestions do
diff_refs: merge_request.diff_refs)
end
let(:position2) do
Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: 15,
diff_refs: merge_request.diff_refs)
end
let(:diff_note) do
create(:diff_note_on_merge_request, noteable: merge_request,
create(:diff_note_on_merge_request,
noteable: merge_request,
position: position,
project: project)
end
describe "PUT /suggestions/:id/apply" do
let(:url) { "/suggestions/#{suggestion.id}/apply" }
let(:diff_note2) do
create(:diff_note_on_merge_request, noteable: merge_request,
position: position2,
project: project)
end
context 'when successfully applies patch' do
let(:suggestion) do
create(:suggestion, note: diff_note,
from_content: " raise RuntimeError, \"System commands must be given as an array of strings\"\n",
to_content: " raise RuntimeError, 'Explosion'\n # explosion?")
end
it 'returns 200 with json content' do
let(:unappliable_suggestion) do
create(:suggestion, :unappliable, note: diff_note2)
end
describe "PUT /suggestions/:id/apply" do
let(:url) { "/suggestions/#{suggestion.id}/apply" }
context 'when successfully applies patch' do
it 'renders an ok response and returns json content' do
project.add_maintainer(user)
put api(url, user), params: { id: suggestion.id }
put api(url, user)
expect(response).to have_gitlab_http_status(:ok)
expect(json_response)
......@@ -48,31 +66,105 @@ describe API::Suggestions do
end
context 'when not able to apply patch' do
let(:suggestion) do
create(:suggestion, :unappliable, note: diff_note)
end
let(:url) { "/suggestions/#{unappliable_suggestion.id}/apply" }
it 'returns 400 with json content' do
it 'renders a bad request error and returns json content' do
project.add_maintainer(user)
put api(url, user), params: { id: suggestion.id }
put api(url, user)
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response).to eq({ 'message' => 'Suggestion is not appliable' })
expect(json_response).to eq({ 'message' => 'A suggestion is not applicable.' })
end
end
context 'when suggestion is not found' do
let(:url) { "/suggestions/foo-123/apply" }
it 'renders a not found error and returns json content' do
project.add_maintainer(user)
put api(url, user)
expect(response).to have_gitlab_http_status(:not_found)
expect(json_response).to eq({ 'message' => 'Suggestion is not applicable as the suggestion was not found.' })
end
end
context 'when unauthorized' do
let(:suggestion) do
create(:suggestion, note: diff_note,
from_content: " raise RuntimeError, \"System commands must be given as an array of strings\"\n",
to_content: " raise RuntimeError, 'Explosion'\n # explosion?")
it 'renders a forbidden error and returns json content' do
project.add_reporter(user)
put api(url, user)
expect(response).to have_gitlab_http_status(:forbidden)
expect(json_response).to eq({ 'message' => '403 Forbidden' })
end
end
end
describe "PUT /suggestions/batch_apply" do
let(:suggestion2) do
create(:suggestion, note: diff_note2,
from_content: " \"PWD\" => path\n",
to_content: " *** FOO ***\n")
end
let(:url) { "/suggestions/batch_apply" }
context 'when successfully applies multiple patches as a batch' do
it 'renders an ok response and returns json content' do
project.add_maintainer(user)
put api(url, user), params: { ids: [suggestion.id, suggestion2.id] }
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to all(include('id', 'from_line', 'to_line',
'appliable', 'applied',
'from_content', 'to_content'))
end
end
it 'returns 403 with json content' do
context 'when not able to apply one or more of the patches' do
it 'renders a bad request error and returns json content' do
project.add_maintainer(user)
put api(url, user),
params: { ids: [suggestion.id, unappliable_suggestion.id] }
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response).to eq({ 'message' => 'A suggestion is not applicable.' })
end
end
context 'with missing suggestions' do
it 'renders a not found error and returns json content if any suggestion is not found' do
project.add_maintainer(user)
put api(url, user), params: { ids: [suggestion.id, 'foo-123'] }
expect(response).to have_gitlab_http_status(:not_found)
expect(json_response)
.to eq({ 'message' => 'Suggestions are not applicable as one or more suggestions were not found.' })
end
it 'renders a bad request error and returns json content when no suggestions are provided' do
project.add_maintainer(user)
put api(url, user), params: {}
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response)
.to eq({ 'error' => "ids is missing" })
end
end
context 'when unauthorized' do
it 'renders a forbidden error and returns json content' do
project.add_reporter(user)
put api(url, user), params: { id: suggestion.id }
put api(url, user),
params: { ids: [suggestion.id, suggestion2.id] }
expect(response).to have_gitlab_http_status(:forbidden)
expect(json_response).to eq({ 'message' => '403 Forbidden' })
......
spec/services/suggestions/apply_service_spec.rb
View file @ c0417474
......@@ -5,41 +5,66 @@ require 'spec_helper'
describe Suggestions::ApplyService do
include ProjectForksHelper
def build_position(args = {})
default_args = { old_path: "files/ruby/popen.rb",
def build_position(**optional_args)
args = { old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: 9,
diff_refs: merge_request.diff_refs }
diff_refs: merge_request.diff_refs,
**optional_args }
Gitlab::Diff::Position.new(default_args.merge(args))
Gitlab::Diff::Position.new(args)
end
shared_examples 'successfully creates commit and updates suggestion' do
def apply(suggestion)
result = subject.execute(suggestion)
def create_suggestion(args)
position_args = args.slice(:old_path, :new_path, :old_line, :new_line)
content_args = args.slice(:from_content, :to_content)
position = build_position(position_args)
diff_note = create(:diff_note_on_merge_request,
noteable: merge_request,
position: position,
project: project)
suggestion_args = { note: diff_note }.merge(content_args)
create(:suggestion, :content_from_repo, suggestion_args)
end
def apply(suggestions)
result = apply_service.new(user, *suggestions).execute
suggestions.map { |suggestion| suggestion.reload }
expect(result[:status]).to eq(:success)
end
it 'updates the file with the new contents' do
apply(suggestion)
shared_examples 'successfully creates commit and updates suggestions' do
it 'updates the files with the new content' do
apply(suggestions)
suggestions.each do |suggestion|
path = suggestion.diff_file.file_path
blob = project.repository.blob_at_branch(merge_request.source_branch,
position.new_path)
path)
expect(blob.data).to eq(expected_content)
expect(blob.data).to eq(expected_content_by_path[path.to_sym])
end
end
it 'updates suggestion applied and commit_id columns' do
expect { apply(suggestion) }
.to change(suggestion, :applied)
.from(false).to(true)
.and change(suggestion, :commit_id)
.from(nil)
expect(suggestions.map(&:applied)).to all(be false)
expect(suggestions.map(&:commit_id)).to all(be nil)
apply(suggestions)
expect(suggestions.map(&:applied)).to all(be true)
expect(suggestions.map(&:commit_id)).to all(be_present)
end
it 'created commit has users email and name' do
apply(suggestion)
apply(suggestions)
commit = project.repository.commit
......@@ -53,46 +78,54 @@ describe Suggestions::ApplyService do
before do
project.update!(suggestion_commit_message: message)
apply(suggestion)
apply(suggestions)
end
context 'is not specified' do
let(:expected_value) { "Apply suggestion to files/ruby/popen.rb" }
context 'is nil' do
let(:message) { nil }
let(:message) { '' }
it 'sets default commit message' do
expect(project.repository.commit.message).to eq(expected_value)
it 'uses the default commit message' do
expect(project.repository.commit.message).to(
match(/\AApply #{suggestions.size} suggestion\(s\) to \d+ file\(s\)\z/)
)
end
end
context 'is an empty string' do
let(:message) { '' }
context 'is specified' do
let(:message) do
'refactor: %{project_name} %{branch_name} %{username}'
end
it 'sets default commit message' do
expect(project.repository.commit.message).to eq(expected_value)
it 'generates a custom commit message' do
expect(project.repository.commit.message).to(
eq("refactor: Project_1 master test.user")
)
end
end
end
end
context 'is specified' do
let(:message) { 'refactor: %{project_path} %{project_name} %{file_path} %{branch_name} %{username} %{user_full_name}' }
subject(:apply_service) { described_class }
it 'sets custom commit message' do
expect(project.repository.commit.message).to eq("refactor: project-1 Project_1 files/ruby/popen.rb master test.user Test User")
end
end
let_it_be(:user) do
create(:user, :commit_email, name: 'Test User', username: 'test.user')
end
let(:project) do
create(:project, :repository, path: 'project-1', name: 'Project_1')
end
let(:project) { create(:project, :repository, path: 'project-1', name: 'Project_1') }
let(:user) { create(:user, :commit_email, name: 'Test User', username: 'test.user') }
let(:merge_request) do
create(:merge_request, source_project: project,
target_project: project,
source_branch: 'master')
end
let(:position) { build_position }
let(:diff_note) do
create(:diff_note_on_merge_request, noteable: merge_request, position: position, project: project)
create(:diff_note_on_merge_request, noteable: merge_request,
position: position, project: project)
end
let(:suggestion) do
......@@ -100,10 +133,24 @@ describe Suggestions::ApplyService do
to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n")
end
subject { described_class.new(user) }
let(:suggestion2) do
create_suggestion(
to_content: " *** SUGGESTION CHANGE ***\n",
new_line: 15)
end
let(:suggestion3) do
create_suggestion(
to_content: " *** ANOTHER SUGGESTION CHANGE ***\n",
old_path: "files/ruby/regex.rb",
new_path: "files/ruby/regex.rb",
new_line: 22)
end
let(:suggestions) { [suggestion, suggestion2, suggestion3] }
context 'patch is appliable' do
let(:expected_content) do
let(:popen_content) do
<<-CONTENT.strip_heredoc
require 'fileutils'
require 'open3'
......@@ -120,7 +167,7 @@ describe Suggestions::ApplyService do
path ||= Dir.pwd
vars = {
"PWD" => path
*** SUGGESTION CHANGE ***
}
options = {
......@@ -146,113 +193,122 @@ describe Suggestions::ApplyService do
CONTENT
end
context 'non-fork project' do
let(:merge_request) do
create(:merge_request, source_project: project,
target_project: project,
source_branch: 'master')
let(:regex_content) do
<<-CONTENT.strip_heredoc
module Gitlab
module Regex
extend self
def username_regex
default_regex
end
before do
project.add_maintainer(user)
def project_name_regex
/\\A[a-zA-Z0-9][a-zA-Z0-9_\\-\\. ]*\\z/
end
it_behaves_like 'successfully creates commit and updates suggestion'
def name_regex
/\\A[a-zA-Z0-9_\\-\\. ]*\\z/
end
context 'when it fails to apply because the file was changed' do
it 'returns error message' do
service = instance_double(Files::UpdateService)
def path_regex
default_regex
end
expect(Files::UpdateService).to receive(:new)
.and_return(service)
def archive_formats_regex
*** ANOTHER SUGGESTION CHANGE ***
end
allow(service).to receive(:execute)
.and_raise(Files::UpdateService::FileChangedError)
def git_reference_regex
# Valid git ref regex, see:
# https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
%r{
(?!
(?# doesn't begins with)
\\/| (?# rule #6)
(?# doesn't contain)
.*(?:
[\\\/.]\\\.| (?# rule #1,3)
\\/\\/| (?# rule #6)
@\\{| (?# rule #8)
\\\\ (?# rule #9)
)
)
[^\\000-\\040\\177~^:?*\\[]+ (?# rule #4-5)
(?# doesn't end with)
(?<!\\.lock) (?# rule #1)
(?<![\\/.]) (?# rule #6-7)
}x
end
result = subject.execute(suggestion)
protected
expect(result).to eq(message: 'The file has been changed', status: :error)
def default_regex
/\\A[.?]?[a-zA-Z0-9][a-zA-Z0-9_\\-\\.]*(?<!\\.git)\\z/
end
end
context 'when HEAD from position is different from source branch HEAD on repo' do
it 'returns error message' do
allow(suggestion).to receive(:appliable?) { true }
allow(suggestion.position).to receive(:head_sha) { 'old-sha' }
allow(suggestion.noteable).to receive(:source_branch_sha) { 'new-sha' }
result = subject.execute(suggestion)
expect(result).to eq(message: 'The file has been changed', status: :error)
end
CONTENT
end
context 'multiple suggestions applied' do
let(:expected_content) do
<<-CONTENT.strip_heredoc
require 'fileutils'
require 'open3'
module Popen
extend self
let(:expected_content_by_path) do
{
"files/ruby/popen.rb": popen_content,
"files/ruby/regex.rb": regex_content
}
end
def popen(cmd, path=nil)
unless cmd.is_a?(Array)
# v1 change
context 'non-fork project' do
before do
project.add_maintainer(user)
end
path ||= Dir.pwd
# v1 change
vars = {
"PWD" => path
}
it_behaves_like 'successfully creates commit and updates suggestions'
options = {
chdir: path
context 'when it fails to apply because a file was changed' do
before do
params = {
file_path: suggestion3.diff_file.file_path,
start_branch: suggestion3.branch,
branch_name: suggestion3.branch,
commit_message: 'Update file',
file_content: 'New content'
}
# v2 change
unless File.directory?(path)
FileUtils.mkdir_p(path)
end
@cmd_output = ""
# v2 change
# Reload the suggestion so it's memoized values get reset after the
# file was changed.
suggestion3.reload
Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
@cmd_output << stdout.read
@cmd_output << stderr.read
@cmd_status = wait_thr.value.exitstatus
Files::UpdateService.new(project, user, params).execute
end
return @cmd_output, @cmd_status
end
it 'returns error message' do
result = apply_service.new(user, suggestion, suggestion3, suggestion2).execute
expect(result).to eq(message: 'A file has been changed.', status: :error)
end
CONTENT
end
def create_suggestion(diff, old_line: nil, new_line: nil, from_content:, to_content:, path:)
position = Gitlab::Diff::Position.new(old_path: path,
new_path: path,
old_line: old_line,
new_line: new_line,
diff_refs: diff.diff_refs)
context 'when HEAD from position is different from source branch HEAD on repo' do
it 'returns error message' do
allow(suggestion).to receive(:appliable?) { true }
allow(suggestion.position).to receive(:head_sha) { 'old-sha' }
allow(suggestion.noteable).to receive(:source_branch_sha) { 'new-sha' }
suggestion_note = create(:diff_note_on_merge_request, noteable: merge_request,
original_position: position,
position: position,
project: project)
create(:suggestion, note: suggestion_note,
from_content: from_content,
to_content: to_content)
result = apply_service.new(user, suggestion).execute
expect(result).to eq(message: 'A file has been changed.', status: :error)
end
end
context 'multiple suggestions applied sequentially' do
def apply_suggestion(suggestion)
suggestion.reload
merge_request.reload
merge_request.clear_memoized_shas
result = subject.execute(suggestion)
result = apply_service.new(user, suggestion).execute
suggestion.reload
expect(result[:status]).to eq(:success)
refresh = MergeRequests::RefreshService.new(project, user)
......@@ -264,34 +320,31 @@ describe Suggestions::ApplyService do
end
def fetch_raw_diff(suggestion)
project.reload.commit(suggestion.commit_id).diffs.diff_files.first.diff.diff
project.reload.commit(suggestion.commit_id)
.diffs.diff_files.first.diff.diff
end
it 'applies multiple suggestions in subsequent versions correctly' do
diff = merge_request.merge_request_diff
path = 'files/ruby/popen.rb'
suggestion_1_changes = { old_line: nil,
new_line: 13,
suggestion1 = create_suggestion(
from_content: "\n",
to_content: "# v1 change\n",
path: path }
old_line: nil,
new_line: 13)
suggestion_2_changes = { old_line: 24,
new_line: 31,
suggestion2 = create_suggestion(
from_content: " @cmd_output << stderr.read\n",
to_content: "# v2 change\n",
path: path }
suggestion_1 = create_suggestion(diff, suggestion_1_changes)
suggestion_2 = create_suggestion(diff, suggestion_2_changes)
old_line: 24,
new_line: 31)
apply_suggestion(suggestion_1)
apply_suggestion(suggestion1)
apply_suggestion(suggestion2)
suggestion_1_diff = fetch_raw_diff(suggestion_1)
suggestion1_diff = fetch_raw_diff(suggestion1)
suggestion2_diff = fetch_raw_diff(suggestion2)
# rubocop: disable Layout/TrailingWhitespace
expected_suggestion_1_diff = <<-CONTENT.strip_heredoc
expected_suggestion1_diff = <<-CONTENT.strip_heredoc
@@ -10,7 +10,7 @@ module Popen
end
......@@ -304,12 +357,8 @@ describe Suggestions::ApplyService do
CONTENT
# rubocop: enable Layout/TrailingWhitespace
apply_suggestion(suggestion_2)
suggestion_2_diff = fetch_raw_diff(suggestion_2)
# rubocop: disable Layout/TrailingWhitespace
expected_suggestion_2_diff = <<-CONTENT.strip_heredoc
expected_suggestion2_diff = <<-CONTENT.strip_heredoc
@@ -28,7 +28,7 @@ module Popen
Open3.popen3(vars, *cmd, options) do |stdin, stdout, stderr, wait_thr|
......@@ -321,14 +370,14 @@ describe Suggestions::ApplyService do
CONTENT
# rubocop: enable Layout/TrailingWhitespace
expect(suggestion_1_diff.strip).to eq(expected_suggestion_1_diff.strip)
expect(suggestion_2_diff.strip).to eq(expected_suggestion_2_diff.strip)
expect(suggestion1_diff.strip).to eq(expected_suggestion1_diff.strip)
expect(suggestion2_diff.strip).to eq(expected_suggestion2_diff.strip)
end
end
context 'multi-line suggestion' do
let(:expected_content) do
<<~CONTENT
let(:popen_content) do
<<~CONTENT.strip_heredoc
require 'fileutils'
require 'open3'
......@@ -365,6 +414,12 @@ describe Suggestions::ApplyService do
CONTENT
end
let(:expected_content_by_path) do
{
"files/ruby/popen.rb": popen_content
}
end
let(:suggestion) do
create(:suggestion, :content_from_repo, note: diff_note,
lines_above: 2,
......@@ -372,12 +427,14 @@ describe Suggestions::ApplyService do
to_content: "# multi\n# line\n")
end
it_behaves_like 'successfully creates commit and updates suggestion'
let(:suggestions) { [suggestion] }
it_behaves_like 'successfully creates commit and updates suggestions'
end
context 'remove an empty line suggestion' do
let(:expected_content) do
<<~CONTENT
let(:popen_content) do
<<~CONTENT.strip_heredoc
require 'fileutils'
require 'open3'
......@@ -417,12 +474,19 @@ describe Suggestions::ApplyService do
CONTENT
end
let(:position) { build_position(new_line: 13) }
let(:expected_content_by_path) do
{
"files/ruby/popen.rb": popen_content
}
end
let(:suggestion) do
create(:suggestion, :content_from_repo, note: diff_note, to_content: "")
create_suggestion( to_content: "", new_line: 13)
end
it_behaves_like 'successfully creates commit and updates suggestion'
let(:suggestions) { [suggestion] }
it_behaves_like 'successfully creates commit and updates suggestions'
end
end
......@@ -430,17 +494,23 @@ describe Suggestions::ApplyService do
let(:project) { create(:project, :public, :repository) }
let(:forked_project) do
fork_project_with_submodules(project, user, repository: project.repository)
fork_project_with_submodules(project,
user, repository: project.repository)
end
let(:merge_request) do
create(:merge_request,
source_branch: 'conflict-resolvable-fork', source_project: forked_project,
target_branch: 'conflict-start', target_project: project)
source_branch: 'conflict-resolvable-fork',
source_project: forked_project,
target_branch: 'conflict-start',
target_project: project)
end
let!(:diff_note) do
create(:diff_note_on_merge_request, noteable: merge_request, position: position, project: project)
create(:diff_note_on_merge_request,
noteable: merge_request,
position: position,
project: project)
end
before do
......@@ -448,11 +518,12 @@ describe Suggestions::ApplyService do
end
it 'updates file in the source project' do
expect(Files::UpdateService).to receive(:new)
.with(merge_request.source_project, user, anything)
.and_call_original
expect(Files::MultiService).to receive(:new)
.with(merge_request.source_project,
user,
anything).and_call_original
subject.execute(suggestion)
apply_service.new(user, suggestion).execute
end
end
end
......@@ -475,7 +546,7 @@ describe Suggestions::ApplyService do
end
it 'returns error' do
result = subject.execute(suggestion)
result = apply_service.new(user, suggestion).execute
expect(result).to eq(message: "You are not allowed to push into this branch",
status: :error)
......@@ -503,29 +574,49 @@ describe Suggestions::ApplyService do
it 'returns error message' do
expect(suggestion.note).to receive(:latest_diff_file) { nil }
result = subject.execute(suggestion)
result = apply_service.new(user, suggestion).execute
expect(result).to eq(message: 'Suggestion is not appliable',
expect(result).to eq(message: 'A file was not found.',
status: :error)
end
end
context 'suggestion is eligible to be outdated' do
it 'returns error message' do
expect(suggestion).to receive(:outdated?) { true }
context 'when not all suggestions belong to the same branch' do
it 'renders error message' do
merge_request2 = create(:merge_request,
:conflict,
source_project: project,
target_project: project)
result = subject.execute(suggestion)
position2 = Gitlab::Diff::Position.new(old_path: "files/ruby/popen.rb",
new_path: "files/ruby/popen.rb",
old_line: nil,
new_line: 15,
diff_refs: merge_request2
.diff_refs)
expect(result).to eq(message: 'Suggestion is not appliable',
diff_note2 = create(:diff_note_on_merge_request,
noteable: merge_request2,
position: position2,
project: project)
other_branch_suggestion = create(:suggestion, note: diff_note2)
result = apply_service.new(user, suggestion, other_branch_suggestion).execute
expect(result).to eq(message: 'Suggestions must all be on the same branch.',
status: :error)
end
end
context 'suggestion was already applied' do
it 'returns success status' do
result = subject.execute(suggestion)
context 'suggestion is eligible to be outdated' do
it 'returns error message' do
expect(suggestion).to receive(:outdated?) { true }
expect(result[:status]).to eq(:success)
result = apply_service.new(user, suggestion).execute
expect(result).to eq(message: 'A suggestion is not applicable.',
status: :error)
end
end
......@@ -535,9 +626,9 @@ describe Suggestions::ApplyService do
end
it 'returns error message' do
result = subject.execute(suggestion)
result = apply_service.new(user, suggestion).execute
expect(result).to eq(message: 'Suggestion is not appliable',
expect(result).to eq(message: 'A suggestion is not applicable.',
status: :error)
end
end
......@@ -548,9 +639,27 @@ describe Suggestions::ApplyService do
end
it 'returns error message' do
result = subject.execute(suggestion)
result = apply_service.new(user, suggestion).execute
expect(result).to eq(message: 'A suggestion is not applicable.',
status: :error)
end
end
context 'lines of suggestions overlap' do
let(:suggestion) do
create_suggestion(
to_content: " raise RuntimeError, 'Explosion'\n # explosion?\n")
end
let(:overlapping_suggestion) do
create_suggestion(to_content: "I Overlap!")
end
it 'returns error message' do
result = apply_service.new(user, suggestion, overlapping_suggestion).execute
expect(result).to eq(message: 'Suggestion is not appliable',
expect(result).to eq(message: 'Suggestions are not applicable as their lines cannot overlap.',
status: :error)
end
end
......
spec/views/projects/edit.html.haml_spec.rb
View file @ c0417474
......@@ -32,26 +32,28 @@ describe 'projects/edit' do
it 'displays all possible variables' do
render
expect(rendered).to have_content('%{project_path}')
expect(rendered).to have_content('%{project_name}')
expect(rendered).to have_content('%{file_path}')
expect(rendered).to have_content('%{branch_name}')
expect(rendered).to have_content('%{username}')
expect(rendered).to have_content('%{files_count}')
expect(rendered).to have_content('%{file_paths}')
expect(rendered).to have_content('%{project_name}')
expect(rendered).to have_content('%{project_path}')
expect(rendered).to have_content('%{user_full_name}')
expect(rendered).to have_content('%{username}')
expect(rendered).to have_content('%{suggestions_count}')
end
it 'displays a placeholder if none is set' do
render
expect(rendered).to have_field('project[suggestion_commit_message]', placeholder: 'Apply suggestion to %{file_path}')
expect(rendered).to have_field('project[suggestion_commit_message]', placeholder: "Apply %{suggestions_count} suggestion(s) to %{files_count} file(s)")
end
it 'displays the user entered value' do
project.update!(suggestion_commit_message: 'refactor: changed %{file_path}')
project.update!(suggestion_commit_message: 'refactor: changed %{file_paths}')
render
expect(rendered).to have_field('project[suggestion_commit_message]', with: 'refactor: changed %{file_path}')
expect(rendered).to have_field('project[suggestion_commit_message]', with: 'refactor: changed %{file_paths}')
end
end
......
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7