diff options
| author | Filipa Lacerda <filipa@gitlab.com> | 2017-04-20 13:38:23 +0100 |
|---|---|---|
| committer | Filipa Lacerda <filipa@gitlab.com> | 2017-04-20 13:38:23 +0100 |
| commit | 3a7982464b613276d10f2a74c3b08aa07c8e63c2 (patch) | |
| tree | 6362c25a0b8ee7b8a9ae84ad15cbb032a4df1bd2 | |
| parent | c2e26dc0e61541ff8f68c19931a662bcd2178d3e (diff) | |
| parent | f99cc765c7cc20372e2c1c915c40e4a71a1dca15 (diff) | |
| download | gitlab-ce-doc-js-side-effetcs.tar.gz | |
Merge branch 'master' into doc-js-side-effetcsdoc-js-side-effetcs
* master:
Refactor into .vue files
Adds vue js example application and documentation
Add ES lint support to identify poorly written Promises
Update plantuml.md to add the actual link.
Fixed wording
Fix headings
Add more information in the User cohort documentation
Add CHANGELOG
Port of fix/29125-ee to CE
Do not append issuable state to links with custom anchor
Update plantuml.md to fix wrong introduced in link.
42 files changed, 674 insertions, 132 deletions
diff --git a/.eslintrc b/.eslintrc index 57a08a06527..aba8112c5a9 100644 --- a/.eslintrc +++ b/.eslintrc @@ -14,7 +14,8 @@ "plugins": [ "filenames", "import", - "html" + "html", + "promise" ], "settings": { "html/html-extensions": [".html", ".html.raw", ".vue"], @@ -26,6 +27,7 @@ }, "rules": { "filenames/match-regex": [2, "^[a-z0-9_]+$"], - "no-multiple-empty-lines": ["error", { "max": 1 }] + "no-multiple-empty-lines": ["error", { "max": 1 }], + "promise/catch-or-return": "error" } } diff --git a/app/assets/javascripts/awards_handler.js b/app/assets/javascripts/awards_handler.js index f93208944a1..adb45b0606d 100644 --- a/app/assets/javascripts/awards_handler.js +++ b/app/assets/javascripts/awards_handler.js @@ -239,6 +239,9 @@ AwardsHandler if (menu) { menu.dispatchEvent(new CustomEvent('build-emoji-menu-finish')); } + }).catch((err) => { + emojiContentElement.insertAdjacentHTML('beforeend', '<p>We encountered an error while adding the remaining categories</p>'); + throw new Error(`Error occurred in addRemainingEmojiMenuCategories: ${err.message}`); }); }; diff --git a/app/assets/javascripts/boards/boards_bundle.js b/app/assets/javascripts/boards/boards_bundle.js index b749ef43cd3..b6dee8177d2 100644 --- a/app/assets/javascripts/boards/boards_bundle.js +++ b/app/assets/javascripts/boards/boards_bundle.js @@ -1,5 +1,6 @@ /* eslint-disable one-var, quote-props, comma-dangle, space-before-function-paren */ /* global BoardService */ +/* global Flash */ import Vue from 'vue'; import VueResource from 'vue-resource'; @@ -93,7 +94,7 @@ $(() => { Store.addBlankState(); this.loading = false; - }); + }).catch(() => new Flash('An error occurred. Please try again.')); }, methods: { updateTokens() { diff --git a/app/assets/javascripts/boards/components/board_list.js b/app/assets/javascripts/boards/components/board_list.js index adbd82cb687..b13386536bf 100644 --- a/app/assets/javascripts/boards/components/board_list.js +++ b/app/assets/javascripts/boards/components/board_list.js @@ -57,12 +57,15 @@ export default { }, loadNextPage() { const getIssues = this.list.nextPage(); + const loadingDone = () => { + this.list.loadingMore = false; + }; if (getIssues) { this.list.loadingMore = true; - getIssues.then(() => { - this.list.loadingMore = false; - }); + getIssues + .then(loadingDone) + .catch(loadingDone); } }, toggleForm() { diff --git a/app/assets/javascripts/boards/components/modal/index.js b/app/assets/javascripts/boards/components/modal/index.js index fb0aac3c0e4..fdab317dc23 100644 --- a/app/assets/javascripts/boards/components/modal/index.js +++ b/app/assets/javascripts/boards/components/modal/index.js @@ -51,11 +51,13 @@ gl.issueBoards.IssuesModal = Vue.extend({ showAddIssuesModal() { if (this.showAddIssuesModal && !this.issues.length) { this.loading = true; + const loadingDone = () => { + this.loading = false; + }; this.loadIssues() - .then(() => { - this.loading = false; - }); + .then(loadingDone) + .catch(loadingDone); } else if (!this.showAddIssuesModal) { this.issues = []; this.selectedIssues = []; @@ -67,11 +69,13 @@ gl.issueBoards.IssuesModal = Vue.extend({ if (this.$el.tagName) { this.page = 1; this.filterLoading = true; + const loadingDone = () => { + this.filterLoading = false; + }; this.loadIssues(true) - .then(() => { - this.filterLoading = false; - }); + .then(loadingDone) + .catch(loadingDone); } }, deep: true, diff --git a/app/assets/javascripts/boards/components/new_list_dropdown.js b/app/assets/javascripts/boards/components/new_list_dropdown.js index 22f20305624..7e3bb79af1d 100644 --- a/app/assets/javascripts/boards/components/new_list_dropdown.js +++ b/app/assets/javascripts/boards/components/new_list_dropdown.js @@ -1,4 +1,5 @@ -/* eslint-disable comma-dangle, func-names, no-new, space-before-function-paren, one-var */ +/* eslint-disable comma-dangle, func-names, no-new, space-before-function-paren, one-var, + promise/catch-or-return */ window.gl = window.gl || {}; window.gl.issueBoards = window.gl.issueBoards || {}; diff --git a/app/assets/javascripts/boards/stores/boards_store.js b/app/assets/javascripts/boards/stores/boards_store.js index 66384d9c038..ccb00099215 100644 --- a/app/assets/javascripts/boards/stores/boards_store.js +++ b/app/assets/javascripts/boards/stores/boards_store.js @@ -36,6 +36,9 @@ gl.issueBoards.BoardsStore = { .save() .then(() => { this.state.lists = _.sortBy(this.state.lists, 'position'); + }) + .catch(() => { + // https://gitlab.com/gitlab-org/gitlab-ce/issues/30821 }); this.removeBlankState(); }, diff --git a/app/assets/javascripts/diff_notes/components/resolve_btn.js b/app/assets/javascripts/diff_notes/components/resolve_btn.js index 8fafd13c6c2..92f6fd654b3 100644 --- a/app/assets/javascripts/diff_notes/components/resolve_btn.js +++ b/app/assets/javascripts/diff_notes/components/resolve_btn.js @@ -64,6 +64,8 @@ const ResolveBtn = Vue.extend({ }); }, resolve: function () { + const errorFlashMsg = 'An error occurred when trying to resolve a comment. Please try again.'; + if (!this.canResolve) return; let promise; @@ -87,10 +89,12 @@ const ResolveBtn = Vue.extend({ CommentsStore.update(this.discussionId, this.noteId, !this.isResolved, resolved_by); this.discussion.updateHeadline(data); } else { - new Flash('An error occurred when trying to resolve a comment. Please try again.', 'alert'); + new Flash(errorFlashMsg); } this.updateTooltip(); + }).catch(() => { + new Flash(errorFlashMsg); }); } }, diff --git a/app/assets/javascripts/diff_notes/services/resolve.js b/app/assets/javascripts/diff_notes/services/resolve.js index e1e2e3e93f9..4ea6ba8a73d 100644 --- a/app/assets/javascripts/diff_notes/services/resolve.js +++ b/app/assets/javascripts/diff_notes/services/resolve.js @@ -51,8 +51,10 @@ class ResolveServiceClass { discussion.updateHeadline(data); } else { - new Flash('An error occurred when trying to resolve a discussion. Please try again.', 'alert'); + throw new Error('An error occurred when trying to resolve discussion.'); } + }).catch(() => { + new Flash('An error occurred when trying to resolve a discussion. Please try again.'); }); } diff --git a/app/assets/javascripts/due_date_select.js b/app/assets/javascripts/due_date_select.js index db10b383913..dd95b6530f6 100644 --- a/app/assets/javascripts/due_date_select.js +++ b/app/assets/javascripts/due_date_select.js @@ -115,11 +115,13 @@ class DueDateSelect { this.$dropdown.trigger('loading.gl.dropdown'); this.$selectbox.hide(); this.$value.css('display', ''); + const fadeOutLoader = () => { + this.$loading.fadeOut(); + }; gl.issueBoards.BoardsStore.detail.issue.update(this.$dropdown.attr('data-issue-update')) - .then(() => { - this.$loading.fadeOut(); - }); + .then(fadeOutLoader) + .catch(fadeOutLoader); } submitSelectedDate(isDropdown) { diff --git a/app/assets/javascripts/environments/components/environment_item.js b/app/assets/javascripts/environments/components/environment_item.js index 915f0ca4ccb..0b174cf97da 100644 --- a/app/assets/javascripts/environments/components/environment_item.js +++ b/app/assets/javascripts/environments/components/environment_item.js @@ -3,9 +3,9 @@ import '../../lib/utils/text_utility'; import ActionsComponent from './environment_actions'; import ExternalUrlComponent from './environment_external_url.vue'; import StopComponent from './environment_stop.vue'; -import RollbackComponent from './environment_rollback'; +import RollbackComponent from './environment_rollback.vue'; import TerminalButtonComponent from './environment_terminal_button.vue'; -import MonitoringButtonComponent from './environment_monitoring'; +import MonitoringButtonComponent from './environment_monitoring.vue'; import CommitComponent from '../../vue_shared/components/commit'; import eventHub from '../event_hub'; diff --git a/app/assets/javascripts/environments/components/environment_monitoring.js b/app/assets/javascripts/environments/components/environment_monitoring.js deleted file mode 100644 index 8c37dd76ae7..00000000000 --- a/app/assets/javascripts/environments/components/environment_monitoring.js +++ /dev/null @@ -1,30 +0,0 @@ -/** - * Renders the Monitoring (Metrics) link in environments table. - */ -export default { - props: { - monitoringUrl: { - type: String, - default: '', - required: true, - }, - }, - - computed: { - title() { - return 'Monitoring'; - }, - }, - - template: ` - <a - class="btn monitoring-url has-tooltip" - data-container="body" - :href="monitoringUrl" - rel="noopener noreferrer nofollow" - :title="title" - :aria-label="title"> - <i class="fa fa-area-chart" aria-hidden="true"></i> - </a> - `, -}; diff --git a/app/assets/javascripts/environments/components/environment_monitoring.vue b/app/assets/javascripts/environments/components/environment_monitoring.vue new file mode 100644 index 00000000000..4b030a27900 --- /dev/null +++ b/app/assets/javascripts/environments/components/environment_monitoring.vue @@ -0,0 +1,33 @@ +<script> +/** + * Renders the Monitoring (Metrics) link in environments table. + */ +export default { + props: { + monitoringUrl: { + type: String, + required: true, + }, + }, + + computed: { + title() { + return 'Monitoring'; + }, + }, +}; +</script> +<template> + <a + class="btn monitoring-url has-tooltip" + data-container="body" + target="_blank" + rel="noopener noreferrer nofollow" + :href="monitoringUrl" + :title="title" + :aria-label="title"> + <i + class="fa fa-area-chart" + aria-hidden="true" /> + </a> +</template> diff --git a/app/assets/javascripts/environments/components/environment_rollback.js b/app/assets/javascripts/environments/components/environment_rollback.vue index 7cbfb651525..f139f24036f 100644 --- a/app/assets/javascripts/environments/components/environment_rollback.js +++ b/app/assets/javascripts/environments/components/environment_rollback.vue @@ -1,3 +1,4 @@ +<script> /* global Flash */ /* eslint-disable no-new */ /** @@ -49,21 +50,25 @@ export default { }); }, }, +}; +</script> +<template> + <button + type="button" + class="btn" + @click="onClick" + :disabled="isLoading"> - template: ` - <button type="button" - class="btn" - @click="onClick" - :disabled="isLoading"> - - <span v-if="isLastDeployment"> - Re-deploy - </span> - <span v-else> - Rollback - </span> + <span v-if="isLastDeployment"> + Re-deploy + </span> + <span v-else> + Rollback + </span> - <i v-if="isLoading" class="fa fa-spinner fa-spin" aria-hidden="true"></i> - </button> - `, -}; + <i + v-if="isLoading" + class="fa fa-spinner fa-spin" + aria-hidden="true" /> + </button> +</template> diff --git a/app/assets/javascripts/filtered_search/filtered_search_manager.js b/app/assets/javascripts/filtered_search/filtered_search_manager.js index a5eb33dd9de..68a832102a0 100644 --- a/app/assets/javascripts/filtered_search/filtered_search_manager.js +++ b/app/assets/javascripts/filtered_search/filtered_search_manager.js @@ -343,6 +343,8 @@ class FilteredSearchManager { const resultantSearches = this.recentSearchesStore.addRecentSearch(searchQuery); this.recentSearchesService.save(resultantSearches); } + }).catch(() => { + // https://gitlab.com/gitlab-org/gitlab-ce/issues/30821 }); } diff --git a/app/assets/javascripts/groups_select.js b/app/assets/javascripts/groups_select.js index 10363c16bae..acfa4bd4c6b 100644 --- a/app/assets/javascripts/groups_select.js +++ b/app/assets/javascripts/groups_select.js @@ -1,4 +1,8 @@ -/* eslint-disable func-names, space-before-function-paren, no-var, wrap-iife, one-var, camelcase, one-var-declaration-per-line, quotes, object-shorthand, prefer-arrow-callback, comma-dangle, consistent-return, yoda, prefer-rest-params, prefer-spread, no-unused-vars, prefer-template, max-len */ +/* eslint-disable func-names, space-before-function-paren, no-var, wrap-iife, one-var, + camelcase, one-var-declaration-per-line, quotes, object-shorthand, + prefer-arrow-callback, comma-dangle, consistent-return, yoda, + prefer-rest-params, prefer-spread, no-unused-vars, prefer-template, + promise/catch-or-return */ /* global Api */ var slice = [].slice; diff --git a/app/assets/javascripts/labels_select.js b/app/assets/javascripts/labels_select.js index 443fb3e0ca9..9a60f5464df 100644 --- a/app/assets/javascripts/labels_select.js +++ b/app/assets/javascripts/labels_select.js @@ -332,6 +332,9 @@ vue: $dropdown.hasClass('js-issue-board-sidebar'), clicked: function(label, $el, e, isMarking) { var isIssueIndex, isMRIndex, page, boardsModel; + var fadeOutLoader = () => { + $loading.fadeOut(); + }; page = $('body').data('page'); isIssueIndex = page === 'projects:issues:index'; @@ -396,9 +399,8 @@ $loading.fadeIn(); gl.issueBoards.BoardsStore.detail.issue.update($dropdown.attr('data-issue-update')) - .then(function () { - $loading.fadeOut(); - }); + .then(fadeOutLoader) + .catch(fadeOutLoader); } else { if ($dropdown.hasClass('js-multiselect')) { diff --git a/app/assets/javascripts/merge_request_widget.js b/app/assets/javascripts/merge_request_widget.js index b0254b17dd2..42ecf0d6cb2 100644 --- a/app/assets/javascripts/merge_request_widget.js +++ b/app/assets/javascripts/merge_request_widget.js @@ -157,7 +157,7 @@ import MiniPipelineGraph from './mini_pipeline_graph_dropdown'; $('.ci-widget-fetching').show(); return $.getJSON(this.opts.ci_status_url, (function(_this) { return function(data) { - var message, status, title; + var message, status, title, callback; _this.status = data.status; _this.hasCi = data.has_ci; _this.updateMergeButton(_this.status, _this.hasCi); @@ -179,6 +179,12 @@ import MiniPipelineGraph from './mini_pipeline_graph_dropdown'; _this.opts.ci_sha = data.sha; _this.updateCommitUrls(data.sha); } + if (data.status === "success" || data.status === "failed") { + callback = function() { + return _this.getMergeStatus(); + }; + return setTimeout(callback, 2000); + } if (showNotification && data.status) { status = _this.ciLabelForStatus(data.status); if (status === "preparing") { diff --git a/app/assets/javascripts/milestone_select.js b/app/assets/javascripts/milestone_select.js index 773fe3233a7..bebd0aa357e 100644 --- a/app/assets/javascripts/milestone_select.js +++ b/app/assets/javascripts/milestone_select.js @@ -164,6 +164,9 @@ .then(function () { $dropdown.trigger('loaded.gl.dropdown'); $loading.fadeOut(); + }) + .catch(() => { + $loading.fadeOut(); }); } else { selected = $selectbox.find('input[type="hidden"]').val(); diff --git a/app/assets/javascripts/monitoring/prometheus_graph.js b/app/assets/javascripts/monitoring/prometheus_graph.js index d82a4eb9642..aff507abb91 100644 --- a/app/assets/javascripts/monitoring/prometheus_graph.js +++ b/app/assets/javascripts/monitoring/prometheus_graph.js @@ -71,6 +71,8 @@ class PrometheusGraph { this.transformData(metricsResponse); this.createGraph(); } + }).catch(() => { + new Flash('An error occurred when trying to load metrics. Please try again.'); }); } diff --git a/app/assets/javascripts/users_select.js b/app/assets/javascripts/users_select.js index 3325a7d429c..30902767705 100644 --- a/app/assets/javascripts/users_select.js +++ b/app/assets/javascripts/users_select.js @@ -56,6 +56,9 @@ gl.issueBoards.BoardsStore.detail.issue.update($dropdown.attr('data-issue-update')) .then(function () { $loading.fadeOut(); + }) + .catch(function () { + $loading.fadeOut(); }); }; diff --git a/app/views/projects/merge_requests/widget/_open.html.haml b/app/views/projects/merge_requests/widget/_open.html.haml index bc426f1dc0c..0872a1a0503 100644 --- a/app/views/projects/merge_requests/widget/_open.html.haml +++ b/app/views/projects/merge_requests/widget/_open.html.haml @@ -19,6 +19,8 @@ = render 'projects/merge_requests/widget/open/conflicts' - elsif @merge_request.work_in_progress? = render 'projects/merge_requests/widget/open/wip' + - elsif @merge_request.merge_when_pipeline_succeeds? && @merge_request.merge_error.present? + = render 'projects/merge_requests/widget/open/error' - elsif @merge_request.merge_when_pipeline_succeeds? = render 'projects/merge_requests/widget/open/merge_when_pipeline_succeeds' - elsif !@merge_request.can_be_merged_by?(current_user) diff --git a/app/views/projects/merge_requests/widget/open/_error.html.haml b/app/views/projects/merge_requests/widget/open/_error.html.haml new file mode 100644 index 00000000000..bbdc053609f --- /dev/null +++ b/app/views/projects/merge_requests/widget/open/_error.html.haml @@ -0,0 +1,6 @@ +%h4 + = icon('exclamation-triangle') + This merge request failed to be merged automatically + +%p + = @merge_request.merge_error diff --git a/changelogs/unreleased/fix-29125.yml b/changelogs/unreleased/fix-29125.yml new file mode 100644 index 00000000000..00b5e8c0a2a --- /dev/null +++ b/changelogs/unreleased/fix-29125.yml @@ -0,0 +1,4 @@ +--- +title: Display custom hook error messages when automatic merge is enabled +merge_request: +author: diff --git a/doc/administration/integration/plantuml.md b/doc/administration/integration/plantuml.md index 6515b1a264a..5c856835039 100644 --- a/doc/administration/integration/plantuml.md +++ b/doc/administration/integration/plantuml.md @@ -1,6 +1,6 @@ # PlantUML & GitLab -> [Introduced][ce-7810] in GitLab 8.16. +> [Introduced][ce-8537] in GitLab 8.16. When [PlantUML](http://plantuml.com) integration is enabled and configured in GitLab we are able to create simple diagrams in AsciiDoc and Markdown documents @@ -93,3 +93,5 @@ Some parameters can be added to the AsciiDoc block definition: - *height*: Height attribute added to the img tag. Markdown does not support any parameters and will always use PNG format. + +[ce-8537]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8537
\ No newline at end of file diff --git a/doc/development/fe_guide/img/boards_diagram.png b/doc/development/fe_guide/img/boards_diagram.png Binary files differnew file mode 100644 index 00000000000..7a2cf972fd0 --- /dev/null +++ b/doc/development/fe_guide/img/boards_diagram.png diff --git a/doc/development/fe_guide/img/vue_arch.png b/doc/development/fe_guide/img/vue_arch.png Binary files differnew file mode 100644 index 00000000000..a67706c7c1e --- /dev/null +++ b/doc/development/fe_guide/img/vue_arch.png diff --git a/doc/development/fe_guide/index.md b/doc/development/fe_guide/index.md index c7d4f2e9c23..e2a198f637f 100644 --- a/doc/development/fe_guide/index.md +++ b/doc/development/fe_guide/index.md @@ -27,6 +27,59 @@ For our currently-supported browsers, see our [requirements][requirements]. --- +## Development Process + +When you are assigned an issue please follow the next steps: + +### Divide a big feature into small Merge Requests +1. Big Merge Request are painful to review. In order to make this process easier we +must break a big feature into smaller ones and create a Merge Request for each step. +1. First step is to create a branch from `master`, let's call it `new-feature`. This branch +will be the recipient of all the smaller Merge Requests. Only this one will be merged to master. +1. Don't do any work on this one, let's keep it synced with master. +1. Create a new branch from `new-feature`, let's call it `new-feature-step-1`. We advise you +to clearly identify which step the branch represents. +1. Do the first part of the modifications in this branch. The target branch of this Merge Request +should be `new-feature`. +1. Once `new-feature-step-1` gets merged into `new-feature` we can continue our work. Create a new +branch from `new-feature`, let's call it `new-feature-step-2` and repeat the process done before. + +```shell +* master +|\ +| * new-feature +| |\ +| | * new-feature-step-1 +| |\ +| | * new-feature-step-2 +| |\ +| | * new-feature-step-3 +``` + +**Tips** +- Make sure `new-feature` branch is always synced with `master`: merge master frequently. +- Do the same for the feature branch you have opened. This can be accomplished by merging `master` into `new-feature` and `new-feature` into `new-feature-step-*` +- Avoid rewriting history. + +### Share your work early +1. Before writing code guarantee your vision of the architecture is aligned with +GitLab's architecture. +1. Add a diagram to the issue and ask a Frontend Architecture about it. + +  + +1. Don't take more than one week between starting work on a feature and +sharing a Merge Request with a reviewer or a maintainer. + +### Vue features +1. Follow the steps in [Vue.js Best Practices](vue.md) +1. Follow the style guide. +1. Only a handful of people are allowed to merge Vue related features. +Reach out to @jschatz, @iamphill, @fatihacet or @filipa early in this process. + + +--- + ## [Architecture](architecture.md) How we go about making fundamental design decisions in GitLab's frontend team or make changes to our frontend development guidelines. diff --git a/doc/development/fe_guide/style_guide_js.md b/doc/development/fe_guide/style_guide_js.md index 02840900547..038a689c09a 100644 --- a/doc/development/fe_guide/style_guide_js.md +++ b/doc/development/fe_guide/style_guide_js.md @@ -58,7 +58,7 @@ See [our current .eslintrc][eslintrc] for specific rules and patterns. import Bar from './bar'; ``` -- **Never** disable the `no-undef` rule. Declare globals with `/* global Foo */` instead. +- **Never** disable the `no-undef` rule. Declare globals with `/* global Foo */` instead. - When declaring multiple globals, always use one `/* global [name] */` line per variable. @@ -200,6 +200,19 @@ A forEach will cause side effects, it will be mutating the array being iterated. parseInt('10', 10); ``` +#### CSS classes used for JavaScript +- 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 @@ -217,6 +230,7 @@ A forEach will cause side effects, it will be mutating the array being iterated. #### Naming - **Extensions**: Use `.vue` extension for Vue components. - **Reference Naming**: Use PascalCase for Vue components and camelCase for their instances: + ```javascript // bad import cardBoard from 'cardBoard'; @@ -234,6 +248,7 @@ A forEach will cause side effects, it will be mutating the array being iterated. cardBoard: CardBoard }; ``` + - **Props Naming:** - Avoid using DOM component prop names. - Use kebab-case instead of camelCase to provide props in templates. @@ -260,12 +275,18 @@ A forEach will cause side effects, it will be mutating the array being iterated. <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> + // if props fit in one line then keep it on the same line <component bar="bar" /> ``` diff --git a/doc/development/fe_guide/testing.md b/doc/development/fe_guide/testing.md index a4631fd0073..66afbf4db4d 100644 --- a/doc/development/fe_guide/testing.md +++ b/doc/development/fe_guide/testing.md @@ -26,6 +26,10 @@ browser and you will not have access to certain APIs, such as [`Notification`](https://developer.mozilla.org/en-US/docs/Web/API/notification), which will have to be stubbed. +### Writing tests +### Vue.js unit tests +See this [section][vue-test]. + ### Running frontend tests `rake karma` runs the frontend-only (JavaScript) tests. @@ -134,3 +138,4 @@ Scenario: Developer can approve merge request [jasmine-focus]: https://jasmine.github.io/2.5/focused_specs.html [jasmine-jquery]: https://github.com/velesin/jasmine-jquery [karma]: http://karma-runner.github.io/ +[vue-test]:https://docs.gitlab.com/ce/development/fe_guide/vue.html#testing-vue-components diff --git a/doc/development/fe_guide/vue.md b/doc/development/fe_guide/vue.md index 3e3406e7d6a..45c8300d9de 100644 --- a/doc/development/fe_guide/vue.md +++ b/doc/development/fe_guide/vue.md @@ -19,13 +19,31 @@ We don't want to refactor all GitLab frontend code into Vue.js, here are some gu when not to use Vue.js: - Adding or changing static information; -- Features that highly depend on jQuery will be hard to work with Vue.js +- Features that highly depend on jQuery will be hard to work with Vue.js; +- Features without reactive data; As always, the Frontend Architectural Experts are available to help with any Vue or JavaScript questions. -## How to build a new feature with Vue.js +## Vue architecture -**Components, Stores and Services** +All new features built with Vue.js must follow a [Flux architecture][flux]. +The main goal we are trying to achieve is to have only one data flow and only one data entry. +In order to achieve this goal, each Vue bundle needs a Store - where we keep all the data -, +a Service - that we use to communicate with the server - and a main Vue component. + +Think of the Main Vue Component as the entry point of your application. This is the only smart +component that should exist in each Vue feature. +This component is responsible for: +1. Calling the Service to get data from the server +1. Calling the Store to store the data received +1. Mounting all the other components + +  + +You can also read about this architecture in vue docs about [state management][state-management] +and about [one way data flow][one-way-data-flow]. + +### Components, Stores and Services In some features implemented with Vue.js, like the [issue board][issue-boards] or [environments table][environments-table] @@ -46,16 +64,17 @@ _For consistency purposes, we recommend you to follow the same structure._ Let's look into each of them: -**A `*_bundle.js` file** +### A `*_bundle.js` file This is the index file of your new feature. This is where the root Vue instance of the new feature should be. -The Store and the Service should be imported and initialized in this file and provided as a prop to the main component. +The Store and the Service should be imported and initialized in this file and +provided as a prop to the main component. Don't forget to follow [these steps.][page_specific_javascript] -**A folder for Components** +### A folder for Components This folder holds all components that are specific of this new feature. If you need to use or create a component that will probably be used somewhere @@ -70,29 +89,320 @@ in one table would not be a good use of this pattern. You can read more about components in Vue.js site, [Component System][component-system] -**A folder for the Store** +### A folder for the Store The Store is a class that allows us to manage the state in a single -source of truth. +source of truth. It is not aware of the service or the components. The concept we are trying to follow is better explained by Vue documentation itself, please read this guide: [State Management][state-management] -**A folder for the Service** +### A folder for the Service + +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. + +### End Result + +The following example shows an application: + +```javascript +// store.js +export default class Store { + + /** + * This is where we will iniatialize the state of our data. + * Usually in a small SPA you don't need any options when starting the store. In the case you do + * need guarantee it's an Object and it's documented. + * + * @param {Object} options + */ + constructor(options) { + this.options = options; + + // Create a state object to handle all our data in the same place + this.todos = []: + } + + setTodos(todos = []) { + this.todos = todos; + } + + addTodo(todo) { + this.todos.push(todo); + } + + removeTodo(todoID) { + const state = this.todos; + + const newState = state.filter((element) => {element.id !== todoID}); + + this.todos = newState; + } +} + +// service.js +import Vue from 'vue'; +import VueResource from 'vue-resource'; +import 'vue_shared/vue_resource_interceptor'; + +Vue.use(VueResource); + +export default class Service { + constructor(options) { + this.todos = Vue.resource(endpoint.todosEndpoint); + } + + getTodos() { + return this.todos.get(); + } + + addTodo(todo) { + return this.todos.put(todo); + } +} +// todo_component.vue +<script> +export default { + props: { + data: { + type: Object, + required: true, + }, + } +} +</script> +<template> + <div> + <h1> + Title: {{data.title}} + </h1> + <p> + {{data.text}} + </p> + </div> +</template> + +// todos_main_component.vue +<script> +import Store from 'store'; +import Service from 'service'; +import TodoComponent from 'todoComponent'; +export default { + /** + * Although most data belongs in the store, each component it's own state. + * We want to show a loading spinner while we are fetching the todos, this state belong + * in the component. + * + * We need to access the store methods through all methods of our component. + * We need to access the state of our store. + */ + data() { + const store = new Store(); + + return { + isLoading: false, + store: store, + todos: store.todos, + }; + }, + + components: { + todo: TodoComponent, + }, + + created() { + this.service = new Service('todos'); + + this.getTodos(); + }, + + methods: { + getTodos() { + this.isLoading = true; + + this.service.getTodos() + .then(response => response.json()) + .then((response) => { + this.store.setTodos(response); + this.isLoading = false; + }) + .catch(() => { + this.isLoading = false; + // Show an error + }); + }, + + addTodo(todo) { + this.service.addTodo(todo) + then(response => response.json()) + .then((response) => { + this.store.addTodo(response); + }) + .catch(() => { + // Show an error + }); + } + } +} +</script> +<template> + <div class="container"> + <div v-if="isLoading"> + <i + class="fa fa-spin fa-spinner" + aria-hidden="true" /> + </div> + + <div + v-if="!isLoading" + class="js-todo-list"> + <template v-for='todo in todos'> + <todo :data="todo" /> + </template> + + <button + @click="addTodo" + class="js-add-todo"> + Add Todo + </button> + </div> + <div> +</template> + +// bundle.js +import todoComponent from 'todos_main_component.vue'; + +new Vue({ + el: '.js-todo-app', + components: { + todoComponent, + }, + render: createElement => createElement('todo-component' { + props: { + someProp: [], + } + }), +}); -The Service is used only to communicate with the server. -It does not store or manipulate any data. -We use [vue-resource][vue-resource-repo] to -communicate with the server. +``` -The [issue boards service][issue-boards-service] -is a good example of this pattern. +The [issue boards service][issue-boards-service] is a good example of this pattern. ## Style guide Please refer to the Vue section of our [style guide](style_guide_js.md#vuejs) for best practices while writing your Vue components and templates. +## Testing Vue Components + +Each Vue component has a unique output. This output is always present in the render function. + +Although we can test each method of a Vue component individually, our goal must be to test the output +of the render/template function, which represents the state at all times. + +Make use of Vue Resource Interceptors to mock data returned by the service. + +Here's how we would test the Todo App above: + +```javascript +import component from 'todos_main_component'; + +describe('Todos App', () => { + it('should render the loading state while the request is being made', () => { + const Component = Vue.extend(component); + + const vm = new Component().$mount(); + + expect(vm.$el.querySelector('i.fa-spin')).toBeDefined(); + }); + + describe('with data', () => { + // Mock the service to return data + const interceptor = (request, next) => { + next(request.respondWith(JSON.stringify([{ + title: 'This is a todo', + body: 'This is the text' + }]), { + status: 200, + })); + }; + + let vm; + + beforeEach(() => { + Vue.http.interceptors.push(interceptor); + + const Component = Vue.extend(component); + + vm = new Component().$mount(); + }); + + afterEach(() => { + Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor); + }); + + + it('should render todos', (done) => { + setTimeout(() => { + expect(vm.$el.querySelectorAll('.js-todo-list div').length).toBe(1); + done(); + }, 0); + }); + }); + + describe('add todo', () => { + let vm; + beforeEach(() => { + const Component = Vue.extend(component); + vm = new Component().$mount(); + }); + it('should add a todos', (done) => { + setTimeout(() => { + vm.$el.querySelector('.js-add-todo').click(); + + // Add a new interceptor to mock the add Todo request + Vue.nextTick(() => { + expect(vm.$el.querySelectorAll('.js-todo-list div').length).toBe(2); + }); + }, 0); + }); + }); +}); +``` + +### Stubbing API responses +[Vue Resource Interceptors][vue-resource-interceptor] allow us to add a interceptor with +the response we need: + +```javascript + // Mock the service to return data + const interceptor = (request, next) => { + next(request.respondWith(JSON.stringify([{ + title: 'This is a todo', + body: 'This is the text' + }]), { + status: 200, + })); + }; + + beforeEach(() => { + Vue.http.interceptors.push(interceptor); + }); + + afterEach(() => { + Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor); + }); + + it('should do something', (done) => { + setTimeout(() => { + // Test received data + done(); + }, 0); + }); +``` + [vue-docs]: http://vuejs.org/guide/index.html [issue-boards]: https://gitlab.com/gitlab-org/gitlab-ce/tree/master/app/assets/javascripts/boards @@ -100,5 +410,8 @@ for best practices while writing your Vue components and templates. [page_specific_javascript]: https://docs.gitlab.com/ce/development/frontend.html#page-specific-javascript [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 [issue-boards-service]: https://gitlab.com/gitlab-org/gitlab-ce/blob/master/app/assets/javascripts/boards/services/board_service.js.es6 +[flux]: https://facebook.github.io/flux diff --git a/doc/user/admin_area/user_cohorts.md b/doc/user/admin_area/user_cohorts.md index 1671487bc8c..e25e7a8bbc3 100644 --- a/doc/user/admin_area/user_cohorts.md +++ b/doc/user/admin_area/user_cohorts.md @@ -1,30 +1,37 @@ # Cohorts > **Notes:** -- [Introduced][ce-23361] in GitLab 9.1. +> [Introduced][ce-23361] in GitLab 9.1. As a benefit of having the [usage ping active](settings/usage_statistics.md), GitLab lets you analyze the users' activities of your GitLab installation. Under `/admin/cohorts`, when the usage ping is active, GitLab will show the monthly cohorts of new users and their activities over time. +## Overview + How do we read the user cohorts table? Let's take an example with the following user cohorts.  -For the cohort of June 2016, 163 users have been created on this server. One -month later, in July 2016, 155 users (or 95% of the June cohort) are still -active. Two months later, 139 users (or 85%) are still active. 9 months later, -we can see that only 6% of this cohort are still active. +For the cohort of June 2016, 163 users have been added on this server and have +been active since this month. One month later, in July 2016, out of +these 163 users, 155 users (or 95% of the June cohort) are still active. Two +months later, 139 users (or 85%) are still active. 9 months later, we can see +that only 6% of this cohort are still active. + +The Inactive users column shows the number of users who have been added during +the month, but who have never actually had any activity in the instance. How do we measure the activity of users? GitLab considers a user active if: + * the user signs in * the user has Git activity (whether push or pull). -### Setup +## Setup -1. Activate the usage ping as defined [in the documentation](settings/usage_statistics.md) +1. [Activate the usage ping](settings/usage_statistics.md) 2. Go to `/admin/cohorts` to see the user cohorts of the server [ce-23361]: https://gitlab.com/gitlab-org/gitlab-ce/issues/23361 diff --git a/lib/banzai/filter/issuable_state_filter.rb b/lib/banzai/filter/issuable_state_filter.rb index 1a9d03beb51..327ea9449a1 100644 --- a/lib/banzai/filter/issuable_state_filter.rb +++ b/lib/banzai/filter/issuable_state_filter.rb @@ -15,8 +15,8 @@ module Banzai issuables = extractor.extract([doc]) issuables.each do |node, issuable| - if VISIBLE_STATES.include?(issuable.state) && node.children.present? - node.add_child(Nokogiri::XML::Text.new(" [#{issuable.state}]", doc)) + if VISIBLE_STATES.include?(issuable.state) && node.inner_html == issuable.reference_link_text(project) + node.content += " (#{issuable.state})" end end diff --git a/package.json b/package.json index a17399ddb8f..e65f30eea77 100644 --- a/package.json +++ b/package.json @@ -60,6 +60,7 @@ "eslint-plugin-filenames": "^1.1.0", "eslint-plugin-import": "^2.2.0", "eslint-plugin-jasmine": "^2.1.0", + "eslint-plugin-promise": "^3.5.0", "istanbul": "^0.4.5", "jasmine-core": "^2.5.2", "jasmine-jquery": "^2.1.1", diff --git a/spec/features/merge_requests/widget_spec.rb b/spec/features/merge_requests/widget_spec.rb index a62c5435748..4e128cd4a7d 100644 --- a/spec/features/merge_requests/widget_spec.rb +++ b/spec/features/merge_requests/widget_spec.rb @@ -141,6 +141,27 @@ describe 'Merge request', :feature, :js do end end + context 'view merge request with MWPS enabled but automatically merge fails' do + before do + merge_request.update( + merge_when_pipeline_succeeds: true, + merge_user: merge_request.author, + merge_error: 'Something went wrong' + ) + + visit namespace_project_merge_request_path(project.namespace, project, merge_request) + end + + it 'shows information about the merge error' do + # Wait for the `ci_status` and `merge_check` requests + wait_for_ajax + + page.within('.mr-widget-body') do + expect(page).to have_content('Something went wrong') + end + end + end + context 'merge error' do before do allow_any_instance_of(Repository).to receive(:merge).and_return(false) diff --git a/spec/javascripts/blob/sketch/index_spec.js b/spec/javascripts/blob/sketch/index_spec.js index 0e4431548c4..79f40559817 100644 --- a/spec/javascripts/blob/sketch/index_spec.js +++ b/spec/javascripts/blob/sketch/index_spec.js @@ -1,4 +1,4 @@ -/* eslint-disable no-new */ +/* eslint-disable no-new, promise/catch-or-return */ import JSZip from 'jszip'; import SketchLoader from '~/blob/sketch'; diff --git a/spec/javascripts/environments/environment_monitoring_spec.js b/spec/javascripts/environments/environment_monitoring_spec.js index fc451cce641..0f3dba66230 100644 --- a/spec/javascripts/environments/environment_monitoring_spec.js +++ b/spec/javascripts/environments/environment_monitoring_spec.js @@ -1,5 +1,5 @@ import Vue from 'vue'; -import monitoringComp from '~/environments/components/environment_monitoring'; +import monitoringComp from '~/environments/components/environment_monitoring.vue'; describe('Monitoring Component', () => { let MonitoringComponent; diff --git a/spec/javascripts/environments/environment_rollback_spec.js b/spec/javascripts/environments/environment_rollback_spec.js index 7cb39d9df03..25397714a76 100644 --- a/spec/javascripts/environments/environment_rollback_spec.js +++ b/spec/javascripts/environments/environment_rollback_spec.js @@ -1,5 +1,5 @@ import Vue from 'vue'; -import rollbackComp from '~/environments/components/environment_rollback'; +import rollbackComp from '~/environments/components/environment_rollback.vue'; describe('Rollback Component', () => { const retryURL = 'https://gitlab.com/retry'; diff --git a/spec/javascripts/filtered_search/services/recent_searches_service_spec.js b/spec/javascripts/filtered_search/services/recent_searches_service_spec.js index 2a58fb3a7df..c255bf7c939 100644 --- a/spec/javascripts/filtered_search/services/recent_searches_service_spec.js +++ b/spec/javascripts/filtered_search/services/recent_searches_service_spec.js @@ -1,3 +1,5 @@ +/* eslint-disable promise/catch-or-return */ + import RecentSearchesService from '~/filtered_search/services/recent_searches_service'; describe('RecentSearchesService', () => { diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js index 56aabc16382..a00efa10119 100644 --- a/spec/javascripts/lib/utils/common_utils_spec.js +++ b/spec/javascripts/lib/utils/common_utils_spec.js @@ -1,3 +1,5 @@ +/* eslint-disable promise/catch-or-return */ + require('~/lib/utils/common_utils'); (() => { diff --git a/spec/lib/banzai/filter/issuable_state_filter_spec.rb b/spec/lib/banzai/filter/issuable_state_filter_spec.rb index 0df259333e4..600f3c123ed 100644 --- a/spec/lib/banzai/filter/issuable_state_filter_spec.rb +++ b/spec/lib/banzai/filter/issuable_state_filter_spec.rb @@ -34,17 +34,41 @@ describe Banzai::Filter::IssuableStateFilter, lib: true do expect(doc.css('a').last.text).to eq('') end - it 'adds text with standard formatting' do + it 'ignores issuable links with custom anchor' do issue = create(:issue, :closed) + link = create_link('something', issue: issue.id, reference_type: 'issue') + doc = filter(link, context) + + expect(doc.css('a').last.text).to eq('something') + end + + it 'ignores issuable links to specific comments' do + issue = create(:issue, :closed) + link = create_link("#{issue.to_reference} (comment 1)", issue: issue.id, reference_type: 'issue') + doc = filter(link, context) + + expect(doc.css('a').last.text).to eq("#{issue.to_reference} (comment 1)") + end + + it 'ignores merge request links to diffs tab' do + merge_request = create(:merge_request, :closed) link = create_link( - 'something <strong>else</strong>'.html_safe, - issue: issue.id, - reference_type: 'issue' + "#{merge_request.to_reference} (diffs)", + merge_request: merge_request.id, + reference_type: 'merge_request' ) doc = filter(link, context) - expect(doc.css('a').last.inner_html). - to eq('something <strong>else</strong> [closed]') + expect(doc.css('a').last.text).to eq("#{merge_request.to_reference} (diffs)") + end + + it 'handles cross project references' do + issue = create(:issue, :closed) + project = create(:empty_project) + link = create_link(issue.to_reference(project), issue: issue.id, reference_type: 'issue') + doc = filter(link, context.merge(project: project)) + + expect(doc.css('a').last.text).to eq("#{issue.to_reference(project)} (closed)") end it 'does not append state when filter is not enabled' do @@ -59,68 +83,88 @@ describe Banzai::Filter::IssuableStateFilter, lib: true do context 'for issue references' do it 'ignores open issue references' do issue = create(:issue) - link = create_link('text', issue: issue.id, reference_type: 'issue') + link = create_link(issue.to_reference, issue: issue.id, reference_type: 'issue') doc = filter(link, context) - expect(doc.css('a').last.text).to eq('text') + expect(doc.css('a').last.text).to eq(issue.to_reference) end it 'ignores reopened issue references' do - reopened_issue = create(:issue, :reopened) - link = create_link('text', issue: reopened_issue.id, reference_type: 'issue') + issue = create(:issue, :reopened) + link = create_link(issue.to_reference, issue: issue.id, reference_type: 'issue') doc = filter(link, context) - expect(doc.css('a').last.text).to eq('text') + expect(doc.css('a').last.text).to eq(issue.to_reference) end - it 'appends [closed] to closed issue references' do - closed_issue = create(:issue, :closed) - link = create_link('text', issue: closed_issue.id, reference_type: 'issue') + it 'appends state to closed issue references' do + issue = create(:issue, :closed) + link = create_link(issue.to_reference, issue: issue.id, reference_type: 'issue') doc = filter(link, context) - expect(doc.css('a').last.text).to eq('text [closed]') + expect(doc.css('a').last.text).to eq("#{issue.to_reference} (closed)") end end context 'for merge request references' do it 'ignores open merge request references' do - mr = create(:merge_request) - link = create_link('text', merge_request: mr.id, reference_type: 'merge_request') + merge_request = create(:merge_request) + link = create_link( + merge_request.to_reference, + merge_request: merge_request.id, + reference_type: 'merge_request' + ) doc = filter(link, context) - expect(doc.css('a').last.text).to eq('text') + expect(doc.css('a').last.text).to eq(merge_request.to_reference) end it 'ignores reopened merge request references' do - mr = create(:merge_request, :reopened) - link = create_link('text', merge_request: mr.id, reference_type: 'merge_request') + merge_request = create(:merge_request, :reopened) + link = create_link( + merge_request.to_reference, + merge_request: merge_request.id, + reference_type: 'merge_request' + ) doc = filter(link, context) - expect(doc.css('a').last.text).to eq('text') + expect(doc.css('a').last.text).to eq(merge_request.to_reference) end it 'ignores locked merge request references' do - mr = create(:merge_request, :locked) - link = create_link('text', merge_request: mr.id, reference_type: 'merge_request') + merge_request = create(:merge_request, :locked) + link = create_link( + merge_request.to_reference, + merge_request: merge_request.id, + reference_type: 'merge_request' + ) doc = filter(link, context) - expect(doc.css('a').last.text).to eq('text') + expect(doc.css('a').last.text).to eq(merge_request.to_reference) end - it 'appends [closed] to closed merge request references' do - mr = create(:merge_request, :closed) - link = create_link('text', merge_request: mr.id, reference_type: 'merge_request') + it 'appends state to closed merge request references' do + merge_request = create(:merge_request, :closed) + link = create_link( + merge_request.to_reference, + merge_request: merge_request.id, + reference_type: 'merge_request' + ) doc = filter(link, context) - expect(doc.css('a').last.text).to eq('text [closed]') + expect(doc.css('a').last.text).to eq("#{merge_request.to_reference} (closed)") end - it 'appends [merged] to merged merge request references' do - mr = create(:merge_request, :merged) - link = create_link('text', merge_request: mr.id, reference_type: 'merge_request') + it 'appends state to merged merge request references' do + merge_request = create(:merge_request, :merged) + link = create_link( + merge_request.to_reference, + merge_request: merge_request.id, + reference_type: 'merge_request' + ) doc = filter(link, context) - expect(doc.css('a').last.text).to eq('text [merged]') + expect(doc.css('a').last.text).to eq("#{merge_request.to_reference} (merged)") end end end diff --git a/yarn.lock b/yarn.lock index e16cd9c3673..90ba39a3251 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1988,6 +1988,10 @@ eslint-plugin-jasmine@^2.1.0: version "2.2.0" resolved "https://registry.yarnpkg.com/eslint-plugin-jasmine/-/eslint-plugin-jasmine-2.2.0.tgz#7135879383c39a667c721d302b9f20f0389543de" +eslint-plugin-promise@^3.5.0: + version "3.5.0" + resolved "https://registry.yarnpkg.com/eslint-plugin-promise/-/eslint-plugin-promise-3.5.0.tgz#78fbb6ffe047201627569e85a6c5373af2a68fca" + eslint@^3.10.1: version "3.15.0" resolved "https://registry.yarnpkg.com/eslint/-/eslint-3.15.0.tgz#bdcc6a6c5ffe08160e7b93c066695362a91e30f2" |
