diff options
Diffstat (limited to 'doc/development')
| -rw-r--r-- | doc/development/ee_features.md | 382 | ||||
| -rw-r--r-- | doc/development/fe_guide/index.md | 3 | ||||
| -rw-r--r-- | doc/development/fe_guide/vue.md | 51 | ||||
| -rw-r--r-- | doc/development/fe_guide/vue_resource.md | 72 |
4 files changed, 458 insertions, 50 deletions
diff --git a/doc/development/ee_features.md b/doc/development/ee_features.md new file mode 100644 index 00000000000..932a44f65e4 --- /dev/null +++ b/doc/development/ee_features.md @@ -0,0 +1,382 @@ +# Guidelines for implementing Enterprise Edition feature + +- **Write the code and the tests.**: As with any code, EE features should have + good test coverage to prevent regressions. +- **Write documentation.**: Add documentation to the `doc/` directory. Describe + the feature and include screenshots, if applicable. +- **Submit a MR to the `www-gitlab-com` project.**: Add the new feature to the + [EE features list][ee-features-list]. + +## Act as CE when unlicensed + +Since the implementation of [GitLab CE features to work with unlicensed EE instance][ee-as-ce] +GitLab Enterprise Edition should work like GitLab Community Edition +when no license is active. So EE features always should be guarded by +`project.feature_available?` or `group.feature_available?` (or +`License.feature_available?` if it is a system-wide feature). + +CE specs should remain untouched as much as possible and extra specs +should be added for EE. Licensed features can be stubbed using the +spec helper `stub_licensed_features` in `EE::LicenseHelpers`. + +[ee-as-ce]: https://gitlab.com/gitlab-org/gitlab-ee/issues/2500 + +## Separation of EE code + +We want a [single code base][] eventually, but before we reach the goal, +we still need to merge changes from GitLab CE to EE. To help us get there, +we should make sure that we no longer edit CE files in place in order to +implement EE features. + +Instead, all EE codes should be put inside the `ee/` top-level directory, and +tests should be put inside `spec/ee/`. We don't use `ee/spec` for now due to +technical limitation. The rest of codes should be as close as to the CE files. + +[single code base]: https://gitlab.com/gitlab-org/gitlab-ee/issues/2952#note_41016454 + +### EE-only features + +If the feature being developed is not present in any form in CE, we don't +need to put the codes under `EE` namespace. For example, an EE model could +go into: `ee/app/models/awesome.rb` using `Awesome` as the class name. This +is applied not only to models. Here's a list of other examples: + +- `ee/app/controllers/foos_controller.rb` +- `ee/app/finders/foos_finder.rb` +- `ee/app/helpers/foos_helper.rb` +- `ee/app/mailers/foos_mailer.rb` +- `ee/app/models/foo.rb` +- `ee/app/policies/foo_policy.rb` +- `ee/app/serializers/foo_entity.rb` +- `ee/app/serializers/foo_serializer.rb` +- `ee/app/services/foo/create_service.rb` +- `ee/app/validators/foo_attr_validator.rb` +- `ee/app/workers/foo_worker.rb` + +### EE features based on CE features + +For features that build on existing CE features, write a module in the +`EE` namespace and `prepend` it in the CE class. This makes conflicts +less likely to happen during CE to EE merges because only one line is +added to the CE class - the `prepend` line. + +Since the module would require an `EE` namespace, the file should also be +put in an `ee/` sub-directory. For example, we want to extend the user model +in EE, so we have a module called `::EE::User` put inside +`ee/app/models/ee/user.rb`. + +This is also not just applied to models. Here's a list of other examples: + +- `ee/app/controllers/ee/foos_controller.rb` +- `ee/app/finders/ee/foos_finder.rb` +- `ee/app/helpers/ee/foos_helper.rb` +- `ee/app/mailers/ee/foos_mailer.rb` +- `ee/app/models/ee/foo.rb` +- `ee/app/policies/ee/foo_policy.rb` +- `ee/app/serializers/ee/foo_entity.rb` +- `ee/app/serializers/ee/foo_serializer.rb` +- `ee/app/services/ee/foo/create_service.rb` +- `ee/app/validators/ee/foo_attr_validator.rb` +- `ee/app/workers/ee/foo_worker.rb` + +#### Overriding CE methods + +To override a method present in the CE codebase, use `prepend`. It +lets you override a method in a class with a method from a module, while +still having access the class's implementation with `super`. + +There are a few gotchas with it: + +- you should always add a `raise NotImplementedError unless defined?(super)` + guard clause in the "overrider" method to ensure that if the method gets + renamed in CE, the EE override won't be silently forgotten. +- when the "overrider" would add a line in the middle of the CE + implementation, you should refactor the CE method and split it in + smaller methods. Or create a "hook" method that is empty in CE, + and with the EE-specific implementation in EE. +- when the original implementation contains a guard clause (e.g. + `return unless condition`), we cannot easily extend the behaviour by + overriding the method, because we can't know when the overridden method + (i.e. calling `super` in the overriding method) would want to stop early. + In this case, we shouldn't just override it, but update the original method + to make it call the other method we want to extend, like a [template method + pattern](https://en.wikipedia.org/wiki/Template_method_pattern). + For example, given this base: + ``` ruby + class Base + def execute + return unless enabled? + + # ... + # ... + end + end + ``` + Instead of just overriding `Base#execute`, we should update it and extract + the behaviour into another method: + ``` ruby + class Base + def execute + return unless enabled? + + do_something + end + + private + + def do_something + # ... + # ... + end + end + ``` + Then we're free to override that `do_something` without worrying about the + guards: + ``` ruby + module EE::Base + def do_something + # Follow the above pattern to call super and extend it + end + end + ``` + This would require updating CE first, or make sure this is back ported to CE. + +When prepending, place them in the `ee/` specific sub-directory, and +wrap class or module in `module EE` to avoid naming conflicts. + +For example to override the CE implementation of +`ApplicationController#after_sign_out_path_for`: + +```ruby +def after_sign_out_path_for(resource) + current_application_settings.after_sign_out_path.presence || new_user_session_path +end +``` + +Instead of modifying the method in place, you should add `prepend` to +the existing file: + +```ruby +class ApplicationController < ActionController::Base + prepend EE::ApplicationController + # ... + + def after_sign_out_path_for(resource) + current_application_settings.after_sign_out_path.presence || new_user_session_path + end + + # ... +end +``` + +And create a new file in the `ee/` sub-directory with the altered +implementation: + +```ruby +module EE + class ApplicationController + def after_sign_out_path_for(resource) + raise NotImplementedError unless defined?(super) + + if Gitlab::Geo.secondary? + Gitlab::Geo.primary_node.oauth_logout_url(@geo_logout_state) + else + super + end + end + end +end +``` + +#### Use self-descriptive wrapper methods + +When it's not possible/logical to modify the implementation of a +method. Wrap it in a self-descriptive method and use that method. + +For example, in CE only an `admin` is allowed to access all private +projects/groups, but in EE also an `auditor` has full private +access. It would be incorrect to override the implementation of +`User#admin?`, so instead add a method `full_private_access?` to +`app/models/users.rb`. The implementation in CE will be: + +```ruby +def full_private_access? + admin? +end +``` + +In EE, the implementation `ee/app/models/ee/users.rb` would be: + +```ruby +def full_private_access? + raise NotImplementedError unless defined?(super) + super || auditor? +end +``` + +In `lib/gitlab/visibility_level.rb` this method is used to return the +allowed visibilty levels: + +```ruby +def levels_for_user(user = nil) + if user.full_private_access? + [PRIVATE, INTERNAL, PUBLIC] + elsif # ... +end +``` + +See [CE MR][ce-mr-full-private] and [EE MR][ee-mr-full-private] for +full implementation details. + +[ce-mr-full-private]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/12373 +[ee-mr-full-private]: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/2199 + +### Code in `app/controllers/` + +In controllers, the most common type of conflict is with `before_action` that +has a list of actions in CE but EE adds some actions to that list. + +The same problem often occurs for `params.require` / `params.permit` calls. + +**Mitigations** + +Separate CE and EE actions/keywords. For instance for `params.require` in +`ProjectsController`: + +```ruby +def project_params + params.require(:project).permit(project_params_attributes) +end + +# Always returns an array of symbols, created however best fits the use case. +# It _should_ be sorted alphabetically. +def project_params_attributes + %i[ + description + name + path + ] +end + +``` + +In the `EE::ProjectsController` module: + +```ruby +def project_params_attributes + super + project_params_attributes_ee +end + +def project_params_attributes_ee + %i[ + approvals_before_merge + approver_group_ids + approver_ids + ... + ] +end +``` + +### Code in `app/models/` + +EE-specific models should `extend EE::Model`. + +For example, if EE has a specific `Tanuki` model, you would +place it in `ee/app/models/ee/tanuki.rb`. + +### Code in `app/views/` + +It's a very frequent problem that EE is adding some specific view code in a CE +view. For instance the approval code in the project's settings page. + +**Mitigations** + +Blocks of code that are EE-specific should be moved to partials. This +avoids conflicts with big chunks of HAML code that that are not fun to +resolve when you add the indentation to the equation. + +EE-specific views should be placed in `ee/app/views/ee/`, using extra +sub-directories if appropriate. + +### Code in `lib/` + +Place EE-specific logic in the top-level `EE` module namespace. Namespace the +class beneath the `EE` module just as you would normally. + +For example, if CE has LDAP classes in `lib/gitlab/ldap/` then you would place +EE-specific LDAP classes in `ee/lib/ee/gitlab/ldap`. + +### Code in `spec/` + +When you're testing EE-only features, avoid adding examples to the +existing CE specs. Also do no change existing CE examples, since they +should remain working as-is when EE is running without a license. + +Instead place EE specs in the `spec/ee/spec` folder. + +## JavaScript code in `assets/javascripts/` + +To separate EE-specific JS-files we can also move the files into an `ee` folder. + +For example there can be an +`app/assets/javascripts/protected_branches/protected_branches_bundle.js` and an +EE counterpart +`ee/app/assets/javascripts/protected_branches/protected_branches_bundle.js`. + +That way we can create a separate webpack bundle in `webpack.config.js`: + +```javascript + protected_branches: '~/protected_branches', + ee_protected_branches: 'ee/protected_branches/protected_branches_bundle.js', +``` + +With the separate bundle in place, we can decide which bundle to load inside the +view, using the `page_specific_javascript_bundle_tag` helper. + +```haml +- content_for :page_specific_javascripts do + = page_specific_javascript_bundle_tag('protected_branches') +``` + +## SCSS code in `assets/stylesheets` + +To separate EE-specific styles in SCSS files, if a component you're adding styles for +is limited to only EE, it is better to have a separate SCSS file in appropriate directory +within `app/assets/stylesheets`. + +In some cases, this is not entirely possible or creating dedicated SCSS file is an overkill, +e.g. a text style of some component is different for EE. In such cases, +styles are usually kept in stylesheet that is common for both CE and EE, and it is wise +to isolate such ruleset from rest of CE rules (along with adding comment describing the same) +to avoid conflicts during CE to EE merge. + +#### Bad +```scss +.section-body { + .section-title { + background: $gl-header-color; + } + + &.ee-section-body { + .section-title { + background: $gl-header-color-cyan; + } + } +} +``` + +#### Good +```scss +.section-body { + .section-title { + background: $gl-header-color; + } +} + +/* EE-specific styles */ +.section-body.ee-section-body { + .section-title { + background: $gl-header-color-cyan; + } +} +``` diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md index c0e1bfc12a1..73366eb9f3f 100644 --- a/doc/development/fe_guide/index.md +++ b/doc/development/fe_guide/index.md @@ -71,6 +71,9 @@ Vue specific design patterns and practices. --- +## [Vue Resource](vue_resource.md) +Vue resource specific practices and gotchas. + ## [Icons](icons.md) How we use SVG for our Icons. diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index 277e0cd5f00..f88f0753687 100644 --- a/doc/development/fe_guide/vue.md +++ b/doc/development/fe_guide/vue.md @@ -179,6 +179,7 @@ itself, please read this guide: [State Management][state-management] The Service is a class used only to communicate with the server. It does not store or manipulate any data. It is not aware of the store or the components. We use [vue-resource][vue-resource-repo] to communicate with the server. +Refer to [vue resource](vue_resource.md) for more details. Vue Resource should only be imported in the service file. @@ -189,55 +190,6 @@ Vue Resource should only be imported in the service file. Vue.use(VueResource); ``` -#### Vue-resource gotchas -#### Headers -Headers are being parsed into a plain object in an interceptor. -In Vue-resource 1.x `headers` object was changed into an `Headers` object. In order to not change all old code, an interceptor was added. - -If you need to write a unit test that takes the headers in consideration, you need to include an interceptor to parse the headers after your test interceptor. -You can see an example in `spec/javascripts/environments/environment_spec.js`: - ```javascript - import { headersInterceptor } from './helpers/vue_resource_helper'; - - beforeEach(() => { - Vue.http.interceptors.push(myInterceptor); - Vue.http.interceptors.push(headersInterceptor); - }); - - afterEach(() => { - Vue.http.interceptors = _.without(Vue.http.interceptors, myInterceptor); - Vue.http.interceptors = _.without(Vue.http.interceptors, headersInterceptor); - }); - ``` - -#### `.json()` -When making a request to the server, you will most likely need to access the body of the response. -Use `.json()` to convert. Because `.json()` returns a Promise the follwoing structure should be used: - - ```javascript - service.get('url') - .then(resp => resp.json()) - .then((data) => { - this.store.storeData(data); - }) - .catch(() => new Flash('Something went wrong')); - ``` - -When using `Poll` (`app/assets/javascripts/lib/utils/poll.js`), the `successCallback` needs to handle `.json()` as a Promise: - ```javascript - successCallback: (response) => { - return response.json().then((data) => { - // handle the response - }); - } - ``` - -#### CSRF token -We use a Vue Resource interceptor to manage the CSRF token. -`app/assets/javascripts/vue_shared/vue_resource_interceptor.js` holds all our common interceptors. -Note: You don't need to load `app/assets/javascripts/vue_shared/vue_resource_interceptor.js` -since it's already being loaded by `common_vue.js`. - ### End Result The following example shows an application: @@ -769,7 +721,6 @@ describe('component', () => { [component-system]: https://vuejs.org/v2/guide/#Composing-with-Components [state-management]: https://vuejs.org/v2/guide/state-management.html#Simple-State-Management-from-Scratch [one-way-data-flow]: https://vuejs.org/v2/guide/components.html#One-Way-Data-Flow -[vue-resource-repo]: https://github.com/pagekit/vue-resource [vue-resource-interceptor]: https://github.com/pagekit/vue-resource/blob/develop/docs/http.md#interceptors [vue-test]: https://vuejs.org/v2/guide/unit-testing.html [issue-boards-service]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/boards/services/board_service.js.es6 diff --git a/doc/development/fe_guide/vue_resource.md b/doc/development/fe_guide/vue_resource.md new file mode 100644 index 00000000000..c376c5c32bf --- /dev/null +++ b/doc/development/fe_guide/vue_resource.md @@ -0,0 +1,72 @@ +# Vue Resouce +In Vue applications we use [vue-resource][vue-resource-repo] to communicate with the server. + +## HTTP Status Codes + +### `.json()` +When making a request to the server, you will most likely need to access the body of the response. +Use `.json()` to convert. Because `.json()` returns a Promise the follwoing structure should be used: + + ```javascript + service.get('url') + .then(resp => resp.json()) + .then((data) => { + this.store.storeData(data); + }) + .catch(() => new Flash('Something went wrong')); + ``` + + +When using `Poll` (`app/assets/javascripts/lib/utils/poll.js`), the `successCallback` needs to handle `.json()` as a Promise: + ```javascript + successCallback: (response) => { + return response.json().then((data) => { + // handle the response + }); + } + ``` + +### 204 +Some endpoints - usually `delete` endpoints - return `204` as the success response. +When handling `204 - No Content` responses, we cannot use `.json()` since it tries to parse the non-existant body content. + +When handling `204` responses, do not use `.json`, otherwise the promise will throw an error and will enter the `catch` statement: + +```javascript + Vue.http.delete('path') + .then(() => { + // success! + }) + .catch(() => { + // handle error + }) +``` + +## Headers +Headers are being parsed into a plain object in an interceptor. +In Vue-resource 1.x `headers` object was changed into an `Headers` object. In order to not change all old code, an interceptor was added. + +If you need to write a unit test that takes the headers in consideration, you need to include an interceptor to parse the headers after your test interceptor. +You can see an example in `spec/javascripts/environments/environment_spec.js`: + ```javascript + import { headersInterceptor } from './helpers/vue_resource_helper'; + + beforeEach(() => { + Vue.http.interceptors.push(myInterceptor); + Vue.http.interceptors.push(headersInterceptor); + }); + + afterEach(() => { + Vue.http.interceptors = _.without(Vue.http.interceptors, myInterceptor); + Vue.http.interceptors = _.without(Vue.http.interceptors, headersInterceptor); + }); + ``` + +## CSRF token +We use a Vue Resource interceptor to manage the CSRF token. +`app/assets/javascripts/vue_shared/vue_resource_interceptor.js` holds all our common interceptors. +Note: You don't need to load `app/assets/javascripts/vue_shared/vue_resource_interceptor.js` +since it's already being loaded by `common_vue.js`. + + +[vue-resource-repo]: https://github.com/pagekit/vue-resource |