diff --git a/app/graphql/resolvers/issues_resolver.rb b/app/graphql/resolvers/issues_resolver.rb index 26092cc12ec..ae27cce9113 100644 --- a/app/graphql/resolvers/issues_resolver.rb +++ b/app/graphql/resolvers/issues_resolver.rb @@ -10,7 +10,7 @@ module Resolvers argument :sort, Types::IssueSortEnum, description: 'Sort issues by this criteria', required: false, - default_value: 'created_desc' + default_value: :created_desc type Types::IssueType.connection_type, null: true diff --git a/app/graphql/resolvers/merge_requests_resolver.rb b/app/graphql/resolvers/merge_requests_resolver.rb index cb4a76243ae..c6b9448c9b6 100644 --- a/app/graphql/resolvers/merge_requests_resolver.rb +++ b/app/graphql/resolvers/merge_requests_resolver.rb @@ -52,7 +52,7 @@ module Resolvers argument :sort, Types::MergeRequestSortEnum, description: 'Sort merge requests by this criteria', required: false, - default_value: 'created_desc' + default_value: :created_desc def self.single ::Resolvers::MergeRequestResolver diff --git a/app/graphql/resolvers/users_resolver.rb b/app/graphql/resolvers/users_resolver.rb index f5838642141..a0ed076595d 100644 --- a/app/graphql/resolvers/users_resolver.rb +++ b/app/graphql/resolvers/users_resolver.rb @@ -17,7 +17,7 @@ module Resolvers argument :sort, Types::SortEnum, description: 'Sort users by this criteria', required: false, - default_value: 'created_desc' + default_value: :created_desc argument :search, GraphQL::STRING_TYPE, required: false, diff --git a/app/graphql/types/sort_enum.rb b/app/graphql/types/sort_enum.rb index d0a6eecb672..c3a76330fe9 100644 --- a/app/graphql/types/sort_enum.rb +++ b/app/graphql/types/sort_enum.rb @@ -7,10 +7,10 @@ module Types # Deprecated, as we prefer uppercase enums # https://gitlab.com/groups/gitlab-org/-/epics/1838 - value 'updated_desc', 'Updated at descending order', deprecated: { reason: 'Use UPDATED_DESC', milestone: '13.5' } - value 'updated_asc', 'Updated at ascending order', deprecated: { reason: 'Use UPDATED_ASC', milestone: '13.5' } - value 'created_desc', 'Created at descending order', deprecated: { reason: 'Use CREATED_DESC', milestone: '13.5' } - value 'created_asc', 'Created at ascending order', deprecated: { reason: 'Use CREATED_ASC', milestone: '13.5' } + value 'updated_desc', 'Updated at descending order', value: :updated_desc, deprecated: { reason: 'Use UPDATED_DESC', milestone: '13.5' } + value 'updated_asc', 'Updated at ascending order', value: :updated_asc, deprecated: { reason: 'Use UPDATED_ASC', milestone: '13.5' } + value 'created_desc', 'Created at descending order', value: :created_desc, deprecated: { reason: 'Use CREATED_DESC', milestone: '13.5' } + value 'created_asc', 'Created at ascending order', value: :created_asc, deprecated: { reason: 'Use CREATED_ASC', milestone: '13.5' } value 'UPDATED_DESC', 'Updated at descending order', value: :updated_desc value 'UPDATED_ASC', 'Updated at ascending order', value: :updated_asc diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb index d126981fb27..6679b6224ed 100644 --- a/app/helpers/users_helper.rb +++ b/app/helpers/users_helper.rb @@ -129,7 +129,7 @@ module UsersHelper } end - def unblock_user_modal_data(user) + def user_unblock_data(user) { path: unblock_admin_user_path(user), method: 'put', @@ -168,6 +168,19 @@ module UsersHelper } end + def user_activation_data(user) + { + path: activate_admin_user_path(user), + method: 'put', + modal_attributes: { + title: s_('AdminUsers|Activate user %{username}?') % { username: sanitize_name(user.name) }, + message: s_('AdminUsers|You can always deactivate their account again if needed.'), + okVariant: 'info', + okTitle: s_('AdminUsers|Activate') + }.to_json + } + end + def user_deactivation_effects header = tag.p s_('AdminUsers|Deactivating a user has the following effects:') diff --git a/app/views/admin/users/_user.html.haml b/app/views/admin/users/_user.html.haml index 73c6534d471..905f2946370 100644 --- a/app/views/admin/users/_user.html.haml +++ b/app/views/admin/users/_user.html.haml @@ -40,7 +40,7 @@ %button.btn.btn-default-tertiary.js-confirm-modal-button{ data: user_block_data(user, user_block_effects) } = s_('AdminUsers|Block') - else - %button.btn.btn-default-tertiary.js-confirm-modal-button{ data: unblock_user_modal_data(user) } + %button.btn.btn-default-tertiary.js-confirm-modal-button{ data: user_unblock_data(user) } = s_('AdminUsers|Unblock') - else %button.btn.btn-default-tertiary.js-confirm-modal-button{ data: user_block_data(user, user_block_effects) } @@ -51,7 +51,8 @@ = s_('AdminUsers|Deactivate') - elsif user.deactivated? %li - = link_to _('Activate'), activate_admin_user_path(user), method: :put + %button.btn.btn-default-tertiary.js-confirm-modal-button{ data: user_activation_data(user) } + = s_('AdminUsers|Activate') - if user.access_locked? %li = link_to _('Unlock'), unlock_admin_user_path(user), method: :put, data: { confirm: _('Are you sure?') } diff --git a/app/views/admin/users/show.html.haml b/app/views/admin/users/show.html.haml index 767bba78e4e..85545e33f0c 100644 --- a/app/views/admin/users/show.html.haml +++ b/app/views/admin/users/show.html.haml @@ -158,7 +158,8 @@ .card-body = render partial: 'admin/users/user_activation_effects' %br - = link_to 'Activate user', activate_admin_user_path(@user), method: :put, class: "btn gl-button btn-info", data: { confirm: 'Are you sure?' } + %button.btn.gl-button.btn-info.js-confirm-modal-button{ data: user_activation_data(@user) } + = s_('AdminUsers|Activate user') - elsif @user.can_be_deactivated? .card.border-warning .card-header.bg-warning.text-white @@ -182,7 +183,7 @@ %li Log in %li Access Git repositories %br - %button.btn.gl-button.btn-info.js-confirm-modal-button{ data: unblock_user_modal_data(@user) } + %button.btn.gl-button.btn-info.js-confirm-modal-button{ data: user_unblock_data(@user) } = s_('AdminUsers|Unblock user') - elsif !@user.internal? = render 'admin/users/block_user', user: @user diff --git a/changelogs/unreleased/267118-add-reactivate-user-admin-approval-modal-to-glmodal.yml b/changelogs/unreleased/267118-add-reactivate-user-admin-approval-modal-to-glmodal.yml new file mode 100644 index 00000000000..39454d2dbc5 --- /dev/null +++ b/changelogs/unreleased/267118-add-reactivate-user-admin-approval-modal-to-glmodal.yml @@ -0,0 +1,5 @@ +--- +title: Add confirm modal to reactivate user +merge_request: 48173 +author: +type: added diff --git a/changelogs/unreleased/improve-feature-flag-logging.yml b/changelogs/unreleased/improve-feature-flag-logging.yml new file mode 100644 index 00000000000..cecaccd2c61 --- /dev/null +++ b/changelogs/unreleased/improve-feature-flag-logging.yml @@ -0,0 +1,5 @@ +--- +title: Improve logging on feature flag modification +merge_request: 48417 +author: +type: other diff --git a/doc/.vale/gitlab/spelling-exceptions.txt b/doc/.vale/gitlab/spelling-exceptions.txt index 27b7ae91e19..772a4165667 100644 --- a/doc/.vale/gitlab/spelling-exceptions.txt +++ b/doc/.vale/gitlab/spelling-exceptions.txt @@ -224,6 +224,7 @@ Kibana Kinesis Knative Kramdown +Kroki Kubecost kubectl Kubernetes diff --git a/doc/administration/integration/kroki.md b/doc/administration/integration/kroki.md index 10706aabe7b..dead5640873 100644 --- a/doc/administration/integration/kroki.md +++ b/doc/administration/integration/kroki.md @@ -1,4 +1,4 @@ -# Kroki diagrams **(CORE)** +# Kroki diagrams **(CORE ONLY)** > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/241744) in GitLab 13.7. @@ -10,7 +10,7 @@ GitLab you can use it to create diagrams in AsciiDoc and Markdown documents. When Kroki is enabled, GitLab sends diagrams to an instance of Kroki to display them as images. You can use the free public cloud instance `https://kroki.io` or you can [install Kroki](https://docs.kroki.io/kroki/setup/install/) on your own infrastructure. -Once you've installed Kroki, make sure to update the server URL to point to your instance. +After you've installed Kroki, make sure to update the server URL to point to your instance. ### Docker @@ -38,12 +38,12 @@ The [`yuzutech/kroki`](https://hub.docker.com/r/yuzutech/kroki) image contains t - [WaveDrom](https://wavedrom.com/) If you want to use additional diagram libraries, -read the [Kroki installation](https://docs.kroki.io/kroki/setup/install/#_images) to learn how to start Kroki companion containers. +read the [Kroki installation](https://docs.kroki.io/kroki/setup/install/#_images) to learn how to start Kroki companion containers. -## Enable Kroki in GitLab **(CORE ONLY)** +## Enable Kroki in GitLab You need to enable Kroki integration from Settings under Admin Area. -To do that, log in with an Admin account and follow these steps: +To do that, log in with an administrator account and follow these steps: 1. Select the Admin Area (**{admin}**) icon. 1. Navigate to **Settings > General**. diff --git a/doc/administration/logs.md b/doc/administration/logs.md index 6e91217b849..117ff27f670 100644 --- a/doc/administration/logs.md +++ b/doc/administration/logs.md @@ -665,6 +665,31 @@ installations from source. It logs the progress of the export process. +## `features_json.log` + +> [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/issues/59587) in GitLab 13.7. + +This file's location depends on how you installed GitLab: + +- For Omnibus GitLab packages: `/var/log/gitlab/gitlab-rails/features_json.log` +- For installations from source: `/home/git/gitlab/log/features_json.log` + +The modification events from [Feature flags in development of GitLab](../development/feature_flags/index.md) +are recorded in this file. For example: + +```json +{"severity":"INFO","time":"2020-11-24T02:30:59.860Z","correlation_id":null,"key":"cd_auto_rollback","action":"enable","extra.thing":"true"} +{"severity":"INFO","time":"2020-11-24T02:31:29.108Z","correlation_id":null,"key":"cd_auto_rollback","action":"enable","extra.thing":"true"} +{"severity":"INFO","time":"2020-11-24T02:31:29.129Z","correlation_id":null,"key":"cd_auto_rollback","action":"disable","extra.thing":"false"} +{"severity":"INFO","time":"2020-11-24T02:31:29.177Z","correlation_id":null,"key":"cd_auto_rollback","action":"enable","extra.thing":"Project:1"} +{"severity":"INFO","time":"2020-11-24T02:31:29.183Z","correlation_id":null,"key":"cd_auto_rollback","action":"disable","extra.thing":"Project:1"} +{"severity":"INFO","time":"2020-11-24T02:31:29.188Z","correlation_id":null,"key":"cd_auto_rollback","action":"enable_percentage_of_time","extra.percentage":"50"} +{"severity":"INFO","time":"2020-11-24T02:31:29.193Z","correlation_id":null,"key":"cd_auto_rollback","action":"disable_percentage_of_time"} +{"severity":"INFO","time":"2020-11-24T02:31:29.198Z","correlation_id":null,"key":"cd_auto_rollback","action":"enable_percentage_of_actors","extra.percentage":"50"} +{"severity":"INFO","time":"2020-11-24T02:31:29.203Z","correlation_id":null,"key":"cd_auto_rollback","action":"disable_percentage_of_actors"} +{"severity":"INFO","time":"2020-11-24T02:31:29.329Z","correlation_id":null,"key":"cd_auto_rollback","action":"remove"} +``` + ## `auth.log` > Introduced in GitLab 12.0. diff --git a/doc/development/feature_flags/controls.md b/doc/development/feature_flags/controls.md index df737912c00..f24a981dc09 100644 --- a/doc/development/feature_flags/controls.md +++ b/doc/development/feature_flags/controls.md @@ -228,8 +228,10 @@ existing gates (e.g. `--group=gitlab-org`) in the above processes. ### Feature flag change logging -Any feature flag change that affects GitLab.com (production) will -automatically be logged in an issue. +#### Chatops level + +Any feature flag change that affects GitLab.com (production) via [Chatops](https://gitlab.com/gitlab-com/chatops) +is automatically logged in an issue. The issue is created in the [gl-infra/feature-flag-log](https://gitlab.com/gitlab-com/gl-infra/feature-flag-log/-/issues?scope=all&utf8=%E2%9C%93&state=closed) @@ -243,6 +245,12 @@ marker to make the change even more visible. Changes to the issue format can be submitted in the [Chatops project](https://gitlab.com/gitlab-com/chatops). +#### Instance level + +Any feature flag change that affects any GitLab instance is automatically logged in +[features_json.log](../../administration/logs.md#features_jsonlog). +You can search the change history in [Kibana](https://about.gitlab.com/handbook/support/workflows/kibana.html). + ## Cleaning up A feature flag should be removed as soon as it is no longer needed. Each additional diff --git a/doc/development/graphql_guide/pagination.md b/doc/development/graphql_guide/pagination.md index 1f659bffab3..130ed5721f3 100644 --- a/doc/development/graphql_guide/pagination.md +++ b/doc/development/graphql_guide/pagination.md @@ -294,28 +294,30 @@ The shared example requires certain `let` variables and methods to be set up: ```ruby describe 'sorting and pagination' do - let(:sort_project) { create(:project, :public) } + let_it_be(:sort_project) { create(:project, :public) } let(:data_path) { [:project, :issues] } - def pagination_query(params, page_info) - graphql_query_for( - 'project', - { 'fullPath' => sort_project.full_path }, - query_graphql_field('issues', params, "#{page_info} edges { node { id } }") + def pagination_query(params) + graphql_query_for( :project, { full_path: sort_project.full_path }, + query_nodes(:issues, :id, include_pagination_info: true, args: params)) ) end - def pagination_results_data(data) - data.map { |issue| issue.dig('node', 'iid').to_i } + def pagination_results_data(nodes) + nodes.map { |issue| issue['iid'].to_i } end context 'when sorting by weight' do - ... + let_it_be(:issues) { make_some_issues_with_weights } + context 'when ascending' do + let(:ordered_issues) { issues.sort_by(&:weight) } + it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'WEIGHT_ASC' } + let(:sort_param) { :WEIGHT_ASC } let(:first_param) { 2 } - let(:expected_results) { [weight_issue3.iid, weight_issue5.iid, weight_issue1.iid, weight_issue4.iid, weight_issue2.iid] } + let(:expected_results) { ordered_issues.map(&:iid) } end end + end ``` diff --git a/doc/development/testing_guide/frontend_testing.md b/doc/development/testing_guide/frontend_testing.md index f933daab9aa..c76e6d1880e 100644 --- a/doc/development/testing_guide/frontend_testing.md +++ b/doc/development/testing_guide/frontend_testing.md @@ -25,17 +25,19 @@ If you are looking for a guide on Vue component testing, you can jump right away ## Jest -We have started to migrate frontend tests to the [Jest](https://jestjs.io) testing framework (see also the corresponding -[epic](https://gitlab.com/groups/gitlab-org/-/epics/895)). - +We use Jest to write frontend unit and integration tests. Jest tests can be found in `/spec/frontend` and `/ee/spec/frontend` in EE. -Most examples have a Jest and Karma example. See the Karma examples only as explanation to what's going on in the code, should you stumble over some use cases during your discovery. The Jest examples are the one you should follow. - ## Karma test suite -While GitLab is switching over to [Jest](https://jestjs.io) you'll still find Karma tests in our application. [Karma](http://karma-runner.github.io/) is a test runner which uses [Jasmine](https://jasmine.github.io/) as its test -framework. Jest also uses Jasmine as foundation, that's why it's looking quite similar. +While GitLab has switched over to [Jest](https://jestjs.io) you'll still find Karma tests in our +application because some of our specs require a browser and can't be easiliy migrated to Jest. +Those specs will eventually drop Karma in favor of either Jest or RSpec. You can track this migration +in the [related epic](https://gitlab.com/groups/gitlab-org/-/epics/4900). + +[Karma](http://karma-runner.github.io/) is a test runner which uses +[Jasmine](https://jasmine.github.io/) as its test framework. Jest also uses Jasmine as foundation, +that's why it's looking quite similar. Karma tests live in `spec/javascripts/` and `/ee/spec/javascripts` in EE. @@ -47,19 +49,6 @@ browser and you will not have access to certain APIs, such as [`Notification`](https://developer.mozilla.org/en-US/docs/Web/API/notification), which have to be stubbed. -### When should I use Jest over Karma? - -If you need to update an existing Karma test file (found in `spec/javascripts`), you do not -need to migrate the whole spec to Jest. Simply updating the Karma spec to test your change -is fine. It is probably more appropriate to migrate to Jest in a separate merge request. - -If you create a new test file, it needs to be created in Jest. This will -help support our migration and we think you'll love using Jest. - -As always, please use discretion. Jest solves a lot of issues we experienced in Karma and -provides a better developer experience, however there are potentially unexpected issues -which could arise (especially with testing against browser specific features). - ### Differences to Karma - Jest runs in a Node.js environment, not in a browser. Support for running Jest tests in a browser [is planned](https://gitlab.com/gitlab-org/gitlab/-/issues/26982). @@ -776,11 +765,10 @@ Please consult the [official Jest docs](https://jestjs.io/docs/en/jest-object#mo For running the frontend tests, you need the following commands: -- `rake frontend:fixtures` (re-)generates [fixtures](#frontend-test-fixtures). -- `yarn test` executes the tests. -- `yarn jest` executes only the Jest tests. - -As long as the fixtures don't change, `yarn test` is sufficient (and saves you some time). +- `rake frontend:fixtures` (re-)generates [fixtures](#frontend-test-fixtures). Make sure that + fixtures are up-to-date before running tests that require them. +- `yarn jest` runs Jest tests. +- `yarn karma` runs Karma tests. ### Live testing and focused testing -- Jest diff --git a/doc/user/permissions.md b/doc/user/permissions.md index 38f25e38937..0cd1d3066fc 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -245,8 +245,8 @@ group. | Browse group | ✓ | ✓ | ✓ | ✓ | ✓ | | View group wiki pages **(PREMIUM)** | ✓ (6) | ✓ | ✓ | ✓ | ✓ | | View Insights charts **(ULTIMATE)** | ✓ | ✓ | ✓ | ✓ | ✓ | -| View group epic **(ULTIMATE)** | ✓ | ✓ | ✓ | ✓ | ✓ | -| Create/edit group epic **(ULTIMATE)** | | ✓ | ✓ | ✓ | ✓ | +| View group epic **(PREMIUM)** | ✓ | ✓ | ✓ | ✓ | ✓ | +| Create/edit group epic **(PREMIUM)** | | ✓ | ✓ | ✓ | ✓ | | Manage group labels | | ✓ | ✓ | ✓ | ✓ | | See a container registry | | ✓ | ✓ | ✓ | ✓ | | Pull [packages](packages/index.md) | | ✓ | ✓ | ✓ | ✓ | @@ -270,7 +270,7 @@ group. | Create/Delete group deploy tokens | | | | | ✓ | | Manage group members | | | | | ✓ | | Delete group | | | | | ✓ | -| Delete group epic **(ULTIMATE)** | | | | | ✓ | +| Delete group epic **(PREMIUM)** | | | | | ✓ | | Edit SAML SSO Billing **(SILVER ONLY)** | ✓ | ✓ | ✓ | ✓ | ✓ (4) | | View group Audit Events | | | | | ✓ | | Disable notification emails | | | | | ✓ | diff --git a/lib/feature.rb b/lib/feature.rb index c9871881dc9..780774ae3e3 100644 --- a/lib/feature.rb +++ b/lib/feature.rb @@ -87,40 +87,39 @@ class Feature end def enable(key, thing = true) + log(key: key, action: __method__, thing: thing) get(key).enable(thing) end def disable(key, thing = false) + log(key: key, action: __method__, thing: thing) get(key).disable(thing) end - def enable_group(key, group) - get(key).enable_group(group) - end - - def disable_group(key, group) - get(key).disable_group(group) - end - def enable_percentage_of_time(key, percentage) + log(key: key, action: __method__, percentage: percentage) get(key).enable_percentage_of_time(percentage) end def disable_percentage_of_time(key) + log(key: key, action: __method__) get(key).disable_percentage_of_time end def enable_percentage_of_actors(key, percentage) + log(key: key, action: __method__, percentage: percentage) get(key).enable_percentage_of_actors(percentage) end def disable_percentage_of_actors(key) + log(key: key, action: __method__) get(key).disable_percentage_of_actors end def remove(key) return unless persisted_name?(key) + log(key: key, action: __method__) get(key).remove end @@ -145,6 +144,10 @@ class Feature Feature::Definition.register_hot_reloader! end + def logger + @logger ||= Feature::Logger.build + end + private def flipper @@ -192,6 +195,14 @@ class Feature def l2_cache_backend Rails.cache end + + def log(key:, action:, **extra) + extra ||= {} + extra = extra.transform_keys { |k| "extra.#{k}" } + extra = extra.transform_values { |v| v.respond_to?(:flipper_id) ? v.flipper_id : v } + extra = extra.transform_values(&:to_s) + logger.info(key: key, action: action, **extra) + end end class Target diff --git a/lib/feature/logger.rb b/lib/feature/logger.rb new file mode 100644 index 00000000000..784a619e182 --- /dev/null +++ b/lib/feature/logger.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class Feature + class Logger < ::Gitlab::JsonLogger + def self.file_name_noext + 'features_json' + end + end +end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 0d03e1ffb64..ce4145eced9 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1489,9 +1489,6 @@ msgstr "" msgid "Actions" msgstr "" -msgid "Activate" -msgstr "" - msgid "Activate Service Desk" msgstr "" @@ -2070,6 +2067,15 @@ msgstr "" msgid "AdminUsers|Access the API" msgstr "" +msgid "AdminUsers|Activate" +msgstr "" + +msgid "AdminUsers|Activate user" +msgstr "" + +msgid "AdminUsers|Activate user %{username}?" +msgstr "" + msgid "AdminUsers|Active" msgstr "" @@ -2259,6 +2265,9 @@ msgstr "" msgid "AdminUsers|You can always block their account again if needed." msgstr "" +msgid "AdminUsers|You can always deactivate their account again if needed." +msgstr "" + msgid "AdminUsers|You can always re-activate their account, their data will remain intact." msgstr "" @@ -26931,9 +26940,6 @@ msgstr "" msgid "The Prometheus server responded with \"bad request\". Please check your queries are correct and are supported in your Prometheus version. %{documentationLink}" msgstr "" -msgid "The Security Dashboard shows the results of the last successful pipeline run on the default branch." -msgstr "" - msgid "The URL defined on the primary node that secondary nodes should use to contact it." msgstr "" @@ -26943,6 +26949,9 @@ msgstr "" msgid "The URL to use for connecting to Elasticsearch. Use a comma-separated list to support clustering (e.g., \"http://localhost:9200, http://localhost:9201\")." msgstr "" +msgid "The Vulnerability Report shows the results of the last successful pipeline run on the default branch." +msgstr "" + msgid "The X509 Certificate to use when mutual TLS is required to communicate with the external authorization service. If left blank, the server certificate is still validated when accessing over HTTPS." msgstr "" diff --git a/package.json b/package.json index db0300f8bd5..b7d567a5354 100644 --- a/package.json +++ b/package.json @@ -30,7 +30,6 @@ "stylelint": "yarn stylelint-file 'app/assets/stylesheets/**/*.*' 'ee/app/assets/stylesheets/**/*.*' '!app/assets/stylesheets/startup/startup-*.scss' '!**/vendors/**'", "stylelint-file": "BROWSERSLIST_IGNORE_OLD_DATA=true node node_modules/stylelint/bin/stylelint.js", "stylelint-create-utility-map": "node scripts/frontend/stylelint/stylelint-utility-map.js", - "test": "node scripts/frontend/test", "webpack": "NODE_OPTIONS=\"--max-old-space-size=3584\" webpack --config config/webpack.config.js", "webpack-vendor": "NODE_OPTIONS=\"--max-old-space-size=3584\" webpack --config config/webpack.vendor.config.js", "webpack-prod": "NODE_OPTIONS=\"--max-old-space-size=3584\" NODE_ENV=production webpack --config config/webpack.config.js" diff --git a/scripts/frontend/test.js b/scripts/frontend/test.js deleted file mode 100755 index 71a8bebf0f2..00000000000 --- a/scripts/frontend/test.js +++ /dev/null @@ -1,123 +0,0 @@ -#!/usr/bin/env node - -const { spawn } = require('child_process'); -const { EOL } = require('os'); -const program = require('commander'); -const chalk = require('chalk'); - -const SUCCESS_CODE = 0; -const JEST_ROUTE = 'spec/frontend'; -const KARMA_ROUTE = 'spec/javascripts'; -const COMMON_ARGS = ['--colors']; -const jestArgs = [...COMMON_ARGS, '--passWithNoTests']; -const karmaArgs = [...COMMON_ARGS, '--no-fail-on-empty-test-suite']; - -program - .usage('[options] ') - .option('-p, --parallel', 'Run tests suites in parallel') - .option( - '-w, --watch', - 'Rerun tests when files change (tests will be run in parallel if this enabled)', - ) - .parse(process.argv); - -const shouldParallelize = program.parallel || program.watch; - -const isSuccess = code => code === SUCCESS_CODE; - -const combineExitCodes = codes => { - const firstFail = codes.find(x => !isSuccess(x)); - - return firstFail === undefined ? SUCCESS_CODE : firstFail; -}; - -const skipIfFail = fn => code => (isSuccess(code) ? fn() : code); - -const endWithEOL = str => (str[str.length - 1] === '\n' ? str : `${str}${EOL}`); - -const runTests = paths => { - if (shouldParallelize) { - return Promise.all([runJest(paths), runKarma(paths)]).then(combineExitCodes); - } else { - return runJest(paths).then(skipIfFail(() => runKarma(paths))); - } -}; - -const spawnYarnScript = (cmd, args) => { - return new Promise((resolve, reject) => { - const proc = spawn('yarn', ['run', cmd, ...args]); - const output = data => { - const text = data - .toString() - .split(/\r?\n/g) - .map((line, idx, { length }) => - idx === length - 1 && !line ? line : `${chalk.gray(cmd)}: ${line}`, - ) - .join(EOL); - - return endWithEOL(text); - }; - - proc.stdout.on('data', data => { - process.stdout.write(output(data)); - }); - - proc.stderr.on('data', data => { - process.stderr.write(output(data)); - }); - - proc.on('close', code => { - process.stdout.write(output(`exited with code ${code}`)); - - // We resolve even on a failure code because a `reject` would cause - // Promise.all to reject immediately (without waiting for other promises) - // to finish. - resolve(code); - }); - }); -}; - -const runJest = args => { - return spawnYarnScript('jest', [...jestArgs, ...toJestArgs(args)]); -}; - -const runKarma = args => { - return spawnYarnScript('karma', [...karmaArgs, ...toKarmaArgs(args)]); -}; - -const replacePath = to => path => - path - .replace(JEST_ROUTE, to) - .replace(KARMA_ROUTE, to) - .replace('app/assets/javascripts', to); - -const replacePathForJest = replacePath(JEST_ROUTE); - -const replacePathForKarma = replacePath(KARMA_ROUTE); - -const toJestArgs = paths => paths.map(replacePathForJest); - -const toKarmaArgs = paths => - paths.reduce((acc, path) => acc.concat('-f', replacePathForKarma(path)), []); - -const main = paths => { - if (program.watch) { - jestArgs.push('--watch'); - karmaArgs.push('--single-run', 'false', '--auto-watch'); - } - runTests(paths).then(code => { - console.log('~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~'); - if (isSuccess(code)) { - console.log(chalk.bgGreen(chalk.black('All tests passed :)'))); - } else { - console.log(chalk.bgRed(chalk.white(`Some tests failed :(`))); - } - console.log('~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~'); - - if (!isSuccess(code)) { - process.exit(code); - } - }); -}; - -main(program.args); diff --git a/spec/features/admin/users/user_spec.rb b/spec/features/admin/users/user_spec.rb index 43df600bbfa..cbf5c8db169 100644 --- a/spec/features/admin/users/user_spec.rb +++ b/spec/features/admin/users/user_spec.rb @@ -77,8 +77,8 @@ RSpec.describe 'Admin::Users::User' do end end - context 'when deactivating the user' do - it 'shows confirmation and allows blocking', :js do + context 'when deactivating/re-activating the user' do + it 'shows confirmation and allows deactivating/re-activating', :js do visit admin_user_path(user) find('button', text: 'Deactivate user').click @@ -94,6 +94,20 @@ RSpec.describe 'Admin::Users::User' do expect(page).to have_content('Successfully deactivated') expect(page).to have_content('Reactivate this user') + + find('button', text: 'Activate user').click + + wait_for_requests + + expect(page).to have_content('Activate user') + expect(page).to have_content('You can always deactivate their account again if needed.') + + find('.modal-footer button', text: 'Activate').click + + wait_for_requests + + expect(page).to have_content('Successfully activated') + expect(page).to have_content('Deactivate this user') end end diff --git a/spec/features/admin/users/users_spec.rb b/spec/features/admin/users/users_spec.rb index f2163c4e900..afabdcf4fb7 100644 --- a/spec/features/admin/users/users_spec.rb +++ b/spec/features/admin/users/users_spec.rb @@ -207,11 +207,7 @@ RSpec.describe 'Admin::Users' do it 'shows confirmation and allows blocking and unblocking', :js do expect(page).to have_content(user.email) - find("[data-testid='user-action-button-#{user.id}']").click - - within find("[data-testid='user-action-dropdown-#{user.id}']") do - find('li button', text: 'Block').click - end + click_action_in_user_dropdown(user.id, 'Block') wait_for_requests @@ -233,13 +229,7 @@ RSpec.describe 'Admin::Users' do expect(page).to have_content(user.email) - find("[data-testid='user-action-button-#{user.id}']").click - - within find("[data-testid='user-action-dropdown-#{user.id}']") do - find('li button', text: 'Unblock').click - end - - wait_for_requests + click_action_in_user_dropdown(user.id, 'Unblock') expect(page).to have_content('Unblock user') expect(page).to have_content('You can always block their account again if needed.') @@ -253,17 +243,11 @@ RSpec.describe 'Admin::Users' do end end - context 'when deactivating a user' do - it 'shows confirmation and allows deactivating', :js do + context 'when deactivating/re-activating a user' do + it 'shows confirmation and allows deactivating and re-activating', :js do expect(page).to have_content(user.email) - find("[data-testid='user-action-button-#{user.id}']").click - - within find("[data-testid='user-action-dropdown-#{user.id}']") do - find('li button', text: 'Deactivate').click - end - - wait_for_requests + click_action_in_user_dropdown(user.id, 'Deactivate') expect(page).to have_content('Deactivate user') expect(page).to have_content('Deactivating a user has the following effects') @@ -276,8 +260,36 @@ RSpec.describe 'Admin::Users' do expect(page).to have_content('Successfully deactivated') expect(page).not_to have_content(user.email) + + click_link 'Deactivated' + + wait_for_requests + + expect(page).to have_content(user.email) + + click_action_in_user_dropdown(user.id, 'Activate') + + expect(page).to have_content('Activate user') + expect(page).to have_content('You can always deactivate their account again if needed.') + + find('.modal-footer button', text: 'Activate').click + + wait_for_requests + + expect(page).to have_content('Successfully activated') + expect(page).not_to have_content(user.email) end end + + def click_action_in_user_dropdown(user_id, action) + find("[data-testid='user-action-button-#{user_id}']").click + + within find("[data-testid='user-action-dropdown-#{user_id}']") do + find('li button', text: action).click + end + + wait_for_requests + end end describe 'GET /admin/users/new' do diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index 5dff9dbd995..33b6ff08532 100644 --- a/spec/lib/feature_spec.rb +++ b/spec/lib/feature_spec.rb @@ -300,7 +300,119 @@ RSpec.describe Feature, stub_feature_flags: false do end end + shared_examples_for 'logging' do + let(:expected_action) { } + let(:expected_extra) { } + + it 'logs the event' do + expect(Feature.logger).to receive(:info).with(key: key, action: expected_action, **expected_extra) + + subject + end + end + + describe '.enable' do + subject { described_class.enable(key, thing) } + + let(:key) { :awesome_feature } + let(:thing) { true } + + it_behaves_like 'logging' do + let(:expected_action) { :enable } + let(:expected_extra) { { "extra.thing" => "true" } } + end + + context 'when thing is an actor' do + let(:thing) { create(:project) } + + it_behaves_like 'logging' do + let(:expected_action) { :enable } + let(:expected_extra) { { "extra.thing" => "#{thing.flipper_id}" } } + end + end + end + + describe '.disable' do + subject { described_class.disable(key, thing) } + + let(:key) { :awesome_feature } + let(:thing) { false } + + it_behaves_like 'logging' do + let(:expected_action) { :disable } + let(:expected_extra) { { "extra.thing" => "false" } } + end + + context 'when thing is an actor' do + let(:thing) { create(:project) } + + it_behaves_like 'logging' do + let(:expected_action) { :disable } + let(:expected_extra) { { "extra.thing" => "#{thing.flipper_id}" } } + end + end + end + + describe '.enable_percentage_of_time' do + subject { described_class.enable_percentage_of_time(key, percentage) } + + let(:key) { :awesome_feature } + let(:percentage) { 50 } + + it_behaves_like 'logging' do + let(:expected_action) { :enable_percentage_of_time } + let(:expected_extra) { { "extra.percentage" => "#{percentage}" } } + end + end + + describe '.disable_percentage_of_time' do + subject { described_class.disable_percentage_of_time(key) } + + let(:key) { :awesome_feature } + + it_behaves_like 'logging' do + let(:expected_action) { :disable_percentage_of_time } + let(:expected_extra) { {} } + end + end + + describe '.enable_percentage_of_actors' do + subject { described_class.enable_percentage_of_actors(key, percentage) } + + let(:key) { :awesome_feature } + let(:percentage) { 50 } + + it_behaves_like 'logging' do + let(:expected_action) { :enable_percentage_of_actors } + let(:expected_extra) { { "extra.percentage" => "#{percentage}" } } + end + end + + describe '.disable_percentage_of_actors' do + subject { described_class.disable_percentage_of_actors(key) } + + let(:key) { :awesome_feature } + + it_behaves_like 'logging' do + let(:expected_action) { :disable_percentage_of_actors } + let(:expected_extra) { {} } + end + end + describe '.remove' do + subject { described_class.remove(key) } + + let(:key) { :awesome_feature } + + before do + described_class.enable(key) + end + + it_behaves_like 'logging' do + let(:expected_action) { :remove } + let(:expected_extra) { {} } + end + context 'for a non-persisted feature' do it 'returns nil' do expect(described_class.remove(:non_persisted_feature_flag)).to be_nil diff --git a/spec/lib/gitlab/diff/lines_unfolder_spec.rb b/spec/lib/gitlab/diff/lines_unfolder_spec.rb index b891f9e8285..4163c0eced5 100644 --- a/spec/lib/gitlab/diff/lines_unfolder_spec.rb +++ b/spec/lib/gitlab/diff/lines_unfolder_spec.rb @@ -188,7 +188,7 @@ RSpec.describe Gitlab::Diff::LinesUnfolder do let(:old_blob) { Blob.decorate(Gitlab::Git::Blob.new(data: raw_old_blob, size: 10)) } let(:diff) do - Gitlab::Git::Diff.new(diff: raw_diff, + Gitlab::Git::Diff.new({ diff: raw_diff, new_path: "build-aux/flatpak/org.gnome.Nautilus.json", old_path: "build-aux/flatpak/org.gnome.Nautilus.json", a_mode: "100644", @@ -196,7 +196,7 @@ RSpec.describe Gitlab::Diff::LinesUnfolder do new_file: false, renamed_file: false, deleted_file: false, - too_large: false) + too_large: false }) end let(:diff_file) do diff --git a/spec/requests/api/graphql/boards/board_lists_query_spec.rb b/spec/requests/api/graphql/boards/board_lists_query_spec.rb index 5d5b963fed5..cd94ce91071 100644 --- a/spec/requests/api/graphql/boards/board_lists_query_spec.rb +++ b/spec/requests/api/graphql/boards/board_lists_query_spec.rb @@ -66,28 +66,22 @@ RSpec.describe 'get board lists' do describe 'sorting and pagination' do let_it_be(:current_user) { user } - let(:data_path) { [board_parent_type, :boards, :edges, 0, :node, :lists] } + let(:data_path) { [board_parent_type, :boards, :nodes, 0, :lists] } - def pagination_query(params, page_info) + def pagination_query(params) graphql_query_for( board_parent_type, { 'fullPath' => board_parent.full_path }, <<~BOARDS boards(first: 1) { - edges { - node { - #{query_graphql_field('lists', params, "#{page_info} edges { node { id } }")} - } + nodes { + #{query_graphql_field(:lists, params, "#{page_info} nodes { id }")} } } BOARDS ) end - def pagination_results_data(data) - data.map { |list| list.dig('node', 'id') } - end - context 'when using default sorting' do let!(:label_list) { create(:list, board: board, label: label, position: 10) } let!(:label_list2) { create(:list, board: board, label: label2, position: 2) } @@ -99,7 +93,7 @@ RSpec.describe 'get board lists' do it_behaves_like 'sorted paginated query' do let(:sort_param) { } let(:first_param) { 2 } - let(:expected_results) { lists.map { |list| list.to_global_id.to_s } } + let(:expected_results) { lists.map { |list| global_id_of(list) } } end end end diff --git a/spec/requests/api/graphql/namespace/projects_spec.rb b/spec/requests/api/graphql/namespace/projects_spec.rb index 03160719389..3e68503b7fb 100644 --- a/spec/requests/api/graphql/namespace/projects_spec.rb +++ b/spec/requests/api/graphql/namespace/projects_spec.rb @@ -80,38 +80,34 @@ RSpec.describe 'getting projects' do end describe 'sorting and pagination' do + let_it_be(:ns) { create(:group) } + let_it_be(:current_user) { create(:user) } + let_it_be(:project_1) { create(:project, name: 'Project', path: 'project', namespace: ns) } + let_it_be(:project_2) { create(:project, name: 'Test Project', path: 'test-project', namespace: ns) } + let_it_be(:project_3) { create(:project, name: 'Test', path: 'test', namespace: ns) } + let_it_be(:project_4) { create(:project, name: 'Test Project Other', path: 'other-test-project', namespace: ns) } + let(:data_path) { [:namespace, :projects] } - def pagination_query(params, page_info) - graphql_query_for( - 'namespace', - { 'fullPath' => subject.full_path }, - <<~QUERY - projects(includeSubgroups: #{include_subgroups}, search: "#{search}", #{params}) { - #{page_info} edges { - node { - #{all_graphql_fields_for('Project')} - } - } - } - QUERY - ) + let(:ns_args) { { full_path: ns.full_path } } + let(:search) { 'test' } + + before do + ns.add_owner(current_user) end - def pagination_results_data(data) - data.map { |project| project.dig('node', 'name') } + def pagination_query(params) + arguments = params.merge(include_subgroups: include_subgroups, search: search) + graphql_query_for(:namespace, ns_args, query_graphql_field(:projects, arguments, <<~GQL)) + #{page_info} + nodes { name } + GQL end context 'when sorting by similarity' do - let!(:project_1) { create(:project, name: 'Project', path: 'project', namespace: subject) } - let!(:project_2) { create(:project, name: 'Test Project', path: 'test-project', namespace: subject) } - let!(:project_3) { create(:project, name: 'Test', path: 'test', namespace: subject) } - let!(:project_4) { create(:project, name: 'Test Project Other', path: 'other-test-project', namespace: subject) } - let(:search) { 'test' } - let(:current_user) { user } - it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'SIMILARITY' } + let(:node_path) { %w[name] } + let(:sort_param) { :SIMILARITY } let(:first_param) { 2 } let(:expected_results) { [project_3.name, project_2.name, project_4.name] } end diff --git a/spec/requests/api/graphql/project/issues_spec.rb b/spec/requests/api/graphql/project/issues_spec.rb index 4f27f08bf98..9c915075c42 100644 --- a/spec/requests/api/graphql/project/issues_spec.rb +++ b/spec/requests/api/graphql/project/issues_spec.rb @@ -142,16 +142,14 @@ RSpec.describe 'getting an issue list for a project' do describe 'sorting and pagination' do let_it_be(:data_path) { [:project, :issues] } - def pagination_query(params, page_info) - graphql_query_for( - 'project', - { 'fullPath' => sort_project.full_path }, - query_graphql_field('issues', params, "#{page_info} edges { node { iid dueDate} }") + def pagination_query(params) + graphql_query_for(:project, { full_path: sort_project.full_path }, + query_graphql_field(:issues, params, "#{page_info} nodes { iid }") ) end def pagination_results_data(data) - data.map { |issue| issue.dig('node', 'iid').to_i } + data.map { |issue| issue.dig('iid').to_i } end context 'when sorting by due date' do @@ -164,7 +162,7 @@ RSpec.describe 'getting an issue list for a project' do context 'when ascending' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'DUE_DATE_ASC' } + let(:sort_param) { :DUE_DATE_ASC } let(:first_param) { 2 } let(:expected_results) { [due_issue3.iid, due_issue5.iid, due_issue1.iid, due_issue4.iid, due_issue2.iid] } end @@ -172,7 +170,7 @@ RSpec.describe 'getting an issue list for a project' do context 'when descending' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'DUE_DATE_DESC' } + let(:sort_param) { :DUE_DATE_DESC } let(:first_param) { 2 } let(:expected_results) { [due_issue1.iid, due_issue5.iid, due_issue3.iid, due_issue4.iid, due_issue2.iid] } end @@ -189,7 +187,7 @@ RSpec.describe 'getting an issue list for a project' do context 'when ascending' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'RELATIVE_POSITION_ASC' } + let(:sort_param) { :RELATIVE_POSITION_ASC } let(:first_param) { 2 } let(:expected_results) { [relative_issue5.iid, relative_issue3.iid, relative_issue1.iid, relative_issue4.iid, relative_issue2.iid] } end @@ -209,7 +207,7 @@ RSpec.describe 'getting an issue list for a project' do context 'when ascending' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'PRIORITY_ASC' } + let(:sort_param) { :PRIORITY_ASC } let(:first_param) { 2 } let(:expected_results) { [priority_issue3.iid, priority_issue1.iid, priority_issue2.iid, priority_issue4.iid] } end @@ -217,7 +215,7 @@ RSpec.describe 'getting an issue list for a project' do context 'when descending' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'PRIORITY_DESC' } + let(:sort_param) { :PRIORITY_DESC } let(:first_param) { 2 } let(:expected_results) { [priority_issue1.iid, priority_issue3.iid, priority_issue2.iid, priority_issue4.iid] } end @@ -236,7 +234,7 @@ RSpec.describe 'getting an issue list for a project' do context 'when ascending' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'LABEL_PRIORITY_ASC' } + let(:sort_param) { :LABEL_PRIORITY_ASC } let(:first_param) { 2 } let(:expected_results) { [label_issue3.iid, label_issue1.iid, label_issue2.iid, label_issue4.iid] } end @@ -244,7 +242,7 @@ RSpec.describe 'getting an issue list for a project' do context 'when descending' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'LABEL_PRIORITY_DESC' } + let(:sort_param) { :LABEL_PRIORITY_DESC } let(:first_param) { 2 } let(:expected_results) { [label_issue2.iid, label_issue3.iid, label_issue1.iid, label_issue4.iid] } end @@ -261,7 +259,7 @@ RSpec.describe 'getting an issue list for a project' do context 'when ascending' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'MILESTONE_DUE_ASC' } + let(:sort_param) { :MILESTONE_DUE_ASC } let(:first_param) { 2 } let(:expected_results) { [milestone_issue2.iid, milestone_issue3.iid, milestone_issue1.iid] } end @@ -269,7 +267,7 @@ RSpec.describe 'getting an issue list for a project' do context 'when descending' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'MILESTONE_DUE_DESC' } + let(:sort_param) { :MILESTONE_DUE_DESC } let(:first_param) { 2 } let(:expected_results) { [milestone_issue3.iid, milestone_issue2.iid, milestone_issue1.iid] } end diff --git a/spec/requests/api/graphql/project/merge_requests_spec.rb b/spec/requests/api/graphql/project/merge_requests_spec.rb index 2b8d537f9fc..c05a620bb62 100644 --- a/spec/requests/api/graphql/project/merge_requests_spec.rb +++ b/spec/requests/api/graphql/project/merge_requests_spec.rb @@ -259,29 +259,19 @@ RSpec.describe 'getting merge request listings nested in a project' do describe 'sorting and pagination' do let(:data_path) { [:project, :mergeRequests] } - def pagination_query(params, page_info) - graphql_query_for( - :project, - { full_path: project.full_path }, + def pagination_query(params) + graphql_query_for(:project, { full_path: project.full_path }, <<~QUERY mergeRequests(#{params}) { - #{page_info} edges { - node { - id - } - } + #{page_info} nodes { id } } QUERY ) end - def pagination_results_data(data) - data.map { |project| project.dig('node', 'id') } - end - context 'when sorting by merged_at DESC' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'MERGED_AT_DESC' } + let(:sort_param) { :MERGED_AT_DESC } let(:first_param) { 2 } let(:expected_results) do @@ -291,7 +281,7 @@ RSpec.describe 'getting merge request listings nested in a project' do merge_request_c, merge_request_e, merge_request_a - ].map(&:to_gid).map(&:to_s) + ].map { |mr| global_id_of(mr) } end before do @@ -304,33 +294,6 @@ RSpec.describe 'getting merge request listings nested in a project' do merge_request_b.metrics.update!(merged_at: 1.day.ago) end - - context 'when paginating backwards' do - let(:params) { 'first: 2, sort: MERGED_AT_DESC' } - let(:page_info) { 'pageInfo { startCursor endCursor }' } - - before do - post_graphql(pagination_query(params, page_info), current_user: current_user) - end - - it 'paginates backwards correctly' do - # first page - first_page_response_data = graphql_dig_at(Gitlab::Json.parse(response.body), :data, *data_path, :edges) - end_cursor = graphql_dig_at(Gitlab::Json.parse(response.body), :data, :project, :mergeRequests, :pageInfo, :endCursor) - - # second page - params = "first: 2, after: \"#{end_cursor}\", sort: MERGED_AT_DESC" - post_graphql(pagination_query(params, page_info), current_user: current_user) - start_cursor = graphql_dig_at(Gitlab::Json.parse(response.body), :data, :project, :mergeRequests, :pageInfo, :start_cursor) - - # going back to the first page - - params = "last: 2, before: \"#{start_cursor}\", sort: MERGED_AT_DESC" - post_graphql(pagination_query(params, page_info), current_user: current_user) - backward_paginated_response_data = graphql_dig_at(Gitlab::Json.parse(response.body), :data, *data_path, :edges) - expect(first_page_response_data).to eq(backward_paginated_response_data) - end - end end end end diff --git a/spec/requests/api/graphql/users_spec.rb b/spec/requests/api/graphql/users_spec.rb index 91ac206676b..72d86c10df1 100644 --- a/spec/requests/api/graphql/users_spec.rb +++ b/spec/requests/api/graphql/users_spec.rb @@ -59,20 +59,16 @@ RSpec.describe 'Users' do describe 'sorting and pagination' do let_it_be(:data_path) { [:users] } - def pagination_query(params, page_info) - graphql_query_for("users", params, "#{page_info} edges { node { id } }") - end - - def pagination_results_data(data) - data.map { |user| user.dig('node', 'id') } + def pagination_query(params) + graphql_query_for(:users, params, "#{page_info} nodes { id }") end context 'when sorting by created_at' do - let_it_be(:ascending_users) { [user3, user2, user1, current_user].map(&:to_global_id).map(&:to_s) } + let_it_be(:ascending_users) { [user3, user2, user1, current_user].map { |u| global_id_of(u) } } context 'when ascending' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'created_asc' } + let(:sort_param) { :CREATED_ASC } let(:first_param) { 1 } let(:expected_results) { ascending_users } end @@ -80,7 +76,7 @@ RSpec.describe 'Users' do context 'when descending' do it_behaves_like 'sorted paginated query' do - let(:sort_param) { 'created_desc' } + let(:sort_param) { :CREATED_DESC } let(:first_param) { 1 } let(:expected_results) { ascending_users.reverse } end diff --git a/spec/support/shared_examples/graphql/sorted_paginated_query_shared_examples.rb b/spec/support/shared_examples/graphql/sorted_paginated_query_shared_examples.rb index 7627a7b4d59..f78ea364147 100644 --- a/spec/support/shared_examples/graphql/sorted_paginated_query_shared_examples.rb +++ b/spec/support/shared_examples/graphql/sorted_paginated_query_shared_examples.rb @@ -16,80 +16,111 @@ # # Example: # describe 'sorting and pagination' do -# let(:sort_project) { create(:project, :public) } +# let_it_be(:sort_project) { create(:project, :public) } # let(:data_path) { [:project, :issues] } # -# def pagination_query(params, page_info) -# graphql_query_for( -# 'project', -# { 'fullPath' => sort_project.full_path }, -# query_graphql_field('issues', params, "#{page_info} edges { node { id } }") +# def pagination_query(arguments) +# graphql_query_for(:project, { full_path: sort_project.full_path }, +# query_nodes(:issues, :iid, include_pagination_info: true, args: arguments) # ) # end # -# def pagination_results_data(data) -# data.map { |issue| issue.dig('node', 'iid').to_i } +# # A method transforming nodes to data to match against +# # default: the identity function +# def pagination_results_data(issues) +# issues.map { |issue| issue['iid].to_i } # end # # context 'when sorting by weight' do -# ... +# let_it_be(:issues) { make_some_issues_with_weights } +# # context 'when ascending' do +# let(:ordered_issues) { issues.sort_by(&:weight) } +# # it_behaves_like 'sorted paginated query' do -# let(:sort_param) { 'WEIGHT_ASC' } +# let(:sort_param) { :WEIGHT_ASC } # let(:first_param) { 2 } -# let(:expected_results) { [weight_issue3.iid, weight_issue5.iid, weight_issue1.iid, weight_issue4.iid, weight_issue2.iid] } +# let(:expected_results) { ordered_issues.map(&:iid) } # end # end # RSpec.shared_examples 'sorted paginated query' do + # Provided as a convenience when constructing queries using string concatenation + let(:page_info) { 'pageInfo { startCursor endCursor }' } + # Convenience for using default implementation of pagination_results_data + let(:node_path) { ['id'] } + it_behaves_like 'requires variables' do let(:required_variables) { [:sort_param, :first_param, :expected_results, :data_path, :current_user] } end describe do - let(:sort_argument) { "sort: #{sort_param}" if sort_param.present? } - let(:first_argument) { "first: #{first_param}" if first_param.present? } + let(:sort_argument) { graphql_args(sort: sort_param) } let(:params) { sort_argument } - let(:start_cursor) { graphql_data_at(*data_path, :pageInfo, :startCursor) } - let(:end_cursor) { graphql_data_at(*data_path, :pageInfo, :endCursor) } - let(:sorted_edges) { graphql_data_at(*data_path, :edges) } - let(:page_info) { "pageInfo { startCursor endCursor }" } - def pagination_query(params, page_info) - raise('pagination_query(params, page_info) must be defined in the test, see example in comment') unless defined?(super) + # Convenience helper for the large number of queries defined as a projection + # from some root value indexed by full_path to a collection of objects with IID + def nested_internal_id_query(root_field, parent, field, args, selection: :iid) + graphql_query_for(root_field, { full_path: parent.full_path }, + query_nodes(field, selection, args: args, include_pagination_info: true) + ) + end + + def pagination_query(params) + raise('pagination_query(params) must be defined in the test, see example in comment') unless defined?(super) super end - def pagination_results_data(data) - raise('pagination_results_data(data) must be defined in the test, see example in comment') unless defined?(super) - - super(data) + def pagination_results_data(nodes) + if defined?(super) + super(nodes) + else + nodes.map { |n| n.dig(*node_path) } + end end + def results + nodes = graphql_dig_at(graphql_data(fresh_response_data), *data_path, :nodes) + pagination_results_data(nodes) + end + + def end_cursor + graphql_dig_at(graphql_data(fresh_response_data), *data_path, :page_info, :end_cursor) + end + + def start_cursor + graphql_dig_at(graphql_data(fresh_response_data), *data_path, :page_info, :start_cursor) + end + + let(:query) { pagination_query(params) } + before do - post_graphql(pagination_query(params, page_info), current_user: current_user) + post_graphql(query, current_user: current_user) end context 'when sorting' do it 'sorts correctly' do - expect(pagination_results_data(sorted_edges)).to eq expected_results + expect(results).to eq expected_results end context 'when paginating' do - let(:params) { [sort_argument, first_argument].compact.join(',') } + let(:params) { sort_argument.merge(first: first_param) } + let(:first_page) { expected_results.first(first_param) } + let(:rest) { expected_results.drop(first_param) } it 'paginates correctly' do - expect(pagination_results_data(sorted_edges)).to eq expected_results.first(first_param) + expect(results).to eq first_page - cursored_query = pagination_query([sort_argument, "after: \"#{end_cursor}\""].compact.join(','), page_info) - post_graphql(cursored_query, current_user: current_user) + fwds = pagination_query(sort_argument.merge(after: end_cursor)) + post_graphql(fwds, current_user: current_user) - expect(response).to have_gitlab_http_status(:ok) + expect(results).to eq rest - response_data = graphql_dig_at(Gitlab::Json.parse(response.body), :data, *data_path, :edges) + bwds = pagination_query(sort_argument.merge(before: start_cursor)) + post_graphql(bwds, current_user: current_user) - expect(pagination_results_data(response_data)).to eq expected_results.drop(first_param) + expect(results).to eq first_page end end end diff --git a/spec/support/shared_examples/requests/api/graphql/group_and_project_boards_query_shared_examples.rb b/spec/support/shared_examples/requests/api/graphql/group_and_project_boards_query_shared_examples.rb index 5145880ef9a..54f4ba7ff73 100644 --- a/spec/support/shared_examples/requests/api/graphql/group_and_project_boards_query_shared_examples.rb +++ b/spec/support/shared_examples/requests/api/graphql/group_and_project_boards_query_shared_examples.rb @@ -47,18 +47,12 @@ RSpec.shared_examples 'group and project boards query' do describe 'sorting and pagination' do let(:data_path) { [board_parent_type, :boards] } - def pagination_query(params, page_info) - graphql_query_for( - board_parent_type, - { 'fullPath' => board_parent.full_path }, - query_graphql_field('boards', params, "#{page_info} edges { node { id } }") + def pagination_query(params) + graphql_query_for(board_parent_type, { full_path: board_parent.full_path }, + query_nodes(:boards, :id, include_pagination_info: true, args: params) ) end - def pagination_results_data(data) - data.map { |board| board.dig('node', 'id') } - end - context 'when using default sorting' do let!(:board_B) { create(:board, resource_parent: board_parent, name: 'B') } let!(:board_C) { create(:board, resource_parent: board_parent, name: 'C') } @@ -72,9 +66,9 @@ RSpec.shared_examples 'group and project boards query' do let(:first_param) { 2 } let(:expected_results) do if board_parent.multiple_issue_boards_available? - boards.map { |board| board.to_global_id.to_s } + boards.map { |board| global_id_of(board) } else - [boards.first.to_global_id.to_s] + [global_id_of(boards.first)] end end end