Add latest changes from gitlab-org/gitlab@master

This commit is contained in:
GitLab Bot 2019-11-29 18:06:24 +00:00
parent eac0da9a47
commit 8263f6ee31
48 changed files with 2321 additions and 1402 deletions

View File

@ -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'

View File

@ -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)

View File

@ -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">

View File

@ -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)"
>

View File

@ -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')

View File

@ -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' }

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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)

View File

@ -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>
```

View File

@ -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).

View File

@ -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);
```

View File

@ -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/

View File

@ -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.

View File

@ -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).

View File

@ -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).

View File

@ -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
},
}
```

View File

@ -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

View File

@ -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.

View File

@ -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).

View File

@ -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).

View File

@ -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).

View File

@ -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).

View File

@ -1,3 +0,0 @@
# SCSS style guide
> TODO: Add content

View File

@ -1,3 +0,0 @@
# Vue style guide
> TODO: Add content

View File

@ -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

View File

@ -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)

23
lib/gitlab/marginalia.rb Normal file
View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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!

View File

@ -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)

View File

@ -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
result = subject.paginate(relation)
context 'for offset pagination' do
let(:params) { {} }
expect(result).to eq(expected_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)
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
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)
subject.paginate(relation)
end
end
end
end

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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

173
spec/lib/marginalia_spec.rb Normal file
View File

@ -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

View File

@ -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

View File

@ -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