diff options
48 files changed, 2321 insertions, 1402 deletions
@@ -22,6 +22,7 @@ gem 'rugged', '~> 0.28' gem 'grape-path-helpers', '~> 1.1' gem 'faraday', '~> 0.12' +gem 'marginalia', '~> 1.8.0' # Authentication libraries gem 'devise', '~> 4.6' diff --git a/Gemfile.lock b/Gemfile.lock index 054a491a019..a290b8f96a5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -591,6 +591,9 @@ GEM mail_room (0.9.1) marcel (0.3.3) mimemagic (~> 0.3.2) + marginalia (1.8.0) + actionpack (>= 2.3) + activerecord (>= 2.3) memoist (0.16.0) memoizable (0.4.2) thread_safe (~> 0.3, >= 0.3.1) @@ -1243,6 +1246,7 @@ DEPENDENCIES lograge (~> 0.5) loofah (~> 2.2) mail_room (~> 0.9.1) + marginalia (~> 1.8.0) memory_profiler (~> 0.9) method_source (~> 0.8) mimemagic (~> 0.3.2) diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue index a2b5a79af36..3daea306fba 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue @@ -155,7 +155,7 @@ export default { {{ cherryPickLabel }} </a> </div> - <section class="mr-info-list"> + <section class="mr-info-list" data-qa-selector="merged_status_content"> <p> {{ s__('mrWidget|The changes were merged into') }} <span class="label-branch"> diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue index 2c113770d8b..aa65b16a3c3 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/ready_to_merge.vue @@ -249,9 +249,10 @@ export default { type="button" class="btn btn-sm btn-info dropdown-toggle js-merge-moment" data-toggle="dropdown" + data-qa-selector="merge_moment_dropdown" :aria-label="__('Select merge moment')" > - <i class="fa fa-chevron-down qa-merge-moment-dropdown" aria-hidden="true"></i> + <i class="fa fa-chevron-down" aria-hidden="true"></i> </button> <ul v-if="shouldShowMergeImmediatelyDropdown" @@ -272,7 +273,8 @@ export default { </li> <li> <a - class="accept-merge-request qa-merge-immediately-option" + class="accept-merge-request" + data-qa-selector="merge_immediately_option" href="#" @click.prevent="handleMergeButtonClick(false, true)" > diff --git a/app/views/layouts/nav/sidebar/_group.html.haml b/app/views/layouts/nav/sidebar/_group.html.haml index a6d2c894185..a027dca1b56 100644 --- a/app/views/layouts/nav/sidebar/_group.html.haml +++ b/app/views/layouts/nav/sidebar/_group.html.haml @@ -44,7 +44,7 @@ - if group_sidebar_link?(:contribution_analytics) = nav_link(path: 'analytics#show') do - = link_to group_analytics_path(@group), title: _('Contribution Analytics'), data: { placement: 'right' } do + = link_to group_analytics_path(@group), title: _('Contribution Analytics'), data: { placement: 'right', qa_selector: 'contribution_analytics_link' } do %span = _('Contribution Analytics') diff --git a/app/views/shared/issuable/_close_reopen_button.html.haml b/app/views/shared/issuable/_close_reopen_button.html.haml index 875cacd1f4f..2eb96a7bc9b 100644 --- a/app/views/shared/issuable/_close_reopen_button.html.haml +++ b/app/views/shared/issuable/_close_reopen_button.html.haml @@ -6,7 +6,7 @@ - if is_current_user - if can_update = link_to "Close #{display_issuable_type}", close_issuable_path(issuable), method: button_method, - class: "d-none d-sm-none d-md-block btn btn-grouped btn-close js-btn-issue-action #{issuable_button_visibility(issuable, true)}", title: "Close #{display_issuable_type}" + class: "d-none d-sm-none d-md-block btn btn-grouped btn-close js-btn-issue-action #{issuable_button_visibility(issuable, true)}", title: "Close #{display_issuable_type}", data: { qa_selector: 'close_issue_button' } - if can_reopen = link_to "Reopen #{display_issuable_type}", reopen_issuable_path(issuable), method: button_method, class: "d-none d-sm-none d-md-block btn btn-grouped btn-reopen js-btn-issue-action #{issuable_button_visibility(issuable, false)}", title: "Reopen #{display_issuable_type}", data: { qa_selector: 'reopen_issue_button' } diff --git a/config/initializers/0_marginalia.rb b/config/initializers/0_marginalia.rb new file mode 100644 index 00000000000..42d8f0bf5e1 --- /dev/null +++ b/config/initializers/0_marginalia.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'marginalia' + +::Marginalia::Comment.extend(::Gitlab::Marginalia::Comment) + +# Patch to include support for 'Marginalia.without_annotation' method. +::Marginalia.singleton_class.prepend(Gitlab::Marginalia::InlineAnnotation) + +# Patch to modify 'Marginalia::ActiveRecordInstrumentation.annotate_sql' method with feature check. +# Orignal Marginalia::ActiveRecordInstrumentation is included to ActiveRecord::ConnectionAdapters::PostgreSQLAdapter in the Marginalia Railtie. +# Refer: https://github.com/basecamp/marginalia/blob/v1.8.0/lib/marginalia/railtie.rb#L67 +ActiveRecord::ConnectionAdapters::PostgreSQLAdapter.prepend(Gitlab::Marginalia::ActiveRecordInstrumentation) + +Marginalia::Comment.components = [:application, :controller, :action, :correlation_id, :jid, :job_class, :line] + +Gitlab::Marginalia.set_application_name + +Gitlab::Marginalia.enable_sidekiq_instrumentation diff --git a/db/post_migrate/20191125024005_cleanup_deploy_access_levels_for_removed_groups.rb b/db/post_migrate/20191125024005_cleanup_deploy_access_levels_for_removed_groups.rb new file mode 100644 index 00000000000..29592612a02 --- /dev/null +++ b/db/post_migrate/20191125024005_cleanup_deploy_access_levels_for_removed_groups.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +class CleanupDeployAccessLevelsForRemovedGroups < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def up + return unless Gitlab.ee? + + delete = <<~SQL + DELETE FROM protected_environment_deploy_access_levels d + USING protected_environments p + WHERE d.protected_environment_id=p.id + AND d.group_id IS NOT NULL + AND NOT EXISTS (SELECT 1 FROM project_group_links WHERE project_id=p.project_id AND group_id=d.group_id) + RETURNING * + SQL + + # At the time of writing there are 4 such records on GitLab.com, + # execution time is expected to be around 15ms. + records = execute(delete) + + logger = Gitlab::BackgroundMigration::Logger.build + records.to_a.each do |record| + logger.info record.as_json.merge(message: "protected_environments_deploy_access_levels was deleted") + end + end + + def down + # There is no pragmatic way to restore + # the records deleted in the `#up` method above. + end +end diff --git a/doc/development/contributing/style_guides.md b/doc/development/contributing/style_guides.md index f825b3d7088..d53675cc716 100644 --- a/doc/development/contributing/style_guides.md +++ b/doc/development/contributing/style_guides.md @@ -40,8 +40,8 @@ This saves you time as you don't have to wait for the same errors to be detected [rss-source]: https://github.com/rubocop-hq/ruby-style-guide/blob/master/README.adoc#source-code-layout [rss-naming]: https://github.com/rubocop-hq/ruby-style-guide/blob/master/README.adoc#naming-conventions [doc-guidelines]: ../documentation/index.md "Documentation guidelines" -[js-styleguide]: ../fe_guide/style_guide_js.md "JavaScript styleguide" -[scss-styleguide]: ../fe_guide/style_guide_scss.md "SCSS styleguide" +[js-styleguide]: ../fe_guide/style/javascript.md "JavaScript styleguide" +[scss-styleguide]: ../fe_guide/style/scss.md "SCSS styleguide" [newlines-styleguide]: ../newlines_styleguide.md "Newlines styleguide" [testing]: ../testing_guide/index.md [us-english]: https://en.wikipedia.org/wiki/American_English diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md index 1cf798cedb6..f13ef767660 100644 --- a/doc/development/fe_guide/index.md +++ b/doc/development/fe_guide/index.md @@ -76,15 +76,18 @@ Read the [frontend's FAQ](frontend_faq.md) for common small pieces of helpful in ## Style Guides -### [JavaScript Style Guide](style_guide_js.md) +See the relevant style guides for our guidelines and for information on linting: -We use eslint to enforce our JavaScript style guides. Our guide is based on +- [JavaScript](style/javascript.md). Our guide is based on the excellent [Airbnb][airbnb-js-style-guide] style guide with a few small changes. +- [SCSS](style/scss.md): our SCSS conventions which are enforced through [`scss-lint`](https://github.com/brigade/scss-lint). +- [HTML](style/html.md). Guidelines for writing HTML code consistent with the rest of the codebase. +- [Vue](style/vue.md). Guidelines and conventions for Vue code may be found here. -### [SCSS Style Guide](style_guide_scss.md) +## Tooling -Our SCSS conventions which are enforced through [scss-lint](https://github.com/sds/scss-lint). +Our code is automatically formatted with [Prettier](https://prettier.io) to follow our guidelines. Read our [Tooling guide](tooling.md) for more detail. ## [Performance](performance.md) diff --git a/doc/development/fe_guide/style/html.md b/doc/development/fe_guide/style/html.md new file mode 100644 index 00000000000..1445da3f0e1 --- /dev/null +++ b/doc/development/fe_guide/style/html.md @@ -0,0 +1,53 @@ +# HTML style guide + +## Buttons + +### Button type + +Button tags requires a `type` attribute according to the [W3C HTML specification](https://www.w3.org/TR/2011/WD-html5-20110525/the-button-element.html#dom-button-type). + +```html +// bad +<button></button> + +// good +<button type="button"></button> +``` + +### Button role + +If an HTML element has an `onClick` handler but is not a button, it should have `role="button"`. This is [more accessible](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role). + +```html +// bad +<div onClick="doSomething"></div> + +// good +<div role="button" onClick="doSomething"></div> +``` + +## Links + +### Blank target + +Use `rel="noopener noreferrer"` whenever your links open in a new window, i.e. `target="_blank"`. This prevents a security vulnerability [documented by JitBit](https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/). + +```html +// bad +<a href="url" target="_blank"></a> + +// good +<a href="url" target="_blank" rel="noopener noreferrer"></a> +``` + +### Fake links + +**Do not use fake links.** Use a button tag if a link only invokes JavaScript click event handlers, which is more semantic. + +```html +// bad +<a class="js-do-something" href="#"></a> + +// good +<button class="js-do-something" type="button"></button> +``` diff --git a/doc/development/fe_guide/style/index.md b/doc/development/fe_guide/style/index.md new file mode 100644 index 00000000000..3b07a8557f5 --- /dev/null +++ b/doc/development/fe_guide/style/index.md @@ -0,0 +1,21 @@ +# GitLab development style guides + +See below the relevant style guides, guidelines, linting, and other information for developing GitLab. + +## JavaScript style guide + +We use `eslint` to enforce our [JavaScript style guides](javascript.md). Our guide is based on +the excellent [AirBnB](https://github.com/airbnb/javascript) style guide with a few small +changes. + +## SCSS style guide + +Our [SCSS conventions](scss.md) which are enforced through [`scss-lint`](https://github.com/brigade/scss-lint). + +## HTML style guide + +Guidelines for writing [HTML code](html.md) consistent with the rest of the codebase. + +## Vue style guide + +Guidelines and conventions for Vue code may be found within the [Vue style guide](vue.md). diff --git a/doc/development/fe_guide/style/javascript.md b/doc/development/fe_guide/style/javascript.md new file mode 100644 index 00000000000..6921d418ea3 --- /dev/null +++ b/doc/development/fe_guide/style/javascript.md @@ -0,0 +1,303 @@ +--- +disqus_identifier: 'https://docs.gitlab.com/ee/development/fe_guide/style_guide_js.html' +--- + +# JavaScript style guide + +We use [Airbnb's JavaScript Style Guide](https://github.com/airbnb/javascript) and it's accompanying +linter to manage most of our JavaScript style guidelines. + +In addition to the style guidelines set by Airbnb, we also have a few specific rules +listed below. + +> **Tip:** +You can run eslint locally by running `yarn eslint` + +## Avoid forEach + +Avoid forEach when mutating data. Use `map`, `reduce` or `filter` instead of `forEach` +when mutating data. This will minimize mutations in functions, +which aligns with [Airbnb's style guide](https://github.com/airbnb/javascript#testing--for-real). + +```javascript +// bad +users.forEach((user, index) => { + user.id = index; +}); + +// good +const usersWithId = users.map((user, index) => { + return Object.assign({}, user, { id: index }); +}); +``` + +## Limit number of parameters + +If your function or method has more than 3 parameters, use an object as a parameter +instead. + +```javascript +// bad +function a(p1, p2, p3) { + // ... +}; + +// good +function a(p) { + // ... +}; +``` + +## Avoid side effects in constructors + +Avoid making asynchronous calls, API requests or DOM manipulations in the `constructor`. +Move them into separate functions instead. This will make tests easier to write and +code easier to maintain. + +```javascript +// bad +class myClass { + constructor(config) { + this.config = config; + axios.get(this.config.endpoint) + } +} + +// good +class myClass { + constructor(config) { + this.config = config; + } + + makeRequest() { + axios.get(this.config.endpoint) + } +} +const instance = new myClass(); +instance.makeRequest(); +``` + +## Avoid classes to handle DOM events + +If the only purpose of the class is to bind a DOM event and handle the callback, prefer +using a function. + +```javascript +// bad +class myClass { + constructor(config) { + this.config = config; + } + + init() { + document.addEventListener('click', () => {}); + } +} + +// good + +const myFunction = () => { + document.addEventListener('click', () => { + // handle callback here + }); +} +``` + +## Pass element container to constructor + +When your class manipulates the DOM, receive the element container as a parameter. +This is more maintainable and performant. + +```javascript +// bad +class a { + constructor() { + document.querySelector('.b'); + } +} + +// good +class a { + constructor(options) { + options.container.querySelector('.b'); + } +} +``` + +## Use ParseInt + +Use `ParseInt` when converting a numeric string into a number. + +```javascript +// bad +Number('10') + +// good +parseInt('10', 10); +``` + +## CSS Selectors - Use `js-` prefix + +If a CSS class is only being used in JavaScript as a reference to the element, prefix +the class name with `js-`. + +```html +// bad +<button class="add-user"></button> + +// good +<button class="js-add-user"></button> +``` + +## ES Module Syntax + +Use ES module syntax to import modules: + +```javascript +// bad +const SomeClass = require('some_class'); + +// good +import SomeClass from 'some_class'; + +// bad +module.exports = SomeClass; + +// good +export default SomeClass; +``` + +_Note:_ We still use `require` in `scripts/` and `config/` files. + +## Absolute vs relative paths for modules + +Use relative paths if the module you are importing is less than two levels up. + +```javascript +// bad +import GitLabStyleGuide from '~/guides/GitLabStyleGuide'; + +// good +import GitLabStyleGuide from '../GitLabStyleGuide'; +``` + +If the module you are importing is two or more levels up, use an absolute path instead: + +```javascript +// bad +import GitLabStyleGuide from '../../../guides/GitLabStyleGuide'; + +// good +import GitLabStyleGuide from '~/GitLabStyleGuide'; +``` + +Additionally, **do not add to global namespace**. + +## Do not use `DOMContentLoaded` in non-page modules + +Imported modules should act the same each time they are loaded. `DOMContentLoaded` +events are only allowed on modules loaded in the `/pages/*` directory because those +are loaded dynamically with webpack. + +## Avoid XSS + +Do not use `innerHTML`, `append()` or `html()` to set content. It opens up too many +vulnerabilities. + +## ESLint + +ESLint behaviour can be found in our [tooling guide](../tooling.md). + +## IIFEs + +Avoid using IIFEs (Immediately-Invoked Function Expressions). Although +we have a lot of examples of files which wrap their contents in IIFEs, +this is no longer necessary after the transition from Sprockets to webpack. +Do not use them anymore and feel free to remove them when refactoring legacy code. + +## Global namespace and side effects + +Avoid adding to the global namespace. + +```javascript +// bad +window.MyClass = class { /* ... */ }; + +// good +export default class MyClass { /* ... */ } +``` + +Top-level side effects are forbidden in any script which contains `export`: + +```javascript +// bad +export default class MyClass { /* ... */ } + +document.addEventListener("DOMContentLoaded", function(event) { + new MyClass(); +} +``` + +Avoid constructors with side effects whenever possible. + +Side effects make constructors difficult to unit test and violate the [Single Responsibility Principle](https://en.wikipedia.org/wiki/Single_responsibility_principle). + +```javascript +// Bad +export class Foo { + constructor() { + this.currentState = state.INITIAL; + document.getElementById('root').addEventListener('click', this.handleCallback) + } + handleCallback() { + } +} + +// Good +export class Foo { + constructor() { + this.currentState = state.INITIAL; + } + initListener(element) { + element.addEventListener('click', this.handleCallback) + } + handleCallback() { + } +} +``` + +On the other hand, if a class only needs to extend a third-party or add +event listeners in some specific cases, they should be initialized outside +of the constructor. + +## Pure Functions and Data Mutation + +Strive to write many small pure functions and minimize where mutations occur + + ```javascript + // bad + const values = {foo: 1}; + + function impureFunction(items) { + const bar = 1; + + items.foo = items.a * bar + 2; + + return items.a; + } + + const c = impureFunction(values); + + // good + var values = {foo: 1}; + + function pureFunction (foo) { + var bar = 1; + + foo = foo * bar + 2; + + return foo; + } + + var c = pureFunction(values.foo); + ``` diff --git a/doc/development/fe_guide/style/scss.md b/doc/development/fe_guide/style/scss.md new file mode 100644 index 00000000000..02e7351d135 --- /dev/null +++ b/doc/development/fe_guide/style/scss.md @@ -0,0 +1,285 @@ +--- +disqus_identifier: 'https://docs.gitlab.com/ee/development/fe_guide/style_guide_scss.html' +--- + +# SCSS style guide + +This style guide recommends best practices for SCSS to make styles easy to read, +easy to maintain, and performant for the end-user. + +## Rules + +### Utility Classes + +As part of the effort for [cleaning up our CSS and moving our components into GitLab-UI](https://gitlab.com/groups/gitlab-org/-/epics/950) +led by the [GitLab UI WG](https://gitlab.com/gitlab-com/www-gitlab-com/merge_requests/20623) we prefer the use of utility classes over adding new CSS. However, complex CSS can be addressed by adding component classes. + +#### Where are utility classes defined? + +- [Bootstrap's Utility Classes](https://getbootstrap.com/docs/4.3/utilities/) +- [`common.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/framework/common.scss) (old) +- [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/utilities.scss) (new) + +#### Where should I put new utility classes? + +New utility classes should be added to [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/utilities.scss). Existing classes include: + +| Name | Pattern | Example | +|------|---------|---------| +| Background color | `.bg-{variant}-{shade}` | `.bg-warning-400` | +| Text color | `.text-{variant}-{shade}` | `.text-success-500` | +| Font size | `.text-{size}` | `.text-2` | + +- `{variant}` is one of 'primary', 'secondary', 'success', 'warning', 'error' +- `{shade}` is one of the shades listed on [colors](https://design.gitlab.com/product-foundations/colors/) +- `{size}` is a number from 1-6 from our [Type scale](https://design.gitlab.com/product-foundations/typography/) + +#### When should I create component classes? + +We recommend a "utility-first" approach. + +1. Start with utility classes. +1. If composing utility classes into a component class removes code duplication and encapsulates a clear responsibility, do it. + +This encourages an organic growth of component classes and prevents the creation of one-off unreusable classes. Also, the kind of classes that emerge from "utility-first" tend to be design-centered (e.g. `.button`, `.alert`, `.card`) rather than domain-centered (e.g. `.security-report-widget`, `.commit-header-icon`). + +Examples of component classes that were created using "utility-first" include: + +- [`.circle-icon-container`](https://gitlab.com/gitlab-org/gitlab/blob/579fa8b8ec7eb38d40c96521f517c9dab8c3b97a/app/assets/stylesheets/framework/icons.scss#L85) +- [`.d-flex-center`](https://gitlab.com/gitlab-org/gitlab/blob/900083d89cd6af391d26ab7922b3f64fa2839bef/app/assets/stylesheets/framework/common.scss#L425) + +Inspiration: + +- <https://tailwindcss.com/docs/utility-first/> +- <https://tailwindcss.com/docs/extracting-components/> + +### Naming + +Filenames should use `snake_case`. + +CSS classes should use the `lowercase-hyphenated` format rather than +`snake_case` or `camelCase`. + +```scss +// Bad +.class_name { + color: #fff; +} + +// Bad +.className { + color: #fff; +} + +// Good +.class-name { + color: #fff; +} +``` + +### Formatting + +You should always use a space before a brace, braces should be on the same +line, each property should each get its own line, and there should be a space +between the property and its value. + +```scss +// Bad +.container-item { + width: 100px; height: 100px; + margin-top: 0; +} + +// Bad +.container-item +{ + width: 100px; + height: 100px; + margin-top: 0; +} + +// Bad +.container-item{ + width:100px; + height:100px; + margin-top:0; +} + +// Good +.container-item { + width: 100px; + height: 100px; + margin-top: 0; +} +``` + +Note that there is an exception for single-line rulesets, although these are +not typically recommended. + +```scss +p { margin: 0; padding: 0; } +``` + +### Colors + +HEX (hexadecimal) colors should use shorthand where possible, and should use +lower case letters to differentiate between letters and numbers, e.g. `#E3E3E3` +vs. `#e3e3e3`. + +```scss +// Bad +p { + color: #ffffff; +} + +// Bad +p { + color: #FFFFFF; +} + +// Good +p { + color: #fff; +} +``` + +### Indentation + +Indentation should always use two spaces for each indentation level. + +```scss +// Bad, four spaces +p { + color: #f00; +} + +// Good +p { + color: #f00; +} +``` + +### Semicolons + +Always include semicolons after every property. When the stylesheets are +minified, the semicolons will be removed automatically. + +```scss +// Bad +.container-item { + width: 100px; + height: 100px +} + +// Good +.container-item { + width: 100px; + height: 100px; +} +``` + +### Shorthand + +The shorthand form should be used for properties that support it. + +```scss +// Bad +margin: 10px 15px 10px 15px; +padding: 10px 10px 10px 10px; + +// Good +margin: 10px 15px; +padding: 10px; +``` + +### Zero Units + +Omit length units on zero values, they're unnecessary and not including them +is slightly more performant. + +```scss +// Bad +.item-with-padding { + padding: 0px; +} + +// Good +.item-with-padding { + padding: 0; +} +``` + +### Selectors with a `js-` Prefix + +Do not use any selector prefixed with `js-` for styling purposes. These +selectors are intended for use only with JavaScript to allow for removal or +renaming without breaking styling. + +### IDs + +Don't use ID selectors in CSS. + +```scss +// Bad +#my-element { + padding: 0; +} + +// Good +.my-element { + padding: 0; +} +``` + +### Variables + +Before adding a new variable for a color or a size, guarantee: + +- There isn't already one +- There isn't a similar one we can use instead. + +## Linting + +We use [SCSS Lint](https://github.com/sds/scss-lint) to check for style guide conformity. It uses the +ruleset in `.scss-lint.yml`, which is located in the home directory of the +project. + +To check if any warnings will be produced by your changes, you can run `rake +scss_lint` in the GitLab directory. SCSS Lint will also run in GitLab CI to +catch any warnings. + +If the Rake task is throwing warnings you don't understand, SCSS Lint's +documentation includes [a full list of their linters][scss-lint-documentation](https://github.com/sds/scss-lint/blob/master/lib/scss_lint/linter/README.md). + +### Fixing issues + +If you want to automate changing a large portion of the codebase to conform to +the SCSS style guide, you can use [CSSComb][csscomb]. First install +[Node][node] and [NPM][npm], then run `npm install csscomb -g` to install +CSSComb globally (system-wide). Run it in the GitLab directory with +`csscomb app/assets/stylesheets` to automatically fix issues with CSS/SCSS. + +Note that this won't fix every problem, but it should fix a majority. + +### Ignoring issues + +If you want a line or set of lines to be ignored by the linter, you can use +`// scss-lint:disable RuleName` ([more info](https://github.com/sds/scss-lint#disabling-linters-via-source)): + +```scss +// This lint rule is disabled because it is supported only in Chrome/Safari +// scss-lint:disable PropertySpelling +body { + text-decoration-skip: ink; +} +// scss-lint:enable PropertySpelling +``` + +Make sure a comment is added on the line above the `disable` rule, otherwise the +linter will throw a warning. `DisableLinterReason` is enabled to make sure the +style guide isn't being ignored, and to communicate to others why the style +guide is ignored in this instance. + +[csscomb]: https://github.com/csscomb/csscomb.js +[node]: https://github.com/nodejs/node +[npm]: https://www.npmjs.com/ diff --git a/doc/development/fe_guide/style/vue.md b/doc/development/fe_guide/style/vue.md new file mode 100644 index 00000000000..2499623e66a --- /dev/null +++ b/doc/development/fe_guide/style/vue.md @@ -0,0 +1,418 @@ +# Vue.js style guide + +## Linting + +We default to [eslint-vue-plugin](https://github.com/vuejs/eslint-plugin-vue), with the `plugin:vue/recommended`. +Please check this [rules](https://github.com/vuejs/eslint-plugin-vue#bulb-rules) for more documentation. + +## Basic Rules + +1. The service has it's own file +1. The store has it's own file +1. Use a function in the bundle file to instantiate the Vue component: + + ```javascript + // bad + class { + init() { + new Component({}) + } + } + + // good + document.addEventListener('DOMContentLoaded', () => new Vue({ + el: '#element', + components: { + componentName + }, + render: createElement => createElement('component-name'), + })); + ``` + +1. Do not use a singleton for the service or the store + + ```javascript + // bad + class Store { + constructor() { + if (!this.prototype.singleton) { + // do something + } + } + } + + // good + class Store { + constructor() { + // do something + } + } + ``` + +1. Use `.vue` for Vue templates. Do not use `%template` in HAML. + +## Naming + +1. **Extensions**: Use `.vue` extension for Vue components. Do not use `.js` as file extension ([#34371]). +1. **Reference Naming**: Use PascalCase for their instances: + + ```javascript + // bad + import cardBoard from 'cardBoard.vue' + + components: { + cardBoard, + }; + + // good + import CardBoard from 'cardBoard.vue' + + components: { + CardBoard, + }; + ``` + +1. **Props Naming:** Avoid using DOM component prop names. +1. **Props Naming:** Use kebab-case instead of camelCase to provide props in templates. + + ```javascript + // bad + <component class="btn"> + + // good + <component css-class="btn"> + + // bad + <component myProp="prop" /> + + // good + <component my-prop="prop" /> + ``` + +[#34371]: https://gitlab.com/gitlab-org/gitlab-foss/issues/34371 + +## Alignment + +1. Follow these alignment styles for the template method: + + 1. With more than one attribute, all attributes should be on a new line: + + ```javascript + // bad + <component v-if="bar" + param="baz" /> + + <button class="btn">Click me</button> + + // good + <component + v-if="bar" + param="baz" + /> + + <button class="btn"> + Click me + </button> + ``` + + 1. The tag can be inline if there is only one attribute: + + ```javascript + // good + <component bar="bar" /> + + // good + <component + bar="bar" + /> + + // bad + <component + bar="bar" /> + ``` + +## Quotes + +1. Always use double quotes `"` inside templates and single quotes `'` for all other JS. + + ```javascript + // bad + template: ` + <button :class='style'>Button</button> + ` + + // good + template: ` + <button :class="style">Button</button> + ` + ``` + +## Props + +1. Props should be declared as an object + + ```javascript + // bad + props: ['foo'] + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } + } + ``` + +1. Required key should always be provided when declaring a prop + + ```javascript + // bad + props: { + foo: { + type: String, + } + } + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } + } + ``` + +1. Default key should be provided if the prop is not required. + _Note:_ There are some scenarios where we need to check for the existence of the property. + On those a default key should not be provided. + + ```javascript + // good + props: { + foo: { + type: String, + required: false, + } + } + + // good + props: { + foo: { + type: String, + required: false, + default: 'bar' + } + } + + // good + props: { + foo: { + type: String, + required: true + } + } + ``` + +## Data + +1. `data` method should always be a function + + ```javascript + // bad + data: { + foo: 'foo' + } + + // good + data() { + return { + foo: 'foo' + }; + } + ``` + +## Directives + +1. Shorthand `@` is preferable over `v-on` + + ```javascript + // bad + <component v-on:click="eventHandler"/> + + // good + <component @click="eventHandler"/> + ``` + +1. Shorthand `:` is preferable over `v-bind` + + ```javascript + // bad + <component v-bind:class="btn"/> + + // good + <component :class="btn"/> + ``` + +1. Shorthand `#` is preferable over `v-slot` + + ```javascript + // bad + <template v-slot:header></template> + + // good + <template #header></template> + ``` + +## Closing tags + +1. Prefer self closing component tags + + ```javascript + // bad + <component></component> + + // good + <component /> + ``` + +## Component usage within templates + +1. Prefer a component's kebab-cased name over other styles when using it in a template + + ```javascript + // bad + <MyComponent /> + + // good + <my-component /> + ``` + +## Ordering + +1. Tag order in `.vue` file + + ``` + <script> + // ... + </script> + + <template> + // ... + </template> + + // We don't use scoped styles but there are few instances of this + <style> + // ... + </style> + ``` + +1. Properties in a Vue Component: + Check [order of properties in components rule](https://github.com/vuejs/eslint-plugin-vue/blob/master/docs/rules/order-in-components.md). + +## `:key` + +When using `v-for` you need to provide a *unique* `:key` attribute for each item. + +1. If the elements of the array being iterated have an unique `id` it is advised to use it: + + ```html + <div + v-for="item in items" + :key="item.id" + > + <!-- content --> + </div> + ``` + +1. When the elements being iterated don't have a unique id, you can use the array index as the `:key` attribute + + ```html + <div + v-for="(item, index) in items" + :key="index" + > + <!-- content --> + </div> + ``` + +1. When using `v-for` with `template` and there is more than one child element, the `:key` values must be unique. It's advised to use `kebab-case` namespaces. + + ```html + <template v-for="(item, index) in items"> + <span :key="`span-${index}`"></span> + <button :key="`button-${index}`"></button> + </template> + ``` + +1. When dealing with nested `v-for` use the same guidelines as above. + + ```html + <div + v-for="item in items" + :key="item.id" + > + <span + v-for="element in array" + :key="element.id" + > + <!-- content --> + </span> + </div> + ``` + +Useful links: + +1. [`key`](https://vuejs.org/v2/guide/list.html#key) +1. [Vue Style Guide: Keyed v-for](https://vuejs.org/v2/style-guide/#Keyed-v-for-essential ) + +## Vue and Bootstrap + +1. Tooltips: Do not rely on `has-tooltip` class name for Vue components + + ```javascript + // bad + <span + class="has-tooltip" + title="Some tooltip text"> + Text + </span> + + // good + <span + v-tooltip + title="Some tooltip text"> + Text + </span> + ``` + +1. Tooltips: When using a tooltip, include the tooltip directive, `./app/assets/javascripts/vue_shared/directives/tooltip.js` + +1. Don't change `data-original-title`. + + ```javascript + // bad + <span data-original-title="tooltip text">Foo</span> + + // good + <span title="tooltip text">Foo</span> + + $('span').tooltip('_fixTitle'); + ``` + +## The JavaScript/Vue Accord + +The goal of this accord is to make sure we are all on the same page. + +1. When writing Vue, you may not use jQuery in your application. + 1. If you need to grab data from the DOM, you may query the DOM 1 time while bootstrapping your application to grab data attributes using `dataset`. You can do this without jQuery. + 1. You may use a jQuery dependency in Vue.js following [this example from the docs](https://vuejs.org/v2/examples/select2.html). + 1. If an outside jQuery Event needs to be listen to inside the Vue application, you may use jQuery event listeners. + 1. We will avoid adding new jQuery events when they are not required. Instead of adding new jQuery events take a look at [different methods to do the same task](https://vuejs.org/v2/api/#vm-emit). +1. You may query the `window` object 1 time, while bootstrapping your application for application specific data (e.g. `scrollTo` is ok to access anytime). Do this access during the bootstrapping of your application. +1. You may have a temporary but immediate need to create technical debt by writing code that does not follow our standards, to be refactored later. Maintainers need to be ok with the tech debt in the first place. An issue should be created for that tech debt to evaluate it further and discuss. In the coming months you should fix that tech debt, with it's priority to be determined by maintainers. +1. When creating tech debt you must write the tests for that code before hand and those tests may not be rewritten. e.g. jQuery tests rewritten to Vue tests. +1. You may choose to use VueX as a centralized state management. If you choose not to use VueX, you must use the *store pattern* which can be found in the [Vue.js documentation](https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch). +1. Once you have chosen a centralized state management solution you must use it for your entire application. i.e. Don't mix and match your state management solutions. diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 43cd8180b6e..f3fa80325ef 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -1,731 +1,5 @@ -# Style guides and linting +--- +redirect_to: 'style/javascript.md' +--- -See the relevant style guides for our guidelines and for information on linting: - -## JavaScript - -We defer to [Airbnb][airbnb-js-style-guide] on most style-related -conventions and enforce them with eslint. - -See [our current .eslintrc](https://gitlab.com/gitlab-org/gitlab/blob/master/.eslintrc.yml) for specific rules and patterns. - -### Common - -#### ESlint - -1. **Never** disable eslint rules unless you have a good reason. - You may see a lot of legacy files with `/* eslint-disable some-rule, some-other-rule */` - at the top, but legacy files are a special case. Any time you develop a new feature or - refactor an existing one, you should abide by the eslint rules. - -1. **Never Ever EVER** disable eslint globally for a file - - ```javascript - // bad - /* eslint-disable */ - - // better - /* eslint-disable some-rule, some-other-rule */ - - // best - // nothing :) - ``` - -1. If you do need to disable a rule for a single violation, try to do it as locally as possible - - ```javascript - // bad - /* eslint-disable no-new */ - - import Foo from 'foo'; - - new Foo(); - - // better - import Foo from 'foo'; - - // eslint-disable-next-line no-new - new Foo(); - ``` - -1. There are few rules that we need to disable due to technical debt. Which are: - 1. [no-new](https://eslint.org/docs/rules/no-new) - 1. [class-methods-use-this](https://eslint.org/docs/rules/class-methods-use-this) - -1. When they are needed _always_ place ESlint directive comment blocks on the first line of a script, - followed by any global declarations, then a blank newline prior to any imports or code. - - ```javascript - // bad - /* global Foo */ - /* eslint-disable no-new */ - import Bar from './bar'; - - // good - /* eslint-disable no-new */ - /* global Foo */ - - import Bar from './bar'; - ``` - -1. **Never** disable the `no-undef` rule. Declare globals with `/* global Foo */` instead. - -1. When declaring multiple globals, always use one `/* global [name] */` line per variable. - - ```javascript - // bad - /* globals Flash, Cookies, jQuery */ - - // good - /* global Flash */ - /* global Cookies */ - /* global jQuery */ - ``` - -1. Use up to 3 parameters for a function or class. If you need more accept an Object instead. - - ```javascript - // bad - fn(p1, p2, p3, p4) {} - - // good - fn(options) {} - ``` - -#### Modules, Imports, and Exports - -1. Use ES module syntax to import modules - - ```javascript - // bad - const SomeClass = require('some_class'); - - // good - import SomeClass from 'some_class'; - - // bad - module.exports = SomeClass; - - // good - export default SomeClass; - ``` - - Import statements are following usual naming guidelines, for example object literals use camel case: - - ```javascript - // some_object file - export default { - key: 'value', - }; - - // bad - import ObjectLiteral from 'some_object'; - - // good - import objectLiteral from 'some_object'; - ``` - -1. Relative paths: when importing a module in the same directory, a child - directory, or an immediate parent directory prefer relative paths. When - importing a module which is two or more levels up, prefer either `~/` or `ee/`. - - In **app/assets/javascripts/my-feature/subdir**: - - ```javascript - // bad - import Foo from '~/my-feature/foo'; - import Bar from '~/my-feature/subdir/bar'; - import Bin from '~/my-feature/subdir/lib/bin'; - - // good - import Foo from '../foo'; - import Bar from './bar'; - import Bin from './lib/bin'; - ``` - - In **spec/javascripts**: - - ```javascript - // bad - import Foo from '../../app/assets/javascripts/my-feature/foo'; - - // good - import Foo from '~/my-feature/foo'; - ``` - - When referencing an **EE component**: - - ```javascript - // bad - import Foo from '../../../../../ee/app/assets/javascripts/my-feature/ee-foo'; - - // good - import Foo from 'ee/my-feature/foo'; - ``` - -1. Avoid using IIFE. Although we have a lot of examples of files which wrap their - contents in IIFEs (immediately-invoked function expressions), - this is no longer necessary after the transition from Sprockets to webpack. - Do not use them anymore and feel free to remove them when refactoring legacy code. - -1. Avoid adding to the global namespace. - - ```javascript - // bad - window.MyClass = class { /* ... */ }; - - // good - export default class MyClass { /* ... */ } - ``` - -1. Side effects are forbidden in any script which contains export - - ```javascript - // bad - export default class MyClass { /* ... */ } - - document.addEventListener("DOMContentLoaded", function(event) { - new MyClass(); - } - ``` - -#### Data Mutation and Pure functions - -1. Strive to write many small pure functions, and minimize where mutations occur. - - ```javascript - // bad - const values = {foo: 1}; - - function impureFunction(items) { - const bar = 1; - - items.foo = items.a * bar + 2; - - return items.a; - } - - const c = impureFunction(values); - - // good - var values = {foo: 1}; - - function pureFunction (foo) { - var bar = 1; - - foo = foo * bar + 2; - - return foo; - } - - var c = pureFunction(values.foo); - ``` - -1. Avoid constructors with side-effects. - Although we aim for code without side-effects we need some side-effects for our code to run. - - If the class won't do anything if we only instantiate it, it's ok to add side effects into the constructor (_Note:_ The following is just an example. If the only purpose of the class is to add an event listener and handle the callback a function will be more suitable.) - - ```javascript - // Bad - export class Foo { - constructor() { - this.init(); - } - init() { - document.addEventListener('click', this.handleCallback) - }, - handleCallback() { - - } - } - - // Good - export class Foo { - constructor() { - document.addEventListener() - } - handleCallback() { - } - } - ``` - - On the other hand, if a class only needs to extend a third party/add event listeners in some specific cases, they should be initialized outside of the constructor. - -1. Prefer `.map`, `.reduce` or `.filter` over `.forEach` - A forEach will most likely cause side effects, it will be mutating the array being iterated. Prefer using `.map`, - `.reduce` or `.filter` - - ```javascript - const users = [ { name: 'Foo' }, { name: 'Bar' } ]; - - // bad - users.forEach((user, index) => { - user.id = index; - }); - - // good - const usersWithId = users.map((user, index) => { - return Object.assign({}, user, { id: index }); - }); - ``` - -#### Parse Strings into Numbers - -1. `parseInt()` is preferable over `Number()` or `+` - - ```javascript - // bad - +'10' // 10 - - // good - Number('10') // 10 - - // better - parseInt('10', 10); - ``` - -#### CSS classes used for JavaScript - -1. If the class is being used in JavaScript it needs to be prepend with `js-` - - ```html - // bad - <button class="add-user"> - Add User - </button> - - // good - <button class="js-add-user"> - Add User - </button> - ``` - -### Vue.js - -#### `eslint-vue-plugin` - -We default to [eslint-vue-plugin][eslint-plugin-vue], with the `plugin:vue/recommended`. -Please check this [rules][eslint-plugin-vue-rules] for more documentation. - -#### Basic Rules - -1. The service has it's own file -1. The store has it's own file -1. Use a function in the bundle file to instantiate the Vue component: - - ```javascript - // bad - class { - init() { - new Component({}) - } - } - - // good - document.addEventListener('DOMContentLoaded', () => new Vue({ - el: '#element', - components: { - componentName - }, - render: createElement => createElement('component-name'), - })); - ``` - -1. Do not use a singleton for the service or the store - - ```javascript - // bad - class Store { - constructor() { - if (!this.prototype.singleton) { - // do something - } - } - } - - // good - class Store { - constructor() { - // do something - } - } - ``` - -1. Use `.vue` for Vue templates. Do not use `%template` in HAML. - -#### Naming - -1. **Extensions**: Use `.vue` extension for Vue components. Do not use `.js` as file extension ([#34371]). -1. **Reference Naming**: Use PascalCase for their instances: - - ```javascript - // bad - import cardBoard from 'cardBoard.vue' - - components: { - cardBoard, - }; - - // good - import CardBoard from 'cardBoard.vue' - - components: { - CardBoard, - }; - ``` - -1. **Props Naming:** Avoid using DOM component prop names. -1. **Props Naming:** Use kebab-case instead of camelCase to provide props in templates. - - ```javascript - // bad - <component class="btn"> - - // good - <component css-class="btn"> - - // bad - <component myProp="prop" /> - - // good - <component my-prop="prop" /> - ``` - -[#34371]: https://gitlab.com/gitlab-org/gitlab-foss/issues/34371 - -#### Alignment - -1. Follow these alignment styles for the template method: - - 1. With more than one attribute, all attributes should be on a new line: - - ```javascript - // bad - <component v-if="bar" - param="baz" /> - - <button class="btn">Click me</button> - - // good - <component - v-if="bar" - param="baz" - /> - - <button class="btn"> - Click me - </button> - ``` - - 1. The tag can be inline if there is only one attribute: - - ```javascript - // good - <component bar="bar" /> - - // good - <component - bar="bar" - /> - - // bad - <component - bar="bar" /> - ``` - -#### Quotes - -1. Always use double quotes `"` inside templates and single quotes `'` for all other JS. - - ```javascript - // bad - template: ` - <button :class='style'>Button</button> - ` - - // good - template: ` - <button :class="style">Button</button> - ` - ``` - -#### Props - -1. Props should be declared as an object - - ```javascript - // bad - props: ['foo'] - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' - } - } - ``` - -1. Required key should always be provided when declaring a prop - - ```javascript - // bad - props: { - foo: { - type: String, - } - } - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' - } - } - ``` - -1. Default key should be provided if the prop is not required. - _Note:_ There are some scenarios where we need to check for the existence of the property. - On those a default key should not be provided. - - ```javascript - // good - props: { - foo: { - type: String, - required: false, - } - } - - // good - props: { - foo: { - type: String, - required: false, - default: 'bar' - } - } - - // good - props: { - foo: { - type: String, - required: true - } - } - ``` - -#### Data - -1. `data` method should always be a function - - ```javascript - // bad - data: { - foo: 'foo' - } - - // good - data() { - return { - foo: 'foo' - }; - } - ``` - -#### Directives - -1. Shorthand `@` is preferable over `v-on` - - ```javascript - // bad - <component v-on:click="eventHandler"/> - - // good - <component @click="eventHandler"/> - ``` - -1. Shorthand `:` is preferable over `v-bind` - - ```javascript - // bad - <component v-bind:class="btn"/> - - // good - <component :class="btn"/> - ``` - -1. Shorthand `#` is preferable over `v-slot` - - ```javascript - // bad - <template v-slot:header></template> - - // good - <template #header></template> - ``` - -#### Closing tags - -1. Prefer self closing component tags - - ```javascript - // bad - <component></component> - - // good - <component /> - ``` - -#### Component usage within templates - -1. Prefer a component's kebab-cased name over other styles when using it in a template - - ```javascript - // bad - <MyComponent /> - - // good - <my-component /> - ``` - -#### Ordering - -1. Tag order in `.vue` file - - ``` - <script> - // ... - </script> - - <template> - // ... - </template> - - // We don't use scoped styles but there are few instances of this - <style> - // ... - </style> - ``` - -1. Properties in a Vue Component: - Check [order of properties in components rule][vue-order]. - -#### `:key` - -When using `v-for` you need to provide a *unique* `:key` attribute for each item. - -1. If the elements of the array being iterated have an unique `id` it is advised to use it: - - ```html - <div - v-for="item in items" - :key="item.id" - > - <!-- content --> - </div> - ``` - -1. When the elements being iterated don't have a unique id, you can use the array index as the `:key` attribute - - ```html - <div - v-for="(item, index) in items" - :key="index" - > - <!-- content --> - </div> - ``` - -1. When using `v-for` with `template` and there is more than one child element, the `:key` values must be unique. It's advised to use `kebab-case` namespaces. - - ```html - <template v-for="(item, index) in items"> - <span :key="`span-${index}`"></span> - <button :key="`button-${index}`"></button> - </template> - ``` - -1. When dealing with nested `v-for` use the same guidelines as above. - - ```html - <div - v-for="item in items" - :key="item.id" - > - <span - v-for="element in array" - :key="element.id" - > - <!-- content --> - </span> - </div> - ``` - -Useful links: - -1. [`key`](https://vuejs.org/v2/guide/list.html#key) -1. [Vue Style Guide: Keyed v-for](https://vuejs.org/v2/style-guide/#Keyed-v-for-essential ) - -#### Vue and Bootstrap - -1. Tooltips: Do not rely on `has-tooltip` class name for Vue components - - ```javascript - // bad - <span - class="has-tooltip" - title="Some tooltip text"> - Text - </span> - - // good - <span - v-tooltip - title="Some tooltip text"> - Text - </span> - ``` - -1. Tooltips: When using a tooltip, include the tooltip directive, `./app/assets/javascripts/vue_shared/directives/tooltip.js` - -1. Don't change `data-original-title`. - - ```javascript - // bad - <span data-original-title="tooltip text">Foo</span> - - // good - <span title="tooltip text">Foo</span> - - $('span').tooltip('_fixTitle'); - ``` - -### The JavaScript/Vue Accord - -The goal of this accord is to make sure we are all on the same page. - -1. When writing Vue, you may not use jQuery in your application. - 1. If you need to grab data from the DOM, you may query the DOM 1 time while bootstrapping your application to grab data attributes using `dataset`. You can do this without jQuery. - 1. You may use a jQuery dependency in Vue.js following [this example from the docs](https://vuejs.org/v2/examples/select2.html). - 1. If an outside jQuery Event needs to be listen to inside the Vue application, you may use jQuery event listeners. - 1. We will avoid adding new jQuery events when they are not required. Instead of adding new jQuery events take a look at [different methods to do the same task](https://vuejs.org/v2/api/#vm-emit). -1. You may query the `window` object 1 time, while bootstrapping your application for application specific data (e.g. `scrollTo` is ok to access anytime). Do this access during the bootstrapping of your application. -1. You may have a temporary but immediate need to create technical debt by writing code that does not follow our standards, to be refactored later. Maintainers need to be ok with the tech debt in the first place. An issue should be created for that tech debt to evaluate it further and discuss. In the coming months you should fix that tech debt, with it's priority to be determined by maintainers. -1. When creating tech debt you must write the tests for that code before hand and those tests may not be rewritten. e.g. jQuery tests rewritten to Vue tests. -1. You may choose to use VueX as a centralized state management. If you choose not to use VueX, you must use the *store pattern* which can be found in the [Vue.js documentation](https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch). -1. Once you have chosen a centralized state management solution you must use it for your entire application. i.e. Don't mix and match your state management solutions. - -## SCSS - -- [SCSS](style_guide_scss.md) - -[airbnb-js-style-guide]: https://github.com/airbnb/javascript -[eslintrc]: https://gitlab.com/gitlab-org/gitlab/blob/master/.eslintrc -[eslint-plugin-vue]: https://github.com/vuejs/eslint-plugin-vue -[eslint-plugin-vue-rules]: https://github.com/vuejs/eslint-plugin-vue#bulb-rules -[vue-order]: https://github.com/vuejs/eslint-plugin-vue/blob/master/docs/rules/order-in-components.md +This document was moved to [another location](style/javascript.md). diff --git a/doc/development/fe_guide/style_guide_scss.md b/doc/development/fe_guide/style_guide_scss.md index 1a5b24ce6f4..2b4e6427a18 100644 --- a/doc/development/fe_guide/style_guide_scss.md +++ b/doc/development/fe_guide/style_guide_scss.md @@ -1,281 +1,5 @@ -# SCSS styleguide +--- +redirect_to: 'style/scss.md' +--- -This style guide recommends best practices for SCSS to make styles easy to read, -easy to maintain, and performant for the end-user. - -## Rules - -### Utility Classes - -As part of the effort for [cleaning up our CSS and moving our components into GitLab-UI](https://gitlab.com/groups/gitlab-org/-/epics/950) -led by the [GitLab UI WG](https://gitlab.com/gitlab-com/www-gitlab-com/merge_requests/20623) we prefer the use of utility classes over adding new CSS. However, complex CSS can be addressed by adding component classes. - -#### Where are utility classes defined? - -- [Bootstrap's Utility Classes](https://getbootstrap.com/docs/4.3/utilities/) -- [`common.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/framework/common.scss) (old) -- [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/utilities.scss) (new) - -#### Where should I put new utility classes? - -New utility classes should be added to [`utilities.scss`](https://gitlab.com/gitlab-org/gitlab/blob/master/app/assets/stylesheets/utilities.scss). Existing classes include: - -| Name | Pattern | Example | -|------|---------|---------| -| Background color | `.bg-{variant}-{shade}` | `.bg-warning-400` | -| Text color | `.text-{variant}-{shade}` | `.text-success-500` | -| Font size | `.text-{size}` | `.text-2` | - -- `{variant}` is one of 'primary', 'secondary', 'success', 'warning', 'error' -- `{shade}` is one of the shades listed on [colors](https://design.gitlab.com/product-foundations/colors/) -- `{size}` is a number from 1-6 from our [Type scale](https://design.gitlab.com/product-foundations/typography/) - -#### When should I create component classes? - -We recommend a "utility-first" approach. - -1. Start with utility classes. -1. If composing utility classes into a component class removes code duplication and encapsulates a clear responsibility, do it. - -This encourages an organic growth of component classes and prevents the creation of one-off unreusable classes. Also, the kind of classes that emerge from "utility-first" tend to be design-centered (e.g. `.button`, `.alert`, `.card`) rather than domain-centered (e.g. `.security-report-widget`, `.commit-header-icon`). - -Examples of component classes that were created using "utility-first" include: - -- [`.circle-icon-container`](https://gitlab.com/gitlab-org/gitlab/blob/579fa8b8ec7eb38d40c96521f517c9dab8c3b97a/app/assets/stylesheets/framework/icons.scss#L85) -- [`.d-flex-center`](https://gitlab.com/gitlab-org/gitlab/blob/900083d89cd6af391d26ab7922b3f64fa2839bef/app/assets/stylesheets/framework/common.scss#L425) - -Inspiration: - -- <https://tailwindcss.com/docs/utility-first/> -- <https://tailwindcss.com/docs/extracting-components/> - -### Naming - -Filenames should use `snake_case`. - -CSS classes should use the `lowercase-hyphenated` format rather than -`snake_case` or `camelCase`. - -```scss -// Bad -.class_name { - color: #fff; -} - -// Bad -.className { - color: #fff; -} - -// Good -.class-name { - color: #fff; -} -``` - -### Formatting - -You should always use a space before a brace, braces should be on the same -line, each property should each get its own line, and there should be a space -between the property and its value. - -```scss -// Bad -.container-item { - width: 100px; height: 100px; - margin-top: 0; -} - -// Bad -.container-item -{ - width: 100px; - height: 100px; - margin-top: 0; -} - -// Bad -.container-item{ - width:100px; - height:100px; - margin-top:0; -} - -// Good -.container-item { - width: 100px; - height: 100px; - margin-top: 0; -} -``` - -Note that there is an exception for single-line rulesets, although these are -not typically recommended. - -```scss -p { margin: 0; padding: 0; } -``` - -### Colors - -HEX (hexadecimal) colors should use shorthand where possible, and should use -lower case letters to differentiate between letters and numbers, e.g. `#E3E3E3` -vs. `#e3e3e3`. - -```scss -// Bad -p { - color: #ffffff; -} - -// Bad -p { - color: #FFFFFF; -} - -// Good -p { - color: #fff; -} -``` - -### Indentation - -Indentation should always use two spaces for each indentation level. - -```scss -// Bad, four spaces -p { - color: #f00; -} - -// Good -p { - color: #f00; -} -``` - -### Semicolons - -Always include semicolons after every property. When the stylesheets are -minified, the semicolons will be removed automatically. - -```scss -// Bad -.container-item { - width: 100px; - height: 100px -} - -// Good -.container-item { - width: 100px; - height: 100px; -} -``` - -### Shorthand - -The shorthand form should be used for properties that support it. - -```scss -// Bad -margin: 10px 15px 10px 15px; -padding: 10px 10px 10px 10px; - -// Good -margin: 10px 15px; -padding: 10px; -``` - -### Zero Units - -Omit length units on zero values, they're unnecessary and not including them -is slightly more performant. - -```scss -// Bad -.item-with-padding { - padding: 0px; -} - -// Good -.item-with-padding { - padding: 0; -} -``` - -### Selectors with a `js-` Prefix - -Do not use any selector prefixed with `js-` for styling purposes. These -selectors are intended for use only with JavaScript to allow for removal or -renaming without breaking styling. - -### IDs - -Don't use ID selectors in CSS. - -```scss -// Bad -#my-element { - padding: 0; -} - -// Good -.my-element { - padding: 0; -} -``` - -### Variables - -Before adding a new variable for a color or a size, guarantee: - -- There isn't already one -- There isn't a similar one we can use instead. - -## Linting - -We use [SCSS Lint](https://github.com/sds/scss-lint) to check for style guide conformity. It uses the -ruleset in `.scss-lint.yml`, which is located in the home directory of the -project. - -To check if any warnings will be produced by your changes, you can run `rake -scss_lint` in the GitLab directory. SCSS Lint will also run in GitLab CI to -catch any warnings. - -If the Rake task is throwing warnings you don't understand, SCSS Lint's -documentation includes [a full list of their linters][scss-lint-documentation](https://github.com/sds/scss-lint/blob/master/lib/scss_lint/linter/README.md). - -### Fixing issues - -If you want to automate changing a large portion of the codebase to conform to -the SCSS style guide, you can use [CSSComb][csscomb]. First install -[Node][node] and [NPM][npm], then run `npm install csscomb -g` to install -CSSComb globally (system-wide). Run it in the GitLab directory with -`csscomb app/assets/stylesheets` to automatically fix issues with CSS/SCSS. - -Note that this won't fix every problem, but it should fix a majority. - -### Ignoring issues - -If you want a line or set of lines to be ignored by the linter, you can use -`// scss-lint:disable RuleName` ([more info](https://github.com/sds/scss-lint#disabling-linters-via-source)): - -```scss -// This lint rule is disabled because it is supported only in Chrome/Safari -// scss-lint:disable PropertySpelling -body { - text-decoration-skip: ink; -} -// scss-lint:enable PropertySpelling -``` - -Make sure a comment is added on the line above the `disable` rule, otherwise the -linter will throw a warning. `DisableLinterReason` is enabled to make sure the -style guide isn't being ignored, and to communicate to others why the style -guide is ignored in this instance. - -[csscomb]: https://github.com/csscomb/csscomb.js -[node]: https://github.com/nodejs/node -[npm]: https://www.npmjs.com/ +This document was moved to [another location](style/scss.md). diff --git a/doc/development/fe_guide/tooling.md b/doc/development/fe_guide/tooling.md new file mode 100644 index 00000000000..ec5e094cfa6 --- /dev/null +++ b/doc/development/fe_guide/tooling.md @@ -0,0 +1,154 @@ +# Tooling + +## ESLint + +We use ESLint to encapsulate and enforce frontend code standards. Our configuration may be found in the [gitlab-eslint-config](https://gitlab.com/gitlab-org/gitlab-eslint-config) project. + +### Disabling ESLint in new files + +Do not disable ESLint when creating new files. Existing files may have existing rules +disabled due to legacy compatibility reasons but they are in the process of being refactored. + +Do not disable specific ESLint rules. To avoid introducing technical debt, you may disable the following +rules only if you are invoking/instantiating existing code modules. + +- [`no-new`](https://eslint.org/docs/rules/no-new) +- [`class-method-use-this`](https://eslint.org/docs/rules/class-methods-use-this) + +NOTE: **Note:** +Disable these rules on a per-line basis. This makes it easier to refactor +in the future. E.g. use `eslint-disable-next-line` or `eslint-disable-line`. + +### Disabling ESLint for a single violation + +If you do need to disable a rule for a single violation, disable it for the smallest amount of code necessary: + +```javascript +// bad +/* eslint-disable no-new */ + +import Foo from 'foo'; + +new Foo(); + +// better +import Foo from 'foo'; + +// eslint-disable-next-line no-new +new Foo(); +``` + +### The `no-undef` rule and declaring globals + +**Never** disable the `no-undef` rule. Declare globals with `/* global Foo */` instead. + +When declaring multiple globals, always use one `/* global [name] */` line per variable. + +```javascript +// bad +/* globals Flash, Cookies, jQuery */ + +// good +/* global Flash */ +/* global Cookies */ +/* global jQuery */ +``` + +## Formatting with Prettier + +Our code is automatically formatted with [Prettier](https://prettier.io) to follow our style guides. Prettier is taking care of formatting .js, .vue, and .scss files based on the standard prettier rules. You can find all settings for Prettier in `.prettierrc`. + +### Editor + +The easiest way to include prettier in your workflow is by setting up your preferred editor (all major editors are supported) accordingly. We suggest setting up prettier to run automatically when each file is saved. Find [here](https://prettier.io/docs/en/editors.html) the best way to set it up in your preferred editor. + +Please take care that you only let Prettier format the same file types as the global Yarn script does (.js, .vue, and .scss). In VSCode by example you can easily exclude file formats in your settings file: + +``` + "prettier.disableLanguages": [ + "json", + "markdown" + ], +``` + +### Yarn Script + +The following yarn scripts are available to do global formatting: + +``` +yarn prettier-staged-save +``` + +Updates all currently staged files (based on `git diff`) with Prettier and saves the needed changes. + +``` +yarn prettier-staged +``` + +Checks all currently staged files (based on `git diff`) with Prettier and log which files would need manual updating to the console. + +``` +yarn prettier-all +``` + +Checks all files with Prettier and logs which files need manual updating to the console. + +``` +yarn prettier-all-save +``` + +Formats all files in the repository with Prettier. (This should only be used to test global rule updates otherwise you would end up with huge MR's). + +The source of these Yarn scripts can be found in `/scripts/frontend/prettier.js`. + +#### Scripts during Conversion period + +``` +node ./scripts/frontend/prettier.js check-all ./vendor/ +``` + +This will go over all files in a specific folder check it. + +``` +node ./scripts/frontend/prettier.js save-all ./vendor/ +``` + +This will go over all files in a specific folder and save it. + +### VSCode Settings + +#### Select Prettier as default formatter + +To select Prettier as a formatter, add the following properties to your User or Workspace Settings: + +```javascript +{ + "[html]": { + "editor.defaultFormatter": "esbenp.prettier-vscode" + }, + "[javascript]": { + "editor.defaultFormatter": "esbenp.prettier-vscode" + }, + "[vue]": { + "editor.defaultFormatter": "esbenp.prettier-vscode" + } +} +``` + +#### Format on Save + +To automatically format your files with Prettier, add the following properties to your User or Workspace Settings: + +```javascript +{ + "[html]": { + "editor.formatOnSave": true + }, + "[javascript]": { + "editor.formatOnSave": true + }, + "[vue]": { + "editor.formatOnSave": true + }, +} +``` diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index e13a7d1e478..c3edaec70c7 100644 --- a/doc/development/fe_guide/vue.md +++ b/doc/development/fe_guide/vue.md @@ -179,7 +179,7 @@ Check this [page](vuex.md) for more details. ## Style guide -Please refer to the Vue section of our [style guide](style_guide_js.md#vuejs) +Please refer to the Vue section of our [style guide](style/vue.md) for best practices while writing your Vue components and templates. ## Testing Vue Components diff --git a/doc/development/new_fe_guide/index.md b/doc/development/new_fe_guide/index.md index 227d03bd86f..152ddcdae64 100644 --- a/doc/development/new_fe_guide/index.md +++ b/doc/development/new_fe_guide/index.md @@ -15,10 +15,6 @@ Learn about all the dependencies that make up our frontend, including some of ou Learn about all the internal JavaScript modules that make up our frontend. -## [Style guides](style/index.md) - -Style guides to keep our code consistent. - ## [Tips](tips.md) Tips from our frontend team to develop more efficiently and effectively. diff --git a/doc/development/new_fe_guide/style/html.md b/doc/development/new_fe_guide/style/html.md index 1445da3f0e1..0b4fce13d90 100644 --- a/doc/development/new_fe_guide/style/html.md +++ b/doc/development/new_fe_guide/style/html.md @@ -1,53 +1,5 @@ -# HTML style guide +--- +redirect_to: '../../fe_guide/style/html.md' +--- -## Buttons - -### Button type - -Button tags requires a `type` attribute according to the [W3C HTML specification](https://www.w3.org/TR/2011/WD-html5-20110525/the-button-element.html#dom-button-type). - -```html -// bad -<button></button> - -// good -<button type="button"></button> -``` - -### Button role - -If an HTML element has an `onClick` handler but is not a button, it should have `role="button"`. This is [more accessible](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role). - -```html -// bad -<div onClick="doSomething"></div> - -// good -<div role="button" onClick="doSomething"></div> -``` - -## Links - -### Blank target - -Use `rel="noopener noreferrer"` whenever your links open in a new window, i.e. `target="_blank"`. This prevents a security vulnerability [documented by JitBit](https://www.jitbit.com/alexblog/256-targetblank---the-most-underestimated-vulnerability-ever/). - -```html -// bad -<a href="url" target="_blank"></a> - -// good -<a href="url" target="_blank" rel="noopener noreferrer"></a> -``` - -### Fake links - -**Do not use fake links.** Use a button tag if a link only invokes JavaScript click event handlers, which is more semantic. - -```html -// bad -<a class="js-do-something" href="#"></a> - -// good -<button class="js-do-something" type="button"></button> -``` +This document was moved to [another location](../../fe_guide/style/html.md). diff --git a/doc/development/new_fe_guide/style/index.md b/doc/development/new_fe_guide/style/index.md index f073dc56f1f..284862a2be9 100644 --- a/doc/development/new_fe_guide/style/index.md +++ b/doc/development/new_fe_guide/style/index.md @@ -1,15 +1,5 @@ -# Style guides +--- +redirect_to: '../../fe_guide/style/index.md' +--- -## [HTML style guide](html.md) - -## [SCSS style guide](scss.md) - -## [JavaScript style guide](javascript.md) - -## [Vue style guide](vue.md) - -## Tooling - -## [Prettier](prettier.md) - -Our code is automatically formatted with [Prettier](https://prettier.io) to follow our guidelines. +This document was moved to [another location](../../fe_guide/style/index.md). diff --git a/doc/development/new_fe_guide/style/javascript.md b/doc/development/new_fe_guide/style/javascript.md index d31edcb372d..003880c2592 100644 --- a/doc/development/new_fe_guide/style/javascript.md +++ b/doc/development/new_fe_guide/style/javascript.md @@ -1,195 +1,5 @@ -# JavaScript style guide +--- +redirect_to: '../../fe_guide/style/javascript.md' +--- -We use [Airbnb's JavaScript Style Guide](https://github.com/airbnb/javascript) and it's accompanying -linter to manage most of our JavaScript style guidelines. - -In addition to the style guidelines set by Airbnb, we also have a few specific rules -listed below. - -> **Tip:** -You can run eslint locally by running `yarn eslint` - -## Avoid forEach - -Avoid forEach when mutating data. Use `map`, `reduce` or `filter` instead of `forEach` -when mutating data. This will minimize mutations in functions, -which aligns with [Airbnb's style guide](https://github.com/airbnb/javascript#testing--for-real). - -```javascript -// bad -users.forEach((user, index) => { - user.id = index; -}); - -// good -const usersWithId = users.map((user, index) => { - return Object.assign({}, user, { id: index }); -}); -``` - -## Limit number of parameters - -If your function or method has more than 3 parameters, use an object as a parameter -instead. - -```javascript -// bad -function a(p1, p2, p3) { - // ... -}; - -// good -function a(p) { - // ... -}; -``` - -## Avoid side effects in constructors - -Avoid making asynchronous calls, API requests or DOM manipulations in the `constructor`. -Move them into separate functions instead. This will make tests easier to write and -code easier to maintain. - -```javascript -// bad -class myClass { - constructor(config) { - this.config = config; - axios.get(this.config.endpoint) - } -} - -// good -class myClass { - constructor(config) { - this.config = config; - } - - makeRequest() { - axios.get(this.config.endpoint) - } -} -const instance = new myClass(); -instance.makeRequest(); -``` - -## Avoid classes to handle DOM events - -If the only purpose of the class is to bind a DOM event and handle the callback, prefer -using a function. - -```javascript -// bad -class myClass { - constructor(config) { - this.config = config; - } - - init() { - document.addEventListener('click', () => {}); - } -} - -// good - -const myFunction = () => { - document.addEventListener('click', () => { - // handle callback here - }); -} -``` - -## Pass element container to constructor - -When your class manipulates the DOM, receive the element container as a parameter. -This is more maintainable and performant. - -```javascript -// bad -class a { - constructor() { - document.querySelector('.b'); - } -} - -// good -class a { - constructor(options) { - options.container.querySelector('.b'); - } -} -``` - -## Use ParseInt - -Use `ParseInt` when converting a numeric string into a number. - -```javascript -// bad -Number('10') - -// good -parseInt('10', 10); -``` - -## CSS Selectors - Use `js-` prefix - -If a CSS class is only being used in JavaScript as a reference to the element, prefix -the class name with `js-`. - -```html -// bad -<button class="add-user"></button> - -// good -<button class="js-add-user"></button> -``` - -## Absolute vs relative paths for modules - -Use relative paths if the module you are importing is less than two levels up. - -```javascript -// bad -import GitLabStyleGuide from '~/guides/GitLabStyleGuide'; - -// good -import GitLabStyleGuide from '../GitLabStyleGuide'; -``` - -If the module you are importing is two or more levels up, use an absolute path instead: - -```javascript -// bad -import GitLabStyleGuide from '../../../guides/GitLabStyleGuide'; - -// good -import GitLabStyleGuide from '~/GitLabStyleGuide'; -``` - -Additionally, **do not add to global namespace**. - -## Do not use `DOMContentLoaded` in non-page modules - -Imported modules should act the same each time they are loaded. `DOMContentLoaded` -events are only allowed on modules loaded in the `/pages/*` directory because those -are loaded dynamically with webpack. - -## Avoid XSS - -Do not use `innerHTML`, `append()` or `html()` to set content. It opens up too many -vulnerabilities. - -## Disabling ESLint in new files - -Do not disable ESLint when creating new files. Existing files may have existing rules -disabled due to legacy compatibility reasons but they are in the process of being refactored. - -Do not disable specific ESLint rules. Due to technical debt, you may disable the following -rules only if you are invoking/instantiating existing code modules. - -- [no-new](https://eslint.org/docs/rules/no-new) -- [class-method-use-this](https://eslint.org/docs/rules/class-methods-use-this) - -> Note: Disable these rules on a per line basis. This makes it easier to refactor -> in the future. E.g. use `eslint-disable-next-line` or `eslint-disable-line`. +This document was moved to [another location](../../fe_guide/style/javascript.md). diff --git a/doc/development/new_fe_guide/style/prettier.md b/doc/development/new_fe_guide/style/prettier.md index 17b209d419e..9a95aa96dff 100644 --- a/doc/development/new_fe_guide/style/prettier.md +++ b/doc/development/new_fe_guide/style/prettier.md @@ -1,98 +1,5 @@ -# Formatting with Prettier +--- +redirect_to: '../../fe_guide/tooling.md#formatting-with-prettier' +--- -Our code is automatically formatted with [Prettier](https://prettier.io) to follow our style guides. Prettier is taking care of formatting .js, .vue, and .scss files based on the standard prettier rules. You can find all settings for Prettier in `.prettierrc`. - -## Editor - -The easiest way to include prettier in your workflow is by setting up your preferred editor (all major editors are supported) accordingly. We suggest setting up prettier to run automatically when each file is saved. Find [here](https://prettier.io/docs/en/editors.html) the best way to set it up in your preferred editor. - -Please take care that you only let Prettier format the same file types as the global Yarn script does (.js, .vue, and .scss). In VSCode by example you can easily exclude file formats in your settings file: - -``` - "prettier.disableLanguages": [ - "json", - "markdown" - ], -``` - -## Yarn Script - -The following yarn scripts are available to do global formatting: - -``` -yarn prettier-staged-save -``` - -Updates all currently staged files (based on `git diff`) with Prettier and saves the needed changes. - -``` -yarn prettier-staged -``` - -Checks all currently staged files (based on `git diff`) with Prettier and log which files would need manual updating to the console. - -``` -yarn prettier-all -``` - -Checks all files with Prettier and logs which files need manual updating to the console. - -``` -yarn prettier-all-save -``` - -Formats all files in the repository with Prettier. (This should only be used to test global rule updates otherwise you would end up with huge MR's). - -The source of these Yarn scripts can be found in `/scripts/frontend/prettier.js`. - -### Scripts during Conversion period - -``` -node ./scripts/frontend/prettier.js check-all ./vendor/ -``` - -This will go over all files in a specific folder check it. - -``` -node ./scripts/frontend/prettier.js save-all ./vendor/ -``` - -This will go over all files in a specific folder and save it. - -## VSCode Settings - -### Select Prettier as default formatter - -To select Prettier as a formatter, add the following properties to your User or Workspace Settings: - -```javascript -{ - "[html]": { - "editor.defaultFormatter": "esbenp.prettier-vscode" - }, - "[javascript]": { - "editor.defaultFormatter": "esbenp.prettier-vscode" - }, - "[vue]": { - "editor.defaultFormatter": "esbenp.prettier-vscode" - } -} -``` - -### Format on Save - -To automatically format your files with Prettier, add the following properties to your User or Workspace Settings: - -```javascript -{ - "[html]": { - "editor.formatOnSave": true - }, - "[javascript]": { - "editor.formatOnSave": true - }, - "[vue]": { - "editor.formatOnSave": true - }, -} -``` +This document was moved to [another location](../../fe_guide/tooling.md#formatting-with-prettier). diff --git a/doc/development/new_fe_guide/style/scss.md b/doc/development/new_fe_guide/style/scss.md deleted file mode 100644 index 6f5e818d7db..00000000000 --- a/doc/development/new_fe_guide/style/scss.md +++ /dev/null @@ -1,3 +0,0 @@ -# SCSS style guide - -> TODO: Add content diff --git a/doc/development/new_fe_guide/style/vue.md b/doc/development/new_fe_guide/style/vue.md deleted file mode 100644 index fd9353e0d3f..00000000000 --- a/doc/development/new_fe_guide/style/vue.md +++ /dev/null @@ -1,3 +0,0 @@ -# Vue style guide - -> TODO: Add content diff --git a/lib/api/helpers/pagination.rb b/lib/api/helpers/pagination.rb index 9c5b355e823..642053949d9 100644 --- a/lib/api/helpers/pagination.rb +++ b/lib/api/helpers/pagination.rb @@ -4,7 +4,21 @@ module API module Helpers module Pagination def paginate(relation) - ::Gitlab::Pagination::OffsetPagination.new(self).paginate(relation) + return Gitlab::Pagination::OffsetPagination.new(self).paginate(relation) unless keyset_pagination_enabled? + + request_context = Gitlab::Pagination::Keyset::RequestContext.new(self) + + unless Gitlab::Pagination::Keyset.available?(request_context, relation) + return error!('Keyset pagination is not yet available for this type of request', 501) + end + + Gitlab::Pagination::Keyset.paginate(request_context, relation) + end + + private + + def keyset_pagination_enabled? + params[:pagination] == 'keyset' && Feature.enabled?(:api_keyset_pagination) end end end diff --git a/lib/api/projects.rb b/lib/api/projects.rb index a1fce9e8b20..666bd2771f9 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -82,7 +82,6 @@ module API def present_projects(projects, options = {}) projects = reorder_projects(projects) projects = apply_filters(projects) - projects = paginate(projects) projects, options = with_custom_attributes(projects, options) options = options.reverse_merge( @@ -93,7 +92,10 @@ module API ) options[:with] = Entities::BasicProjectDetails if params[:simple] - present options[:with].prepare_relation(projects, options), options + projects = options[:with].prepare_relation(projects, options) + projects = paginate(projects) + + present projects, options end def translate_params_for_compatibility(params) diff --git a/lib/gitlab/marginalia.rb b/lib/gitlab/marginalia.rb new file mode 100644 index 00000000000..d2e0e335127 --- /dev/null +++ b/lib/gitlab/marginalia.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Gitlab + module Marginalia + MARGINALIA_FEATURE_FLAG = :marginalia + + def self.set_application_name + ::Marginalia.application_name = Gitlab.process_name + end + + def self.enable_sidekiq_instrumentation + if Sidekiq.server? + ::Marginalia::SidekiqInstrumentation.enable! + end + end + + def self.feature_enabled? + return false unless Gitlab::Database.cached_table_exists?('features') + + Feature.enabled?(MARGINALIA_FEATURE_FLAG) + end + end +end diff --git a/lib/gitlab/marginalia/active_record_instrumentation.rb b/lib/gitlab/marginalia/active_record_instrumentation.rb new file mode 100644 index 00000000000..f4500a48090 --- /dev/null +++ b/lib/gitlab/marginalia/active_record_instrumentation.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +# Patch to annotate sql only when the feature is enabled. +module Gitlab + module Marginalia + module ActiveRecordInstrumentation + # CAUTION: + # Any method call which generates a query inside this function will get into a recursive loop unless called within `Marginalia.without_annotation` method. + def annotate_sql(sql) + if ActiveRecord::Base.connected? && + ::Marginalia.annotation_allowed? && + ::Marginalia.without_annotation { Gitlab::Marginalia.feature_enabled? } + super(sql) + else + sql + end + end + end + end +end diff --git a/lib/gitlab/marginalia/comment.rb b/lib/gitlab/marginalia/comment.rb new file mode 100644 index 00000000000..a0eee823763 --- /dev/null +++ b/lib/gitlab/marginalia/comment.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +# Module to support correlation_id and additional job details. +module Gitlab + module Marginalia + module Comment + private + + def jid + bg_job["jid"] if bg_job.present? + end + + def job_class + bg_job["class"] if bg_job.present? + end + + def correlation_id + if bg_job.present? + bg_job["correlation_id"] + else + Labkit::Correlation::CorrelationId.current_id + end + end + + def bg_job + job = ::Marginalia::Comment.marginalia_job + + # We are using 'Marginalia::SidekiqInstrumentation' which does not support 'ActiveJob::Base'. + # Gitlab also uses 'ActionMailer::DeliveryJob' which inherits from ActiveJob::Base. + # So below condition is used to return metadata for such jobs. + if job && job.is_a?(ActionMailer::DeliveryJob) + { + "class" => job.arguments.first, + "jid" => job.job_id + } + else + job + end + end + end + end +end diff --git a/lib/gitlab/marginalia/inline_annotation.rb b/lib/gitlab/marginalia/inline_annotation.rb new file mode 100644 index 00000000000..6d78f3b81ec --- /dev/null +++ b/lib/gitlab/marginalia/inline_annotation.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true + +# Module with util methods to support ::Marginalia.without_annotation method. + +module Gitlab + module Marginalia + module InlineAnnotation + def without_annotation(&block) + return unless block.present? + + annotation_stack.push(false) + yield + ensure + annotation_stack.pop + end + + def with_annotation(comment, &block) + annotation_stack.push(true) + super(comment, &block) + ensure + annotation_stack.pop + end + + def annotation_stack + Thread.current[:annotation_stack] ||= [] + end + + def annotation_stack_top + annotation_stack.last + end + + def annotation_allowed? + annotation_stack.empty? ? true : annotation_stack_top + end + end + end +end diff --git a/lib/gitlab/pagination/keyset.rb b/lib/gitlab/pagination/keyset.rb new file mode 100644 index 00000000000..5bd45fa9b56 --- /dev/null +++ b/lib/gitlab/pagination/keyset.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Gitlab + module Pagination + module Keyset + def self.paginate(request_context, relation) + Gitlab::Pagination::Keyset::Pager.new(request_context).paginate(relation) + end + + def self.available?(request_context, relation) + order_by = request_context.page.order_by + + # This is only available for Project and order-by id (asc/desc) + return false unless relation.klass == Project + return false unless order_by.size == 1 && order_by[:id] + + true + end + end + end +end diff --git a/lib/gitlab/pagination/keyset/page.rb b/lib/gitlab/pagination/keyset/page.rb new file mode 100644 index 00000000000..3f71822a7c7 --- /dev/null +++ b/lib/gitlab/pagination/keyset/page.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +module Gitlab + module Pagination + module Keyset + # A Page models the pagination information for a particular page of the collection + class Page + # Default and maximum size of records for a page + DEFAULT_PAGE_SIZE = 20 + + attr_accessor :lower_bounds, :end_reached + attr_reader :order_by + + def initialize(order_by: {}, lower_bounds: nil, per_page: DEFAULT_PAGE_SIZE, end_reached: false) + @order_by = order_by.symbolize_keys + @lower_bounds = lower_bounds&.symbolize_keys + @per_page = per_page + @end_reached = end_reached + end + + # Number of records to return per page + def per_page + return DEFAULT_PAGE_SIZE if @per_page <= 0 + + [@per_page, DEFAULT_PAGE_SIZE].min + end + + # Determine whether this page indicates the end of the collection + def end_reached? + @end_reached + end + + # Construct a Page for the next page + # Uses identical order_by/per_page information for the next page + def next(lower_bounds, end_reached) + dup.tap do |next_page| + next_page.lower_bounds = lower_bounds&.symbolize_keys + next_page.end_reached = end_reached + end + end + end + end + end +end diff --git a/lib/gitlab/pagination/keyset/pager.rb b/lib/gitlab/pagination/keyset/pager.rb new file mode 100644 index 00000000000..99b125cc2a0 --- /dev/null +++ b/lib/gitlab/pagination/keyset/pager.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module Gitlab + module Pagination + module Keyset + class Pager + attr_reader :request + + def initialize(request) + @request = request + end + + def paginate(relation) + # Validate assumption: The last two columns must match the page order_by + validate_order!(relation) + + # This performs the database query and retrieves records + # We retrieve one record more to check if we have data beyond this page + all_records = relation.limit(page.per_page + 1).to_a # rubocop: disable CodeReuse/ActiveRecord + + records_for_page = all_records.first(page.per_page) + + # If we retrieved more records than belong on this page, + # we know there's a next page + there_is_more = all_records.size > records_for_page.size + apply_headers(records_for_page.last, there_is_more) + + records_for_page + end + + private + + def apply_headers(last_record_in_page, there_is_more) + end_reached = last_record_in_page.nil? || !there_is_more + lower_bounds = last_record_in_page&.slice(page.order_by.keys) + + next_page = page.next(lower_bounds, end_reached) + + request.apply_headers(next_page) + end + + def page + @page ||= request.page + end + + def validate_order!(rel) + present_order = rel.order_values.map { |val| [val.expr.name.to_sym, val.direction] }.last(2).to_h + + unless page.order_by == present_order + raise ArgumentError, "Page's order_by does not match the relation's order: #{present_order} vs #{page.order_by}" + end + end + end + end + end +end diff --git a/lib/gitlab/pagination/keyset/request_context.rb b/lib/gitlab/pagination/keyset/request_context.rb new file mode 100644 index 00000000000..aeaed7587b3 --- /dev/null +++ b/lib/gitlab/pagination/keyset/request_context.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +module Gitlab + module Pagination + module Keyset + class RequestContext + attr_reader :request + + DEFAULT_SORT_DIRECTION = :desc + PRIMARY_KEY = :id + + # A tie breaker is added as an additional order-by column + # to establish a well-defined order. We use the primary key + # column here. + TIE_BREAKER = { PRIMARY_KEY => DEFAULT_SORT_DIRECTION }.freeze + + def initialize(request) + @request = request + end + + # extracts Paging information from request parameters + def page + @page ||= Page.new(order_by: order_by, per_page: params[:per_page]) + end + + def apply_headers(next_page) + request.header('Links', pagination_links(next_page)) + end + + private + + def order_by + return TIE_BREAKER.dup unless params[:order_by] + + order_by = { params[:order_by].to_sym => params[:sort]&.to_sym || DEFAULT_SORT_DIRECTION } + + # Order by an additional unique key, we use the primary key here + order_by = order_by.merge(TIE_BREAKER) unless order_by[PRIMARY_KEY] + + order_by + end + + def params + @params ||= request.params + end + + def lower_bounds_params(page) + page.lower_bounds.each_with_object({}) do |(column, value), params| + filter = filter_with_comparator(page, column) + params[filter] = value + end + end + + def filter_with_comparator(page, column) + direction = page.order_by[column] + + if direction&.to_sym == :desc + "#{column}_before" + else + "#{column}_after" + end + end + + def page_href(page) + base_request_uri.tap do |uri| + uri.query = query_params_for(page).to_query + end.to_s + end + + def pagination_links(next_page) + return if next_page.end_reached? + + %(<#{page_href(next_page)}>; rel="next") + end + + def base_request_uri + @base_request_uri ||= URI.parse(request.request.url).tap do |uri| + uri.host = Gitlab.config.gitlab.host + uri.port = Gitlab.config.gitlab.port + end + end + + def query_params_for(page) + request.params.merge(lower_bounds_params(page)) + end + end + end + end +end diff --git a/qa/qa/page/group/menu.rb b/qa/qa/page/group/menu.rb index 6353895ffd4..2b3b872aff4 100644 --- a/qa/qa/page/group/menu.rb +++ b/qa/qa/page/group/menu.rb @@ -10,6 +10,7 @@ module QA element :group_settings_item element :group_members_item element :general_settings_link + element :contribution_analytics_link end def click_group_members_item @@ -18,6 +19,12 @@ module QA end end + def click_group_analytics_item + within_sidebar do + click_element(:contribution_analytics_link) + end + end + def click_group_general_settings_item hover_element(:group_settings_item) do within_submenu(:group_sidebar_submenu) do diff --git a/qa/qa/page/merge_request/show.rb b/qa/qa/page/merge_request/show.rb index 54a08d911cb..9ad53636c42 100644 --- a/qa/qa/page/merge_request/show.rb +++ b/qa/qa/page/merge_request/show.rb @@ -26,7 +26,7 @@ module QA end view 'app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue' do - element :merged_status, 'The changes were merged into' # rubocop:disable QA/ElementWithPattern + element :merged_status_content end view 'app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_rebase.vue' do @@ -86,13 +86,31 @@ module QA has_element?(:merge_moment_dropdown) end + def merged? + has_element? :merged_status_content, text: 'The changes were merged into' + end + def merge_immediately + wait(reload: false, max: 60) do + has_merge_options? + end + if has_merge_options? - click_element :merge_moment_dropdown + if has_no_element? :merge_immediately_option + retry_until do + click_element :merge_moment_dropdown + has_element? :merge_immediately_option + end + end + click_element :merge_immediately_option else click_element :merge_button end + + wait(reload: false, max: 60) do + merged? + end end def rebase! diff --git a/qa/qa/page/project/issue/show.rb b/qa/qa/page/project/issue/show.rb index 2322b5607b0..0622cb925f9 100644 --- a/qa/qa/page/project/issue/show.rb +++ b/qa/qa/page/project/issue/show.rb @@ -44,6 +44,7 @@ module QA end view 'app/views/shared/issuable/_close_reopen_button.html.haml' do + element :close_issue_button element :reopen_issue_button end @@ -84,6 +85,10 @@ module QA click_element(:remove_related_issue_button) end + def click_close_issue_button + click_element :close_issue_button + end + # Adds a comment to an issue # attachment option should be an absolute path def comment(text, attachment: nil, filter: :all_activities) diff --git a/spec/lib/api/helpers/pagination_spec.rb b/spec/lib/api/helpers/pagination_spec.rb index 040ff1a8ebe..d61341ed3e5 100644 --- a/spec/lib/api/helpers/pagination_spec.rb +++ b/spec/lib/api/helpers/pagination_spec.rb @@ -10,13 +10,47 @@ describe API::Helpers::Pagination do let(:offset_pagination) { double("offset pagination") } let(:expected_result) { double("result") } - it 'delegates to OffsetPagination' do - expect(::Gitlab::Pagination::OffsetPagination).to receive(:new).with(subject).and_return(offset_pagination) - expect(offset_pagination).to receive(:paginate).with(relation).and_return(expected_result) + before do + allow(subject).to receive(:params).and_return(params) + end + + context 'for offset pagination' do + let(:params) { {} } + + it 'delegates to OffsetPagination' do + expect(::Gitlab::Pagination::OffsetPagination).to receive(:new).with(subject).and_return(offset_pagination) + expect(offset_pagination).to receive(:paginate).with(relation).and_return(expected_result) + + result = subject.paginate(relation) + + expect(result).to eq(expected_result) + end + end + + context 'for keyset pagination' do + let(:params) { { pagination: 'keyset' } } + let(:request_context) { double('request context') } + + before do + allow(Gitlab::Pagination::Keyset::RequestContext).to receive(:new).with(subject).and_return(request_context) + allow(Gitlab::Pagination::Keyset).to receive(:available?).and_return(true) + end + + it 'delegates to KeysetPagination' do + expect(Gitlab::Pagination::Keyset).to receive(:paginate).with(request_context, relation).and_return(expected_result) + + result = subject.paginate(relation) + + expect(result).to eq(expected_result) + end - result = subject.paginate(relation) + it 'renders a 501 error if keyset pagination isnt available yet' do + expect(Gitlab::Pagination::Keyset).to receive(:available?).with(request_context, relation).and_return(false) + expect(Gitlab::Pagination::Keyset).not_to receive(:paginate) + expect(subject).to receive(:error!).with(/not yet available/, 501) - expect(result).to eq(expected_result) + subject.paginate(relation) + end end end end diff --git a/spec/lib/feature_spec.rb b/spec/lib/feature_spec.rb index 3d59b1f35a9..7254981d09a 100644 --- a/spec/lib/feature_spec.rb +++ b/spec/lib/feature_spec.rb @@ -175,6 +175,7 @@ describe Feature do let(:flag) { :some_feature_flag } before do + stub_feature_flags(Gitlab::Marginalia::MARGINALIA_FEATURE_FLAG => false) described_class.flipper.memoize = false described_class.enabled?(flag) end diff --git a/spec/lib/gitlab/pagination/keyset/page_spec.rb b/spec/lib/gitlab/pagination/keyset/page_spec.rb new file mode 100644 index 00000000000..bda9e6ecd13 --- /dev/null +++ b/spec/lib/gitlab/pagination/keyset/page_spec.rb @@ -0,0 +1,66 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Pagination::Keyset::Page do + describe '#per_page' do + it 'limits to a maximum of 20 records per page' do + per_page = described_class.new(per_page: 21).per_page + + expect(per_page).to eq(described_class::DEFAULT_PAGE_SIZE) + end + + it 'uses default value when given 0' do + per_page = described_class.new(per_page: 0).per_page + + expect(per_page).to eq(described_class::DEFAULT_PAGE_SIZE) + end + + it 'uses default value when given negative values' do + per_page = described_class.new(per_page: -1).per_page + + expect(per_page).to eq(described_class::DEFAULT_PAGE_SIZE) + end + + it 'uses the given value if it is within range' do + per_page = described_class.new(per_page: 10).per_page + + expect(per_page).to eq(10) + end + end + + describe '#next' do + let(:page) { described_class.new(order_by: order_by, lower_bounds: lower_bounds, per_page: per_page, end_reached: end_reached) } + subject { page.next(new_lower_bounds, new_end_reached) } + + let(:order_by) { { id: :desc } } + let(:lower_bounds) { { id: 42 } } + let(:per_page) { 10 } + let(:end_reached) { false } + + let(:new_lower_bounds) { { id: 21 } } + let(:new_end_reached) { true } + + it 'copies over order_by' do + expect(subject.order_by).to eq(page.order_by) + end + + it 'copies over per_page' do + expect(subject.per_page).to eq(page.per_page) + end + + it 'dups the instance' do + expect(subject).not_to eq(page) + end + + it 'sets lower_bounds only on new instance' do + expect(subject.lower_bounds).to eq(new_lower_bounds) + expect(page.lower_bounds).to eq(lower_bounds) + end + + it 'sets end_reached only on new instance' do + expect(subject.end_reached?).to eq(new_end_reached) + expect(page.end_reached?).to eq(end_reached) + end + end +end diff --git a/spec/lib/gitlab/pagination/keyset/pager_spec.rb b/spec/lib/gitlab/pagination/keyset/pager_spec.rb new file mode 100644 index 00000000000..6d23fe2adcc --- /dev/null +++ b/spec/lib/gitlab/pagination/keyset/pager_spec.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Pagination::Keyset::Pager do + let(:relation) { Project.all.order(id: :asc) } + let(:request) { double('request', page: page, apply_headers: nil) } + let(:page) { Gitlab::Pagination::Keyset::Page.new(order_by: { id: :asc }, per_page: 3) } + let(:next_page) { double('next page') } + + before_all do + create_list(:project, 7) + end + + describe '#paginate' do + subject { described_class.new(request).paginate(relation) } + + it 'loads the result relation only once' do + expect do + subject + end.not_to exceed_query_limit(1) + end + + it 'passes information about next page to request' do + lower_bounds = relation.limit(page.per_page).last.slice(:id) + expect(page).to receive(:next).with(lower_bounds, false).and_return(next_page) + expect(request).to receive(:apply_headers).with(next_page) + + subject + end + + context 'when retrieving the last page' do + let(:relation) { Project.where('id > ?', Project.maximum(:id) - page.per_page).order(id: :asc) } + + it 'indicates this is the last page' do + expect(request).to receive(:apply_headers) do |next_page| + expect(next_page.end_reached?).to be_truthy + end + + subject + end + end + + context 'when retrieving an empty page' do + let(:relation) { Project.where('id > ?', Project.maximum(:id) + 1).order(id: :asc) } + + it 'indicates this is the last page' do + expect(request).to receive(:apply_headers) do |next_page| + expect(next_page.end_reached?).to be_truthy + end + + subject + end + end + + it 'returns an array with the loaded records' do + expect(subject).to eq(relation.limit(page.per_page).to_a) + end + + context 'validating the order clause' do + let(:page) { Gitlab::Pagination::Keyset::Page.new(order_by: { created_at: :asc }, per_page: 3) } + + it 'raises an error if has a different order clause than the page' do + expect { subject }.to raise_error(ArgumentError, /order_by does not match/) + end + end + end +end diff --git a/spec/lib/gitlab/pagination/keyset/request_context_spec.rb b/spec/lib/gitlab/pagination/keyset/request_context_spec.rb new file mode 100644 index 00000000000..344ef90efa3 --- /dev/null +++ b/spec/lib/gitlab/pagination/keyset/request_context_spec.rb @@ -0,0 +1,115 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Pagination::Keyset::RequestContext do + let(:request) { double('request', params: params) } + + describe '#page' do + subject { described_class.new(request).page } + + context 'with only order_by given' do + let(:params) { { order_by: :id } } + + it 'extracts order_by/sorting information' do + page = subject + + expect(page.order_by).to eq(id: :desc) + end + end + + context 'with order_by and sort given' do + let(:params) { { order_by: :created_at, sort: :desc } } + + it 'extracts order_by/sorting information and adds tie breaker' do + page = subject + + expect(page.order_by).to eq(created_at: :desc, id: :desc) + end + end + + context 'with no order_by information given' do + let(:params) { {} } + + it 'defaults to tie breaker' do + page = subject + + expect(page.order_by).to eq({ id: :desc }) + end + end + + context 'with per_page params given' do + let(:params) { { per_page: 10 } } + + it 'extracts per_page information' do + page = subject + + expect(page.per_page).to eq(params[:per_page]) + end + end + end + + describe '#apply_headers' do + let(:request) { double('request', url: "http://#{Gitlab.config.gitlab.host}/api/v4/projects?foo=bar") } + let(:params) { { foo: 'bar' } } + let(:request_context) { double('request context', params: params, request: request) } + let(:next_page) { double('next page', order_by: { id: :asc }, lower_bounds: { id: 42 }, end_reached?: false) } + + subject { described_class.new(request_context).apply_headers(next_page) } + + it 'sets Links header with same host/path as the original request' do + orig_uri = URI.parse(request_context.request.url) + + expect(request_context).to receive(:header) do |name, header| + expect(name).to eq('Links') + + first_link, _ = /<([^>]+)>; rel="next"/.match(header).captures + + uri = URI.parse(first_link) + + expect(uri.host).to eq(orig_uri.host) + expect(uri.path).to eq(orig_uri.path) + end + + subject + end + + it 'sets Links header with a link to the next page' do + orig_uri = URI.parse(request_context.request.url) + + expect(request_context).to receive(:header) do |name, header| + expect(name).to eq('Links') + + first_link, _ = /<([^>]+)>; rel="next"/.match(header).captures + + query = CGI.parse(URI.parse(first_link).query) + + expect(query.except('id_after')).to eq(CGI.parse(orig_uri.query).except('id_after')) + expect(query['id_after']).to eq(['42']) + end + + subject + end + + context 'with descending order' do + let(:next_page) { double('next page', order_by: { id: :desc }, lower_bounds: { id: 42 }, end_reached?: false) } + + it 'sets Links header with a link to the next page' do + orig_uri = URI.parse(request_context.request.url) + + expect(request_context).to receive(:header) do |name, header| + expect(name).to eq('Links') + + first_link, _ = /<([^>]+)>; rel="next"/.match(header).captures + + query = CGI.parse(URI.parse(first_link).query) + + expect(query.except('id_before')).to eq(CGI.parse(orig_uri.query).except('id_before')) + expect(query['id_before']).to eq(['42']) + end + + subject + end + end + end +end diff --git a/spec/lib/gitlab/pagination/keyset_spec.rb b/spec/lib/gitlab/pagination/keyset_spec.rb new file mode 100644 index 00000000000..755c422c46a --- /dev/null +++ b/spec/lib/gitlab/pagination/keyset_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Gitlab::Pagination::Keyset do + describe '.paginate' do + subject { described_class.paginate(request_context, relation) } + + let(:request_context) { double } + let(:relation) { double } + let(:pager) { double } + let(:result) { double } + + it 'uses Pager to paginate the relation' do + expect(Gitlab::Pagination::Keyset::Pager).to receive(:new).with(request_context).and_return(pager) + expect(pager).to receive(:paginate).with(relation).and_return(result) + + expect(subject).to eq(result) + end + end + + describe '.available?' do + subject { described_class } + + let(:request_context) { double("request context", page: page)} + let(:page) { double("page", order_by: order_by) } + + shared_examples_for 'keyset pagination is available' do + it 'returns true for Project' do + expect(subject.available?(request_context, Project.all)).to be_truthy + end + + it 'return false for other types of relations' do + expect(subject.available?(request_context, User.all)).to be_falsey + end + end + + context 'with order-by id asc' do + let(:order_by) { { id: :asc } } + + it_behaves_like 'keyset pagination is available' + end + + context 'with order-by id desc' do + let(:order_by) { { id: :desc } } + + it_behaves_like 'keyset pagination is available' + end + + context 'with other order-by columns' do + let(:order_by) { { created_at: :desc, id: :desc } } + it 'returns false for Project' do + expect(subject.available?(request_context, Project.all)).to be_falsey + end + + it 'return false for other types of relations' do + expect(subject.available?(request_context, User.all)).to be_falsey + end + end + end +end diff --git a/spec/lib/marginalia_spec.rb b/spec/lib/marginalia_spec.rb new file mode 100644 index 00000000000..2a1dbd94a9e --- /dev/null +++ b/spec/lib/marginalia_spec.rb @@ -0,0 +1,173 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe 'Marginalia spec' do + class MarginaliaTestController < ActionController::Base + def first_user + User.first + render body: nil + end + end + + class MarginaliaTestJob + include Sidekiq::Worker + + def perform + User.first + end + end + + class MarginaliaTestMailer < BaseMailer + def first_user + User.first + end + end + + def add_sidekiq_middleware + # Reference: https://github.com/mperham/sidekiq/wiki/Testing#testing-server-middlewaresidekiq + # Sidekiq test harness fakes worker without its server middlewares, so include instrumentation to 'Sidekiq::Testing' server middleware. + Sidekiq::Testing.server_middleware do |chain| + chain.add Marginalia::SidekiqInstrumentation::Middleware + end + end + + def remove_sidekiq_middleware + Sidekiq::Testing.server_middleware do |chain| + chain.remove Marginalia::SidekiqInstrumentation::Middleware + end + end + + def stub_feature(value) + stub_feature_flags(Gitlab::Marginalia::MARGINALIA_FEATURE_FLAG => value) + end + + def make_request(correlation_id) + request_env = Rack::MockRequest.env_for('/') + + ::Labkit::Correlation::CorrelationId.use_id(correlation_id) do + MarginaliaTestController.action(:first_user).call(request_env) + end + end + + describe 'For rails web requests' do + let(:correlation_id) { SecureRandom.uuid } + let(:recorded) { ActiveRecord::QueryRecorder.new { make_request(correlation_id) } } + + let(:component_map) do + { + "application" => "test", + "controller" => "marginalia_test", + "action" => "first_user", + "line" => "/spec/support/helpers/query_recorder.rb", + "correlation_id" => correlation_id + } + end + + context 'when the feature is enabled' do + before do + stub_feature(true) + end + + it 'generates a query that includes the component and value' do + component_map.each do |component, value| + expect(recorded.log.last).to include("#{component}:#{value}") + end + end + end + + context 'when the feature is disabled' do + before do + stub_feature(false) + end + + it 'excludes annotations in generated queries' do + expect(recorded.log.last).not_to include("/*") + expect(recorded.log.last).not_to include("*/") + end + end + end + + describe 'for Sidekiq worker jobs' do + before(:all) do + add_sidekiq_middleware + + # Because of faking, 'Sidekiq.server?' does not work so implicitly set application name which is done in config/initializers/0_marginalia.rb + Marginalia.application_name = "sidekiq" + end + + after(:all) do + MarginaliaTestJob.clear + remove_sidekiq_middleware + end + + around do |example| + Sidekiq::Testing.fake! { example.run } + end + + before do + MarginaliaTestJob.perform_async + end + + let(:sidekiq_job) { MarginaliaTestJob.jobs.first } + let(:recorded) { ActiveRecord::QueryRecorder.new { MarginaliaTestJob.drain } } + + let(:component_map) do + { + "application" => "sidekiq", + "job_class" => "MarginaliaTestJob", + "line" => "/spec/support/sidekiq_middleware.rb", + "correlation_id" => sidekiq_job['correlation_id'], + "jid" => sidekiq_job['jid'] + } + end + + context 'when the feature is enabled' do + before do + stub_feature(true) + end + + it 'generates a query that includes the component and value' do + component_map.each do |component, value| + expect(recorded.log.last).to include("#{component}:#{value}") + end + end + + describe 'for ActionMailer delivery jobs' do + let(:delivery_job) { MarginaliaTestMailer.first_user.deliver_later } + + let(:recorded) do + ActiveRecord::QueryRecorder.new do + delivery_job.perform_now + end + end + + let(:component_map) do + { + "application" => "sidekiq", + "line" => "/lib/gitlab/i18n.rb", + "jid" => delivery_job.job_id, + "job_class" => delivery_job.arguments.first + } + end + + it 'generates a query that includes the component and value' do + component_map.each do |component, value| + expect(recorded.log.last).to include("#{component}:#{value}") + end + end + end + end + + context 'when the feature is disabled' do + before do + stub_feature(false) + end + + it 'excludes annotations in generated queries' do + expect(recorded.log.last).not_to include("/*") + expect(recorded.log.last).not_to include("*/") + end + end + end +end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index cda2dd7d2f4..4c1db4f0d18 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -570,6 +570,87 @@ describe API::Projects do let(:projects) { Project.all } end end + + context 'with keyset pagination' do + let(:current_user) { user } + let(:projects) { [public_project, project, project2, project3] } + + context 'headers and records' do + let(:params) { { pagination: 'keyset', order_by: :id, sort: :asc, per_page: 1 } } + + it 'includes a pagination header with link to the next page' do + get api('/projects', current_user), params: params + + expect(response.header).to include('Links') + expect(response.header['Links']).to include('pagination=keyset') + expect(response.header['Links']).to include("id_after=#{public_project.id}") + end + + it 'contains only the first project with per_page = 1' do + get api('/projects', current_user), params: params + + expect(response).to have_gitlab_http_status(200) + expect(json_response).to be_an Array + expect(json_response.map { |p| p['id'] }).to contain_exactly(public_project.id) + end + + it 'does not include a link if the end has reached and there is no more data' do + get api('/projects', current_user), params: params.merge(id_after: project2.id) + + expect(response.header).not_to include('Links') + end + + it 'responds with 501 if order_by is different from id' do + get api('/projects', current_user), params: params.merge(order_by: :created_at) + + expect(response).to have_gitlab_http_status(501) + end + end + + context 'with descending sorting' do + let(:params) { { pagination: 'keyset', order_by: :id, sort: :desc, per_page: 1 } } + + it 'includes a pagination header with link to the next page' do + get api('/projects', current_user), params: params + + expect(response.header).to include('Links') + expect(response.header['Links']).to include('pagination=keyset') + expect(response.header['Links']).to include("id_before=#{project3.id}") + end + + it 'contains only the last project with per_page = 1' do + get api('/projects', current_user), params: params + + expect(response).to have_gitlab_http_status(200) + expect(json_response).to be_an Array + expect(json_response.map { |p| p['id'] }).to contain_exactly(project3.id) + end + end + + context 'retrieving the full relation' do + let(:params) { { pagination: 'keyset', order_by: :id, sort: :desc, per_page: 2 } } + + it 'returns all projects' do + url = '/projects' + requests = 0 + ids = [] + + while url && requests <= 5 # circuit breaker + requests += 1 + get api(url, current_user), params: params + + links = response.header['Links'] + url = links&.match(/<[^>]+(\/projects\?[^>]+)>; rel="next"/) do |match| + match[1] + end + + ids += JSON.parse(response.body).map { |p| p['id'] } + end + + expect(ids).to contain_exactly(*projects.map(&:id)) + end + end + end end describe 'POST /projects' do diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 7661c8acc13..f1f761a6fd0 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -130,10 +130,10 @@ describe PipelineSerializer do it 'preloads related merge requests' do recorded = ActiveRecord::QueryRecorder.new { subject } + expected_query = "SELECT \"merge_requests\".* FROM \"merge_requests\" " \ + "WHERE \"merge_requests\".\"id\" IN (#{merge_request_1.id}, #{merge_request_2.id})" - expect(recorded.log) - .to include("SELECT \"merge_requests\".* FROM \"merge_requests\" " \ - "WHERE \"merge_requests\".\"id\" IN (#{merge_request_1.id}, #{merge_request_2.id})") + expect(recorded.log).to include(a_string_starting_with(expected_query)) end end |