diff options
96 files changed, 736 insertions, 1013 deletions
diff --git a/.gitlab/ci/rules.gitlab-ci.yml b/.gitlab/ci/rules.gitlab-ci.yml index e5b278f20b3..fa4a5d3e3d8 100644 --- a/.gitlab/ci/rules.gitlab-ci.yml +++ b/.gitlab/ci/rules.gitlab-ci.yml @@ -1057,6 +1057,8 @@ .reports:rules:package_hunter: rules: + - if: "$PACKAGE_HUNTER_USER == null || $PACKAGE_HUNTER_USER == ''" + when: never - <<: *if-default-branch-schedule-2-hourly - <<: *if-merge-request changes: ["yarn.lock"] diff --git a/app/assets/javascripts/environments/components/environments_app.vue b/app/assets/javascripts/environments/components/environments_app.vue index 1ceb3cb9828..e4cf5760987 100644 --- a/app/assets/javascripts/environments/components/environments_app.vue +++ b/app/assets/javascripts/environments/components/environments_app.vue @@ -135,7 +135,7 @@ export default { >{{ $options.i18n.newEnvironmentButtonLabel }}</gl-button > </div> - <gl-tabs content-class="gl-display-none"> + <gl-tabs :value="activeTab" content-class="gl-display-none"> <gl-tab v-for="(tab, idx) in tabs" :key="idx" diff --git a/app/assets/javascripts/environments/mixins/environments_mixin.js b/app/assets/javascripts/environments/mixins/environments_mixin.js index 280f5799503..c80839a117f 100644 --- a/app/assets/javascripts/environments/mixins/environments_mixin.js +++ b/app/assets/javascripts/environments/mixins/environments_mixin.js @@ -208,6 +208,9 @@ export default { }, ]; }, + activeTab() { + return this.tabs.findIndex(({ isActive }) => isActive) ?? 0; + }, }, /** diff --git a/app/assets/javascripts/issues_list/components/issues_list_app.vue b/app/assets/javascripts/issues_list/components/issues_list_app.vue index 921af766796..051512e9a66 100644 --- a/app/assets/javascripts/issues_list/components/issues_list_app.vue +++ b/app/assets/javascripts/issues_list/components/issues_list_app.vue @@ -16,7 +16,6 @@ import IssuableByEmail from '~/issuable/components/issuable_by_email.vue'; import IssuableList from '~/issuable_list/components/issuable_list_root.vue'; import { IssuableListTabs, IssuableStates } from '~/issuable_list/constants'; import { - API_PARAM, CREATED_DESC, i18n, initialPageParams, @@ -25,7 +24,7 @@ import { PARAM_DUE_DATE, PARAM_SORT, PARAM_STATE, - RELATIVE_POSITION_DESC, + RELATIVE_POSITION_ASC, TOKEN_TYPE_ASSIGNEE, TOKEN_TYPE_AUTHOR, TOKEN_TYPE_CONFIDENTIAL, @@ -36,12 +35,12 @@ import { TOKEN_TYPE_MILESTONE, TOKEN_TYPE_WEIGHT, UPDATED_DESC, - URL_PARAM, urlSortParams, } from '~/issues_list/constants'; import { - convertToParams, + convertToApiParams, convertToSearchQuery, + convertToUrlParams, getDueDateValue, getFilterTokens, getSortKey, @@ -192,10 +191,10 @@ export default { ...this.apiFilterParams, }; }, - update: ({ project }) => project.issues.nodes, + update: ({ project }) => project?.issues.nodes ?? [], result({ data }) { - this.pageInfo = data.project.issues.pageInfo; - this.totalIssues = data.project.issues.count; + this.pageInfo = data.project?.issues.pageInfo ?? {}; + this.totalIssues = data.project?.issues.count ?? 0; this.exportCsvPathWithQuery = this.getExportCsvPathWithQuery(); }, error(error) { @@ -215,32 +214,30 @@ export default { return this.showBulkEditSidebar || !this.issues.length; }, isManualOrdering() { - return this.sortKey === RELATIVE_POSITION_DESC; + return this.sortKey === RELATIVE_POSITION_ASC; }, isOpenTab() { return this.state === IssuableStates.Opened; }, apiFilterParams() { - return convertToParams(this.filterTokens, API_PARAM); + return convertToApiParams(this.filterTokens); }, urlFilterParams() { - return convertToParams(this.filterTokens, URL_PARAM); + return convertToUrlParams(this.filterTokens); }, searchQuery() { return convertToSearchQuery(this.filterTokens) || undefined; }, searchTokens() { - let preloadedAuthors = []; + const preloadedAuthors = []; if (gon.current_user_id) { - preloadedAuthors = [ - { - id: gon.current_user_id, - name: gon.current_user_fullname, - username: gon.current_username, - avatar_url: gon.current_user_avatar_url, - }, - ]; + preloadedAuthors.push({ + id: gon.current_user_id, + name: gon.current_user_fullname, + username: gon.current_username, + avatar_url: gon.current_user_avatar_url, + }); } const tokens = [ @@ -252,6 +249,7 @@ export default { dataType: 'user', unique: true, defaultAuthors: [], + operators: OPERATOR_IS_ONLY, fetchAuthors: this.fetchUsers, preloadedAuthors, }, @@ -280,7 +278,7 @@ export default { title: TOKEN_TITLE_LABEL, icon: 'labels', token: LabelToken, - defaultLabels: [], + defaultLabels: DEFAULT_NONE_ANY, fetchLabels: this.fetchLabels, }, ]; @@ -329,6 +327,7 @@ export default { token: EpicToken, unique: true, idProperty: 'id', + useIdValue: true, fetchEpics: this.fetchEpics, }); } @@ -346,7 +345,7 @@ export default { return tokens; }, showPaginationControls() { - return this.issues.length > 0; + return this.issues.length > 0 && (this.pageInfo.hasNextPage || this.pageInfo.hasPreviousPage); }, sortOptions() { return getSortOptions(this.hasIssueWeightsFeature, this.hasBlockedIssuesFeature); @@ -361,22 +360,12 @@ export default { ); }, urlParams() { - const filterParams = { - ...this.urlFilterParams, - }; - - if (filterParams.epic_id) { - filterParams.epic_id = encodeURIComponent(filterParams.epic_id); - } else if (filterParams['not[epic_id]']) { - filterParams['not[epic_id]'] = encodeURIComponent(filterParams['not[epic_id]']); - } - return { due_date: this.dueDateFilter, search: this.searchQuery, + sort: urlSortParams[this.sortKey], state: this.state, - ...urlSortParams[this.sortKey], - ...filterParams, + ...this.urlFilterParams, }; }, }, @@ -424,7 +413,10 @@ export default { return this.fetchWithCache(this.projectMilestonesPath, 'milestones', 'title', search, true); }, fetchIterations(search) { - return axios.get(this.projectIterationsPath, { params: { search } }); + const id = Number(search); + return !search || Number.isNaN(id) + ? axios.get(this.projectIterationsPath, { params: { search } }) + : axios.get(this.projectIterationsPath, { params: { id } }); }, fetchUsers(search) { return axios.get(this.autocompleteUsersPath, { params: { search } }); @@ -471,6 +463,7 @@ export default { this.state = state; }, handleFilter(filter) { + this.pageParams = initialPageParams; this.filterTokens = filter; }, handleNextPage() { diff --git a/app/assets/javascripts/issues_list/constants.js b/app/assets/javascripts/issues_list/constants.js index 76006f9081d..6337ccc4926 100644 --- a/app/assets/javascripts/issues_list/constants.js +++ b/app/assets/javascripts/issues_list/constants.js @@ -128,21 +128,21 @@ export const CREATED_ASC = 'CREATED_ASC'; export const CREATED_DESC = 'CREATED_DESC'; export const DUE_DATE_ASC = 'DUE_DATE_ASC'; export const DUE_DATE_DESC = 'DUE_DATE_DESC'; +export const LABEL_PRIORITY_ASC = 'LABEL_PRIORITY_ASC'; export const LABEL_PRIORITY_DESC = 'LABEL_PRIORITY_DESC'; export const MILESTONE_DUE_ASC = 'MILESTONE_DUE_ASC'; export const MILESTONE_DUE_DESC = 'MILESTONE_DUE_DESC'; export const POPULARITY_ASC = 'POPULARITY_ASC'; export const POPULARITY_DESC = 'POPULARITY_DESC'; +export const PRIORITY_ASC = 'PRIORITY_ASC'; export const PRIORITY_DESC = 'PRIORITY_DESC'; -export const RELATIVE_POSITION_DESC = 'RELATIVE_POSITION_DESC'; +export const RELATIVE_POSITION_ASC = 'RELATIVE_POSITION_ASC'; export const UPDATED_ASC = 'UPDATED_ASC'; export const UPDATED_DESC = 'UPDATED_DESC'; export const WEIGHT_ASC = 'WEIGHT_ASC'; export const WEIGHT_DESC = 'WEIGHT_DESC'; -const SORT_ASC = 'asc'; -const SORT_DESC = 'desc'; - +const PRIORITY_ASC_SORT = 'priority_asc'; const CREATED_DATE_SORT = 'created_date'; const CREATED_ASC_SORT = 'created_asc'; const UPDATED_DESC_SORT = 'updated_desc'; @@ -150,129 +150,30 @@ const UPDATED_ASC_SORT = 'updated_asc'; const MILESTONE_SORT = 'milestone'; const MILESTONE_DUE_DESC_SORT = 'milestone_due_desc'; const DUE_DATE_DESC_SORT = 'due_date_desc'; +const LABEL_PRIORITY_ASC_SORT = 'label_priority_asc'; const POPULARITY_ASC_SORT = 'popularity_asc'; const WEIGHT_DESC_SORT = 'weight_desc'; const BLOCKING_ISSUES_DESC_SORT = 'blocking_issues_desc'; -const BLOCKING_ISSUES = 'blocking_issues'; - -export const apiSortParams = { - [PRIORITY_DESC]: { - order_by: PRIORITY, - sort: SORT_DESC, - }, - [CREATED_ASC]: { - order_by: CREATED_AT, - sort: SORT_ASC, - }, - [CREATED_DESC]: { - order_by: CREATED_AT, - sort: SORT_DESC, - }, - [UPDATED_ASC]: { - order_by: UPDATED_AT, - sort: SORT_ASC, - }, - [UPDATED_DESC]: { - order_by: UPDATED_AT, - sort: SORT_DESC, - }, - [MILESTONE_DUE_ASC]: { - order_by: MILESTONE_DUE, - sort: SORT_ASC, - }, - [MILESTONE_DUE_DESC]: { - order_by: MILESTONE_DUE, - sort: SORT_DESC, - }, - [DUE_DATE_ASC]: { - order_by: DUE_DATE, - sort: SORT_ASC, - }, - [DUE_DATE_DESC]: { - order_by: DUE_DATE, - sort: SORT_DESC, - }, - [POPULARITY_ASC]: { - order_by: POPULARITY, - sort: SORT_ASC, - }, - [POPULARITY_DESC]: { - order_by: POPULARITY, - sort: SORT_DESC, - }, - [LABEL_PRIORITY_DESC]: { - order_by: LABEL_PRIORITY, - sort: SORT_DESC, - }, - [RELATIVE_POSITION_DESC]: { - order_by: RELATIVE_POSITION, - per_page: 100, - sort: SORT_ASC, - }, - [WEIGHT_ASC]: { - order_by: WEIGHT, - sort: SORT_ASC, - }, - [WEIGHT_DESC]: { - order_by: WEIGHT, - sort: SORT_DESC, - }, - [BLOCKING_ISSUES_DESC]: { - order_by: BLOCKING_ISSUES, - sort: SORT_DESC, - }, -}; export const urlSortParams = { - [PRIORITY_DESC]: { - sort: PRIORITY, - }, - [CREATED_ASC]: { - sort: CREATED_ASC_SORT, - }, - [CREATED_DESC]: { - sort: CREATED_DATE_SORT, - }, - [UPDATED_ASC]: { - sort: UPDATED_ASC_SORT, - }, - [UPDATED_DESC]: { - sort: UPDATED_DESC_SORT, - }, - [MILESTONE_DUE_ASC]: { - sort: MILESTONE_SORT, - }, - [MILESTONE_DUE_DESC]: { - sort: MILESTONE_DUE_DESC_SORT, - }, - [DUE_DATE_ASC]: { - sort: DUE_DATE, - }, - [DUE_DATE_DESC]: { - sort: DUE_DATE_DESC_SORT, - }, - [POPULARITY_ASC]: { - sort: POPULARITY_ASC_SORT, - }, - [POPULARITY_DESC]: { - sort: POPULARITY, - }, - [LABEL_PRIORITY_DESC]: { - sort: LABEL_PRIORITY, - }, - [RELATIVE_POSITION_DESC]: { - sort: RELATIVE_POSITION, - per_page: 100, - }, - [WEIGHT_ASC]: { - sort: WEIGHT, - }, - [WEIGHT_DESC]: { - sort: WEIGHT_DESC_SORT, - }, - [BLOCKING_ISSUES_DESC]: { - sort: BLOCKING_ISSUES_DESC_SORT, - }, + [PRIORITY_ASC]: PRIORITY_ASC_SORT, + [PRIORITY_DESC]: PRIORITY, + [CREATED_ASC]: CREATED_ASC_SORT, + [CREATED_DESC]: CREATED_DATE_SORT, + [UPDATED_ASC]: UPDATED_ASC_SORT, + [UPDATED_DESC]: UPDATED_DESC_SORT, + [MILESTONE_DUE_ASC]: MILESTONE_SORT, + [MILESTONE_DUE_DESC]: MILESTONE_DUE_DESC_SORT, + [DUE_DATE_ASC]: DUE_DATE, + [DUE_DATE_DESC]: DUE_DATE_DESC_SORT, + [POPULARITY_ASC]: POPULARITY_ASC_SORT, + [POPULARITY_DESC]: POPULARITY, + [LABEL_PRIORITY_ASC]: LABEL_PRIORITY_ASC_SORT, + [LABEL_PRIORITY_DESC]: LABEL_PRIORITY, + [RELATIVE_POSITION_ASC]: RELATIVE_POSITION, + [WEIGHT_ASC]: WEIGHT, + [WEIGHT_DESC]: WEIGHT_DESC_SORT, + [BLOCKING_ISSUES_DESC]: BLOCKING_ISSUES_DESC_SORT, }; export const MAX_LIST_SIZE = 10; @@ -297,12 +198,7 @@ export const TOKEN_TYPE_WEIGHT = 'weight'; export const filters = { [TOKEN_TYPE_AUTHOR]: { [API_PARAM]: { - [OPERATOR_IS]: { - [NORMAL_FILTER]: 'author_username', - }, - [OPERATOR_IS_NOT]: { - [NORMAL_FILTER]: 'not[author_username]', - }, + [NORMAL_FILTER]: 'authorUsername', }, [URL_PARAM]: { [OPERATOR_IS]: { @@ -315,13 +211,8 @@ export const filters = { }, [TOKEN_TYPE_ASSIGNEE]: { [API_PARAM]: { - [OPERATOR_IS]: { - [NORMAL_FILTER]: 'assignee_username', - [SPECIAL_FILTER]: 'assignee_id', - }, - [OPERATOR_IS_NOT]: { - [NORMAL_FILTER]: 'not[assignee_username]', - }, + [NORMAL_FILTER]: 'assigneeUsernames', + [SPECIAL_FILTER]: 'assigneeId', }, [URL_PARAM]: { [OPERATOR_IS]: { @@ -336,12 +227,7 @@ export const filters = { }, [TOKEN_TYPE_MILESTONE]: { [API_PARAM]: { - [OPERATOR_IS]: { - [NORMAL_FILTER]: 'milestone', - }, - [OPERATOR_IS_NOT]: { - [NORMAL_FILTER]: 'not[milestone]', - }, + [NORMAL_FILTER]: 'milestoneTitle', }, [URL_PARAM]: { [OPERATOR_IS]: { @@ -354,16 +240,13 @@ export const filters = { }, [TOKEN_TYPE_LABEL]: { [API_PARAM]: { - [OPERATOR_IS]: { - [NORMAL_FILTER]: 'labels', - }, - [OPERATOR_IS_NOT]: { - [NORMAL_FILTER]: 'not[labels]', - }, + [NORMAL_FILTER]: 'labelName', + [SPECIAL_FILTER]: 'labelName', }, [URL_PARAM]: { [OPERATOR_IS]: { [NORMAL_FILTER]: 'label_name[]', + [SPECIAL_FILTER]: 'label_name[]', }, [OPERATOR_IS_NOT]: { [NORMAL_FILTER]: 'not[label_name][]', @@ -372,10 +255,8 @@ export const filters = { }, [TOKEN_TYPE_MY_REACTION]: { [API_PARAM]: { - [OPERATOR_IS]: { - [NORMAL_FILTER]: 'my_reaction_emoji', - [SPECIAL_FILTER]: 'my_reaction_emoji', - }, + [NORMAL_FILTER]: 'myReactionEmoji', + [SPECIAL_FILTER]: 'myReactionEmoji', }, [URL_PARAM]: { [OPERATOR_IS]: { @@ -386,9 +267,7 @@ export const filters = { }, [TOKEN_TYPE_CONFIDENTIAL]: { [API_PARAM]: { - [OPERATOR_IS]: { - [NORMAL_FILTER]: 'confidential', - }, + [NORMAL_FILTER]: 'confidential', }, [URL_PARAM]: { [OPERATOR_IS]: { @@ -398,33 +277,23 @@ export const filters = { }, [TOKEN_TYPE_ITERATION]: { [API_PARAM]: { - [OPERATOR_IS]: { - [NORMAL_FILTER]: 'iteration_title', - [SPECIAL_FILTER]: 'iteration_id', - }, - [OPERATOR_IS_NOT]: { - [NORMAL_FILTER]: 'not[iteration_title]', - }, + [NORMAL_FILTER]: 'iterationId', + [SPECIAL_FILTER]: 'iterationWildcardId', }, [URL_PARAM]: { [OPERATOR_IS]: { - [NORMAL_FILTER]: 'iteration_title', + [NORMAL_FILTER]: 'iteration_id', [SPECIAL_FILTER]: 'iteration_id', }, [OPERATOR_IS_NOT]: { - [NORMAL_FILTER]: 'not[iteration_title]', + [NORMAL_FILTER]: 'not[iteration_id]', }, }, }, [TOKEN_TYPE_EPIC]: { [API_PARAM]: { - [OPERATOR_IS]: { - [NORMAL_FILTER]: 'epic_id', - [SPECIAL_FILTER]: 'epic_id', - }, - [OPERATOR_IS_NOT]: { - [NORMAL_FILTER]: 'not[epic_id]', - }, + [NORMAL_FILTER]: 'epicId', + [SPECIAL_FILTER]: 'epicId', }, [URL_PARAM]: { [OPERATOR_IS]: { @@ -438,13 +307,8 @@ export const filters = { }, [TOKEN_TYPE_WEIGHT]: { [API_PARAM]: { - [OPERATOR_IS]: { - [NORMAL_FILTER]: 'weight', - [SPECIAL_FILTER]: 'weight', - }, - [OPERATOR_IS_NOT]: { - [NORMAL_FILTER]: 'not[weight]', - }, + [NORMAL_FILTER]: 'weight', + [SPECIAL_FILTER]: 'weight', }, [URL_PARAM]: { [OPERATOR_IS]: { diff --git a/app/assets/javascripts/issues_list/utils.js b/app/assets/javascripts/issues_list/utils.js index b5ec44198da..49f937cc453 100644 --- a/app/assets/javascripts/issues_list/utils.js +++ b/app/assets/javascripts/issues_list/utils.js @@ -1,4 +1,5 @@ import { + API_PARAM, BLOCKING_ISSUES_DESC, CREATED_ASC, CREATED_DESC, @@ -6,29 +7,36 @@ import { DUE_DATE_DESC, DUE_DATE_VALUES, filters, + LABEL_PRIORITY_ASC, LABEL_PRIORITY_DESC, MILESTONE_DUE_ASC, MILESTONE_DUE_DESC, NORMAL_FILTER, POPULARITY_ASC, POPULARITY_DESC, + PRIORITY_ASC, PRIORITY_DESC, - RELATIVE_POSITION_DESC, + RELATIVE_POSITION_ASC, SPECIAL_FILTER, SPECIAL_FILTER_VALUES, TOKEN_TYPE_ASSIGNEE, + TOKEN_TYPE_ITERATION, UPDATED_ASC, UPDATED_DESC, + URL_PARAM, urlSortParams, WEIGHT_ASC, WEIGHT_DESC, } from '~/issues_list/constants'; import { isPositiveInteger } from '~/lib/utils/number_utils'; import { __ } from '~/locale'; -import { FILTERED_SEARCH_TERM } from '~/vue_shared/components/filtered_search_bar/constants'; +import { + FILTERED_SEARCH_TERM, + OPERATOR_IS_NOT, +} from '~/vue_shared/components/filtered_search_bar/constants'; export const getSortKey = (sort) => - Object.keys(urlSortParams).find((key) => urlSortParams[key].sort === sort); + Object.keys(urlSortParams).find((key) => urlSortParams[key] === sort); export const getDueDateValue = (value) => (DUE_DATE_VALUES.includes(value) ? value : undefined); @@ -38,7 +46,7 @@ export const getSortOptions = (hasIssueWeightsFeature, hasBlockedIssuesFeature) id: 1, title: __('Priority'), sortDirection: { - ascending: PRIORITY_DESC, + ascending: PRIORITY_ASC, descending: PRIORITY_DESC, }, }, @@ -86,7 +94,7 @@ export const getSortOptions = (hasIssueWeightsFeature, hasBlockedIssuesFeature) id: 7, title: __('Label priority'), sortDirection: { - ascending: LABEL_PRIORITY_DESC, + ascending: LABEL_PRIORITY_ASC, descending: LABEL_PRIORITY_DESC, }, }, @@ -94,8 +102,8 @@ export const getSortOptions = (hasIssueWeightsFeature, hasBlockedIssuesFeature) id: 8, title: __('Manual'), sortDirection: { - ascending: RELATIVE_POSITION_DESC, - descending: RELATIVE_POSITION_DESC, + ascending: RELATIVE_POSITION_ASC, + descending: RELATIVE_POSITION_ASC, }, }, ]; @@ -128,7 +136,7 @@ export const getSortOptions = (hasIssueWeightsFeature, hasBlockedIssuesFeature) const tokenTypes = Object.keys(filters); const getUrlParams = (tokenType) => - Object.values(filters[tokenType].urlParam).flatMap((filterObj) => Object.values(filterObj)); + Object.values(filters[tokenType][URL_PARAM]).flatMap((filterObj) => Object.values(filterObj)); const urlParamKeys = tokenTypes.flatMap(getUrlParams); @@ -136,7 +144,7 @@ const getTokenTypeFromUrlParamKey = (urlParamKey) => tokenTypes.find((tokenType) => getUrlParams(tokenType).includes(urlParamKey)); const getOperatorFromUrlParamKey = (tokenType, urlParamKey) => - Object.entries(filters[tokenType].urlParam).find(([, filterObj]) => + Object.entries(filters[tokenType][URL_PARAM]).find(([, filterObj]) => Object.values(filterObj).includes(urlParamKey), )[0]; @@ -178,12 +186,36 @@ const getFilterType = (data, tokenType = '') => ? SPECIAL_FILTER : NORMAL_FILTER; -export const convertToParams = (filterTokens, paramType) => +const isIterationSpecialValue = (tokenType, value) => + tokenType === TOKEN_TYPE_ITERATION && SPECIAL_FILTER_VALUES.includes(value); + +export const convertToApiParams = (filterTokens) => { + const params = {}; + const not = {}; + + filterTokens + .filter((token) => token.type !== FILTERED_SEARCH_TERM) + .forEach((token) => { + const filterType = getFilterType(token.value.data, token.type); + const field = filters[token.type][API_PARAM][filterType]; + const obj = token.value.operator === OPERATOR_IS_NOT ? not : params; + const data = isIterationSpecialValue(token.type, token.value.data) + ? token.value.data.toUpperCase() + : token.value.data; + Object.assign(obj, { + [field]: obj[field] ? [obj[field], data].flat() : data, + }); + }); + + return Object.keys(not).length ? Object.assign(params, { not }) : params; +}; + +export const convertToUrlParams = (filterTokens) => filterTokens .filter((token) => token.type !== FILTERED_SEARCH_TERM) .reduce((acc, token) => { const filterType = getFilterType(token.value.data, token.type); - const param = filters[token.type][paramType][token.value.operator]?.[filterType]; + const param = filters[token.type][URL_PARAM][token.value.operator]?.[filterType]; return Object.assign(acc, { [param]: acc[param] ? [acc[param], token.value.data].flat() : token.value.data, }); diff --git a/app/assets/javascripts/nav/components/top_nav_dropdown_menu.vue b/app/assets/javascripts/nav/components/top_nav_dropdown_menu.vue index cac8fecb6b1..97856eaf256 100644 --- a/app/assets/javascripts/nav/components/top_nav_dropdown_menu.vue +++ b/app/assets/javascripts/nav/components/top_nav_dropdown_menu.vue @@ -72,7 +72,7 @@ export default { <template> <div class="gl-display-flex gl-align-items-stretch"> <div - class="gl-w-grid-size-30 gl-flex-shrink-0 gl-bg-gray-10 gl-py-3 gl-px-5" + class="gl-w-grid-size-30 gl-flex-shrink-0 gl-bg-gray-10 gl-p-3" :class="menuClass" data-testid="menu-sidebar" > @@ -81,7 +81,7 @@ export default { <keep-alive-slots v-show="activeView" :slot-key="activeView" - class="gl-w-grid-size-40 gl-overflow-hidden gl-py-3 gl-px-5" + class="gl-w-grid-size-40 gl-overflow-hidden gl-p-3" data-testid="menu-subview" data-qa-selector="menu_subview_container" > diff --git a/app/assets/javascripts/vue_shared/components/filtered_search_bar/tokens/epic_token.vue b/app/assets/javascripts/vue_shared/components/filtered_search_bar/tokens/epic_token.vue index d21fa9a344a..5b05187955c 100644 --- a/app/assets/javascripts/vue_shared/components/filtered_search_bar/tokens/epic_token.vue +++ b/app/assets/javascripts/vue_shared/components/filtered_search_bar/tokens/epic_token.vue @@ -56,7 +56,7 @@ export default { } // Current value is a string. - const [groupPath, idProperty] = this.currentValue?.split('::&'); + const [groupPath, idProperty] = this.currentValue?.split(this.$options.separator); return this.epics.find( (epic) => epic.group_full_path === groupPath && @@ -65,6 +65,9 @@ export default { } return null; }, + displayText() { + return `${this.activeEpic?.title}${this.$options.separator}${this.activeEpic?.iid}`; + }, }, watch: { active: { @@ -103,8 +106,10 @@ export default { this.fetchEpicsBySearchTerm({ epicPath, search: data }); }, DEBOUNCE_DELAY), - getEpicDisplayText(epic) { - return `${epic.title}${this.$options.separator}${epic.iid}`; + getValue(epic) { + return this.config.useIdValue + ? String(epic[this.idProperty]) + : `${epic.group_full_path}${this.$options.separator}${epic[this.idProperty]}`; }, }, }; @@ -118,7 +123,7 @@ export default { @input="searchEpics" > <template #view="{ inputValue }"> - {{ activeEpic ? getEpicDisplayText(activeEpic) : inputValue }} + {{ activeEpic ? displayText : inputValue }} </template> <template #suggestions> <gl-filtered-search-suggestion @@ -131,11 +136,7 @@ export default { <gl-dropdown-divider v-if="defaultEpics.length" /> <gl-loading-icon v-if="loading" /> <template v-else> - <gl-filtered-search-suggestion - v-for="epic in epics" - :key="epic.id" - :value="`${epic.group_full_path}::&${epic[idProperty]}`" - > + <gl-filtered-search-suggestion v-for="epic in epics" :key="epic.id" :value="getValue(epic)"> {{ epic.title }} </gl-filtered-search-suggestion> </template> diff --git a/app/assets/javascripts/vue_shared/components/filtered_search_bar/tokens/iteration_token.vue b/app/assets/javascripts/vue_shared/components/filtered_search_bar/tokens/iteration_token.vue index 7b6a590279a..cdb04ea79e1 100644 --- a/app/assets/javascripts/vue_shared/components/filtered_search_bar/tokens/iteration_token.vue +++ b/app/assets/javascripts/vue_shared/components/filtered_search_bar/tokens/iteration_token.vue @@ -30,7 +30,6 @@ export default { data() { return { iterations: this.config.initialIterations || [], - defaultIterations: this.config.defaultIterations || DEFAULT_ITERATIONS, loading: true, }; }, @@ -39,7 +38,10 @@ export default { return this.value.data; }, activeIteration() { - return this.iterations.find((iteration) => iteration.title === this.currentValue); + return this.iterations.find((iteration) => iteration.id === Number(this.currentValue)); + }, + defaultIterations() { + return this.config.defaultIterations || DEFAULT_ITERATIONS; }, }, watch: { @@ -99,8 +101,8 @@ export default { <template v-else> <gl-filtered-search-suggestion v-for="iteration in iterations" - :key="iteration.title" - :value="iteration.title" + :key="iteration.id" + :value="String(iteration.id)" > {{ iteration.title }} </gl-filtered-search-suggestion> diff --git a/app/assets/javascripts/vue_shared/components/user_select/user_select.vue b/app/assets/javascripts/vue_shared/components/user_select/user_select.vue index 04e44aa2ed1..b85cae0c64f 100644 --- a/app/assets/javascripts/vue_shared/components/user_select/user_select.vue +++ b/app/assets/javascripts/vue_shared/components/user_select/user_select.vue @@ -96,9 +96,6 @@ export default { }, }, searchUsers: { - // TODO Remove error policy - // https://gitlab.com/gitlab-org/gitlab/-/issues/329750 - errorPolicy: 'all', query: searchUsers, variables() { return { @@ -111,28 +108,10 @@ export default { return !this.isEditing; }, update(data) { - // TODO Remove null filter (BE fix required) - // https://gitlab.com/gitlab-org/gitlab/-/issues/329750 return data.workspace?.users?.nodes.filter((x) => x?.user).map(({ user }) => user) || []; }, debounce: ASSIGNEES_DEBOUNCE_DELAY, - error({ graphQLErrors }) { - // TODO This error suppression is temporary (BE fix required) - // https://gitlab.com/gitlab-org/gitlab/-/issues/329750 - const isNullError = ({ message }) => { - return message === 'Cannot return null for non-nullable field GroupMember.user'; - }; - - if (graphQLErrors?.length > 0 && graphQLErrors.every(isNullError)) { - // only null-related errors exist, suppress them. - // eslint-disable-next-line no-console - console.error( - "Suppressing the error 'Cannot return null for non-nullable field GroupMember.user'. Please see https://gitlab.com/gitlab-org/gitlab/-/issues/329750", - ); - this.isSearching = false; - return; - } - + error() { this.$emit('error'); this.isSearching = false; }, diff --git a/app/controllers/boards/issues_controller.rb b/app/controllers/boards/issues_controller.rb index 003ed45adb5..f0f074792ed 100644 --- a/app/controllers/boards/issues_controller.rb +++ b/app/controllers/boards/issues_controller.rb @@ -136,7 +136,7 @@ module Boards def issue_params params.require(:issue) .permit(:title, :milestone_id, :project_id) - .merge(board_id: params[:board_id], list_id: params[:list_id], request: request) + .merge(board_id: params[:board_id], list_id: params[:list_id]) end def serializer diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index 9e861d2859d..5f47d9fadef 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -47,31 +47,20 @@ module SpammableActions end end - def spammable_params - # NOTE: For the legacy reCAPTCHA implementation based on the HTML/HAML form, the - # 'g-recaptcha-response' field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the - # recaptcha gem, which is called from the HAML `_recaptcha_form.html.haml` form. - # - # It is used in the `Recaptcha::Verify#verify_recaptcha` to extract the value from `params`, - # if the `response` option is not passed explicitly. - # - # Instead of relying on this behavior, we are extracting and passing it explicitly. This will - # make it consistent with the newer, modern reCAPTCHA verification process as it will be - # implemented via the GraphQL API and in Vue components via the native reCAPTCHA Javascript API, - # which requires that the recaptcha response param be obtained and passed explicitly. - # - # It can also be expanded to multiple fields when we move to future alternative captcha - # implementations such as FriendlyCaptcha. See https://gitlab.com/gitlab-org/gitlab/-/issues/273480 - - # After this newer GraphQL/JS API process is fully supported by the backend, we can remove the - # check for the 'g-recaptcha-response' field and other HTML/HAML form-specific support. - captcha_response = params['g-recaptcha-response'] || params[:captcha_response] - - { - request: request, - spam_log_id: params[:spam_log_id], - captcha_response: captcha_response - } + # TODO: This method is currently only needed for issue create and update. It can be removed when: + # + # 1. Issue create is is converted to a client/JS based approach instead of the legacy HAML + # `_recaptcha_form.html.haml` which is rendered via the `projects/issues/verify` template. + # In this case, which is based on the legacy reCAPTCHA implementation using the HTML/HAML form, + # the 'g-recaptcha-response' field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the + # recaptcha gem, which is called from the HAML `_recaptcha_form.html.haml` form. + # 2. Issue update is converted to use the headers-based approach, which will require adding + # support to captcha_modal_axios_interceptor.js like we have already added to + # apollo_captcha_link.js. + # In this case, the `captcha_response` field name comes from our captcha_modal_axios_interceptor.js. + def extract_legacy_spam_params_to_headers + request.headers['X-GitLab-Captcha-Response'] = params['g-recaptcha-response'] || params[:captcha_response] + request.headers['X-GitLab-Spam-Log-Id'] = params[:spam_log_id] end def spammable diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index ea473f9b1a2..0bb0228cfea 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -129,12 +129,14 @@ class Projects::IssuesController < Projects::ApplicationController end def create - create_params = issue_params.merge(spammable_params).merge( + extract_legacy_spam_params_to_headers + create_params = issue_params.merge( merge_request_to_resolve_discussions_of: params[:merge_request_to_resolve_discussions_of], discussion_to_resolve: params[:discussion_to_resolve] ) - service = ::Issues::CreateService.new(project: project, current_user: current_user, params: create_params) + spam_params = ::Spam::SpamParams.new_from_request(request: request) + service = ::Issues::CreateService.new(project: project, current_user: current_user, params: create_params, spam_params: spam_params) @issue = service.execute create_vulnerability_issue_feedback(issue) @@ -334,8 +336,9 @@ class Projects::IssuesController < Projects::ApplicationController end def update_service - update_params = issue_params.merge(spammable_params) - ::Issues::UpdateService.new(project: project, current_user: current_user, params: update_params) + extract_legacy_spam_params_to_headers + spam_params = ::Spam::SpamParams.new_from_request(request: request) + ::Issues::UpdateService.new(project: project, current_user: current_user, params: issue_params, spam_params: spam_params) end def finder_type diff --git a/app/controllers/projects/usage_ping_controller.rb b/app/controllers/projects/service_ping_controller.rb index 77ee53f2e5d..e211ab3de0b 100644 --- a/app/controllers/projects/usage_ping_controller.rb +++ b/app/controllers/projects/service_ping_controller.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class Projects::UsagePingController < Projects::ApplicationController +class Projects::ServicePingController < Projects::ApplicationController before_action :authenticate_user! feature_category :usage_ping diff --git a/app/graphql/mutations/concerns/mutations/spam_protection.rb b/app/graphql/mutations/concerns/mutations/spam_protection.rb index d765da23a4b..341067710b2 100644 --- a/app/graphql/mutations/concerns/mutations/spam_protection.rb +++ b/app/graphql/mutations/concerns/mutations/spam_protection.rb @@ -16,25 +16,6 @@ module Mutations private - # additional_spam_params -> hash - # - # Used from a spammable mutation's #resolve method to generate - # the required additional spam/CAPTCHA params which must be merged into the params - # passed to the constructor of a service, where they can then be used in the service - # to perform spam checking via SpamActionService. - # - # Also accesses the #context of the mutation's Resolver superclass to obtain the request. - # - # Example: - # - # existing_args.merge!(additional_spam_params) - def additional_spam_params - { - api: true, - request: context[:request] - } - end - def spam_action_response(object) fields = spam_action_response_fields(object) diff --git a/app/graphql/mutations/issues/create.rb b/app/graphql/mutations/issues/create.rb index 3a57e2434a5..7c4a851f8aa 100644 --- a/app/graphql/mutations/issues/create.rb +++ b/app/graphql/mutations/issues/create.rb @@ -73,7 +73,8 @@ module Mutations project = authorized_find!(project_path) params = build_create_issue_params(attributes.merge(author_id: current_user.id)) - issue = ::Issues::CreateService.new(project: project, current_user: current_user, params: params).execute + spam_params = ::Spam::SpamParams.new_from_request(request: context[:request]) + issue = ::Issues::CreateService.new(project: project, current_user: current_user, params: params, spam_params: spam_params).execute if issue.spam? issue.errors.add(:base, 'Spam detected.') diff --git a/app/graphql/mutations/issues/set_confidential.rb b/app/graphql/mutations/issues/set_confidential.rb index 8e88b31d9ed..4f2ca7e029c 100644 --- a/app/graphql/mutations/issues/set_confidential.rb +++ b/app/graphql/mutations/issues/set_confidential.rb @@ -13,8 +13,11 @@ module Mutations def resolve(project_path:, iid:, confidential:) issue = authorized_find!(project_path: project_path, iid: iid) project = issue.project + # Changing confidentiality affects spam checking rules, therefore we need to provide + # spam_params so a check can be performed. + spam_params = ::Spam::SpamParams.new_from_request(request: context[:request]) - ::Issues::UpdateService.new(project: project, current_user: current_user, params: { confidential: confidential }) + ::Issues::UpdateService.new(project: project, current_user: current_user, params: { confidential: confidential }, spam_params: spam_params) .execute(issue) { diff --git a/app/graphql/mutations/issues/update.rb b/app/graphql/mutations/issues/update.rb index eb16b7b38d0..1ceed868a6c 100644 --- a/app/graphql/mutations/issues/update.rb +++ b/app/graphql/mutations/issues/update.rb @@ -31,7 +31,8 @@ module Mutations issue = authorized_find!(project_path: project_path, iid: iid) project = issue.project - ::Issues::UpdateService.new(project: project, current_user: current_user, params: args).execute(issue) + spam_params = ::Spam::SpamParams.new_from_request(request: context[:request]) + ::Issues::UpdateService.new(project: project, current_user: current_user, params: args, spam_params: spam_params).execute(issue) { issue: issue, diff --git a/app/graphql/mutations/snippets/create.rb b/app/graphql/mutations/snippets/create.rb index d1ad0697acd..765163e73a1 100644 --- a/app/graphql/mutations/snippets/create.rb +++ b/app/graphql/mutations/snippets/create.rb @@ -49,7 +49,9 @@ module Mutations process_args_for_params!(args) - service_response = ::Snippets::CreateService.new(project: project, current_user: current_user, params: args).execute + spam_params = ::Spam::SpamParams.new_from_request(request: context[:request]) + service = ::Snippets::CreateService.new(project: project, current_user: current_user, params: args, spam_params: spam_params) + service_response = service.execute # Only when the user is not an api user and the operation was successful if !api_user? && service_response.success? @@ -81,12 +83,6 @@ module Mutations # it's the expected key param args[:files] = args.delete(:uploaded_files) - if Feature.enabled?(:snippet_spam) - args.merge!(additional_spam_params) - else - args[:disable_spam_action_service] = true - end - # Return nil to make it explicit that this method is mutating the args parameter, and that # the return value is not relevant and is not to be used. nil diff --git a/app/graphql/mutations/snippets/update.rb b/app/graphql/mutations/snippets/update.rb index 2e1382e1cb1..792c631e5ca 100644 --- a/app/graphql/mutations/snippets/update.rb +++ b/app/graphql/mutations/snippets/update.rb @@ -34,7 +34,9 @@ module Mutations process_args_for_params!(args) - service_response = ::Snippets::UpdateService.new(project: snippet.project, current_user: current_user, params: args).execute(snippet) + spam_params = ::Spam::SpamParams.new_from_request(request: context[:request]) + service = ::Snippets::UpdateService.new(project: snippet.project, current_user: current_user, params: args, spam_params: spam_params) + service_response = service.execute(snippet) # TODO: DRY this up - From here down, this is all duplicated with Mutations::Snippets::Create#resolve, except for # `snippet.reset`, which is required in order to return the object in its non-dirty, unmodified, database state @@ -62,12 +64,6 @@ module Mutations def process_args_for_params!(args) convert_blob_actions_to_snippet_actions!(args) - if Feature.enabled?(:snippet_spam) - args.merge!(additional_spam_params) - else - args[:disable_spam_action_service] = true - end - # Return nil to make it explicit that this method is mutating the args parameter, and that # the return value is not relevant and is not to be used. nil diff --git a/app/mailers/emails/service_desk.rb b/app/mailers/emails/service_desk.rb index e8034ef9b57..66eb2c646a9 100644 --- a/app/mailers/emails/service_desk.rb +++ b/app/mailers/emails/service_desk.rb @@ -20,9 +20,7 @@ module Emails options = service_desk_options(email_sender, 'thank_you', @issue.external_author) .merge(subject: "Re: #{subject_base}") - mail_new_thread(@issue, options).tap do - Gitlab::Metrics::BackgroundTransaction.current&.add_event(:service_desk_thank_you_email) - end + mail_new_thread(@issue, options) end def service_desk_new_note_email(issue_id, note_id, recipient) @@ -33,9 +31,7 @@ module Emails options = service_desk_options(email_sender, 'new_note', recipient) .merge(subject: subject_base) - mail_answer_thread(@issue, options).tap do - Gitlab::Metrics::BackgroundTransaction.current&.add_event(:service_desk_new_note_email) - end + mail_answer_thread(@issue, options) end private diff --git a/app/models/integration.rb b/app/models/integration.rb index 4b0aa3ad247..5ab70554a0c 100644 --- a/app/models/integration.rb +++ b/app/models/integration.rb @@ -14,14 +14,14 @@ class Integration < ApplicationRecord self.table_name = 'services' INTEGRATION_NAMES = %w[ - asana assembla bamboo bugzilla buildkite campfire confluence custom_issue_tracker discord + asana assembla bamboo bugzilla buildkite campfire confluence custom_issue_tracker datadog discord drone_ci emails_on_push ewm external_wiki flowdock hangouts_chat irker jira mattermost mattermost_slash_commands microsoft_teams packagist pipelines_email pivotaltracker prometheus pushover redmine slack slack_slash_commands teamcity unify_circuit webex_teams youtrack ].freeze PROJECT_SPECIFIC_INTEGRATION_NAMES = %w[ - datadog jenkins + jenkins ].freeze # Fake integrations to help with local development. @@ -54,6 +54,9 @@ class Integration < ApplicationRecord redmine slack slack_slash_commands teamcity + unify_circuit + webex_teams + youtrack ].to_set.freeze def self.renamed?(name) diff --git a/app/models/project.rb b/app/models/project.rb index 5a1210a9aa2..7a3783e53b7 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -188,9 +188,9 @@ class Project < ApplicationRecord has_one :slack_integration, class_name: 'Integrations::Slack' has_one :slack_slash_commands_integration, class_name: 'Integrations::SlackSlashCommands' has_one :teamcity_integration, class_name: 'Integrations::Teamcity' - has_one :unify_circuit_service, class_name: 'Integrations::UnifyCircuit' - has_one :webex_teams_service, class_name: 'Integrations::WebexTeams' - has_one :youtrack_service, class_name: 'Integrations::Youtrack' + has_one :unify_circuit_integration, class_name: 'Integrations::UnifyCircuit' + has_one :webex_teams_integration, class_name: 'Integrations::WebexTeams' + has_one :youtrack_integration, class_name: 'Integrations::Youtrack' has_one :root_of_fork_network, foreign_key: 'root_project_id', @@ -1407,8 +1407,6 @@ class Project < ApplicationRecord end def disabled_services - return %w[datadog] unless Feature.enabled?(:datadog_ci_integration, self) - [] end diff --git a/app/services/boards/issues/create_service.rb b/app/services/boards/issues/create_service.rb index 0639acfb399..e3d4da7fb07 100644 --- a/app/services/boards/issues/create_service.rb +++ b/app/services/boards/issues/create_service.rb @@ -30,7 +30,9 @@ module Boards end def create_issue(params) - ::Issues::CreateService.new(project: project, current_user: current_user, params: params).execute + # NOTE: We are intentionally not doing a spam/CAPTCHA check for issues created via boards. + # See https://gitlab.com/gitlab-org/gitlab/-/issues/29400#note_598479184 for more context. + ::Issues::CreateService.new(project: project, current_user: current_user, params: params, spam_params: nil).execute end end end diff --git a/app/services/captcha/captcha_verification_service.rb b/app/services/captcha/captcha_verification_service.rb index 45a5a52367c..3ed8ea12f3a 100644 --- a/app/services/captcha/captcha_verification_service.rb +++ b/app/services/captcha/captcha_verification_service.rb @@ -7,20 +7,27 @@ module Captcha class CaptchaVerificationService include Recaptcha::Verify + # Currently the only value that is used out of the request by the reCAPTCHA library + # is 'remote_ip'. Therefore, we just create a struct to avoid passing the full request + # object through all the service layer objects, and instead just rely on passing only + # the required remote_ip value. This eliminates the need to couple the service layer + # to the HTTP request (for the purpose of this service, at least). + RequestStruct = Struct.new(:remote_ip) + + def initialize(spam_params:) + @spam_params = spam_params + end + ## # Performs verification of a captcha response. # - # 'captcha_response' parameter is the response from the user solving a client-side captcha. - # - # 'request' parameter is the request which submitted the captcha. - # # NOTE: Currently only supports reCAPTCHA, and is not yet used in all places of the app in which # captchas are verified, but these can be addressed in future MRs. See: # https://gitlab.com/gitlab-org/gitlab/-/issues/273480 - def execute(captcha_response: nil, request:) - return false unless captcha_response + def execute + return false unless spam_params.captcha_response - @request = request + @request = RequestStruct.new(spam_params.ip_address) Gitlab::Recaptcha.load_configurations! @@ -31,11 +38,13 @@ module Captcha # 2. We want control over the wording and i18n of the message # 3. We want a consistent interface and behavior when adding support for other captcha # libraries which may not support automatically adding errors to the model. - verify_recaptcha(response: captcha_response) + verify_recaptcha(response: spam_params.captcha_response) end private + attr_reader :spam_params + # The recaptcha library's Recaptcha::Verify#verify_recaptcha method requires that # 'request' be a readable attribute - it doesn't support passing it as an options argument. attr_reader :request diff --git a/app/services/incident_management/incidents/create_service.rb b/app/services/incident_management/incidents/create_service.rb index 7497ee00d74..5d92a33eb2b 100644 --- a/app/services/incident_management/incidents/create_service.rb +++ b/app/services/incident_management/incidents/create_service.rb @@ -21,7 +21,8 @@ module IncidentManagement title: title, description: description, issue_type: ISSUE_TYPE - } + }, + spam_params: nil ).execute return error(issue.errors.full_messages.to_sentence, issue) unless issue.valid? diff --git a/app/services/issuable/import_csv/base_service.rb b/app/services/issuable/import_csv/base_service.rb index 27dbc8b3cc4..4a6b7540ded 100644 --- a/app/services/issuable/import_csv/base_service.rb +++ b/app/services/issuable/import_csv/base_service.rb @@ -68,7 +68,10 @@ module Issuable end def create_issuable(attributes) - create_issuable_class.new(project: @project, current_user: @user, params: attributes).execute + # NOTE: CSV imports are performed by workers, so we do not have a request context in order + # to create a SpamParams object to pass to the issuable create service. + spam_params = nil + create_issuable_class.new(project: @project, current_user: @user, params: attributes, spam_params: spam_params).execute end def email_results_to_user diff --git a/app/services/issues/clone_service.rb b/app/services/issues/clone_service.rb index 6df32f1104c..cb42334fe32 100644 --- a/app/services/issues/clone_service.rb +++ b/app/services/issues/clone_service.rb @@ -55,9 +55,13 @@ module Issues new_params = original_entity.serializable_hash.symbolize_keys.merge(new_params) + # spam checking is not necessary, as no new content is being created. Passing nil for + # spam_params will cause SpamActionService to skip checking and return a success response. + spam_params = nil + # Skip creation of system notes for existing attributes of the issue. The system notes of the old # issue are copied over so we don't want to end up with duplicate notes. - CreateService.new(project: target_project, current_user: current_user, params: new_params).execute(skip_system_notes: true) + CreateService.new(project: target_project, current_user: current_user, params: new_params, spam_params: spam_params).execute(skip_system_notes: true) end def queue_copy_designs diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 53f3dc39ba3..30d081996b1 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -4,10 +4,21 @@ module Issues class CreateService < Issues::BaseService include ResolveDiscussions - def execute(skip_system_notes: false) - @request = params.delete(:request) - @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request) + # NOTE: For Issues::CreateService, we require the spam_params and do not default it to nil, because + # spam_checking is likely to be necessary. However, if there is not a request available in scope + # in the caller (for example, an issue created via email) and the required arguments to the + # SpamParams constructor are not otherwise available, spam_params: must be explicitly passed as nil. + def initialize(project:, current_user: nil, params: {}, spam_params:) + # Temporary check to ensure we are no longer passing request in params now that we have + # introduced spam_params. Raise an exception if it is present. + # Remove after https://gitlab.com/gitlab-org/gitlab/-/merge_requests/58603 is complete. + raise if params[:request] + + super(project: project, current_user: current_user, params: params) + @spam_params = spam_params + end + def execute(skip_system_notes: false) @issue = BuildService.new(project: project, current_user: current_user, params: params).execute filter_resolve_discussion_params @@ -18,10 +29,10 @@ module Issues def before_create(issue) Spam::SpamActionService.new( spammable: issue, - request: request, + spam_params: spam_params, user: current_user, action: :create - ).execute(spam_params: spam_params) + ).execute # current_user (defined in BaseService) is not available within run_after_commit block user = current_user @@ -64,10 +75,10 @@ module Issues private - attr_reader :request, :spam_params + attr_reader :spam_params def user_agent_detail_service - UserAgentDetailService.new(@issue, request) + UserAgentDetailService.new(spammable: @issue, spam_params: spam_params) end end end diff --git a/app/services/issues/duplicate_service.rb b/app/services/issues/duplicate_service.rb index d150f0e5917..9547698d916 100644 --- a/app/services/issues/duplicate_service.rb +++ b/app/services/issues/duplicate_service.rb @@ -28,6 +28,7 @@ module Issues def relate_two_issues(duplicate_issue, canonical_issue) params = { target_issuable: canonical_issue } + IssueLinks::CreateService.new(duplicate_issue, current_user, params).execute end end diff --git a/app/services/issues/move_service.rb b/app/services/issues/move_service.rb index e49123a2993..ff78221c941 100644 --- a/app/services/issues/move_service.rb +++ b/app/services/issues/move_service.rb @@ -58,10 +58,13 @@ module Issues } new_params = original_entity.serializable_hash.symbolize_keys.merge(new_params) + # spam checking is not necessary, as no new content is being created. Passing nil for + # spam_params will cause SpamActionService to skip checking and return a success response. + spam_params = nil # Skip creation of system notes for existing attributes of the issue. The system notes of the old # issue are copied over so we don't want to end up with duplicate notes. - CreateService.new(project: @target_project, current_user: @current_user, params: new_params).execute(skip_system_notes: true) + CreateService.new(project: @target_project, current_user: @current_user, params: new_params, spam_params: spam_params).execute(skip_system_notes: true) end def queue_copy_designs diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index cf2892bf413..dc442a2fca1 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -4,12 +4,17 @@ module Issues class UpdateService < Issues::BaseService extend ::Gitlab::Utils::Override + # NOTE: For Issues::UpdateService, we default the spam_params to nil, because spam_checking is not + # necessary in many cases, and we don't want to require every caller to explicitly pass it as nil + # to disable spam checking. + def initialize(project:, current_user: nil, params: {}, spam_params: nil) + super(project: project, current_user: current_user, params: params) + @spam_params = spam_params + end + def execute(issue) handle_move_between_ids(issue) - @request = params.delete(:request) - @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request) - change_issue_duplicate(issue) move_issue_to_new_project(issue) || clone_issue(issue) || update_task_event(issue) || update(issue) end @@ -25,10 +30,10 @@ module Issues Spam::SpamActionService.new( spammable: issue, - request: request, + spam_params: spam_params, user: current_user, action: :update - ).execute(spam_params: spam_params) + ).execute end def handle_changes(issue, options) @@ -127,7 +132,7 @@ module Issues private - attr_reader :request, :spam_params + attr_reader :spam_params def clone_issue(issue) target_project = params.delete(:target_clone_project) diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 9dfcfe748da..b2674be4aae 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -396,6 +396,7 @@ class NotificationService recipients.each do |recipient| mailer.service_desk_new_note_email(issue.id, note.id, recipient).deliver_later + Gitlab::Metrics::BackgroundTransaction.current&.add_event(:service_desk_new_note_email) end end diff --git a/app/services/snippets/create_service.rb b/app/services/snippets/create_service.rb index 8f1b481d307..d9a46890c45 100644 --- a/app/services/snippets/create_service.rb +++ b/app/services/snippets/create_service.rb @@ -2,12 +2,14 @@ module Snippets class CreateService < Snippets::BaseService - def execute - # NOTE: disable_spam_action_service can be removed when the ':snippet_spam' feature flag is removed. - disable_spam_action_service = params.delete(:disable_spam_action_service) == true - @request = params.delete(:request) - @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request) + # NOTE: For Issues::CreateService, we require the spam_params and do not default it to nil, because + # spam_checking is likely to be necessary. + def initialize(project:, current_user: nil, params: {}, spam_params:) + super(project: project, current_user: current_user, params: params) + @spam_params = spam_params + end + def execute @snippet = build_from_params return invalid_params_error(@snippet) unless valid_params? @@ -18,17 +20,17 @@ module Snippets @snippet.author = current_user - unless disable_spam_action_service + if Feature.enabled?(:snippet_spam) Spam::SpamActionService.new( spammable: @snippet, - request: request, + spam_params: spam_params, user: current_user, action: :create - ).execute(spam_params: spam_params) + ).execute end if save_and_commit - UserAgentDetailService.new(@snippet, request).create + UserAgentDetailService.new(spammable: @snippet, spam_params: spam_params).create Gitlab::UsageDataCounters::SnippetCounter.count(:create) move_temporary_files @@ -41,7 +43,7 @@ module Snippets private - attr_reader :snippet, :request, :spam_params + attr_reader :snippet, :spam_params def build_from_params if project diff --git a/app/services/snippets/update_service.rb b/app/services/snippets/update_service.rb index 8571bc9c869..f8374cc88bb 100644 --- a/app/services/snippets/update_service.rb +++ b/app/services/snippets/update_service.rb @@ -6,12 +6,15 @@ module Snippets UpdateError = Class.new(StandardError) - def execute(snippet) - # NOTE: disable_spam_action_service can be removed when the ':snippet_spam' feature flag is removed. - disable_spam_action_service = params.delete(:disable_spam_action_service) == true - @request = params.delete(:request) - @spam_params = Spam::SpamActionService.filter_spam_params!(params, @request) + # NOTE: For Snippets::UpdateService, we default the spam_params to nil, because spam_checking is not + # necessary in many cases, and we don't want every caller to have to explicitly pass it as nil + # to disable spam checking. + def initialize(project:, current_user: nil, params: {}, spam_params: nil) + super(project: project, current_user: current_user, params: params) + @spam_params = spam_params + end + def execute(snippet) return invalid_params_error(snippet) unless valid_params? if visibility_changed?(snippet) && !visibility_allowed?(visibility_level) @@ -20,13 +23,13 @@ module Snippets update_snippet_attributes(snippet) - unless disable_spam_action_service + if Feature.enabled?(:snippet_spam) Spam::SpamActionService.new( spammable: snippet, - request: request, + spam_params: spam_params, user: current_user, action: :update - ).execute(spam_params: spam_params) + ).execute end if save_and_commit(snippet) @@ -40,7 +43,7 @@ module Snippets private - attr_reader :request, :spam_params + attr_reader :spam_params def visibility_changed?(snippet) visibility_level && visibility_level.to_i != snippet.visibility_level diff --git a/app/services/spam/akismet_service.rb b/app/services/spam/akismet_service.rb index e9843497dd7..d31b904f549 100644 --- a/app/services/spam/akismet_service.rb +++ b/app/services/spam/akismet_service.rb @@ -20,6 +20,7 @@ module Spam created_at: DateTime.current, author: owner_name, author_email: owner_email, + # NOTE: The akismet_client needs the option to be named `:referrer`, not `:referer` referrer: options[:referer] } diff --git a/app/services/spam/spam_action_service.rb b/app/services/spam/spam_action_service.rb index 3ae5111b994..ec16ce19cf6 100644 --- a/app/services/spam/spam_action_service.rb +++ b/app/services/spam/spam_action_service.rb @@ -4,67 +4,22 @@ module Spam class SpamActionService include SpamConstants - ## - # Utility method to filter SpamParams from parameters, which will later be passed to #execute - # after the spammable is created/updated based on the remaining parameters. - # - # Takes a hash of parameters from an incoming request to modify a model (via a controller, - # service, or GraphQL mutation). The parameters will either be camelCase (if they are - # received directly via controller params) or underscore_case (if they have come from - # a GraphQL mutation which has converted them to underscore), or in the - # headers when using the header based flow. - # - # Deletes the parameters which are related to spam and captcha processing, and returns - # them in a SpamParams parameters object. See: - # https://refactoring.com/catalog/introduceParameterObject.html - def self.filter_spam_params!(params, request) - # NOTE: The 'captcha_response' field can be expanded to multiple fields when we move to future - # alternative captcha implementations such as FriendlyCaptcha. See - # https://gitlab.com/gitlab-org/gitlab/-/issues/273480 - headers = request&.headers || {} - api = params.delete(:api) - captcha_response = read_parameter(:captcha_response, params, headers) - spam_log_id = read_parameter(:spam_log_id, params, headers)&.to_i - - SpamParams.new(api: api, captcha_response: captcha_response, spam_log_id: spam_log_id) - end - - def self.read_parameter(name, params, headers) - [ - params.delete(name), - params.delete(name.to_s.camelize(:lower).to_sym), - headers["X-GitLab-#{name.to_s.titlecase(keep_id_suffix: true).tr(' ', '-')}"] - ].compact.first - end - - attr_accessor :target, :request, :options - attr_reader :spam_log - - def initialize(spammable:, request:, user:, action:) + def initialize(spammable:, spam_params:, user:, action:) @target = spammable - @request = request + @spam_params = spam_params @user = user @action = action - @options = {} end # rubocop:disable Metrics/AbcSize - def execute(spam_params:) - if request - options[:ip_address] = request.env['action_dispatch.remote_ip'].to_s - options[:user_agent] = request.env['HTTP_USER_AGENT'] - options[:referer] = request.env['HTTP_REFERER'] - else - # TODO: This code is never used, because we do not perform a verification if there is not a - # request. Why? Should it be deleted? Or should we check even if there is no request? - options[:ip_address] = target.ip_address - options[:user_agent] = target.user_agent - end + def execute + # If spam_params is passed as `nil`, no check will be performed. This is the easiest way to allow + # composed services which may not need to do spam checking to "opt out". For example, when + # MoveService is calling CreateService, spam checking is not necessary, as no new content is + # being created. + return ServiceResponse.success(message: 'Skipped spam check because spam_params was not present') unless spam_params - recaptcha_verified = Captcha::CaptchaVerificationService.new.execute( - captcha_response: spam_params.captcha_response, - request: request - ) + recaptcha_verified = Captcha::CaptchaVerificationService.new(spam_params: spam_params).execute if recaptcha_verified # If it's a request which is already verified through CAPTCHA, @@ -73,10 +28,9 @@ module Spam ServiceResponse.success(message: "CAPTCHA successfully verified") else return ServiceResponse.success(message: 'Skipped spam check because user was allowlisted') if allowlisted?(user) - return ServiceResponse.success(message: 'Skipped spam check because request was not present') unless request return ServiceResponse.success(message: 'Skipped spam check because it was not required') unless check_for_spam? - perform_spam_service_check(spam_params.api) + perform_spam_service_check ServiceResponse.success(message: "Spam check performed. Check #{target.class.name} spammable model for any errors or CAPTCHA requirement") end end @@ -86,7 +40,7 @@ module Spam private - attr_reader :user, :action + attr_reader :user, :action, :target, :spam_params, :spam_log ## # In order to be proceed to the spam check process, the target must be @@ -104,7 +58,7 @@ module Spam ## # Performs the spam check using the spam verdict service, and modifies the target model # accordingly based on the result. - def perform_spam_service_check(api) + def perform_spam_service_check ensure_target_is_dirty # since we can check for spam, and recaptcha is not verified, @@ -113,7 +67,7 @@ module Spam case result when CONDITIONAL_ALLOW # at the moment, this means "ask for reCAPTCHA" - create_spam_log(api) + create_spam_log break if target.allow_possible_spam? @@ -122,12 +76,12 @@ module Spam # TODO: remove `unless target.allow_possible_spam?` once this flag has been passed to `SpamVerdictService` # https://gitlab.com/gitlab-org/gitlab/-/issues/214739 target.spam! unless target.allow_possible_spam? - create_spam_log(api) + create_spam_log when BLOCK_USER # TODO: improve BLOCK_USER handling, non-existent until now # https://gitlab.com/gitlab-org/gitlab/-/issues/329666 target.spam! unless target.allow_possible_spam? - create_spam_log(api) + create_spam_log when ALLOW target.clear_spam_flags! when NOOP @@ -137,16 +91,21 @@ module Spam end end - def create_spam_log(api) + def create_spam_log @spam_log = SpamLog.create!( { user_id: target.author_id, title: target.spam_title, description: target.spam_description, - source_ip: options[:ip_address], - user_agent: options[:user_agent], + source_ip: spam_params.ip_address, + user_agent: spam_params.user_agent, noteable_type: noteable_type, - via_api: api + # Now, all requests are via the API, so hardcode it to true to simplify the logic and API + # of this service. See https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/2266 + # for original introduction of `via_api` field. + # See discussion here about possibly deprecating this field: + # https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/2266#note_542527450 + via_api: true } ) @@ -159,9 +118,14 @@ module Spam target_type: noteable_type } + options = { + ip_address: spam_params.ip_address, + user_agent: spam_params.user_agent, + referer: spam_params.referer + } + SpamVerdictService.new(target: target, user: user, - request: request, options: options, context: context) end diff --git a/app/services/spam/spam_params.rb b/app/services/spam/spam_params.rb index 3420748822d..ccc17a42f01 100644 --- a/app/services/spam/spam_params.rb +++ b/app/services/spam/spam_params.rb @@ -3,30 +3,54 @@ module Spam ## # This class is a Parameter Object (https://refactoring.com/catalog/introduceParameterObject.html) - # which acts as an container abstraction for multiple parameter values related to spam and - # captcha processing for a request. + # which acts as an container abstraction for multiple values related to spam and + # captcha processing for a provided HTTP request object. + # + # It is used to encapsulate these values and allow them to be passed from the Controller/GraphQL + # layers down into to the Service layer, without needing to pass the entire request and therefore + # unnecessarily couple the Service layer to the HTTP request. # # Values contained are: # - # api: A boolean flag indicating if the request was submitted via the REST or GraphQL API # captcha_response: The response resulting from the user solving a captcha. Currently it is # a scalar reCAPTCHA response string, but it can be expanded to an object in the future to - # support other captcha implementations such as FriendlyCaptcha. - # spam_log_id: The id of a SpamLog record. + # support other captcha implementations such as FriendlyCaptcha. Obtained from + # request.headers['X-GitLab-Captcha-Response'] + # spam_log_id: The id of a SpamLog record. Obtained from request.headers['X-GitLab-Spam-Log-Id'] + # ip_address = The remote IP. Obtained from request.env['action_dispatch.remote_ip'] + # user_agent = The user agent. Obtained from request.env['HTTP_USER_AGENT'] + # referer = The HTTP referer. Obtained from request.env['HTTP_REFERER'] + # + # NOTE: The presence of these values in the request is not currently enforced. If they are missing, + # then the spam check may fail, or the SpamLog or UserAgentDetail may have missing fields. class SpamParams - attr_reader :api, :captcha_response, :spam_log_id + def self.new_from_request(request:) + self.new( + captcha_response: request.headers['X-GitLab-Captcha-Response'], + spam_log_id: request.headers['X-GitLab-Spam-Log-Id'], + ip_address: request.env['action_dispatch.remote_ip'].to_s, + user_agent: request.env['HTTP_USER_AGENT'], + referer: request.env['HTTP_REFERER'] + ) + end + + attr_reader :captcha_response, :spam_log_id, :ip_address, :user_agent, :referer - def initialize(api:, captcha_response:, spam_log_id:) - @api = api.present? + def initialize(captcha_response:, spam_log_id:, ip_address:, user_agent:, referer:) @captcha_response = captcha_response @spam_log_id = spam_log_id + @ip_address = ip_address + @user_agent = user_agent + @referer = referer end def ==(other) other.class <= self.class && - other.api == api && other.captcha_response == captcha_response && - other.spam_log_id == spam_log_id + other.spam_log_id == spam_log_id && + other.ip_address == ip_address && + other.user_agent == user_agent && + other.referer == referer end end end diff --git a/app/services/spam/spam_verdict_service.rb b/app/services/spam/spam_verdict_service.rb index 7155017b73f..8d995631db6 100644 --- a/app/services/spam/spam_verdict_service.rb +++ b/app/services/spam/spam_verdict_service.rb @@ -5,9 +5,8 @@ module Spam include AkismetMethods include SpamConstants - def initialize(user:, target:, request:, options:, context: {}) + def initialize(user:, target:, options:, context: {}) @target = target - @request = request @user = user @options = options @context = context @@ -59,7 +58,7 @@ module Spam private - attr_reader :user, :target, :request, :options, :context + attr_reader :user, :target, :options, :context def akismet_verdict if akismet.spam? diff --git a/app/services/user_agent_detail_service.rb b/app/services/user_agent_detail_service.rb index 9302c86d3e6..01a98a15869 100644 --- a/app/services/user_agent_detail_service.rb +++ b/app/services/user_agent_detail_service.rb @@ -1,16 +1,21 @@ # frozen_string_literal: true class UserAgentDetailService - attr_accessor :spammable, :request - - def initialize(spammable, request) + def initialize(spammable:, spam_params:) @spammable = spammable - @request = request + @spam_params = spam_params end def create - return unless request + unless spam_params&.user_agent && spam_params&.ip_address + messasge = 'Skipped UserAgentDetail creation because necessary spam_params were not provided' + return ServiceResponse.success(message: messasge) + end - spammable.create_user_agent_detail(user_agent: request.env['HTTP_USER_AGENT'], ip_address: request.env['action_dispatch.remote_ip'].to_s) + spammable.create_user_agent_detail(user_agent: spam_params.user_agent, ip_address: spam_params.ip_address) end + + private + + attr_reader :spammable, :spam_params end diff --git a/config/feature_flags/development/datadog_ci_integration.yml b/config/feature_flags/development/datadog_ci_integration.yml deleted file mode 100644 index 4f8fca4950a..00000000000 --- a/config/feature_flags/development/datadog_ci_integration.yml +++ /dev/null @@ -1,8 +0,0 @@ ---- -name: datadog_ci_integration -introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/46564 -rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/284088 -type: development -group: group::ecosystem -default_enabled: false -milestone: '13.7' diff --git a/config/routes/project.rb b/config/routes/project.rb index 641ca399547..7e2a97a7a6a 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -547,7 +547,13 @@ constraints(::Constraints::ProjectUrlConstrainer.new) do end end - scope :usage_ping, controller: :usage_ping do + # TODO: Deprecated. This should be removed in 14.2. + scope :usage_ping, controller: :service_ping do + post :web_ide_clientside_preview # rubocop:todo Cop/PutProjectRoutesUnderScope + post :web_ide_pipelines_count # rubocop:todo Cop/PutProjectRoutesUnderScope + end + + scope :service_ping, controller: :service_ping do post :web_ide_clientside_preview # rubocop:todo Cop/PutProjectRoutesUnderScope post :web_ide_pipelines_count # rubocop:todo Cop/PutProjectRoutesUnderScope end diff --git a/danger/feature_flag/Dangerfile b/danger/feature_flag/Dangerfile index bf2194724fc..ac9ea812ebb 100644 --- a/danger/feature_flag/Dangerfile +++ b/danger/feature_flag/Dangerfile @@ -68,27 +68,3 @@ end if helper.security_mr? && feature_flag_file_added? fail "Feature flags are discouraged from security merge requests. Read the [security documentation](https://gitlab.com/gitlab-org/release/docs/-/blob/master/general/security/utilities/feature_flags.md) for details." end - -if feature_flag_file_added_or_removed? - new_mr_title = helper.mr_title.dup - new_mr_title << ' [RUN ALL RSPEC]' unless helper.run_all_rspec_mr? - new_mr_title << ' [RUN AS-IF-FOSS]' unless helper.run_as_if_foss_mr? - - changes = {} - changes[:add_labels] = FEATURE_FLAG_LABEL unless helper.mr_has_labels?(FEATURE_FLAG_LABEL) - - if new_mr_title != helper.mr_title - changes[:title] = new_mr_title - else - message "You're adding or removing a feature flag, your MR title needs to include `[RUN ALL RSPEC] [RUN AS-IF-FOSS]` (we may have updated it automatically for you and started a new MR pipeline) to ensure everything is covered." - end - - if changes.any? - gitlab.api.update_merge_request( - gitlab.mr_json['project_id'], - gitlab.mr_json['iid'], - **changes - ) - gitlab.api.post("/projects/#{gitlab.mr_json['project_id']}/merge_requests/#{gitlab.mr_json['iid']}/pipelines") - end -end diff --git a/danger/specialization_labels/Dangerfile b/danger/specialization_labels/Dangerfile index 2261fe23e4e..35125f20b14 100644 --- a/danger/specialization_labels/Dangerfile +++ b/danger/specialization_labels/Dangerfile @@ -9,7 +9,8 @@ SPECIALIZATIONS = { docs: 'documentation', qa: 'QA', engineering_productivity: 'Engineering Productivity', - ci_template: 'ci::templates' + ci_template: 'ci::templates', + feature_flag: 'feature flag' }.freeze labels_to_add = project_helper.changes_by_category.each_with_object([]) do |(category, _changes), memo| diff --git a/doc/user/profile/notifications.md b/doc/user/profile/notifications.md index b9410be791e..eaf1d33a938 100644 --- a/doc/user/profile/notifications.md +++ b/doc/user/profile/notifications.md @@ -176,14 +176,14 @@ Users are notified of the following events: | Event | Sent to | Settings level | |------------------------------|---------------------|------------------------------| | New SSH key added | User | Security email, always sent. | -| SSH key has expired | User | Security email, always sent. | +| SSH key has expired | User | Security email, always sent. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/322637) in GitLab 13.12 | | New email added | User | Security email, always sent. | | Email changed | User | Security email, always sent. | | Password changed | User | Security email, always sent when user changes their own password | | Password changed by administrator | User | Security email, always sent when an administrator changes the password of another user | | Two-factor authentication disabled | User | Security email, always sent. | | New user created | User | Sent on user creation, except for OmniAuth (LDAP)| -| New SAML/SCIM user provisioned. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/276018) in GitLab 13.8 | User | Sent when a user is provisioned through SAML/SCIM | +| New SAML/SCIM user provisioned | User | Sent when a user is provisioned through SAML/SCIM. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/276018) in GitLab 13.8 | | User added to project | User | Sent when user is added to project | | Project access level changed | User | Sent when user project access level is changed | | User added to group | User | Sent when user is added to group | @@ -237,10 +237,10 @@ epics: | Close merge request | | | Due issue | Participants and Custom notification level with this event selected | | Failed pipeline | The author of the pipeline | -| Fixed pipeline ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/24309) in GitLab 13.1.) | The author of the pipeline. Enabled by default. | +| Fixed pipeline | The author of the pipeline. Enabled by default. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/24309) in GitLab 13.1 | | Merge merge request | | -| Merge when pipeline succeeds ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/211961) in GitLab 13.4) | Author, Participants, Watchers, Subscribers, and Custom notification level with this event selected. `Note:` Custom notification level is ignored for Author, Watchers and Subscribers | -| Merge request [marked as ready](../project/merge_requests/drafts.md) (introduced in [GitLab 13.10](https://gitlab.com/gitlab-org/gitlab/-/issues/15332)) | Watchers and participants | +| Merge when pipeline succeeds | Author, Participants, Watchers, Subscribers, and Custom notification level with this event selected. Custom notification level is ignored for Author, Watchers and Subscribers. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/211961) in GitLab 13.4 | +| Merge request [marked as ready](../project/merge_requests/drafts.md) | Watchers and participants. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/15332) in GitLab 13.10 | | New comment | Participants, Watchers, Subscribers, and Custom notification level with this event selected, plus anyone mentioned by `@username` in the comment, with notification level "Mention" or higher | | New epic | | | New issue | | diff --git a/doc/user/project/integrations/overview.md b/doc/user/project/integrations/overview.md index 8cf5bfeaa1a..bbeef80c83f 100644 --- a/doc/user/project/integrations/overview.md +++ b/doc/user/project/integrations/overview.md @@ -32,6 +32,7 @@ Click on the service links to see further configuration instructions and details | Campfire | Connect to chat. | **{dotted-circle}** No | | [Confluence Workspace](../../../api/services.md#confluence-service) | Replace the link to the internal wiki with a link to a Confluence Cloud Workspace. | **{dotted-circle}** No | | [Custom issue tracker](custom_issue_tracker.md) | Use a custom issue tracker. | **{dotted-circle}** No | +| Datadog | Trace your GitLab pipelines with Datadog. | **{check-circle}** Yes | | [Discord Notifications](discord_notifications.md) | Send notifications about project events to a Discord channel. | **{dotted-circle}** No | | Drone CI | Run CI/CD pipelines with Drone. | **{check-circle}** Yes | | [Emails on push](emails_on_push.md) | Send commits and diff of each push by email. | **{dotted-circle}** No | diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index 6ce04be373f..3398d5da7f5 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -577,10 +577,6 @@ module API Gitlab::AppLogger.warn("Redis tracking event failed for event: #{event_name}, message: #{error.message}") end - def with_api_params(&block) - yield({ api: true, request: request }) - end - protected def project_finder_params_visibility_ce diff --git a/lib/api/helpers/snippets_helpers.rb b/lib/api/helpers/snippets_helpers.rb index 42f56680ded..2d8c761101a 100644 --- a/lib/api/helpers/snippets_helpers.rb +++ b/lib/api/helpers/snippets_helpers.rb @@ -72,22 +72,18 @@ module API end def process_create_params(args) - with_api_params do |api_params| - args[:snippet_actions] = args.delete(:files)&.map do |file| - file[:action] = :create - file.symbolize_keys - end - - args.merge(api_params) + args[:snippet_actions] = args.delete(:files)&.map do |file| + file[:action] = :create + file.symbolize_keys end + + args end def process_update_params(args) - with_api_params do |api_params| - args[:snippet_actions] = args.delete(:files)&.map(&:symbolize_keys) + args[:snippet_actions] = args.delete(:files)&.map(&:symbolize_keys) - args.merge(api_params) - end + args end def validate_params_for_multiple_files(snippet) diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 355b5ed3a1f..54013d0e7b4 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -255,9 +255,11 @@ module API issue_params = convert_parameters_from_legacy_format(issue_params) begin + spam_params = ::Spam::SpamParams.new_from_request(request: request) issue = ::Issues::CreateService.new(project: user_project, current_user: current_user, - params: issue_params.merge(request: request, api: true)).execute + params: issue_params, + spam_params: spam_params).execute if issue.spam? render_api_error!({ error: 'Spam detected' }, 400) @@ -294,13 +296,15 @@ module API issue = user_project.issues.find_by!(iid: params.delete(:issue_iid)) authorize! :update_issue, issue - update_params = declared_params(include_missing: false).merge(request: request, api: true) + update_params = declared_params(include_missing: false) update_params = convert_parameters_from_legacy_format(update_params) + spam_params = ::Spam::SpamParams.new_from_request(request: request) issue = ::Issues::UpdateService.new(project: user_project, current_user: current_user, - params: update_params).execute(issue) + params: update_params, + spam_params: spam_params).execute(issue) render_spam_error! if issue.spam? diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb index 084492fd503..fdbfdf1f7a9 100644 --- a/lib/api/project_snippets.rb +++ b/lib/api/project_snippets.rb @@ -75,7 +75,8 @@ module API snippet_params = process_create_params(declared_params(include_missing: false)) - service_response = ::Snippets::CreateService.new(project: user_project, current_user: current_user, params: snippet_params).execute + spam_params = ::Spam::SpamParams.new_from_request(request: request) + service_response = ::Snippets::CreateService.new(project: user_project, current_user: current_user, params: snippet_params, spam_params: spam_params).execute snippet = service_response.payload[:snippet] if service_response.success? @@ -116,7 +117,8 @@ module API snippet_params = process_update_params(declared_params(include_missing: false)) - service_response = ::Snippets::UpdateService.new(project: user_project, current_user: current_user, params: snippet_params).execute(snippet) + spam_params = ::Spam::SpamParams.new_from_request(request: request) + service_response = ::Snippets::UpdateService.new(project: user_project, current_user: current_user, params: snippet_params, spam_params: spam_params).execute(snippet) snippet = service_response.payload[:snippet] if service_response.success? diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb index b506192fe1c..f1ec1024492 100644 --- a/lib/api/snippets.rb +++ b/lib/api/snippets.rb @@ -84,7 +84,8 @@ module API attrs = process_create_params(declared_params(include_missing: false)) - service_response = ::Snippets::CreateService.new(project: nil, current_user: current_user, params: attrs).execute + spam_params = ::Spam::SpamParams.new_from_request(request: request) + service_response = ::Snippets::CreateService.new(project: nil, current_user: current_user, params: attrs, spam_params: spam_params).execute snippet = service_response.payload[:snippet] if service_response.success? @@ -126,7 +127,8 @@ module API attrs = process_update_params(declared_params(include_missing: false)) - service_response = ::Snippets::UpdateService.new(project: nil, current_user: current_user, params: attrs).execute(snippet) + spam_params = ::Spam::SpamParams.new_from_request(request: request) + service_response = ::Snippets::UpdateService.new(project: nil, current_user: current_user, params: attrs, spam_params: spam_params).execute(snippet) snippet = service_response.payload[:snippet] diff --git a/lib/gitlab/email/handler/create_issue_handler.rb b/lib/gitlab/email/handler/create_issue_handler.rb index e927a5641e5..b110d39818d 100644 --- a/lib/gitlab/email/handler/create_issue_handler.rb +++ b/lib/gitlab/email/handler/create_issue_handler.rb @@ -61,7 +61,8 @@ module Gitlab params: { title: mail.subject, description: message_including_reply - } + }, + spam_params: nil ).execute end diff --git a/lib/gitlab/email/handler/service_desk_handler.rb b/lib/gitlab/email/handler/service_desk_handler.rb index 05daa08530e..2187c01acfb 100644 --- a/lib/gitlab/email/handler/service_desk_handler.rb +++ b/lib/gitlab/email/handler/service_desk_handler.rb @@ -83,7 +83,8 @@ module Gitlab description: message_including_template, confidential: true, external_author: from_address - } + }, + spam_params: nil ).execute raise InvalidIssueError unless @issue.persisted? @@ -95,6 +96,7 @@ module Gitlab def send_thank_you_email Notify.service_desk_thank_you_email(@issue.id).deliver_later + Gitlab::Metrics::BackgroundTransaction.current&.add_event(:service_desk_thank_you_email) end def message_including_template diff --git a/lib/gitlab/slash_commands/issue_new.rb b/lib/gitlab/slash_commands/issue_new.rb index 99a056c97fc..fab016d2e1b 100644 --- a/lib/gitlab/slash_commands/issue_new.rb +++ b/lib/gitlab/slash_commands/issue_new.rb @@ -33,7 +33,7 @@ module Gitlab private def create_issue(title:, description:) - Issues::CreateService.new(project: project, current_user: current_user, params: { title: title, description: description }).execute + Issues::CreateService.new(project: project, current_user: current_user, params: { title: title, description: description }, spam_params: nil).execute end def presenter(issue) diff --git a/lib/quality/seeders/issues.rb b/lib/quality/seeders/issues.rb index ea2db2aa5fe..3eb0245f8a2 100644 --- a/lib/quality/seeders/issues.rb +++ b/lib/quality/seeders/issues.rb @@ -30,7 +30,7 @@ module Quality labels: labels.join(',') } params[:closed_at] = params[:created_at] + rand(35).days if params[:state] == 'closed' - issue = ::Issues::CreateService.new(project: project, current_user: team.sample, params: params).execute + issue = ::Issues::CreateService.new(project: project, current_user: team.sample, params: params, spam_params: nil).execute if issue.persisted? created_issues_count += 1 diff --git a/spec/controllers/projects/usage_ping_controller_spec.rb b/spec/controllers/projects/service_ping_controller_spec.rb index 9ace072d561..e6afaadc75f 100644 --- a/spec/controllers/projects/usage_ping_controller_spec.rb +++ b/spec/controllers/projects/service_ping_controller_spec.rb @@ -2,7 +2,7 @@ require 'spec_helper' -RSpec.describe Projects::UsagePingController do +RSpec.describe Projects::ServicePingController do let_it_be(:project) { create(:project) } let_it_be(:user) { create(:user) } diff --git a/spec/factories/integrations.rb b/spec/factories/integrations.rb index 8e0f5b50db5..ed8a562b331 100644 --- a/spec/factories/integrations.rb +++ b/spec/factories/integrations.rb @@ -97,7 +97,7 @@ FactoryBot.define do issue_tracker end - factory :youtrack_service, class: 'Integrations::Youtrack' do + factory :youtrack_integration, class: 'Integrations::Youtrack' do project active { true } issue_tracker diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index fa538551987..84686c58a8e 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -402,7 +402,7 @@ FactoryBot.define do factory :youtrack_project, parent: :project do has_external_issue_tracker { true } - youtrack_service + youtrack_integration end factory :jira_project, parent: :project do diff --git a/spec/features/calendar_spec.rb b/spec/features/calendar_spec.rb index 21da92c9f43..a8aa3f0b36a 100644 --- a/spec/features/calendar_spec.rb +++ b/spec/features/calendar_spec.rb @@ -145,7 +145,7 @@ RSpec.describe 'Contributions Calendar', :js do describe '1 issue creation calendar activity' do before do - Issues::CreateService.new(project: contributed_project, current_user: user, params: issue_params).execute + Issues::CreateService.new(project: contributed_project, current_user: user, params: issue_params, spam_params: nil).execute end it_behaves_like 'a day with activity', contribution_count: 1 @@ -180,7 +180,7 @@ RSpec.describe 'Contributions Calendar', :js do push_code_contribution travel_to(Date.yesterday) do - Issues::CreateService.new(project: contributed_project, current_user: user, params: issue_params).execute + Issues::CreateService.new(project: contributed_project, current_user: user, params: issue_params, spam_params: nil).execute end end include_context 'visit user page' diff --git a/spec/features/unsubscribe_links_spec.rb b/spec/features/unsubscribe_links_spec.rb index b2d0f29808c..b7471720008 100644 --- a/spec/features/unsubscribe_links_spec.rb +++ b/spec/features/unsubscribe_links_spec.rb @@ -9,7 +9,7 @@ RSpec.describe 'Unsubscribe links', :sidekiq_might_not_need_inline do let(:author) { create(:user) } let(:project) { create(:project, :public) } let(:params) { { title: 'A bug!', description: 'Fix it!', assignees: [recipient] } } - let(:issue) { Issues::CreateService.new(project: project, current_user: author, params: params).execute } + let(:issue) { Issues::CreateService.new(project: project, current_user: author, params: params, spam_params: nil).execute } let(:mail) { ActionMailer::Base.deliveries.last } let(:body) { Capybara::Node::Simple.new(mail.default_part_body.to_s) } diff --git a/spec/features/users/user_browses_projects_on_user_page_spec.rb b/spec/features/users/user_browses_projects_on_user_page_spec.rb index ded90be3924..5e7d7b76843 100644 --- a/spec/features/users/user_browses_projects_on_user_page_spec.rb +++ b/spec/features/users/user_browses_projects_on_user_page_spec.rb @@ -125,7 +125,7 @@ RSpec.describe 'Users > User browses projects on user page', :js do end before do - Issues::CreateService.new(project: contributed_project, current_user: user, params: { title: 'Bug in old browser' }).execute + Issues::CreateService.new(project: contributed_project, current_user: user, params: { title: 'Bug in old browser' }, spam_params: nil).execute event = create(:push_event, project: contributed_project, author: user) create(:push_event_payload, event: event, commit_count: 3) end diff --git a/spec/frontend/environments/environments_app_spec.js b/spec/frontend/environments/environments_app_spec.js index 542cf58b079..5403d5d06c0 100644 --- a/spec/frontend/environments/environments_app_spec.js +++ b/spec/frontend/environments/environments_app_spec.js @@ -1,3 +1,4 @@ +import { GlTabs } from '@gitlab/ui'; import { mount, shallowMount } from '@vue/test-utils'; import MockAdapter from 'axios-mock-adapter'; import { extendedWrapper } from 'helpers/vue_test_utils_helper'; @@ -7,6 +8,7 @@ import EmptyState from '~/environments/components/empty_state.vue'; import EnableReviewAppModal from '~/environments/components/enable_review_app_modal.vue'; import EnvironmentsApp from '~/environments/components/environments_app.vue'; import axios from '~/lib/utils/axios_utils'; +import * as utils from '~/lib/utils/common_utils'; import { environment, folder } from './mock_data'; describe('Environment', () => { @@ -264,4 +266,18 @@ describe('Environment', () => { }); }); }); + + describe('tabs', () => { + beforeEach(() => { + mockRequest(200, { environments: [] }); + jest + .spyOn(utils, 'getParameterByName') + .mockImplementation((param) => (param === 'scope' ? 'stopped' : null)); + return createWrapper(true); + }); + + it('selects the tab for the parameter', () => { + expect(wrapper.findComponent(GlTabs).attributes('value')).toBe('1'); + }); + }); }); diff --git a/spec/frontend/issues_list/components/issues_list_app_spec.js b/spec/frontend/issues_list/components/issues_list_app_spec.js index a3ac57ee1bb..00b03488f0c 100644 --- a/spec/frontend/issues_list/components/issues_list_app_spec.js +++ b/spec/frontend/issues_list/components/issues_list_app_spec.js @@ -21,7 +21,6 @@ import IssuableList from '~/issuable_list/components/issuable_list_root.vue'; import { IssuableListTabs, IssuableStates } from '~/issuable_list/constants'; import IssuesListApp from '~/issues_list/components/issues_list_app.vue'; import { - apiSortParams, CREATED_DESC, DUE_DATE_OVERDUE, PARAM_DUE_DATE, @@ -148,8 +147,8 @@ describe('IssuesListApp component', () => { hasPreviousPage: getIssuesQueryResponse.data.project.issues.pageInfo.hasPreviousPage, hasNextPage: getIssuesQueryResponse.data.project.issues.pageInfo.hasNextPage, urlParams: { + sort: urlSortParams[CREATED_DESC], state: IssuableStates.Opened, - ...urlSortParams[CREATED_DESC], }, }); }); @@ -178,7 +177,7 @@ describe('IssuesListApp component', () => { describe('csv import/export component', () => { describe('when user is signed in', () => { - const search = '?search=refactor&state=opened&sort=created_date'; + const search = '?search=refactor&sort=created_date&state=opened'; beforeEach(() => { global.jsdom.reconfigure({ url: `${TEST_HOST}${search}` }); @@ -273,13 +272,17 @@ describe('IssuesListApp component', () => { describe('sort', () => { it.each(Object.keys(urlSortParams))('is set as %s from the url params', (sortKey) => { - global.jsdom.reconfigure({ url: setUrlParams(urlSortParams[sortKey], TEST_HOST) }); + global.jsdom.reconfigure({ + url: setUrlParams({ sort: urlSortParams[sortKey] }, TEST_HOST), + }); wrapper = mountComponent(); expect(findIssuableList().props()).toMatchObject({ initialSortBy: sortKey, - urlParams: urlSortParams[sortKey], + urlParams: { + sort: urlSortParams[sortKey], + }, }); }); }); @@ -640,7 +643,7 @@ describe('IssuesListApp component', () => { }); describe('when "sort" event is emitted by IssuableList', () => { - it.each(Object.keys(apiSortParams))( + it.each(Object.keys(urlSortParams))( 'updates to the new sort when payload is `%s`', async (sortKey) => { wrapper = mountComponent(); @@ -650,7 +653,9 @@ describe('IssuesListApp component', () => { jest.runOnlyPendingTimers(); await nextTick(); - expect(findIssuableList().props('urlParams')).toMatchObject(urlSortParams[sortKey]); + expect(findIssuableList().props('urlParams')).toMatchObject({ + sort: urlSortParams[sortKey], + }); }, ); }); diff --git a/spec/frontend/issues_list/mock_data.js b/spec/frontend/issues_list/mock_data.js index 6c669e02070..e6ee038c688 100644 --- a/spec/frontend/issues_list/mock_data.js +++ b/spec/frontend/issues_list/mock_data.js @@ -9,7 +9,7 @@ export const getIssuesQueryResponse = { issues: { count: 1, pageInfo: { - hasNextPage: false, + hasNextPage: true, hasPreviousPage: false, startCursor: 'startcursor', endCursor: 'endcursor', @@ -86,10 +86,10 @@ export const locationSearch = [ 'not[label_name][]=drama', 'my_reaction_emoji=thumbsup', 'confidential=no', - 'iteration_title=season:+%234', - 'not[iteration_title]=season:+%2320', - 'epic_id=gitlab-org%3A%3A%2612', - 'not[epic_id]=gitlab-org%3A%3A%2634', + 'iteration_id=4', + 'not[iteration_id]=20', + 'epic_id=12', + 'not[epic_id]=34', 'weight=1', 'not[weight]=3', ].join('&'); @@ -118,10 +118,10 @@ export const filteredTokens = [ { type: 'labels', value: { data: 'drama', operator: OPERATOR_IS_NOT } }, { type: 'my_reaction_emoji', value: { data: 'thumbsup', operator: OPERATOR_IS } }, { type: 'confidential', value: { data: 'no', operator: OPERATOR_IS } }, - { type: 'iteration', value: { data: 'season: #4', operator: OPERATOR_IS } }, - { type: 'iteration', value: { data: 'season: #20', operator: OPERATOR_IS_NOT } }, - { type: 'epic_id', value: { data: 'gitlab-org::&12', operator: OPERATOR_IS } }, - { type: 'epic_id', value: { data: 'gitlab-org::&34', operator: OPERATOR_IS_NOT } }, + { type: 'iteration', value: { data: '4', operator: OPERATOR_IS } }, + { type: 'iteration', value: { data: '20', operator: OPERATOR_IS_NOT } }, + { type: 'epic_id', value: { data: '12', operator: OPERATOR_IS } }, + { type: 'epic_id', value: { data: '34', operator: OPERATOR_IS_NOT } }, { type: 'weight', value: { data: '1', operator: OPERATOR_IS } }, { type: 'weight', value: { data: '3', operator: OPERATOR_IS_NOT } }, { type: 'filtered-search-term', value: { data: 'find' } }, @@ -138,30 +138,32 @@ export const filteredTokensWithSpecialValues = [ ]; export const apiParams = { - author_username: 'homer', - 'not[author_username]': 'marge', - assignee_username: ['bart', 'lisa'], - 'not[assignee_username]': ['patty', 'selma'], - milestone: 'season 4', - 'not[milestone]': 'season 20', - labels: ['cartoon', 'tv'], - 'not[labels]': ['live action', 'drama'], - my_reaction_emoji: 'thumbsup', + authorUsername: 'homer', + assigneeUsernames: ['bart', 'lisa'], + milestoneTitle: 'season 4', + labelName: ['cartoon', 'tv'], + myReactionEmoji: 'thumbsup', confidential: 'no', - iteration_title: 'season: #4', - 'not[iteration_title]': 'season: #20', - epic_id: '12', - 'not[epic_id]': 'gitlab-org::&34', + iterationId: '4', + epicId: '12', weight: '1', - 'not[weight]': '3', + not: { + authorUsername: 'marge', + assigneeUsernames: ['patty', 'selma'], + milestoneTitle: 'season 20', + labelName: ['live action', 'drama'], + iterationId: '20', + epicId: '34', + weight: '3', + }, }; export const apiParamsWithSpecialValues = { - assignee_id: '123', - assignee_username: 'bart', - my_reaction_emoji: 'None', - iteration_id: 'Current', - epic_id: 'None', + assigneeId: '123', + assigneeUsernames: 'bart', + myReactionEmoji: 'None', + iterationWildcardId: 'CURRENT', + epicId: 'None', weight: 'None', }; @@ -176,10 +178,10 @@ export const urlParams = { 'not[label_name][]': ['live action', 'drama'], my_reaction_emoji: 'thumbsup', confidential: 'no', - iteration_title: 'season: #4', - 'not[iteration_title]': 'season: #20', - epic_id: 'gitlab-org%3A%3A%2612', - 'not[epic_id]': 'gitlab-org::&34', + iteration_id: '4', + 'not[iteration_id]': '20', + epic_id: '12', + 'not[epic_id]': '34', weight: '1', 'not[weight]': '3', }; diff --git a/spec/frontend/issues_list/utils_spec.js b/spec/frontend/issues_list/utils_spec.js index e377c35a0aa..b7863068570 100644 --- a/spec/frontend/issues_list/utils_spec.js +++ b/spec/frontend/issues_list/utils_spec.js @@ -8,10 +8,11 @@ import { urlParams, urlParamsWithSpecialValues, } from 'jest/issues_list/mock_data'; -import { API_PARAM, DUE_DATE_VALUES, URL_PARAM, urlSortParams } from '~/issues_list/constants'; +import { DUE_DATE_VALUES, urlSortParams } from '~/issues_list/constants'; import { - convertToParams, + convertToApiParams, convertToSearchQuery, + convertToUrlParams, getDueDateValue, getFilterTokens, getSortKey, @@ -20,7 +21,7 @@ import { describe('getSortKey', () => { it.each(Object.keys(urlSortParams))('returns %s given the correct inputs', (sortKey) => { - const { sort } = urlSortParams[sortKey]; + const sort = urlSortParams[sortKey]; expect(getSortKey(sort)).toBe(sortKey); }); }); @@ -80,31 +81,23 @@ describe('getFilterTokens', () => { }); }); -describe('convertToParams', () => { +describe('convertToApiParams', () => { it('returns api params given filtered tokens', () => { - expect(convertToParams(filteredTokens, API_PARAM)).toEqual({ - ...apiParams, - epic_id: 'gitlab-org::&12', - }); + expect(convertToApiParams(filteredTokens)).toEqual(apiParams); }); it('returns api params given filtered tokens with special values', () => { - expect(convertToParams(filteredTokensWithSpecialValues, API_PARAM)).toEqual( - apiParamsWithSpecialValues, - ); + expect(convertToApiParams(filteredTokensWithSpecialValues)).toEqual(apiParamsWithSpecialValues); }); +}); +describe('convertToUrlParams', () => { it('returns url params given filtered tokens', () => { - expect(convertToParams(filteredTokens, URL_PARAM)).toEqual({ - ...urlParams, - epic_id: 'gitlab-org::&12', - }); + expect(convertToUrlParams(filteredTokens)).toEqual(urlParams); }); it('returns url params given filtered tokens with special values', () => { - expect(convertToParams(filteredTokensWithSpecialValues, URL_PARAM)).toEqual( - urlParamsWithSpecialValues, - ); + expect(convertToUrlParams(filteredTokensWithSpecialValues)).toEqual(urlParamsWithSpecialValues); }); }); diff --git a/spec/frontend/vue_shared/components/filtered_search_bar/tokens/iteration_token_spec.js b/spec/frontend/vue_shared/components/filtered_search_bar/tokens/iteration_token_spec.js index ca5dc984ae0..bd654c5a9cb 100644 --- a/spec/frontend/vue_shared/components/filtered_search_bar/tokens/iteration_token_spec.js +++ b/spec/frontend/vue_shared/components/filtered_search_bar/tokens/iteration_token_spec.js @@ -7,7 +7,7 @@ import { mockIterationToken } from '../mock_data'; jest.mock('~/flash'); describe('IterationToken', () => { - const title = 'gitlab-org: #1'; + const id = 123; let wrapper; const createComponent = ({ config = mockIterationToken, value = { data: '' } } = {}) => @@ -28,14 +28,14 @@ describe('IterationToken', () => { }); it('renders iteration value', async () => { - wrapper = createComponent({ value: { data: title } }); + wrapper = createComponent({ value: { data: id } }); await wrapper.vm.$nextTick(); const tokenSegments = wrapper.findAllComponents(GlFilteredSearchTokenSegment); expect(tokenSegments).toHaveLength(3); // `Iteration` `=` `gitlab-org: #1` - expect(tokenSegments.at(2).text()).toBe(title); + expect(tokenSegments.at(2).text()).toBe(id.toString()); }); it('fetches initial values', () => { @@ -43,10 +43,10 @@ describe('IterationToken', () => { wrapper = createComponent({ config: { ...mockIterationToken, fetchIterations: fetchIterationsSpy }, - value: { data: title }, + value: { data: id }, }); - expect(fetchIterationsSpy).toHaveBeenCalledWith(title); + expect(fetchIterationsSpy).toHaveBeenCalledWith(id); }); it('fetches iterations on user input', () => { diff --git a/spec/frontend/vue_shared/components/user_select_spec.js b/spec/frontend/vue_shared/components/user_select_spec.js index 0fabc6525ea..b777ac0a0a4 100644 --- a/spec/frontend/vue_shared/components/user_select_spec.js +++ b/spec/frontend/vue_shared/components/user_select_spec.js @@ -275,48 +275,4 @@ describe('User select dropdown', () => { expect(findEmptySearchResults().exists()).toBe(true); }); }); - - // TODO Remove this test after the following issue is resolved in the backend - // https://gitlab.com/gitlab-org/gitlab/-/issues/329750 - describe('temporary error suppression', () => { - beforeEach(() => { - jest.spyOn(console, 'error').mockImplementation(); - }); - - const nullError = { message: 'Cannot return null for non-nullable field GroupMember.user' }; - - it.each` - mockErrors - ${[nullError]} - ${[nullError, nullError]} - `('does not emit errors', async ({ mockErrors }) => { - createComponent({ - searchQueryHandler: jest.fn().mockResolvedValue({ - errors: mockErrors, - }), - }); - await waitForSearch(); - - expect(wrapper.emitted()).toEqual({}); - // eslint-disable-next-line no-console - expect(console.error).toHaveBeenCalled(); - }); - - it.each` - mockErrors - ${[{ message: 'serious error' }]} - ${[nullError, { message: 'serious error' }]} - `('emits error when non-null related errors are included', async ({ mockErrors }) => { - createComponent({ - searchQueryHandler: jest.fn().mockResolvedValue({ - errors: mockErrors, - }), - }); - await waitForSearch(); - - expect(wrapper.emitted('error')).toEqual([[]]); - // eslint-disable-next-line no-console - expect(console.error).not.toHaveBeenCalled(); - }); - }); }); diff --git a/spec/graphql/mutations/issues/create_spec.rb b/spec/graphql/mutations/issues/create_spec.rb index b32f0991959..0e7ef0e55b9 100644 --- a/spec/graphql/mutations/issues/create_spec.rb +++ b/spec/graphql/mutations/issues/create_spec.rb @@ -50,6 +50,7 @@ RSpec.describe Mutations::Issues::Create do stub_licensed_features(multiple_issue_assignees: false, issue_weights: false) project.add_guest(assignee1) project.add_guest(assignee2) + stub_spam_services end subject { mutation.resolve(**mutation_params) } diff --git a/spec/graphql/mutations/issues/set_confidential_spec.rb b/spec/graphql/mutations/issues/set_confidential_spec.rb index c3269e5c0c0..495b8442d95 100644 --- a/spec/graphql/mutations/issues/set_confidential_spec.rb +++ b/spec/graphql/mutations/issues/set_confidential_spec.rb @@ -17,6 +17,10 @@ RSpec.describe Mutations::Issues::SetConfidential do subject { mutation.resolve(project_path: project.full_path, iid: issue.iid, confidential: confidential) } + before do + stub_spam_services + end + it_behaves_like 'permission level for issue mutation is correctly verified' context 'when the user can update the issue' do diff --git a/spec/graphql/mutations/issues/update_spec.rb b/spec/graphql/mutations/issues/update_spec.rb index bd780477658..80f43338bb5 100644 --- a/spec/graphql/mutations/issues/update_spec.rb +++ b/spec/graphql/mutations/issues/update_spec.rb @@ -35,6 +35,10 @@ RSpec.describe Mutations::Issues::Update do subject { mutation.resolve(**mutation_params) } + before do + stub_spam_services + end + it_behaves_like 'permission level for issue mutation is correctly verified' context 'when the user can update the issue' do diff --git a/spec/lib/banzai/filter/references/external_issue_reference_filter_spec.rb b/spec/lib/banzai/filter/references/external_issue_reference_filter_spec.rb index 503477b2115..dbb8bfc82f3 100644 --- a/spec/lib/banzai/filter/references/external_issue_reference_filter_spec.rb +++ b/spec/lib/banzai/filter/references/external_issue_reference_filter_spec.rb @@ -140,7 +140,9 @@ RSpec.describe Banzai::Filter::References::ExternalIssueReferenceFilter do end context "youtrack project" do - let_it_be(:service) { create(:youtrack_service, project: project) } + before_all do + create(:youtrack_integration, project: project) + end before do project.update!(issues_enabled: false) diff --git a/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb b/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb index 3a60564d8d2..f6b618163f0 100644 --- a/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/service_desk_handler_spec.rb @@ -50,6 +50,15 @@ RSpec.describe Gitlab::Email::Handler::ServiceDeskHandler do it 'sends thank you email' do expect { receiver.execute }.to have_enqueued_job.on_queue('mailers') end + + it 'adds metric events for incoming and reply emails' do + metric_transaction = double('Gitlab::Metrics::WebTransaction', increment: true, observe: true) + allow(::Gitlab::Metrics::BackgroundTransaction).to receive(:current).and_return(metric_transaction) + expect(metric_transaction).to receive(:add_event).with(:receive_email_service_desk, anything) + expect(metric_transaction).to receive(:add_event).with(:service_desk_thank_you_email) + + receiver.execute + end end context 'when everything is fine' do diff --git a/spec/lib/gitlab/import_export/all_models.yml b/spec/lib/gitlab/import_export/all_models.yml index f84f06da9cf..a7642d5e3c3 100644 --- a/spec/lib/gitlab/import_export/all_models.yml +++ b/spec/lib/gitlab/import_export/all_models.yml @@ -381,14 +381,14 @@ project: - microsoft_teams_integration - mattermost_integration - hangouts_chat_integration -- unify_circuit_service +- unify_circuit_integration - buildkite_integration - bamboo_integration - teamcity_integration - pushover_integration - jira_integration - redmine_integration -- youtrack_service +- youtrack_integration - custom_issue_tracker_integration - bugzilla_integration - ewm_integration @@ -557,7 +557,7 @@ project: - alert_management_alerts - repository_storage_moves - freeze_periods -- webex_teams_service +- webex_teams_integration - build_report_results - vulnerability_statistic - vulnerability_historical_statistics diff --git a/spec/mailers/emails/service_desk_spec.rb b/spec/mailers/emails/service_desk_spec.rb index 995e6c006cd..57fa990d399 100644 --- a/spec/mailers/emails/service_desk_spec.rb +++ b/spec/mailers/emails/service_desk_spec.rb @@ -115,16 +115,6 @@ RSpec.describe Emails::ServiceDesk do end end - shared_examples 'notification with metric event' do |event_type| - it 'adds metric event' do - metric_transaction = double('Gitlab::Metrics::WebTransaction', increment: true, observe: true) - allow(::Gitlab::Metrics::BackgroundTransaction).to receive(:current).and_return(metric_transaction) - expect(metric_transaction).to receive(:add_event).with(event_type) - - subject.content_type - end - end - describe '.service_desk_thank_you_email' do let_it_be(:reply_in_subject) { true } let_it_be(:default_text) do @@ -134,7 +124,6 @@ RSpec.describe Emails::ServiceDesk do subject { ServiceEmailClass.service_desk_thank_you_email(issue.id) } it_behaves_like 'read template from repository', 'thank_you' - it_behaves_like 'notification with metric event', :service_desk_thank_you_email context 'handling template markdown' do context 'with a simple text' do @@ -175,7 +164,6 @@ RSpec.describe Emails::ServiceDesk do subject { ServiceEmailClass.service_desk_new_note_email(issue.id, note.id, email) } it_behaves_like 'read template from repository', 'new_note' - it_behaves_like 'notification with metric event', :service_desk_new_note_email context 'handling template markdown' do context 'with a simple text' do diff --git a/spec/migrations/20190924152703_migrate_issue_trackers_data_spec.rb b/spec/migrations/20190924152703_migrate_issue_trackers_data_spec.rb index ea5192375f3..dad95760306 100644 --- a/spec/migrations/20190924152703_migrate_issue_trackers_data_spec.rb +++ b/spec/migrations/20190924152703_migrate_issue_trackers_data_spec.rb @@ -26,11 +26,11 @@ RSpec.describe MigrateIssueTrackersData do services.create!(type: 'BugzillaService', properties: properties, category: 'issue_tracker') end - let!(:youtrack_service) do + let!(:youtrack_integration) do services.create!(type: 'YoutrackService', properties: properties, category: 'issue_tracker') end - let!(:youtrack_service_empty) do + let!(:youtrack_integration_empty) do services.create!(type: 'YoutrackService', properties: '', category: 'issue_tracker') end @@ -56,7 +56,7 @@ RSpec.describe MigrateIssueTrackersData do migrate! expect(migration_name).to be_scheduled_delayed_migration(3.minutes, jira_integration.id, bugzilla_integration.id) - expect(migration_name).to be_scheduled_delayed_migration(6.minutes, youtrack_service.id, gitlab_service.id) + expect(migration_name).to be_scheduled_delayed_migration(6.minutes, youtrack_integration.id, gitlab_service.id) expect(BackgroundMigrationWorker.jobs.size).to eq(2) end end diff --git a/spec/migrations/20200130145430_reschedule_migrate_issue_trackers_data_spec.rb b/spec/migrations/20200130145430_reschedule_migrate_issue_trackers_data_spec.rb index 90bbdca4d9c..cf8bc608483 100644 --- a/spec/migrations/20200130145430_reschedule_migrate_issue_trackers_data_spec.rb +++ b/spec/migrations/20200130145430_reschedule_migrate_issue_trackers_data_spec.rb @@ -26,11 +26,11 @@ RSpec.describe RescheduleMigrateIssueTrackersData do services.create!(id: 12, type: 'BugzillaService', properties: properties, category: 'issue_tracker') end - let!(:youtrack_service) do + let!(:youtrack_integration) do services.create!(id: 13, type: 'YoutrackService', properties: properties, category: 'issue_tracker') end - let!(:youtrack_service_empty) do + let!(:youtrack_integration_empty) do services.create!(id: 14, type: 'YoutrackService', properties: '', category: 'issue_tracker') end @@ -57,7 +57,7 @@ RSpec.describe RescheduleMigrateIssueTrackersData do migrate! expect(migration_name).to be_scheduled_delayed_migration(3.minutes, jira_integration.id, bugzilla_integration.id) - expect(migration_name).to be_scheduled_delayed_migration(6.minutes, youtrack_service.id, gitlab_service.id) + expect(migration_name).to be_scheduled_delayed_migration(6.minutes, youtrack_integration.id, gitlab_service.id) expect(BackgroundMigrationWorker.jobs.size).to eq(2) end end diff --git a/spec/models/integrations/microsoft_teams_spec.rb b/spec/models/integrations/microsoft_teams_spec.rb index 7341905e71b..a7b45ebec87 100644 --- a/spec/models/integrations/microsoft_teams_spec.rb +++ b/spec/models/integrations/microsoft_teams_spec.rb @@ -74,7 +74,7 @@ RSpec.describe Integrations::MicrosoftTeams do context 'with issue events' do let(:opts) { { title: 'Awesome issue', description: 'please fix' } } let(:issues_sample_data) do - service = Issues::CreateService.new(project: project, current_user: user, params: opts) + service = Issues::CreateService.new(project: project, current_user: user, params: opts, spam_params: nil) issue = service.execute service.hook_data(issue, 'open') end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 00fee3a3104..1db8e1d81a4 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -39,8 +39,8 @@ RSpec.describe Project, factory_default: :keep do it { is_expected.to have_one(:microsoft_teams_integration) } it { is_expected.to have_one(:mattermost_integration) } it { is_expected.to have_one(:hangouts_chat_integration) } - it { is_expected.to have_one(:unify_circuit_service) } - it { is_expected.to have_one(:webex_teams_service) } + it { is_expected.to have_one(:unify_circuit_integration) } + it { is_expected.to have_one(:webex_teams_integration) } it { is_expected.to have_one(:packagist_integration) } it { is_expected.to have_one(:pushover_integration) } it { is_expected.to have_one(:asana_integration) } @@ -62,7 +62,7 @@ RSpec.describe Project, factory_default: :keep do it { is_expected.to have_one(:teamcity_integration) } it { is_expected.to have_one(:jira_integration) } it { is_expected.to have_one(:redmine_integration) } - it { is_expected.to have_one(:youtrack_service) } + it { is_expected.to have_one(:youtrack_integration) } it { is_expected.to have_one(:custom_issue_tracker_integration) } it { is_expected.to have_one(:bugzilla_integration) } it { is_expected.to have_one(:ewm_integration) } @@ -5870,26 +5870,6 @@ RSpec.describe Project, factory_default: :keep do end end - describe '#disabled_services' do - subject { build(:project).disabled_services } - - context 'without datadog_ci_integration' do - before do - stub_feature_flags(datadog_ci_integration: false) - end - - it { is_expected.to include('datadog') } - end - - context 'with datadog_ci_integration' do - before do - stub_feature_flags(datadog_ci_integration: true) - end - - it { is_expected.not_to include('datadog') } - end - end - describe '#find_or_initialize_service' do it 'avoids N+1 database queries' do allow(Integration).to receive(:available_services_names).and_return(%w[prometheus pushover]) diff --git a/spec/requests/api/graphql/mutations/snippets/create_spec.rb b/spec/requests/api/graphql/mutations/snippets/create_spec.rb index 214c804c519..d329b9aea6a 100644 --- a/spec/requests/api/graphql/mutations/snippets/create_spec.rb +++ b/spec/requests/api/graphql/mutations/snippets/create_spec.rb @@ -17,7 +17,6 @@ RSpec.describe 'Creating a Snippet' do let(:actions) { [{ action: action }.merge(file_1), { action: action }.merge(file_2)] } let(:project_path) { nil } let(:uploaded_files) { nil } - let(:spam_mutation_vars) { {} } let(:mutation_vars) do { description: description, @@ -26,7 +25,7 @@ RSpec.describe 'Creating a Snippet' do project_path: project_path, uploaded_files: uploaded_files, blob_actions: actions - }.merge(spam_mutation_vars) + } end let(:mutation) do @@ -77,21 +76,6 @@ RSpec.describe 'Creating a Snippet' do expect(mutation_response['snippet']).to be_nil end - - context 'when snippet_spam flag is disabled' do - before do - stub_feature_flags(snippet_spam: false) - end - - it 'passes disable_spam_action_service param to service' do - expect(::Snippets::CreateService) - .to receive(:new) - .with(project: anything, current_user: anything, params: hash_including(disable_spam_action_service: true)) - .and_call_original - - subject - end - end end shared_examples 'creates snippet' do @@ -121,15 +105,6 @@ RSpec.describe 'Creating a Snippet' do it_behaves_like 'snippet edit usage data counters' it_behaves_like 'a mutation which can mutate a spammable' do - let(:captcha_response) { 'abc123' } - let(:spam_log_id) { 1234 } - let(:spam_mutation_vars) do - { - captcha_response: captcha_response, - spam_log_id: spam_log_id - } - end - let(:service) { Snippets::CreateService } end end @@ -190,7 +165,7 @@ RSpec.describe 'Creating a Snippet' do it do expect(::Snippets::CreateService).to receive(:new) - .with(project: nil, current_user: user, params: hash_including(files: expected_value)) + .with(project: nil, current_user: user, params: hash_including(files: expected_value), spam_params: instance_of(::Spam::SpamParams)) .and_return(double(execute: creation_response)) subject diff --git a/spec/requests/api/graphql/mutations/snippets/update_spec.rb b/spec/requests/api/graphql/mutations/snippets/update_spec.rb index 77efb786dcb..a3a6ce65e9f 100644 --- a/spec/requests/api/graphql/mutations/snippets/update_spec.rb +++ b/spec/requests/api/graphql/mutations/snippets/update_spec.rb @@ -16,7 +16,6 @@ RSpec.describe 'Updating a Snippet' do let(:updated_file) { 'CHANGELOG' } let(:deleted_file) { 'README' } let(:snippet_gid) { GitlabSchema.id_from_object(snippet).to_s } - let(:spam_mutation_vars) { {} } let(:mutation_vars) do { id: snippet_gid, @@ -27,7 +26,7 @@ RSpec.describe 'Updating a Snippet' do { action: :update, filePath: updated_file, content: updated_content }, { action: :delete, filePath: deleted_file } ] - }.merge(spam_mutation_vars) + } end let(:mutation) do @@ -82,21 +81,6 @@ RSpec.describe 'Updating a Snippet' do end end - context 'when snippet_spam flag is disabled' do - before do - stub_feature_flags(snippet_spam: false) - end - - it 'passes disable_spam_action_service param to service' do - expect(::Snippets::UpdateService) - .to receive(:new) - .with(project: anything, current_user: anything, params: hash_including(disable_spam_action_service: true)) - .and_call_original - - subject - end - end - context 'when there are ActiveRecord validation errors' do let(:updated_title) { '' } @@ -125,15 +109,6 @@ RSpec.describe 'Updating a Snippet' do end it_behaves_like 'a mutation which can mutate a spammable' do - let(:captcha_response) { 'abc123' } - let(:spam_log_id) { 1234 } - let(:spam_mutation_vars) do - { - captcha_response: captcha_response, - spam_log_id: spam_log_id - } - end - let(:service) { Snippets::UpdateService } end diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb index fe04a1d7c4a..7b194ef8122 100644 --- a/spec/routing/project_routing_spec.rb +++ b/spec/routing/project_routing_spec.rb @@ -770,13 +770,23 @@ RSpec.describe 'project routing' do end end - describe Projects::UsagePingController, 'routing' do - it 'routes to usage_ping#web_ide_clientside_preview' do - expect(post('/gitlab/gitlabhq/usage_ping/web_ide_clientside_preview')).to route_to('projects/usage_ping#web_ide_clientside_preview', namespace_id: 'gitlab', project_id: 'gitlabhq') + describe Projects::ServicePingController, 'routing' do + describe 'deprecated routing' do + it 'routes to service_ping#web_ide_clientside_preview' do + expect(post('/gitlab/gitlabhq/usage_ping/web_ide_clientside_preview')).to route_to('projects/service_ping#web_ide_clientside_preview', namespace_id: 'gitlab', project_id: 'gitlabhq') + end + + it 'routes to service_ping#web_ide_pipelines_count' do + expect(post('/gitlab/gitlabhq/usage_ping/web_ide_pipelines_count')).to route_to('projects/service_ping#web_ide_pipelines_count', namespace_id: 'gitlab', project_id: 'gitlabhq') + end + end + + it 'routes to service_ping#web_ide_clientside_preview' do + expect(post('/gitlab/gitlabhq/service_ping/web_ide_clientside_preview')).to route_to('projects/service_ping#web_ide_clientside_preview', namespace_id: 'gitlab', project_id: 'gitlabhq') end - it 'routes to usage_ping#web_ide_pipelines_count' do - expect(post('/gitlab/gitlabhq/usage_ping/web_ide_pipelines_count')).to route_to('projects/usage_ping#web_ide_pipelines_count', namespace_id: 'gitlab', project_id: 'gitlabhq') + it 'routes to service_ping#web_ide_pipelines_count' do + expect(post('/gitlab/gitlabhq/service_ping/web_ide_pipelines_count')).to route_to('projects/service_ping#web_ide_pipelines_count', namespace_id: 'gitlab', project_id: 'gitlabhq') end end diff --git a/spec/services/captcha/captcha_verification_service_spec.rb b/spec/services/captcha/captcha_verification_service_spec.rb index 245e06703f5..fe2199fb53e 100644 --- a/spec/services/captcha/captcha_verification_service_spec.rb +++ b/spec/services/captcha/captcha_verification_service_spec.rb @@ -4,21 +4,31 @@ require 'spec_helper' RSpec.describe Captcha::CaptchaVerificationService do describe '#execute' do - let(:captcha_response) { nil } - let(:request) { double(:request) } - let(:service) { described_class.new } + let(:captcha_response) { 'abc123' } + let(:fake_ip) { '1.2.3.4' } + let(:spam_params) do + ::Spam::SpamParams.new( + captcha_response: captcha_response, + spam_log_id: double, + ip_address: fake_ip, + user_agent: double, + referer: double + ) + end + + let(:service) { described_class.new(spam_params: spam_params) } - subject { service.execute(captcha_response: captcha_response, request: request) } + subject { service.execute } context 'when there is no captcha_response' do + let(:captcha_response) { nil } + it 'returns false' do expect(subject).to eq(false) end end context 'when there is a captcha_response' do - let(:captcha_response) { 'abc123' } - before do expect(Gitlab::Recaptcha).to receive(:load_configurations!) end @@ -29,10 +39,12 @@ RSpec.describe Captcha::CaptchaVerificationService do expect(subject).to eq(true) end - it 'has a request method which returns the request' do + it 'has a request method which returns an object with the ip address #remote_ip' do subject - expect(service.send(:request)).to eq(request) + request_struct = service.send(:request) + expect(request_struct).to respond_to(:remote_ip) + expect(request_struct.remote_ip).to eq(fake_ip) end end end diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index 94810d6134a..8ace5f08988 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -8,11 +8,17 @@ RSpec.describe Issues::CreateService do let_it_be_with_reload(:project) { create(:project) } let_it_be(:user) { create(:user) } + let(:spam_params) { double } + describe '#execute' do let_it_be(:assignee) { create(:user) } let_it_be(:milestone) { create(:milestone, project: project) } - let(:issue) { described_class.new(project: project, current_user: user, params: opts).execute } + let(:issue) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute } + + before do + stub_spam_services + end context 'when params are valid' do let_it_be(:labels) { create_pair(:label, project: project) } @@ -44,7 +50,7 @@ RSpec.describe Issues::CreateService do end context 'when skip_system_notes is true' do - let(:issue) { described_class.new(project: project, current_user: user, params: opts).execute(skip_system_notes: true) } + let(:issue) { described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute(skip_system_notes: true) } it 'does not call Issuable::CommonSystemNotesService' do expect(Issuable::CommonSystemNotesService).not_to receive(:new) @@ -92,7 +98,7 @@ RSpec.describe Issues::CreateService do let_it_be(:non_member) { create(:user) } it 'filters out params that cannot be set without the :set_issue_metadata permission' do - issue = described_class.new(project: project, current_user: non_member, params: opts).execute + issue = described_class.new(project: project, current_user: non_member, params: opts, spam_params: spam_params).execute expect(issue).to be_persisted expect(issue.title).to eq('Awesome issue') @@ -104,7 +110,7 @@ RSpec.describe Issues::CreateService do end it 'can create confidential issues' do - issue = described_class.new(project: project, current_user: non_member, params: { confidential: true }).execute + issue = described_class.new(project: project, current_user: non_member, params: { confidential: true }, spam_params: spam_params).execute expect(issue.confidential).to be_truthy end @@ -113,7 +119,7 @@ RSpec.describe Issues::CreateService do it 'moves the issue to the end, in an asynchronous worker' do expect(IssuePlacementWorker).to receive(:perform_async).with(be_nil, Integer) - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute end context 'when label belongs to project group' do @@ -200,7 +206,7 @@ RSpec.describe Issues::CreateService do it 'invalidates open issues counter for assignees when issue is assigned' do project.add_maintainer(assignee) - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(assignee.assigned_open_issues_count).to eq 1 end @@ -226,7 +232,7 @@ RSpec.describe Issues::CreateService do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) expect(project).to receive(:execute_services).with(an_instance_of(Hash), :issue_hooks) - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute end it 'executes confidential issue hooks when issue is confidential' do @@ -235,7 +241,7 @@ RSpec.describe Issues::CreateService do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :confidential_issue_hooks) expect(project).to receive(:execute_services).with(an_instance_of(Hash), :confidential_issue_hooks) - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute end context 'after_save callback to store_mentions' do @@ -279,7 +285,7 @@ RSpec.describe Issues::CreateService do it 'removes assignee when user id is invalid' do opts = { title: 'Title', description: 'Description', assignee_ids: [-1] } - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.assignees).to be_empty end @@ -287,7 +293,7 @@ RSpec.describe Issues::CreateService do it 'removes assignee when user id is 0' do opts = { title: 'Title', description: 'Description', assignee_ids: [0] } - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.assignees).to be_empty end @@ -296,7 +302,7 @@ RSpec.describe Issues::CreateService do project.add_maintainer(assignee) opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.assignees).to eq([assignee]) end @@ -314,7 +320,7 @@ RSpec.describe Issues::CreateService do project.update!(visibility_level: level) opts = { title: 'Title', description: 'Description', assignee_ids: [assignee.id] } - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.assignees).to be_empty end @@ -324,7 +330,7 @@ RSpec.describe Issues::CreateService do end it_behaves_like 'issuable record that supports quick actions' do - let(:issuable) { described_class.new(project: project, current_user: user, params: params).execute } + let(:issuable) { described_class.new(project: project, current_user: user, params: params, spam_params: spam_params).execute } end context 'Quick actions' do @@ -364,14 +370,14 @@ RSpec.describe Issues::CreateService do let(:opts) { { discussion_to_resolve: discussion.id, merge_request_to_resolve_discussions_of: merge_request.iid } } it 'resolves the discussion' do - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute discussion.first_note.reload expect(discussion.resolved?).to be(true) end it 'added a system note to the discussion' do - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute reloaded_discussion = MergeRequest.find(merge_request.id).discussions.first @@ -379,7 +385,7 @@ RSpec.describe Issues::CreateService do end it 'assigns the title and description for the issue' do - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.title).not_to be_nil expect(issue.description).not_to be_nil @@ -391,7 +397,8 @@ RSpec.describe Issues::CreateService do merge_request_to_resolve_discussions_of: merge_request, description: nil, title: nil - }).execute + }, + spam_params: spam_params).execute expect(issue.description).to be_nil expect(issue.title).to be_nil @@ -402,14 +409,14 @@ RSpec.describe Issues::CreateService do let(:opts) { { merge_request_to_resolve_discussions_of: merge_request.iid } } it 'resolves the discussion' do - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute discussion.first_note.reload expect(discussion.resolved?).to be(true) end it 'added a system note to the discussion' do - described_class.new(project: project, current_user: user, params: opts).execute + described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute reloaded_discussion = MergeRequest.find(merge_request.id).discussions.first @@ -417,7 +424,7 @@ RSpec.describe Issues::CreateService do end it 'assigns the title and description for the issue' do - issue = described_class.new(project: project, current_user: user, params: opts).execute + issue = described_class.new(project: project, current_user: user, params: opts, spam_params: spam_params).execute expect(issue.title).not_to be_nil expect(issue.description).not_to be_nil @@ -429,7 +436,8 @@ RSpec.describe Issues::CreateService do merge_request_to_resolve_discussions_of: merge_request, description: nil, title: nil - }).execute + }, + spam_params: spam_params).execute expect(issue.description).to be_nil expect(issue.title).to be_nil @@ -438,47 +446,27 @@ RSpec.describe Issues::CreateService do end context 'checking spam' do - let(:request) { double(:request, headers: nil) } - let(:api) { true } - let(:captcha_response) { 'abc123' } - let(:spam_log_id) { 1 } - let(:params) do { - title: 'Spam issue', - request: request, - api: api, - captcha_response: captcha_response, - spam_log_id: spam_log_id + title: 'Spam issue' } end subject do - described_class.new(project: project, current_user: user, params: params) - end - - before do - allow_next_instance_of(UserAgentDetailService) do |instance| - allow(instance).to receive(:create) - end + described_class.new(project: project, current_user: user, params: params, spam_params: spam_params) end it 'executes SpamActionService' do - spam_params = Spam::SpamParams.new( - api: api, - captcha_response: captcha_response, - spam_log_id: spam_log_id - ) expect_next_instance_of( Spam::SpamActionService, { - spammable: an_instance_of(Issue), - request: request, - user: user, + spammable: kind_of(Issue), + spam_params: spam_params, + user: an_instance_of(User), action: :create } ) do |instance| - expect(instance).to receive(:execute).with(spam_params: spam_params) + expect(instance).to receive(:execute) end subject.execute diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index c3a0766cb17..63ef007350d 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -376,41 +376,31 @@ RSpec.describe NotificationService, :mailer do let(:subject) { NotificationService.new } let(:mailer) { double(deliver_later: true) } + let(:issue) { create(:issue, author: User.support_bot) } + let(:project) { issue.project } + let(:note) { create(:note, noteable: issue, project: project) } - def should_email! - expect(Notify).to receive(:service_desk_new_note_email) - .with(issue.id, note.id, issue.external_author) - end + shared_examples 'notification with exact metric events' do |number_of_events| + it 'adds metric event' do + metric_transaction = double('Gitlab::Metrics::WebTransaction', increment: true, observe: true) + allow(::Gitlab::Metrics::BackgroundTransaction).to receive(:current).and_return(metric_transaction) + expect(metric_transaction).to receive(:add_event).with(:service_desk_new_note_email).exactly(number_of_events).times - def should_not_email! - expect(Notify).not_to receive(:service_desk_new_note_email) + subject.new_note(note) + end end - def execute! - subject.new_note(note) - end + shared_examples 'no participants are notified' do + it 'does not send the email' do + expect(Notify).not_to receive(:service_desk_new_note_email) - def self.it_should_email! - it 'sends the email' do - should_email! - execute! + subject.new_note(note) end - end - def self.it_should_not_email! - it 'doesn\'t send the email' do - should_not_email! - execute! - end + it_behaves_like 'notification with exact metric events', 0 end - let(:issue) { create(:issue, author: User.support_bot) } - let(:project) { issue.project } - let(:note) { create(:note, noteable: issue, project: project) } - - context 'do not exist' do - it_should_not_email! - end + it_behaves_like 'no participants are notified' context 'do exist and note not confidential' do let!(:issue_email_participant) { issue.issue_email_participants.create!(email: 'service.desk@example.com') } @@ -420,7 +410,14 @@ RSpec.describe NotificationService, :mailer do project.update!(service_desk_enabled: true) end - it_should_email! + it 'sends the email' do + expect(Notify).to receive(:service_desk_new_note_email) + .with(issue.id, note.id, issue.external_author) + + subject.new_note(note) + end + + it_behaves_like 'notification with exact metric events', 1 end context 'do exist and note is confidential' do @@ -432,7 +429,7 @@ RSpec.describe NotificationService, :mailer do project.update!(service_desk_enabled: true) end - it_should_not_email! + it_behaves_like 'no participants are notified' end end diff --git a/spec/services/snippets/create_service_spec.rb b/spec/services/snippets/create_service_spec.rb index eb6e85eb408..0b73808433f 100644 --- a/spec/services/snippets/create_service_spec.rb +++ b/spec/services/snippets/create_service_spec.rb @@ -19,8 +19,9 @@ RSpec.describe Snippets::CreateService do let(:extra_opts) { {} } let(:creator) { admin } + let(:spam_params) { double } - subject { described_class.new(project: project, current_user: creator, params: opts).execute } + subject { described_class.new(project: project, current_user: creator, params: opts, spam_params: spam_params).execute } let(:snippet) { subject.payload[:snippet] } @@ -301,6 +302,10 @@ RSpec.describe Snippets::CreateService do end end + before do + stub_spam_services + end + context 'when ProjectSnippet' do let_it_be(:project) { create(:project) } diff --git a/spec/services/snippets/update_service_spec.rb b/spec/services/snippets/update_service_spec.rb index 46bc62e11ef..7fac500f224 100644 --- a/spec/services/snippets/update_service_spec.rb +++ b/spec/services/snippets/update_service_spec.rb @@ -20,7 +20,9 @@ RSpec.describe Snippets::UpdateService do let(:extra_opts) { {} } let(:options) { base_opts.merge(extra_opts) } let(:updater) { user } - let(:service) { Snippets::UpdateService.new(project: project, current_user: updater, params: options) } + let(:spam_params) { double } + + let(:service) { Snippets::UpdateService.new(project: project, current_user: updater, params: options, spam_params: spam_params) } subject { service.execute(snippet) } @@ -721,6 +723,10 @@ RSpec.describe Snippets::UpdateService do end end + before do + stub_spam_services + end + context 'when Project Snippet' do let_it_be(:project) { create(:project) } let!(:snippet) { create(:project_snippet, :repository, author: user, project: project) } diff --git a/spec/services/spam/akismet_service_spec.rb b/spec/services/spam/akismet_service_spec.rb index 1cd049da592..d9f62258a53 100644 --- a/spec/services/spam/akismet_service_spec.rb +++ b/spec/services/spam/akismet_service_spec.rb @@ -59,7 +59,7 @@ RSpec.describe Spam::AkismetService do it_behaves_like 'no activity if Akismet is not enabled', :spam?, :check context 'if Akismet is enabled' do - it 'correctly transforms options for the akismet client' do + it 'correctly transforms options for the akismet client, including spelling of referrer key' do expected_check_params = { type: 'comment', text: text, diff --git a/spec/services/spam/spam_action_service_spec.rb b/spec/services/spam/spam_action_service_spec.rb index 9ca52b92267..3a92e5acb5a 100644 --- a/spec/services/spam/spam_action_service_spec.rb +++ b/spec/services/spam/spam_action_service_spec.rb @@ -5,15 +5,20 @@ require 'spec_helper' RSpec.describe Spam::SpamActionService do include_context 'includes Spam constants' - let(:request) { double(:request, env: env, headers: {}) } let(:issue) { create(:issue, project: project, author: user) } let(:fake_ip) { '1.2.3.4' } let(:fake_user_agent) { 'fake-user-agent' } let(:fake_referer) { 'fake-http-referer' } - let(:env) do - { 'action_dispatch.remote_ip' => fake_ip, - 'HTTP_USER_AGENT' => fake_user_agent, - 'HTTP_REFERER' => fake_referer } + let(:captcha_response) { 'abc123' } + let(:spam_log_id) { existing_spam_log.id } + let(:spam_params) do + ::Spam::SpamParams.new( + captcha_response: captcha_response, + spam_log_id: spam_log_id, + ip_address: fake_ip, + user_agent: fake_user_agent, + referer: fake_referer + ) end let_it_be(:project) { create(:project, :public) } @@ -23,32 +28,33 @@ RSpec.describe Spam::SpamActionService do issue.spam = false end - shared_examples 'only checks for spam if a request is provided' do - context 'when request is missing' do - let(:request) { nil } + describe 'constructor argument validation' do + subject do + described_service = described_class.new(spammable: issue, spam_params: spam_params, user: user, action: :create) + described_service.execute + end - it "doesn't check as spam" do - expect(fake_verdict_service).not_to receive(:execute) + context 'when spam_params is nil' do + let(:spam_params) { nil } + let(:expected_service_params_not_present_message) do + /Skipped spam check because spam_params was not present/ + end + it "returns success with a messaage" do response = subject - expect(response.message).to match(/request was not present/) + expect(response.message).to match(expected_service_params_not_present_message) expect(issue).not_to be_spam end end - - context 'when request exists' do - it 'creates a spam log' do - expect { subject } - .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue') - end - end end shared_examples 'creates a spam log' do it do - expect { subject }.to change(SpamLog, :count).by(1) + expect { subject } + .to log_spam(title: issue.title, description: issue.description, noteable_type: 'Issue') + # TODO: These checks should be incorporated into the `log_spam` RSpec matcher above new_spam_log = SpamLog.last expect(new_spam_log.user_id).to eq(user.id) expect(new_spam_log.title).to eq(issue.title) @@ -56,25 +62,14 @@ RSpec.describe Spam::SpamActionService do expect(new_spam_log.source_ip).to eq(fake_ip) expect(new_spam_log.user_agent).to eq(fake_user_agent) expect(new_spam_log.noteable_type).to eq('Issue') - expect(new_spam_log.via_api).to eq(false) + expect(new_spam_log.via_api).to eq(true) end end describe '#execute' do - let(:request) { double(:request, env: env, headers: nil) } let(:fake_captcha_verification_service) { double(:captcha_verification_service) } let(:fake_verdict_service) { double(:spam_verdict_service) } let(:allowlisted) { false } - let(:api) { nil } - let(:captcha_response) { 'abc123' } - let(:spam_log_id) { existing_spam_log.id } - let(:spam_params) do - ::Spam::SpamParams.new( - api: api, - captcha_response: captcha_response, - spam_log_id: spam_log_id - ) - end let(:verdict_service_opts) do { @@ -88,7 +83,6 @@ RSpec.describe Spam::SpamActionService do { target: issue, user: user, - request: request, options: verdict_service_opts, context: { action: :create, @@ -100,40 +94,20 @@ RSpec.describe Spam::SpamActionService do let_it_be(:existing_spam_log) { create(:spam_log, user: user, recaptcha_verified: false) } subject do - described_service = described_class.new(spammable: issue, request: request, user: user, action: :create) + described_service = described_class.new(spammable: issue, spam_params: spam_params, user: user, action: :create) allow(described_service).to receive(:allowlisted?).and_return(allowlisted) - described_service.execute(spam_params: spam_params) + described_service.execute end before do - allow(Captcha::CaptchaVerificationService).to receive(:new) { fake_captcha_verification_service } + allow(Captcha::CaptchaVerificationService).to receive(:new).with(spam_params: spam_params) { fake_captcha_verification_service } allow(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args).and_return(fake_verdict_service) end - context 'when the captcha params are passed in the headers' do - let(:request) { double(:request, env: env, headers: headers) } - let(:spam_params) { Spam::SpamActionService.filter_spam_params!({ api: api }, request) } - let(:headers) do - { - 'X-GitLab-Captcha-Response' => captcha_response, - 'X-GitLab-Spam-Log-Id' => spam_log_id - } - end - - it 'extracts the headers correctly' do - expect(fake_captcha_verification_service) - .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(true) - expect(SpamLog) - .to receive(:verify_recaptcha!).with(user_id: user.id, id: spam_log_id) - - subject - end - end - context 'when captcha response verification returns true' do before do allow(fake_captcha_verification_service) - .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(true) + .to receive(:execute).and_return(true) end it "doesn't check with the SpamVerdictService" do @@ -156,7 +130,7 @@ RSpec.describe Spam::SpamActionService do context 'when captcha response verification returns false' do before do allow(fake_captcha_verification_service) - .to receive(:execute).with(captcha_response: captcha_response, request: request).and_return(false) + .to receive(:execute).and_return(false) end context 'when spammable attributes have not changed' do @@ -200,8 +174,6 @@ RSpec.describe Spam::SpamActionService do stub_feature_flags(allow_possible_spam: false) end - it_behaves_like 'only checks for spam if a request is provided' - it 'marks as spam' do response = subject @@ -211,8 +183,6 @@ RSpec.describe Spam::SpamActionService do end context 'when allow_possible_spam feature flag is true' do - it_behaves_like 'only checks for spam if a request is provided' - it 'does not mark as spam' do response = subject @@ -232,8 +202,6 @@ RSpec.describe Spam::SpamActionService do stub_feature_flags(allow_possible_spam: false) end - it_behaves_like 'only checks for spam if a request is provided' - it 'marks as spam' do response = subject @@ -243,8 +211,6 @@ RSpec.describe Spam::SpamActionService do end context 'when allow_possible_spam feature flag is true' do - it_behaves_like 'only checks for spam if a request is provided' - it 'does not mark as spam' do response = subject @@ -264,8 +230,6 @@ RSpec.describe Spam::SpamActionService do stub_feature_flags(allow_possible_spam: false) end - it_behaves_like 'only checks for spam if a request is provided' - it_behaves_like 'creates a spam log' it 'does not mark as spam' do @@ -284,8 +248,6 @@ RSpec.describe Spam::SpamActionService do end context 'when allow_possible_spam feature flag is true' do - it_behaves_like 'only checks for spam if a request is provided' - it_behaves_like 'creates a spam log' it 'does not mark as needing reCAPTCHA' do @@ -334,37 +296,10 @@ RSpec.describe Spam::SpamActionService do allow(fake_verdict_service).to receive(:execute).and_return(ALLOW) end - context 'when the request is nil' do - let(:request) { nil } - let(:issue_ip_address) { '1.2.3.4' } - let(:issue_user_agent) { 'lynx' } - let(:verdict_service_opts) do - { - ip_address: issue_ip_address, - user_agent: issue_user_agent - } - end - - before do - allow(issue).to receive(:ip_address) { issue_ip_address } - allow(issue).to receive(:user_agent) { issue_user_agent } - end - - it 'assembles the options with information from the spammable' do - # TODO: This code untestable, because we do not perform a verification if there is not a - # request. See corresponding comment in code - # expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args) - - subject - end - end - - context 'when the request is present' do - it 'assembles the options with information from the request' do - expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args) + it 'assembles the options with information from the request' do + expect(Spam::SpamVerdictService).to receive(:new).with(verdict_service_args) - subject - end + subject end end end diff --git a/spec/services/spam/spam_params_spec.rb b/spec/services/spam/spam_params_spec.rb new file mode 100644 index 00000000000..e7e8b468adb --- /dev/null +++ b/spec/services/spam/spam_params_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Spam::SpamParams do + describe '.new_from_request' do + let(:captcha_response) { 'abc123' } + let(:spam_log_id) { 42 } + let(:ip_address) { '0.0.0.0' } + let(:user_agent) { 'Lynx' } + let(:referer) { 'http://localhost' } + let(:headers) do + { + 'X-GitLab-Captcha-Response' => captcha_response, + 'X-GitLab-Spam-Log-Id' => spam_log_id + } + end + + let(:env) do + { + 'action_dispatch.remote_ip' => ip_address, + 'HTTP_USER_AGENT' => user_agent, + 'HTTP_REFERER' => referer + } + end + + let(:request) {double(:request, headers: headers, env: env)} + + it 'constructs from a request' do + expected = ::Spam::SpamParams.new( + captcha_response: captcha_response, + spam_log_id: spam_log_id, + ip_address: ip_address, + user_agent: user_agent, + referer: referer + ) + expect(described_class.new_from_request(request: request)).to eq(expected) + end + end +end diff --git a/spec/services/spam/spam_verdict_service_spec.rb b/spec/services/spam/spam_verdict_service_spec.rb index 215df81de63..9ff2e0ddf55 100644 --- a/spec/services/spam/spam_verdict_service_spec.rb +++ b/spec/services/spam/spam_verdict_service_spec.rb @@ -14,13 +14,11 @@ RSpec.describe Spam::SpamVerdictService do 'HTTP_REFERER' => fake_referer } end - let(:request) { double(:request, env: env) } - let(:check_for_spam) { true } let_it_be(:user) { create(:user) } let_it_be(:issue) { create(:issue, author: user) } let(:service) do - described_class.new(user: user, target: issue, request: request, options: {}) + described_class.new(user: user, target: issue, options: {}) end let(:attribs) do diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index a71fc172ebd..e94ff5bcb45 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -190,6 +190,7 @@ RSpec.configure do |config| config.include RailsHelpers config.include SidekiqMiddleware config.include StubActionCableConnection, type: :channel + config.include StubSpamServices include StubFeatureFlags diff --git a/spec/support/helpers/stub_spam_services.rb b/spec/support/helpers/stub_spam_services.rb new file mode 100644 index 00000000000..841e8366845 --- /dev/null +++ b/spec/support/helpers/stub_spam_services.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module StubSpamServices + def stub_spam_services + allow(::Spam::SpamParams).to receive(:new_from_request) do + ::Spam::SpamParams.new( + captcha_response: double(:captcha_response), + spam_log_id: double(:spam_log_id), + ip_address: double(:ip_address), + user_agent: double(:user_agent), + referer: double(:referer) + ) + end + + allow_next_instance_of(::Spam::SpamActionService) do |service| + allow(service).to receive(:execute) + end + + allow_next_instance_of(::UserAgentDetailService) do |service| + allow(service).to receive(:create) + end + end +end diff --git a/spec/support/shared_examples/graphql/mutations/can_mutate_spammable_examples.rb b/spec/support/shared_examples/graphql/mutations/can_mutate_spammable_examples.rb index 5e15c91cd41..011a2157f24 100644 --- a/spec/support/shared_examples/graphql/mutations/can_mutate_spammable_examples.rb +++ b/spec/support/shared_examples/graphql/mutations/can_mutate_spammable_examples.rb @@ -3,17 +3,13 @@ require 'spec_helper' RSpec.shared_examples 'a mutation which can mutate a spammable' do - describe "#additional_spam_params" do - it 'passes additional spam params to the service' do + describe "#spam_params" do + it 'passes spam params to the service constructor' do args = [ project: anything, current_user: anything, - params: hash_including( - api: true, - request: instance_of(ActionDispatch::Request), - captcha_response: captcha_response, - spam_log_id: spam_log_id - ) + params: anything, + spam_params: instance_of(::Spam::SpamParams) ] expect(service).to receive(:new).with(*args).and_call_original diff --git a/spec/support/shared_examples/graphql/spam_protection_shared_examples.rb b/spec/support/shared_examples/graphql/spam_protection_shared_examples.rb index 8fb89a4f80e..c0b71a494d0 100644 --- a/spec/support/shared_examples/graphql/spam_protection_shared_examples.rb +++ b/spec/support/shared_examples/graphql/spam_protection_shared_examples.rb @@ -57,7 +57,7 @@ RSpec.shared_examples 'has spam protection' do context 'and no CAPTCHA is required' do let(:render_captcha) { false } - it 'does not return a to-level error' do + it 'does not return a top-level error' do send_request expect(graphql_errors).to be_blank diff --git a/spec/support/shared_examples/models/chat_integration_shared_examples.rb b/spec/support/shared_examples/models/chat_integration_shared_examples.rb index 9f3be3e2e06..b5d73350124 100644 --- a/spec/support/shared_examples/models/chat_integration_shared_examples.rb +++ b/spec/support/shared_examples/models/chat_integration_shared_examples.rb @@ -163,7 +163,7 @@ RSpec.shared_examples "chat integration" do |integration_name| context "with issue events" do let(:opts) { { title: "Awesome issue", description: "please fix" } } let(:sample_data) do - service = Issues::CreateService.new(project: project, current_user: user, params: opts) + service = Issues::CreateService.new(project: project, current_user: user, params: opts, spam_params: nil) issue = service.execute service.hook_data(issue, "open") end diff --git a/spec/support/shared_examples/services/snippets_shared_examples.rb b/spec/support/shared_examples/services/snippets_shared_examples.rb index 0c4db7ded69..56b2ce3353d 100644 --- a/spec/support/shared_examples/services/snippets_shared_examples.rb +++ b/spec/support/shared_examples/services/snippets_shared_examples.rb @@ -1,23 +1,6 @@ # frozen_string_literal: true RSpec.shared_examples 'checking spam' do - let(:request) { double(:request, headers: headers) } - let(:headers) { nil } - let(:api) { true } - let(:captcha_response) { 'abc123' } - let(:spam_log_id) { 1 } - let(:disable_spam_action_service) { false } - - let(:extra_opts) do - { - request: request, - api: api, - captcha_response: captcha_response, - spam_log_id: spam_log_id, - disable_spam_action_service: disable_spam_action_service - } - end - before do allow_next_instance_of(UserAgentDetailService) do |instance| allow(instance).to receive(:create) @@ -25,67 +8,26 @@ RSpec.shared_examples 'checking spam' do end it 'executes SpamActionService' do - spam_params = Spam::SpamParams.new( - api: api, - captcha_response: captcha_response, - spam_log_id: spam_log_id - ) expect_next_instance_of( Spam::SpamActionService, { spammable: kind_of(Snippet), - request: request, + spam_params: spam_params, user: an_instance_of(User), action: action } ) do |instance| - expect(instance).to receive(:execute).with(spam_params: spam_params) + expect(instance).to receive(:execute) end subject end - context 'when CAPTCHA arguments are passed in the headers' do - let(:headers) do - { - 'X-GitLab-Spam-Log-Id' => spam_log_id, - 'X-GitLab-Captcha-Response' => captcha_response - } + context 'when snippet_spam flag is disabled' do + before do + stub_feature_flags(snippet_spam: false) end - let(:extra_opts) do - { - request: request, - api: api, - disable_spam_action_service: disable_spam_action_service - } - end - - it 'executes the SpamActionService correctly' do - spam_params = Spam::SpamParams.new( - api: api, - captcha_response: captcha_response, - spam_log_id: spam_log_id - ) - expect_next_instance_of( - Spam::SpamActionService, - { - spammable: kind_of(Snippet), - request: request, - user: an_instance_of(User), - action: action - } - ) do |instance| - expect(instance).to receive(:execute).with(spam_params: spam_params) - end - - subject - end - end - - context 'when spam action service is disabled' do - let(:disable_spam_action_service) { true } - it 'request parameter is not passed to the service' do expect(Spam::SpamActionService).not_to receive(:new) |